Thanks. See inline comments: On Mon, Nov 10, 2014 at 11:37 AM, Taras Kondratiuk < [email protected]> wrote:
> On 11/10/2014 07:10 PM, Bill Fischofer wrote: > >> Signed-off-by: Bill Fischofer <[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. > > + >> + 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; What's happening is that a pool allocates a single large block of shared memory and then carves it up into the individual sections (buffers, udata, and data blocks), and forms elements of those types that can be then manipulated. The freelists for the sections are being created from high addresses to low addreses. So the memory is organized like this: Shared memory block buf[0]...buf[n-1] udata[0]...udata[n-1] blk[0]...blk[m-1] A pool has n bufffers (and possible associated udata areas) and m data blocks. After the above statement udata_base_addr is the address of udata[0], so backing up one buf from that (bufs are of size buf_stride) gives the addres of buf[n-1]. The do/while loop then forms the buffer freelist from buf[n-1] down to buf[0]. The block freelist is formed similarly from blk[m-1] down to blk[0] a bit later in the code. Hope that helps. Bill > > + uint8_t *udat = (udata_stride == 0) ? NULL : >> + block_base_addr - udata_stride; >> + >> + /* Init buffer common header and add to pool buffer >> freelist */ >> + do { >> + odp_buffer_hdr_t *tmp = >> + (odp_buffer_hdr_t *)(void *)buf; >> + tmp->pool_hdl = pool->s.pool_hdl; >> + tmp->size = 0; >> + tmp->type = params->buf_type; >> + tmp->udata_addr = (void *)udat; >> + tmp->udata_size = init_params->udata_size; >> + tmp->segcount = 0; >> + tmp->segsize = pool->s.seg_size; >> + tmp->buf_hdl.handle = >> + odp_buffer_encode_handle((odp_buffer_hdr_t >> *) >> + (void *)buf); >> + ret_buf(&pool->s, tmp); >> + buf -= buf_stride; >> + udat -= udata_stride; >> + } while (buf >= pool->s.pool_base_addr); >> + >> + /* Form block freelist for pool */ >> + uint8_t *blk = pool->s.pool_base_addr + pool->s.pool_size >> - >> + pool->s.seg_size; >> + >> + if (blk_size > 0) >> + do { >> + ret_blk(&pool->s, blk); >> + blk -= pool->s.seg_size; >> + } while (blk >= block_base_addr); >> + >> UNLOCK(&pool->s.lock); >> + >> + pool_hdl = pool->s.pool_hdl; >> + break; >> > >
_______________________________________________ lng-odp mailing list [email protected] http://lists.linaro.org/mailman/listinfo/lng-odp
