Re: [dpdk-dev] [PATCH v4 2/4] mempool: add non-IO flag

[Olivier Matz <olivier.matz@6wind.com>]

Hi Dmitry,

On Wed, Oct 13, 2021 at 02:01:29PM +0300, Dmitry Kozlyuk wrote:
> Mempool is a generic allocator that is not necessarily used for device
> IO operations and its memory for DMA. Add MEMPOOL_F_NON_IO flag to mark
> such mempools automatically if their objects are not contiguous
> or IOVA are not available. Components can inspect this flag
> in order to optimize their memory management.
> Discussion: https://mails.dpdk.org/archives/dev/2021-August/216654.html
> 
> Signed-off-by: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
> Acked-by: Matan Azrad <matan@nvidia.com>
> ---
>  app/test/test_mempool.c                | 76 ++++++++++++++++++++++++++
>  doc/guides/rel_notes/release_21_11.rst |  3 +
>  lib/mempool/rte_mempool.c              |  2 +
>  lib/mempool/rte_mempool.h              |  5 ++
>  4 files changed, 86 insertions(+)
> 
> diff --git a/app/test/test_mempool.c b/app/test/test_mempool.c
> index bc0cc9ed48..15146dd737 100644
> --- a/app/test/test_mempool.c
> +++ b/app/test/test_mempool.c
> @@ -672,6 +672,74 @@ test_mempool_events_safety(void)
>  	return 0;
>  }
>  
> +static int
> +test_mempool_flag_non_io_set_when_no_iova_contig_set(void)
> +{
> +	struct rte_mempool *mp;
> +	int ret;
> +
> +	mp = rte_mempool_create_empty("empty", MEMPOOL_SIZE,
> +				      MEMPOOL_ELT_SIZE, 0, 0,
> +				      SOCKET_ID_ANY, MEMPOOL_F_NO_IOVA_CONTIG);
> +	RTE_TEST_ASSERT_NOT_NULL(mp, "Cannot create mempool: %s",
> +				 rte_strerror(rte_errno));
> +	rte_mempool_set_ops_byname(mp, rte_mbuf_best_mempool_ops(), NULL);
> +	ret = rte_mempool_populate_default(mp);
> +	RTE_TEST_ASSERT(ret > 0, "Failed to populate mempool: %s",
> +			rte_strerror(rte_errno));
> +	RTE_TEST_ASSERT(mp->flags & MEMPOOL_F_NON_IO,
> +			"NON_IO flag is not set when NO_IOVA_CONTIG is set");
> +	rte_mempool_free(mp);
> +	return 0;
> +}

One comment that also applies to the previous patch. Using
RTE_TEST_ASSERT_*() is convenient to test a condition, display an error
message and return on error in one operation. But here it can cause a
leak on test failure.

I don't know what is the best approach to solve the issue. Having
equivalent test macros that do "goto fail" instead of "return -1" would
help here. I mean something like:
  RTE_TEST_ASSERT_GOTO_*(cond, label, fmt, ...)

What do you think?