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]>> wrote:
>
> On 11/10/2014 07:10 PM, Bill Fischofer wrote:
>
> Signed-off-by: Bill Fischofer <[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
