[v2] Change invalid buffer pool handle to zero

For consistency and easier debugging, use zero as the value of
an invalid pool handle (in linux-generic).

Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org>
---
 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(-)

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 <petri.savolainen@linaro.org>
> ---
>  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
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp