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 < [email protected]> 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 > > <[email protected] <mailto:[email protected]>> > 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 > > > <[email protected] <mailto:[email protected]> > > <mailto:[email protected] > > <mailto:[email protected]>>> wrote: > > > > > > On 11/10/2014 07:10 PM, Bill Fischofer wrote: > > > > > > Signed-off-by: Bill Fischofer <[email protected] > <mailto:[email protected]> > > > <mailto:[email protected] > > <mailto:[email protected]>>> > > > --- > > > 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? > > > > > >
_______________________________________________ lng-odp mailing list [email protected] http://lists.linaro.org/mailman/listinfo/lng-odp
