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 <dkozl...@nvidia.com>
> Signed-off-by: David Marchand <david.march...@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).
