On 2014-08-20 11:11, Petri Savolainen wrote:
> For consistency and easier debugging, use zero as the value of
> an invalid pool handle (in linux-generic).
>
> Signed-off-by: Petri Savolainen <[email protected]>
> ---
> include/odp_buffer_pool.h | 2 +-
> .../linux-generic/include/odp_buffer_internal.h | 4 +-
> .../include/odp_buffer_pool_internal.h | 4 +-
> platform/linux-generic/odp_buffer_pool.c | 54
> ++++++++++++++--------
> 4 files changed, 41 insertions(+), 23 deletions(-)
>
> diff --git a/include/odp_buffer_pool.h b/include/odp_buffer_pool.h
> index 26d9f14..fe88898 100644
> --- a/include/odp_buffer_pool.h
> +++ b/include/odp_buffer_pool.h
> @@ -27,7 +27,7 @@ extern "C" {
> #define ODP_BUFFER_POOL_NAME_LEN 32
>
> /** Invalid buffer pool */
> -#define ODP_BUFFER_POOL_INVALID (0xffffffff)
> +#define ODP_BUFFER_POOL_INVALID 0
>
> /** ODP buffer pool */
> typedef uint32_t odp_buffer_pool_t;
> diff --git a/platform/linux-generic/include/odp_buffer_internal.h
> b/platform/linux-generic/include/odp_buffer_internal.h
> index bffd0dd..56f9759 100644
> --- a/platform/linux-generic/include/odp_buffer_internal.h
> +++ b/platform/linux-generic/include/odp_buffer_internal.h
> @@ -48,7 +48,7 @@ typedef union odp_buffer_bits_t {
> odp_buffer_t handle;
>
> struct {
> - uint32_t pool:ODP_BUFFER_POOL_BITS;
> + uint32_t pool_id:ODP_BUFFER_POOL_BITS; /* pool index */
Confusing, variable named pool_id and comment says "pool index"
Maybe the comment should say "pool identifier"
> uint32_t index:ODP_BUFFER_INDEX_BITS;
> };
> } odp_buffer_bits_t;
> @@ -91,7 +91,7 @@ typedef struct odp_buffer_hdr_t {
> odp_atomic_int_t ref_count; /* reference count */
> odp_buffer_scatter_t scatter; /* Scatter/gather list */
> int type; /* type of next header */
> - odp_buffer_pool_t pool; /* buffer pool */
> + odp_buffer_pool_t pool; /* buffer pool handle */
If this variable would be called pool_hdl, the comment would not
be necessary and you don't have to go back to the definition to see what
type it is.
>
> } odp_buffer_hdr_t;
>
> diff --git a/platform/linux-generic/include/odp_buffer_pool_internal.h
> b/platform/linux-generic/include/odp_buffer_pool_internal.h
> index 1c0a9fc..7e3f0c2 100644
> --- a/platform/linux-generic/include/odp_buffer_pool_internal.h
> +++ b/platform/linux-generic/include/odp_buffer_pool_internal.h
> @@ -68,7 +68,7 @@ struct pool_entry_s {
> extern void *pool_entry_ptr[];
>
>
> -static inline void *get_pool_entry(odp_buffer_pool_t pool_id)
> +static inline void *get_pool_entry(uint32_t pool_id)
> {
> return pool_entry_ptr[pool_id];
> }
> @@ -83,7 +83,7 @@ static inline odp_buffer_hdr_t *odp_buf_to_hdr(odp_buffer_t
> buf)
> odp_buffer_hdr_t *hdr;
>
> handle.u32 = buf;
> - pool_id = handle.pool;
> + pool_id = handle.pool_id;
> index = handle.index;
>
> #ifdef POOL_ERROR_CHECK
> diff --git a/platform/linux-generic/odp_buffer_pool.c
> b/platform/linux-generic/odp_buffer_pool.c
> index a48781a..82aab3a 100644
> --- a/platform/linux-generic/odp_buffer_pool.c
> +++ b/platform/linux-generic/odp_buffer_pool.c
> @@ -80,25 +80,40 @@ void *pool_entry_ptr[ODP_CONFIG_BUFFER_POOLS];
> static __thread odp_buffer_chunk_hdr_t *local_chunk[ODP_CONFIG_BUFFER_POOLS];
>
>
> +static inline odp_buffer_pool_t pool_index_to_handle(uint32_t pool_id)
> +{
> + return pool_id + 1;
> +}
> +
> +
> +static inline uint32_t pool_handle_to_index(odp_buffer_pool_t pool_hdl)
> +{
> + return pool_hdl - 1;
> +}
> +
> +
> static inline void set_handle(odp_buffer_hdr_t *hdr,
> pool_entry_t *pool, uint32_t index)
> {
> - uint32_t pool_id = (uint32_t) pool->s.pool;
> + odp_buffer_pool_t pool_hdl = pool->s.pool;
> + uint32_t pool_id = pool_handle_to_index(pool_hdl);
>
> - if (pool_id > ODP_CONFIG_BUFFER_POOLS)
> - ODP_ERR("set_handle: Bad pool id\n");
> + if (pool_id >= ODP_CONFIG_BUFFER_POOLS) {
> + ODP_ERR("set_handle: Bad pool handle %u\n", pool_hdl);
> + exit(0);
> + }
>
> if (index > ODP_BUFFER_MAX_INDEX)
> ODP_ERR("set_handle: Bad buffer index\n");
>
> - hdr->handle.pool = pool_id;
> - hdr->handle.index = index;
> + hdr->handle.pool_id = pool_id;
> + hdr->handle.index = index;
> }
>
>
> int odp_buffer_pool_init_global(void)
> {
> - odp_buffer_pool_t i;
> + uint32_t i;
>
> pool_tbl = odp_shm_reserve("odp_buffer_pools",
> sizeof(pool_table_t),
> @@ -113,7 +128,7 @@ int odp_buffer_pool_init_global(void)
> /* init locks */
> pool_entry_t *pool = &pool_tbl->pool[i];
> LOCK_INIT(&pool->s.lock);
> - pool->s.pool = i;
> + pool->s.pool = pool_index_to_handle(i);
What is pool here?
Should we be more specific when using pool as a variable (we use pool a lot
and it has different meanings), maybe something like this:
pool_entry->s.pool_hdl = pool_index_to_handel(i);
Cheers,
Anders
>
> pool_entry_ptr[i] = pool;
> }
> @@ -363,9 +378,9 @@ odp_buffer_pool_t odp_buffer_pool_create(const char *name,
> size_t buf_size, size_t buf_align,
> int buf_type)
> {
> - odp_buffer_pool_t i;
> + odp_buffer_pool_t pool_hdl = ODP_BUFFER_POOL_INVALID;
> pool_entry_t *pool;
> - odp_buffer_pool_t pool_id = ODP_BUFFER_POOL_INVALID;
> + uint32_t i;
>
> for (i = 0; i < ODP_CONFIG_BUFFER_POOLS; i++) {
> pool = get_pool_entry(i);
> @@ -388,20 +403,20 @@ odp_buffer_pool_t odp_buffer_pool_create(const char
> *name,
>
> UNLOCK(&pool->s.lock);
>
> - pool_id = i;
> + pool_hdl = pool->s.pool;
> break;
> }
>
> UNLOCK(&pool->s.lock);
> }
>
> - return pool_id;
> + return pool_hdl;
> }
>
>
> odp_buffer_pool_t odp_buffer_pool_lookup(const char *name)
> {
> - odp_buffer_pool_t i;
> + uint32_t i;
> pool_entry_t *pool;
>
> for (i = 0; i < ODP_CONFIG_BUFFER_POOLS; i++) {
> @@ -411,7 +426,7 @@ odp_buffer_pool_t odp_buffer_pool_lookup(const char *name)
> if (strcmp(name, pool->s.name) == 0) {
> /* found it */
> UNLOCK(&pool->s.lock);
> - return i;
> + return pool->s.pool;
> }
> UNLOCK(&pool->s.lock);
> }
> @@ -420,11 +435,12 @@ odp_buffer_pool_t odp_buffer_pool_lookup(const char
> *name)
> }
>
>
> -odp_buffer_t odp_buffer_alloc(odp_buffer_pool_t pool_id)
> +odp_buffer_t odp_buffer_alloc(odp_buffer_pool_t pool_hdl)
> {
> pool_entry_t *pool;
> odp_buffer_chunk_hdr_t *chunk;
> odp_buffer_bits_t handle;
> + uint32_t pool_id = pool_handle_to_index(pool_hdl);
>
> pool = get_pool_entry(pool_id);
> chunk = local_chunk[pool_id];
> @@ -462,12 +478,12 @@ odp_buffer_t odp_buffer_alloc(odp_buffer_pool_t pool_id)
> void odp_buffer_free(odp_buffer_t buf)
> {
> odp_buffer_hdr_t *hdr;
> - odp_buffer_pool_t pool_id;
> + uint32_t pool_id;
> pool_entry_t *pool;
> odp_buffer_chunk_hdr_t *chunk_hdr;
>
> hdr = odp_buf_to_hdr(buf);
> - pool_id = hdr->pool;
> + pool_id = pool_handle_to_index(hdr->pool);
> pool = get_pool_entry(pool_id);
> chunk_hdr = local_chunk[pool_id];
>
> @@ -500,13 +516,15 @@ odp_buffer_pool_t odp_buffer_pool(odp_buffer_t buf)
> }
>
>
> -void odp_buffer_pool_print(odp_buffer_pool_t pool_id)
> +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 = get_pool_entry(pool_id);
> + pool_id = pool_handle_to_index(pool_hdl);
> + pool = get_pool_entry(pool_id);
>
> printf("Pool info\n");
> printf("---------\n");
> --
> 2.1.0
>
>
> _______________________________________________
> lng-odp mailing list
> [email protected]
> http://lists.linaro.org/mailman/listinfo/lng-odp
_______________________________________________
lng-odp mailing list
[email protected]
http://lists.linaro.org/mailman/listinfo/lng-odp
