Re: [dpdk-dev] [PATCH] mempool: accept user flags only

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

On 10/18/21 11:26 AM, David Marchand wrote:
> As reported by Dmitry, MEMPOOL_F_POOL_CREATED is a flag only manipulated
> internally.
> This flag is not supposed to be requested from an application and would
> probably result in an incorrect behavior if an application did pass it.
> 
> Other internal flags may be introduced later.
> 
> Rework the check and export a mask of valid user flags for use in the
> unit test.
> 
> Fixes: b240af8b10f9 ("mempool: enforce valid flags at creation")
> 
> Reported-by: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>  app/test/test_mempool.c   |  8 ++++----
>  lib/mempool/rte_mempool.c | 11 ++---------
>  lib/mempool/rte_mempool.h |  9 +++++++++
>  3 files changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/app/test/test_mempool.c b/app/test/test_mempool.c
> index 66bc8d86b7..ba05742f76 100644
> --- a/app/test/test_mempool.c
> +++ b/app/test/test_mempool.c
> @@ -205,15 +205,15 @@ static int test_mempool_creation_with_exceeded_cache_size(void)
>  	return 0;
>  }
>  
> -static int test_mempool_creation_with_unknown_flag(void)
> +static int test_mempool_creation_with_invalid_flags(void)
>  {
>  	struct rte_mempool *mp_cov;
>  
> -	mp_cov = rte_mempool_create("test_mempool_unknown_flag", MEMPOOL_SIZE,
> +	mp_cov = rte_mempool_create("test_mempool_invalid_flags", MEMPOOL_SIZE,
>  		MEMPOOL_ELT_SIZE, 0, 0,
>  		NULL, NULL,
>  		NULL, NULL,
> -		SOCKET_ID_ANY, MEMPOOL_F_NO_IOVA_CONTIG << 1);
> +		SOCKET_ID_ANY, ~MEMPOOL_VALID_USER_FLAGS);
>  
>  	if (mp_cov != NULL) {
>  		rte_mempool_free(mp_cov);
> @@ -653,7 +653,7 @@ test_mempool(void)
>  	if (test_mempool_creation_with_exceeded_cache_size() < 0)
>  		GOTO_ERR(ret, err);
>  
> -	if (test_mempool_creation_with_unknown_flag() < 0)
> +	if (test_mempool_creation_with_invalid_flags() < 0)
>  		GOTO_ERR(ret, err);
>  
>  	if (test_mempool_same_name_twice_creation() < 0)
> diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
> index 607419ccaf..7f92d79c89 100644
> --- a/lib/mempool/rte_mempool.c
> +++ b/lib/mempool/rte_mempool.c
> @@ -777,13 +777,6 @@ rte_mempool_cache_free(struct rte_mempool_cache *cache)
>  	rte_free(cache);
>  }
>  
> -#define MEMPOOL_KNOWN_FLAGS (MEMPOOL_F_NO_SPREAD \
> -	| MEMPOOL_F_NO_CACHE_ALIGN \
> -	| MEMPOOL_F_SP_PUT \
> -	| MEMPOOL_F_SC_GET \
> -	| MEMPOOL_F_POOL_CREATED \
> -	| MEMPOOL_F_NO_IOVA_CONTIG \
> -	)
>  /* create an empty mempool */
>  struct rte_mempool *
>  rte_mempool_create_empty(const char *name, unsigned n, unsigned elt_size,
> @@ -828,8 +821,8 @@ rte_mempool_create_empty(const char *name, unsigned n, unsigned elt_size,
>  		return NULL;
>  	}
>  
> -	/* enforce no unknown flag is passed by the application */
> -	if ((flags & ~MEMPOOL_KNOWN_FLAGS) != 0) {
> +	/* enforce only user flags are passed by the application */
> +	if ((flags & ~MEMPOOL_VALID_USER_FLAGS) != 0) {
>  		rte_errno = EINVAL;
>  		return NULL;
>  	}
> diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h
> index 88bcbc51ef..d2bf2843f7 100644
> --- a/lib/mempool/rte_mempool.h
> +++ b/lib/mempool/rte_mempool.h
> @@ -258,6 +258,15 @@ struct rte_mempool {
>  #define MEMPOOL_F_POOL_CREATED   0x0010 /**< Internal: pool is created. */
>  #define MEMPOOL_F_NO_IOVA_CONTIG 0x0020 /**< Don't need IOVA contiguous objs. */
>  
> +/**
> + * This macro lists all the mempool flags an application may request.
> + */
> +#define MEMPOOL_VALID_USER_FLAGS (MEMPOOL_F_NO_SPREAD \

I think RTE_ prefix is missing here since it is in a public
header now.

> +	| MEMPOOL_F_NO_CACHE_ALIGN \
> +	| MEMPOOL_F_SP_PUT \
> +	| MEMPOOL_F_SC_GET \
> +	| MEMPOOL_F_NO_IOVA_CONTIG \
> +	)
>  /**
>   * @internal When debug is enabled, store some statistics.
>   *
> 

Should we make a patch to add defines with RTE_ prefix and
add a deprecation for old flags without RTE_ prefix?

Olivier, what do you think? If you have time to care about it,
it would be great. (I'm unfamiliar with coccinelle yet).