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.
>>
>>
>> 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.
>
> 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.
>
>>
>> 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
>> >> >>
>> >> >
>> >
>> >
>
>
