On 2 May 2017 at 13:44, Bill Fischofer <bill.fischo...@linaro.org> wrote:
>
>
> On Mon, May 1, 2017 at 10:49 PM, Honnappa Nagarahalli
> <honnappa.nagaraha...@linaro.org> wrote:
>>
>> On 1 May 2017 at 13:07, Bill Fischofer <bill.fischo...@linaro.org> wrote:
>> >
>> >
>> > On Sun, Apr 30, 2017 at 10:54 PM, Honnappa Nagarahalli
>> > <honnappa.nagaraha...@linaro.org> wrote:
>> >>
>> >> The question we are trying to answer is why are there #ifdefs? #ifdefs
>> >> are needed if there are multiple code paths.
>> >
>> >
>> > That's what function tables do. The #ifdefs are restricted to the
>> > scheduler
>> > interface. It's really no different than DDF or the function tables used
>> > in
>> > the existing pktio and queue implementations.
>> >
>> >>
>> >>
>> >> The function loopback_recv calls 'queue_deq_multi'. 'queue_deq_multi'
>> >> while using default queues returns odp_buffer_hdr_t (memory address).
>> >> 'queue_deq_multi' while using scalable queues returns odp_buffer_hdr_t
>> >> ({pool_id, index}).
>> >
>> >
>> > A first question is why do the queue routines need to return anything
>> > different with a different scheduler? These are "pluggable" functions.
>> >
>>
>> The default queue implementation requires the event to be of the type
>> odp_buffer_hdr_t. It cannot store an event of type odp_event_t. Hence
>> it converts odp_event_t to odp_buffer_hdr_t.
>>
>>
>> The scalable queues do not have any such requirement. All the ODP
>> public APIs use odp_event_t, hence scalable queues store just
>> odp_event_t on the queues and return the same to avoid any
>> conversions.
>
>
> That's fine, it just means that the "conversion" routines you use are
> no-ops. Generalize the existing conversion routines to be pluggable
> functions and both get what they need.
>
>>
>>
>> >>
>> >>
>> >> loopback_recv returns odp_packet_t (memory address), hence both the
>> >
>> >
>> > No, odp_packet_t is an odp_packet_t. To convert that into an internal
>> > odp_packet_hdr_t involve a call to an internal API. How that internal
>> > API
>> > does this is opaque to the caller.
>> >
>> >>
>> >> queue implementations need to convert whatever they get from
>> >> 'queue_deq_multi' into odp_packet_t. Since 'queue_deq_multi' in both
>> >> the implementations returns different things, they need to have
>> >> different conversions.
>> >
>> >
>> > Again since these are internal APIs that are called via the function
>> > tables
>> > associated with each qentry. Internal conversion routines should give
>> > the
>> > caller what it needs without having the caller resort to #ifdefs.
>>
>> These APIs are not called via function tables.
>
>
> My suggestion is that making them function tables (like the rest of the
> scheduler framework) is a better approach than #ifdefs.
>
>>
>>
>> >
>> > The issue here is one of scalability. If the addition of each new
>> > scheduler
>> > requires a bunch of ever-growing #ifdefs in non-schedule modules that
>> > quickly becomes unmanageable.The face that the previous three schedulers
>> > didn't need #ifdefs suggests that the fourth should be able to do
>> > without
>> > them as well.
>>
>> We will not have few more scheduler/queue implementations. We have to
>> consolidate the schedulers if that happens. The previous 3 schedulers
>> did not need #ifdefs in other modules as they did not change the queue
>> implementation. All of them use the same queue implementation.
>
>
> Then either the new one should use the same queue implementation or else we
> should generalize this into a pluggable framework like the other scheduler
> APIs.
The existing queue implementation uses linked list. Many benefits are
due to using array to implement the queue.
We have seen the performance drops due to using function calls, moving
to function pointers would drop the performance even further.
>
>>
>>
>> >
>> >>
>> >> On 30 April 2017 at 14:05, Bill Fischofer <bill.fischo...@linaro.org>
>> >> wrote:
>> >> >
>> >> >
>> >> > On Sun, Apr 30, 2017 at 12:25 PM, Honnappa Nagarahalli
>> >> > <honnappa.nagaraha...@linaro.org> wrote:
>> >> >>
>> >> >> On 26 April 2017 at 07:10, Elo, Matias (Nokia - FI/Espoo)
>> >> >> <matias....@nokia.com> wrote:
>> >> >> >
>> >> >> >> On 19 Apr 2017, at 10:14, Brian Brooks <brian.bro...@arm.com>
>> >> >> >> wrote:
>> >> >> >>
>> >> >> >> Signed-off-by: Kevin Wang <kevin.w...@arm.com>
>> >> >> >> ---
>> >> >> >> platform/linux-generic/pktio/loop.c | 11 +++++++++--
>> >> >> >> 1 file changed, 9 insertions(+), 2 deletions(-)
>> >> >> >>
>> >> >> >> diff --git a/platform/linux-generic/pktio/loop.c
>> >> >> >> b/platform/linux-generic/pktio/loop.c
>> >> >> >> index e9ad22ba..cbb15179 100644
>> >> >> >> --- a/platform/linux-generic/pktio/loop.c
>> >> >> >> +++ b/platform/linux-generic/pktio/loop.c
>> >> >> >> @@ -80,11 +80,13 @@ static int loopback_recv(pktio_entry_t
>> >> >> >> *pktio_entry, int index ODP_UNUSED,
>> >> >> >>
>> >> >> >> for (i = 0; i < nbr; i++) {
>> >> >> >> uint32_t pkt_len;
>> >> >> >> -
>> >> >> >> +#ifdef ODP_SCHEDULE_SCALABLE
>> >> >> >> + pkt =
>> >> >> >> _odp_packet_from_buffer((odp_buffer_t)(hdr_tbl[i]));
>> >> >> >> +#else
>> >> >> >> pkt =
>> >> >> >> _odp_packet_from_buffer(odp_hdr_to_buf(hdr_tbl[i]));
>> >> >> >> +#endif
>> >> >> >> pkt_len = odp_packet_len(pkt);
>> >> >> >>
>> >> >> >> -
>> >> >> >
>> >> >> > No #ifdef code please. Especially since the pktio should be
>> >> >> > completely
>> >> >> > independent
>> >> >> > from the scheduler code.
>> >> >>
>> >> >> Agree. I wish the pktio was independent of scheduler/queues.
>> >> >
>> >> >
>> >> > It is.
>> >> >
>> >> >>
>> >> >>
>> >> >> odp_packet_t -> pointer to void (memory address) -> not symmetrical
>> >> >> to
>> >> >> odp_buffer_t
>> >> >> odp_packet_hdr_t -> pointer to void (memory address)
>> >> >>
>> >> >> odp_buffer_t -> pointer to void (stores {pool_id, index})
>> >> >> odp_buffer_hdr_t -> pointer to void (memory address)
>> >> >>
>> >> >> odp_event_t -> pointer to void (stores {pool_id, index})
>> >> >
>> >> >
>> >> > That's why you're using the _odp_packet_from_buffer() internal API.
>> >> > If
>> >> > this
>> >> > doesn't work for you, the answer is create another internal packet
>> >> > API
>> >> > that
>> >> > does. In no case should you need #ifdefs like this in pktio code. In
>> >> > this
>> >> > case the original code should work just fine. hdr_tbl is an
>> >> > odp_buffer_hdr_t
>> >> > and odp_hdr_to_buf turns that into an odp_buffer_t that
>> >> > _odp_packet_from_buffer() turns into an odp_packet_t.
>> >> >
>> >> >>
>> >> >>
>> >> >> The default queue stores odp_buffer_hdr_t in its linked list. So,
>> >> >> odp_event_t is converted to odp_buffer_hdr_t as the linked list
>> >> >> 'next'
>> >> >> pointer is part of odp_buffer_hdr_t.
>> >> >>
>> >> >> The scalable queues store odp_event_t because it does not depend on
>> >> >> the 'next' pointer in the odp_buffer_hdr_t, there is no need to take
>> >> >> the overhead of conversion.
>> >> >
>> >> >
>> >> > Questionable, but even if true the answer is to create an internal
>> >> > API
>> >> > that
>> >> > takes an odp_buffer_hdr_t and returns an odp_packet_t directly.
>> >> >
>> >> >>
>> >> >>
>> >> >> In the loopback_recv function, the odp_packet_t needs to be
>> >> >> converted
>> >> >> to odp_buffer_hdr_t for default queues (there is multi-level
>> >> >> conversion happening here (memory address->handle->memory address),
>> >> >> should be just a typecast)
>> >> >>
>> >> >> For scalable queues, the conversion needs to be happen from
>> >> >> odp_packet_t to odp_event_t.
>> >> >
>> >> >
>> >> > odp_packet_to_event() does this conversion, no?
>> >> >
>> >> >>
>> >> >>
>> >> >> >
>> >> >> >> if (pktio_cls_enabled(pktio_entry)) {
>> >> >> >> odp_packet_t new_pkt;
>> >> >> >> odp_pool_t new_pool;
>> >> >> >> @@ -163,7 +165,12 @@ static int loopback_send(pktio_entry_t
>> >> >> >> *pktio_entry, int index ODP_UNUSED,
>> >> >> >> len = QUEUE_MULTI_MAX;
>> >> >> >>
>> >> >> >> for (i = 0; i < len; ++i) {
>> >> >> >> +#ifdef ODP_SCHEDULE_SCALABLE
>> >> >> >> + hdr_tbl[i] = (odp_buffer_hdr_t *)(uintptr_t)
>> >> >> >> + _odp_packet_to_buffer(pkt_tbl[i]);
>> >> >> >> +#else
>> >> >> >> hdr_tbl[i] =
>> >> >> >> buf_hdl_to_hdr(_odp_packet_to_buffer(pkt_tbl[i]));
>> >> >> >> +#endif
>> >> >> >> bytes += odp_packet_len(pkt_tbl[i]);
>> >> >> >> }
>> >> >> >>
>> >> >> >> --
>> >> >> >> 2.12.2
>> >> >> >>
>> >> >> >
>> >> >
>> >> >
>> >
>> >
>
>
