Re: [lng-odp] [API-NEXT PATCH] linux-generic: pool: reset origin_qe on buffer allocation

[Bill Fischofer]

On Wed, Nov 30, 2016 at 8:09 AM, Maxim Uvarov  wrote:
> On 11/30/16 03:48, Bill Fischofer wrote:
>>
>> Resolve bug https://bugs.linaro.org/show_bug.cgi?id=2622 by
>> re-initializing origin_qe to NULL when a buffer is allocated. This step
>> was omitted in the switch to ring pool allocation introduced in
>> commit ID c8cf1d87783d4b4c628f219803b78731b8d4ade4
>>
>> Signed-off-by: Bill Fischofer 
>> ---
>>   platform/linux-generic/odp_pool.c | 9 ++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/platform/linux-generic/odp_pool.c
>> b/platform/linux-generic/odp_pool.c
>> index 4be3827..0b3d694 100644
>> --- a/platform/linux-generic/odp_pool.c
>> +++ b/platform/linux-generic/odp_pool.c
>> @@ -648,9 +648,12 @@ int buffer_alloc_multi(pool_t *pool, odp_buffer_t
>> buf[],
>> cache->num = cache_num - num_ch;
>> }
>>   - if (buf_hdr) {
>> -   for (i = 0; i < num_ch; i++)
>> -   buf_hdr[i] = buf_hdl_to_hdr(buf[i]);
>> +   for (i = 0; i < num_ch; i++) {
>> +   odp_buffer_hdr_t *hdr = buf_hdl_to_hdr(buf[i]);
>> +
>> +   hdr->origin_qe = NULL;
>> +   if (buf_hdr)
>> +   buf_hdr[i] = hdr;
>> }
>>
>
>
> isn't it better unset orig_qe in loop in line 611? In that case you need to
> walk loop 2 times.

This seemed the cleanest place to put this. The issue is that the
rings contain buffer handles, not headers addresses, so I want to
minimize the number of times buf_hdl_to_hdr() is called. For Line 611,
that's only satisfying from the local cache, so you'd need to do a
similar operation on the global cache repopulation code that follows.
Having this here keeps it all in one place and is consistent with the
existing code to return buf_hdrs if requested.

>
> Maxim.
>
>> return num_ch + num_deq;
>
>

Re: [lng-odp] [API-NEXT PATCH] linux-generic: pool: reset origin_qe on buffer allocation

[Maxim Uvarov]

On 11/30/16 03:48, Bill Fischofer wrote:

Resolve bug https://bugs.linaro.org/show_bug.cgi?id=2622 by
re-initializing origin_qe to NULL when a buffer is allocated. This step
was omitted in the switch to ring pool allocation introduced in
commit ID c8cf1d87783d4b4c628f219803b78731b8d4ade4

Signed-off-by: Bill Fischofer 
---
  platform/linux-generic/odp_pool.c | 9 ++---
  1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/platform/linux-generic/odp_pool.c 
b/platform/linux-generic/odp_pool.c
index 4be3827..0b3d694 100644
--- a/platform/linux-generic/odp_pool.c
+++ b/platform/linux-generic/odp_pool.c
@@ -648,9 +648,12 @@ int buffer_alloc_multi(pool_t *pool, odp_buffer_t buf[],
cache->num = cache_num - num_ch;
}
  
-	if (buf_hdr) {

-   for (i = 0; i < num_ch; i++)
-   buf_hdr[i] = buf_hdl_to_hdr(buf[i]);
+   for (i = 0; i < num_ch; i++) {
+   odp_buffer_hdr_t *hdr = buf_hdl_to_hdr(buf[i]);
+
+   hdr->origin_qe = NULL;
+   if (buf_hdr)
+   buf_hdr[i] = hdr;
}
  


isn't it better unset orig_qe in loop in line 611? In that case you need 
to walk loop 2 times.


Maxim.


return num_ch + num_deq;

Re: [lng-odp] [API-NEXT PATCH] linux-generic: pool: reset origin_qe on buffer allocation

[Mike Holmes]

On 30 November 2016 at 04:07, Savolainen, Petri (Nokia - FI/Espoo)  wrote:

> > -Original Message-
> > From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of
> Bill
> > Fischofer
> > Sent: Wednesday, November 30, 2016 2:48 AM
> > To: lng-odp@lists.linaro.org
> > Subject: [lng-odp] [API-NEXT PATCH] linux-generic: pool: reset origin_qe
> > on buffer allocation
> >
> > Resolve bug https://bugs.linaro.org/show_bug.cgi?id=2622 by
> > re-initializing origin_qe to NULL when a buffer is allocated. This step
> > was omitted in the switch to ring pool allocation introduced in
> > commit ID c8cf1d87783d4b4c628f219803b78731b8d4ade4
> >
> > Signed-off-by: Bill Fischofer 
> > ---
> >  platform/linux-generic/odp_pool.c | 9 ++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/platform/linux-generic/odp_pool.c b/platform/linux-
> > generic/odp_pool.c
> > index 4be3827..0b3d694 100644
> > --- a/platform/linux-generic/odp_pool.c
> > +++ b/platform/linux-generic/odp_pool.c
> > @@ -648,9 +648,12 @@ int buffer_alloc_multi(pool_t *pool, odp_buffer_t
> > buf[],
> >   cache->num = cache_num - num_ch;
> >   }
> >
> > - if (buf_hdr) {
> > - for (i = 0; i < num_ch; i++)
> > - buf_hdr[i] = buf_hdl_to_hdr(buf[i]);
> > + for (i = 0; i < num_ch; i++) {
> > + odp_buffer_hdr_t *hdr = buf_hdl_to_hdr(buf[i]);
> > +
> > + hdr->origin_qe = NULL;
> > + if (buf_hdr)
> > + buf_hdr[i] = hdr;
> >   }
> >
> >   return num_ch + num_deq;
> > --
> > 2.7.4
>
> The new ordered queue implementation removes origin_qe altogether. So,
> this change is not needed as soon as the new implementation is merged.
>

How long will that be ?
If this was to master we would have to take it because master cannot be
broken, but this is a bug in api-next and the code is in use.

Is there any problem fixing this now, it is small conflict if any to a
future patch.
Every time we wait for a future planned change it takes a lot longer than
expected and I don't think postponing is a good idea, not  unless Maxim can
confirm the superseding work will be merged immediately.

Mike


>
> -Petri
>
>
>
>
>


-- 
Mike Holmes
Program Manager - Linaro Networking Group
Linaro.org  *│ *Open source software for ARM SoCs
"Work should be fun and collaborative, the rest follows"

Re: [lng-odp] [API-NEXT PATCH] linux-generic: pool: reset origin_qe on buffer allocation

[Savolainen, Petri (Nokia - FI/Espoo)]

> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of Bill
> Fischofer
> Sent: Wednesday, November 30, 2016 2:48 AM
> To: lng-odp@lists.linaro.org
> Subject: [lng-odp] [API-NEXT PATCH] linux-generic: pool: reset origin_qe
> on buffer allocation
> 
> Resolve bug https://bugs.linaro.org/show_bug.cgi?id=2622 by
> re-initializing origin_qe to NULL when a buffer is allocated. This step
> was omitted in the switch to ring pool allocation introduced in
> commit ID c8cf1d87783d4b4c628f219803b78731b8d4ade4
> 
> Signed-off-by: Bill Fischofer 
> ---
>  platform/linux-generic/odp_pool.c | 9 ++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/platform/linux-generic/odp_pool.c b/platform/linux-
> generic/odp_pool.c
> index 4be3827..0b3d694 100644
> --- a/platform/linux-generic/odp_pool.c
> +++ b/platform/linux-generic/odp_pool.c
> @@ -648,9 +648,12 @@ int buffer_alloc_multi(pool_t *pool, odp_buffer_t
> buf[],
>   cache->num = cache_num - num_ch;
>   }
> 
> - if (buf_hdr) {
> - for (i = 0; i < num_ch; i++)
> - buf_hdr[i] = buf_hdl_to_hdr(buf[i]);
> + for (i = 0; i < num_ch; i++) {
> + odp_buffer_hdr_t *hdr = buf_hdl_to_hdr(buf[i]);
> +
> + hdr->origin_qe = NULL;
> + if (buf_hdr)
> + buf_hdr[i] = hdr;
>   }
> 
>   return num_ch + num_deq;
> --
> 2.7.4

The new ordered queue implementation removes origin_qe altogether. So, this 
change is not needed as soon as the new implementation is merged.

-Petri