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