Thanks. Looks like there are a number of uninitialized fields in pool_create. Will fix in next version of path.
udata_base_addr is not referenced from the pool. Individual udata_addrs are set in the bufs and are zero for buffers without user metadata. buffer_alloc() is an internal function. len=0 is conventional and simply means no explicit length specified, so default behavior (one segment) is allocated for the buffer. hr/tr are only functional for packets. Can add the fail check in the setters. odp_buffer_pool_print() is not an external API. It's legacy debug code. Bill On Wed, Nov 12, 2014 at 11:27 AM, Taras Kondratiuk < [email protected]> wrote: > On 11/12/2014 02:59 AM, Bill Fischofer wrote: > > [snip] > > +static inline odp_buffer_hdr_t *validate_buf(odp_buffer_t buf) >> +{ >> + odp_buffer_bits_t handle; >> + odp_buffer_hdr_t *buf_hdr; >> + handle.u32 = buf; >> + >> + /* For buffer handles, segment index must be 0 */ >> + if (handle.seg != 0) >> + return NULL; >> + >> + pool_entry_t *pool = odp_pool_to_entry(handle.pool_id); >> + >> + /* If pool not created, handle is invalid */ >> + if (pool->s.shm == ODP_SHM_INVALID) >> + return NULL; >> + >> + /* A valid buffer index must be on stride, and must be in range */ >> + if ((handle.index % pool->s.mdata_stride != 0) || >> + ((uint32_t)(handle.index / pool->s.mdata_stride) >= >> + pool->s.num_bufs)) >> + return NULL; >> > > pool->s.mdata_stride is not initialized. > > [snip] > > -odp_buffer_pool_t odp_buffer_pool_create(const char *name, >> - void *base_addr, uint64_t size, >> - size_t buf_size, size_t >> buf_align, >> - int buf_type) >> -{ >> - odp_buffer_pool_t pool_hdl = ODP_BUFFER_POOL_INVALID; >> - pool_entry_t *pool; >> - uint32_t i; >> >> for (i = 0; i < ODP_CONFIG_BUFFER_POOLS; i++) { >> pool = get_pool_entry(i); >> >> LOCK(&pool->s.lock); >> + if (pool->s.shm != ODP_SHM_INVALID) { >> + UNLOCK(&pool->s.lock); >> + continue; >> + } >> >> - if (pool->s.buf_base == 0) { >> - /* found free pool */ >> + /* found free pool */ >> + size_t block_size, mdata_size, udata_size; >> >> - strncpy(pool->s.name, name, >> - ODP_BUFFER_POOL_NAME_LEN - 1); >> - pool->s.name[ODP_BUFFER_POOL_NAME_LEN - 1] = 0; >> - pool->s.pool_base_addr = base_addr; >> - pool->s.pool_size = size; >> - pool->s.user_size = buf_size; >> - pool->s.user_align = buf_align; >> - pool->s.buf_type = buf_type; >> + strncpy(pool->s.name, name, >> + ODP_BUFFER_POOL_NAME_LEN - 1); >> + pool->s.name[ODP_BUFFER_POOL_NAME_LEN - 1] = 0; >> >> - link_bufs(pool); >> + pool->s.params = *params; >> + pool->s.init_params = *init_params; >> >> - UNLOCK(&pool->s.lock); >> + mdata_size = params->buf_num * buf_stride; >> + udata_size = params->buf_num * udata_stride; >> + block_size = params->buf_num * blk_size; >> + >> + pool->s.pool_size = ODP_PAGE_SIZE_ROUNDUP(mdata_size + >> + udata_size + >> + block_size); >> >> - pool_hdl = pool->s.pool_hdl; >> - break; >> + pool->s.shm = odp_shm_reserve(pool->s.name, >> pool->s.pool_size, >> + ODP_PAGE_SIZE, 0); >> + if (pool->s.shm == ODP_SHM_INVALID) { >> + UNLOCK(&pool->s.lock); >> + return ODP_BUFFER_POOL_INVALID; >> } >> >> + pool->s.pool_base_addr = (uint8_t >> *)odp_shm_addr(pool->s.shm); >> + pool->s.flags.unsegmented = unsegmented; >> + pool->s.seg_size = unsegmented ? >> + blk_size : ODP_CONFIG_BUF_SEG_SIZE; >> + 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 *udat = (udata_stride == 0) ? NULL : >> + block_base_addr - udata_stride; >> > > Initialized pool->s.udata_base_addr is still here. > > [snip] > > +odp_buffer_t odp_buffer_alloc(odp_buffer_pool_t pool_hdl) >> +{ >> + return buffer_alloc(pool_hdl, 0); >> > > Here buffer is allocated with 0 size, but my understanding was that > allocated buffer size should be equal to value passed to > odp_buffer_pool_create(). > > As far as I remember we talked about zero _packet_ size after > packet allocation, but not buffer size. > > Also odp_buffer_hdr_t.size field is not initialized during buffer > allocation. > > BTW head/tailroom are added at buffer pool level, but they are a > feature of PACKET buffer type and doesn't make sense for RAW buffers. > IMO odp_buffer_pool_set_headroom() should return failure for pools, > that are not PACKET. > > +} >> + >> +void odp_buffer_free(odp_buffer_t buf) >> +{ >> + odp_buffer_hdr_t *hdr = odp_buf_to_hdr(buf); >> + pool_entry_t *pool = odp_buf_to_pool(hdr); >> + ret_buf(&pool->s, hdr); >> +} >> >> void odp_buffer_pool_print(odp_buffer_pool_t pool_hdl) >> { >> pool_entry_t *pool; >> - odp_buffer_chunk_hdr_t *chunk_hdr; >> - uint32_t i; >> uint32_t pool_id; >> >> pool_id = pool_handle_to_index(pool_hdl); >> @@ -528,47 +460,4 @@ void odp_buffer_pool_print(odp_buffer_pool_t >> pool_hdl) >> printf(" pool %i\n", pool->s.pool_hdl); >> printf(" name %s\n", pool->s.name); >> printf(" pool base %p\n", pool->s.pool_base_addr); >> - printf(" buf base 0x%"PRIxPTR"\n", pool->s.buf_base); >> - printf(" pool size 0x%"PRIx64"\n", pool->s.pool_size); >> - printf(" buf size %zu\n", pool->s.user_size); >> - printf(" buf align %zu\n", pool->s.user_align); >> - printf(" hdr size %zu\n", pool->s.hdr_size); >> - printf(" alloc size %zu\n", pool->s.buf_size); >> - printf(" offset to hdr %zu\n", pool->s.buf_offset); >> - printf(" num bufs %"PRIu64"\n", pool->s.num_bufs); >> - printf(" free bufs %"PRIu64"\n", pool->s.free_bufs); >> > > Should all odp_*_print() functions use ODP_LOG() instead of direct > printf? >
_______________________________________________ lng-odp mailing list [email protected] http://lists.linaro.org/mailman/listinfo/lng-odp
