| Message ID | 1480466883-31974-1-git-send-email-bill.fischofer@linaro.org |
|---|---|
| State | New |
| Headers | show |
> -----Original Message----- > From: lng-odp [mailto:lng-odp-bounces@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 <bill.fischofer@linaro.org> > --- > 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
On 30 November 2016 at 04:07, Savolainen, Petri (Nokia - FI/Espoo) <petri. savolainen@nokia-bell-labs.com> wrote: > > -----Original Message----- > > From: lng-odp [mailto:lng-odp-bounces@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 <bill.fischofer@linaro.org> > > --- > > 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 <http://www.linaro.org/> *│ *Open source software for ARM SoCs "Work should be fun and collaborative, the rest follows"
This is fixing a problem that exists today and has existed since the revised pool code was merged nine days ago. Matias' patch is already waiting for a v2 so that can include any changes needed to adjust for this. But I'd prefer to fix the current code and then work on more radical improvements. On Wed, Nov 30, 2016 at 6:45 AM, Mike Holmes <mike.holmes@linaro.org> wrote: > > > On 30 November 2016 at 04:07, Savolainen, Petri (Nokia - FI/Espoo) > <petri.savolainen@nokia-bell-labs.com> wrote: >> >> > -----Original Message----- >> > From: lng-odp [mailto:lng-odp-bounces@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 <bill.fischofer@linaro.org> >> > --- >> > 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" > >
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 <bill.fischofer@linaro.org> > --- > 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;
On Wed, Nov 30, 2016 at 8:09 AM, Maxim Uvarov <maxim.uvarov@linaro.org> 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 <bill.fischofer@linaro.org> >> --- >> 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; > >
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;
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 <bill.fischofer@linaro.org> --- platform/linux-generic/odp_pool.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) -- 2.7.4