Re: [lng-odp] [API-NEXT PATCH v4 3/8] pktio: loop: use handle instead of pointer to buffer
> >> 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. Function inlining is most effective for short functions that are called often (like odp_packet_len()). For more complex functions, inlining or usage of functions pointer, does not have such large relative effect. E.g. if inlining saves 7 cycles, it can be >2x improvement for short function, but only couple percent for a complex function like schedule. When you add application code into the calculation, e.g. the 7 cycle save for each schedule() call is not very significant. When the function pointer based schedule interface was introduced e.g. l2fwd performance drop was negligible. The steps are: * first, clean integration * then, optimization if needed (without breaking modularity) -Petri
Re: [lng-odp] [API-NEXT PATCH v4 3/8] pktio: loop: use handle instead of pointer to buffer
On 2 May 2017 at 13:44, Bill Fischofer wrote:
>
>
> On Mon, May 1, 2017 at 10:49 PM, Honnappa Nagarahalli
> wrote:
>>
>> On 1 May 2017 at 13:07, Bill Fischofer wrote:
>> >
>> >
>> > On Sun, Apr 30, 2017 at 10:54 PM, Honnappa Nagarahalli
>> > 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
>> >> wrote:
>> >> >
>> >> >
>> >> > On Sun, Apr 30, 2017 at 12:25 PM, Honnappa Nagarahalli
>> >> > wrote:
>> >> >>
>> >> >> On 26 April 2017 at 07:10, Elo, Matias (Nokia - FI/Espoo)
>> >> >> wrote:
>> >> >> >
>> >> >> >> On 19 Apr 2017, at 10:14, Brian Brooks
>> >> >> >> wrote:
>> >> >> >>
>> >> >> >> Signed-off-by: Kevin Wang
>> >> >> >> ---
>> >> >> >> 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_hd
Re: [lng-odp] [API-NEXT PATCH v4 3/8] pktio: loop: use handle instead of pointer to buffer
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 wrote:
> >
> >
> > On Sun, Apr 30, 2017 at 10:54 PM, Honnappa Nagarahalli
> > 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
> >> wrote:
> >> >
> >> >
> >> > On Sun, Apr 30, 2017 at 12:25 PM, Honnappa Nagarahalli
> >> > wrote:
> >> >>
> >> >> On 26 April 2017 at 07:10, Elo, Matias (Nokia - FI/Espoo)
> >> >> wrote:
> >> >> >
> >> >> >> On 19 Apr 2017, at 10:14, Brian Brooks
> wrote:
> >> >> >>
> >> >> >> Signed-off-by: Kevin Wang
> >> >> >> ---
> >> >> >> 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 v
Re: [lng-odp] [API-NEXT PATCH v4 3/8] pktio: loop: use handle instead of pointer to buffer
On 1 May 2017 at 13:07, Bill Fischofer wrote:
>
>
> On Sun, Apr 30, 2017 at 10:54 PM, Honnappa Nagarahalli
> 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
>> wrote:
>> >
>> >
>> > On Sun, Apr 30, 2017 at 12:25 PM, Honnappa Nagarahalli
>> > wrote:
>> >>
>> >> On 26 April 2017 at 07:10, Elo, Matias (Nokia - FI/Espoo)
>> >> wrote:
>> >> >
>> >> >> On 19 Apr 2017, at 10:14, Brian Brooks wrote:
>> >> >>
>> >> >> Signed-off-by: Kevin Wang
>> >> >> ---
>> >> >> 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 defa
Re: [lng-odp] [API-NEXT PATCH v4 3/8] pktio: loop: use handle instead of pointer to buffer
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.
>
> 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
> wrote:
> >
> >
> > On Sun, Apr 30, 2017 at 12:25 PM, Honnappa Nagarahalli
> > wrote:
> >>
> >> On 26 April 2017 at 07:10, Elo, Matias (Nokia - FI/Espoo)
> >> wrote:
> >> >
> >> >> On 19 Apr 2017, at 10:14, Brian Brooks wrote:
> >> >>
> >> >> Signed-off-by: Kevin Wang
> >> >> ---
> >> >> 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.
> >
>
Re: [lng-odp] [API-NEXT PATCH v4 3/8] pktio: loop: use handle instead of pointer to buffer
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)
> wrote:
> >
> >> On 19 Apr 2017, at 10:14, Brian Brooks wrote:
> >>
> >> Signed-off-by: Kevin Wang
> >> ---
> >> 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
> >>
> >
>
Re: [lng-odp] [API-NEXT PATCH v4 3/8] pktio: loop: use handle instead of pointer to buffer
On 26 April 2017 at 07:10, Elo, Matias (Nokia - FI/Espoo)
wrote:
>
>> On 19 Apr 2017, at 10:14, Brian Brooks wrote:
>>
>> Signed-off-by: Kevin Wang
>> ---
>> 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.
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})
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.
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.
>
>> 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
>>
>
Re: [lng-odp] [API-NEXT PATCH v4 3/8] pktio: loop: use handle instead of pointer to buffer
> On 19 Apr 2017, at 10:14, Brian Brooks wrote:
>
> Signed-off-by: Kevin Wang
> ---
> 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.
> 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
>
Re: [lng-odp] [API-NEXT PATCH v4 3/8] pktio: loop: use handle instead of pointer to buffer
On Wed, Apr 19, 2017 at 2:14 AM, Brian Brooks wrote:
> Signed-off-by: Kevin Wang
> ---
> 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
>
Why is this variant form needed?
> pkt_len = odp_packet_len(pkt);
>
> -
> 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
>
Same question here. These conversions should be independent of the
scheduler being used.
> bytes += odp_packet_len(pkt_tbl[i]);
> }
>
> --
> 2.12.2
>
>
[lng-odp] [API-NEXT PATCH v4 3/8] pktio: loop: use handle instead of pointer to buffer
Signed-off-by: Kevin Wang
---
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);
-
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
