On Mon, May 1, 2017 at 10:49 PM, Honnappa Nagarahalli < [email protected]> wrote:
> On 1 May 2017 at 13:07, Bill Fischofer <[email protected]> wrote: > > > > > > 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. > > > > 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. > > > > >> > >> 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 > >> >> >> > >> >> > > >> > > >> > > > > > >
