On Sun, Apr 30, 2017 at 10:54 PM, Honnappa Nagarahalli <
[email protected]> 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.
>
> 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.
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.
> On 30 April 2017 at 14:05, Bill Fischofer <[email protected]>
> wrote:
> >
> >
> > On Sun, Apr 30, 2017 at 12:25 PM, Honnappa Nagarahalli
> > <[email protected]> wrote:
> >>
> >> On 26 April 2017 at 07:10, Elo, Matias (Nokia - FI/Espoo)
> >> <[email protected]> wrote:
> >> >
> >> >> On 19 Apr 2017, at 10:14, Brian Brooks <[email protected]> wrote:
> >> >>
> >> >> Signed-off-by: Kevin Wang <[email protected]>
> >> >> ---
> >> >> 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
> >> >>
> >> >
> >
> >
>
