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
