| Message ID | 54620DB1.4010805@linaro.org |
|---|---|
| State | New |
| Headers | show |
v3 patch posted to correct 2nd problem (ret_buf returning to blk_freelist instead of buf_freelist). The pool bufcount is decremented as part of ret_buf processing. It reflects the number of allocated buffers and will return to 0 naturally when the last allocated buffer is freed, so this should not be set to 0 unconditionally. Bill On Tue, Nov 11, 2014 at 7:22 AM, Taras Kondratiuk < taras.kondratiuk@linaro.org> wrote: > On 11/11/2014 03:14 PM, Bill Fischofer wrote: > > Good catch. Yes, the inner routines are missing an &. Will send out v2 > > to correct that shortly. > > There are a few other issues: > - ret_buf() operates with blk_freelist instead of buf_freelist > - pool->s.bufcount should be set to 0 after buffers are pushed to a free > list > > With fixes below buffer create/destroy works fine: > > diff --git a/platform/linux-generic/include/odp_buffer_pool_internal.h > b/platform/linux-generic/include/odp_buffer_pool_internal.h > index 28ef8f2..07d920a 100644 > --- a/platform/linux-generic/include/odp_buffer_pool_internal.h > +++ b/platform/linux-generic/include/odp_buffer_pool_internal.h > @@ -64,7 +64,6 @@ struct pool_entry_s { > uint8_t *pool_base_addr; > size_t pool_size; > int mdata_stride; > - uint8_t *udata_base_addr; > int buf_udata_size; > int udata_stride; > odp_buffer_hdr_t *buf_freelist; > @@ -89,11 +88,11 @@ extern void *pool_entry_ptr[]; > > #if UINTPTR_MAX == 0xffffffffffffffff > #define odp_at odp_atomic_u64_t > -#define odp_cs(p, o, n) odp_atomic_cmpset_u64((odp_at *)(p), \ > +#define odp_cs(p, o, n) odp_atomic_cmpset_u64((odp_at *)(&p), \ > (uint64_t)(o), (uint64_t)(n)) > #else > #define odp_at odp_atomic_u32_t > -#define odp_cs(p, o, n) odp_atomic_cmpset_u32((odp_at *)(p), \ > +#define odp_cs(p, o, n) odp_atomic_cmpset_u32((odp_at *)(&p), \ > (uint32_t)(o), (uint32_t)(n)) > #endif > > @@ -154,7 +153,7 @@ static inline void ret_buf(struct pool_entry_s *pool, > odp_buffer_hdr_t *buf) > do { > oldhead = odp_ref(pool->buf_freelist); > buf->next = oldhead; > - } while (odp_cs(pool->blk_freelist, oldhead, buf) == 0); > + } while (odp_cs(pool->buf_freelist, oldhead, buf) == 0); > > odp_atomic_dec_u32(&pool->bufcount); > } > diff --git a/platform/linux-generic/odp_buffer_pool.c > b/platform/linux-generic/odp_buffer_pool.c > index 39aed56..a45625f 100644 > --- a/platform/linux-generic/odp_buffer_pool.c > +++ b/platform/linux-generic/odp_buffer_pool.c > @@ -221,11 +221,10 @@ odp_buffer_pool_t odp_buffer_pool_create(const char > *name, > uint8_t *udata_base_addr = pool->s.pool_base_addr + > mdata_size; > uint8_t *block_base_addr = udata_base_addr + udata_size; > > - pool->s.bufcount = 0; > pool->s.buf_freelist = NULL; > pool->s.blk_freelist = NULL; > > - uint8_t *buf = pool->s.udata_base_addr - buf_stride; > + uint8_t *buf = udata_base_addr - buf_stride; > uint8_t *udat = (udata_stride == 0) ? NULL : > block_base_addr - udata_stride; > > @@ -247,6 +246,8 @@ odp_buffer_pool_t odp_buffer_pool_create(const char > *name, > udat -= udata_stride; > } while (buf >= pool->s.pool_base_addr); > > + pool->s.bufcount = 0; > + > /* Form block freelist for pool */ > uint8_t *blk = pool->s.pool_base_addr + pool->s.pool_size - > pool->s.seg_size; > > > > > > > > Bill > > > > On Tue, Nov 11, 2014 at 2:26 AM, Taras Kondratiuk > > <taras.kondratiuk@linaro.org <mailto:taras.kondratiuk@linaro.org>> > wrote: > > > > On 11/10/2014 08:07 PM, Bill Fischofer wrote: > > > Thanks. See inline comments: > > > > > > On Mon, Nov 10, 2014 at 11:37 AM, Taras Kondratiuk > > > <taras.kondratiuk@linaro.org <mailto:taras.kondratiuk@linaro.org> > > <mailto:taras.kondratiuk@linaro.org > > <mailto:taras.kondratiuk@linaro.org>>> wrote: > > > > > > On 11/10/2014 07:10 PM, Bill Fischofer wrote: > > > > > > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org > <mailto:bill.fischofer@linaro.org> > > > <mailto:bill.fischofer@linaro.org > > <mailto:bill.fischofer@linaro.org>>> > > > --- > > > Versions of this patch > > > == > > > v1 - Original (missing files) > > > v2 - Added missing odp_buffer_inlines.h > > > v3 - Added missing odph_tcp.h > > > v4 - (mispublished as v3): Allow NULL arguments to > > > odp_buffer_pool_create() > > > v5 - Correctly handle timer allocs (no blocks), add > > support for > > > buf_init calls > > > on allocation. > > > v6 - Add remaining segment APIs + unmaps + additional > > (void *) > > > casts needed > > > for some C compiler environments, as noted by Taras > > > > > > > > > > > > + > > > + pool->s.bufcount = 0; > > > + pool->s.buf_freelist = NULL; > > > + pool->s.blk_freelist = NULL; > > > > > > > > > Looks like pool->s.blk_freelist and pool->s.buf_freelist are > not > > > updated anywhere. And then dereferenced in ret_buf(). > > > > > > > > > They are initialized in the lines just above your comment. Not > sure > > > what problem you see here. The ret_buf()/ret_blk() routines push > > > buffers/blocks onto their respective freelists, so the contents > > of these > > > pointers become the next pointer for the freed elements. Result > > is that > > > the NULL propagates to the last element of the list, and is > > restored to > > > the list anchor when the last element on that list is popped off > > of it. > > > These are push/pop stack structures. > > > > That's is not how it works right now. Current code does: > > 1. pool->s.blk_freelist = NULL; > > 2. later odp_cs(pool->blk_freelist, oldhead, newhead) -> > > __sync_bool_compare_and_swap(pool->blk_freelist, oldhead, > > newhead) > > 3. __sync_bool_compare_and_swap() dereferences blk_freelist and get > > segfault, because it is NULL. > > > > To get behavior that you have described a *pointer* to blk_freelist > > have to passed in odp_cs instead of blk_freelist itself. > > > > -odp_cs(pool->blk_freelist, oldhead, newhead) > > +odp_cs(&pool->blk_freelist, oldhead, newhead) > > > > > > > > > > > + > > > + uint8_t *buf = pool->s.udata_base_addr - > buf_stride; > > > > > > > > > pool->s.udata_base_addr is not initialized here. > > > Should it be just udata_base_addr? > > > > > > > > > No, it is initialized a few lines above in the statement: > > > uint8_t *udata_base_addr = pool->s.pool_base_addr + mdata_size; > > > uint8_t *block_base_addr = udata_base_addr + udata_size; > > > > That's the point. Local udata_base_addr is initialized, but then > > uninitialized pool->s.udata_base_addr is used, which leads to > segfault. > > > > Maybe you have sent not the latest patch? > > > > > >
On 11/11/2014 03:47 PM, Bill Fischofer wrote: > v3 patch posted to correct 2nd problem (ret_buf returning to > blk_freelist instead of buf_freelist). > > The pool bufcount is decremented as part of ret_buf processing. It > reflects the number of allocated buffers and will return to 0 naturally > when the last allocated buffer is freed, so this should not be set to 0 > unconditionally. It should, because what currently happens during pool creation: - bufcount is set to 0 - new buffers are pushed to a free list via ret_buf() - ret_buf() decrements bufcount -> bufcount underflows So after buffer is created its bufcount is not zero. As a result buffer can't be destroyed.
v4 submitted to correct that. Thanks. On Tue, Nov 11, 2014 at 8:00 AM, Taras Kondratiuk < taras.kondratiuk@linaro.org> wrote: > On 11/11/2014 03:47 PM, Bill Fischofer wrote: > >> v3 patch posted to correct 2nd problem (ret_buf returning to >> blk_freelist instead of buf_freelist). >> >> The pool bufcount is decremented as part of ret_buf processing. It >> reflects the number of allocated buffers and will return to 0 naturally >> when the last allocated buffer is freed, so this should not be set to 0 >> unconditionally. >> > > It should, because what currently happens during pool creation: > - bufcount is set to 0 > - new buffers are pushed to a free list via ret_buf() > - ret_buf() decrements bufcount -> bufcount underflows > > So after buffer is created its bufcount is not zero. As a result buffer > can't be destroyed. >
diff --git a/platform/linux-generic/include/odp_buffer_pool_internal.h b/platform/linux-generic/include/odp_buffer_pool_internal.h index 28ef8f2..07d920a 100644 --- a/platform/linux-generic/include/odp_buffer_pool_internal.h +++ b/platform/linux-generic/include/odp_buffer_pool_internal.h @@ -64,7 +64,6 @@ struct pool_entry_s { uint8_t *pool_base_addr; size_t pool_size; int mdata_stride; - uint8_t *udata_base_addr; int buf_udata_size; int udata_stride; odp_buffer_hdr_t *buf_freelist; @@ -89,11 +88,11 @@ extern void *pool_entry_ptr[]; #if UINTPTR_MAX == 0xffffffffffffffff #define odp_at odp_atomic_u64_t -#define odp_cs(p, o, n) odp_atomic_cmpset_u64((odp_at *)(p), \ +#define odp_cs(p, o, n) odp_atomic_cmpset_u64((odp_at *)(&p), \ (uint64_t)(o), (uint64_t)(n)) #else #define odp_at odp_atomic_u32_t -#define odp_cs(p, o, n) odp_atomic_cmpset_u32((odp_at *)(p), \ +#define odp_cs(p, o, n) odp_atomic_cmpset_u32((odp_at *)(&p), \ (uint32_t)(o), (uint32_t)(n)) #endif @@ -154,7 +153,7 @@ static inline void ret_buf(struct pool_entry_s *pool, odp_buffer_hdr_t *buf) do { oldhead = odp_ref(pool->buf_freelist); buf->next = oldhead; - } while (odp_cs(pool->blk_freelist, oldhead, buf) == 0); + } while (odp_cs(pool->buf_freelist, oldhead, buf) == 0); odp_atomic_dec_u32(&pool->bufcount); } diff --git a/platform/linux-generic/odp_buffer_pool.c b/platform/linux-generic/odp_buffer_pool.c index 39aed56..a45625f 100644 --- a/platform/linux-generic/odp_buffer_pool.c +++ b/platform/linux-generic/odp_buffer_pool.c @@ -221,11 +221,10 @@ odp_buffer_pool_t odp_buffer_pool_create(const char *name, uint8_t *udata_base_addr = pool->s.pool_base_addr + mdata_size; uint8_t *block_base_addr = udata_base_addr + udata_size; - pool->s.bufcount = 0; pool->s.buf_freelist = NULL; pool->s.blk_freelist = NULL; - uint8_t *buf = pool->s.udata_base_addr - buf_stride; + uint8_t *buf = udata_base_addr - buf_stride; uint8_t *udat = (udata_stride == 0) ? NULL : block_base_addr - udata_stride; @@ -247,6 +246,8 @@ odp_buffer_pool_t odp_buffer_pool_create(const char *name, udat -= udata_stride; } while (buf >= pool->s.pool_base_addr); + pool->s.bufcount = 0; + /* Form block freelist for pool */ uint8_t *blk = pool->s.pool_base_addr + pool->s.pool_size - pool->s.seg_size;