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

[Dmitry Kozlyuk <dkozlyuk@oss.nvidia.com>]

> -----Original Message-----
> From: Olivier Matz <olivier.matz@6wind.com>
> Sent: 15 октября 2021 г. 16:43
> To: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
> Cc: dev@dpdk.org; Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; Matan
> Azrad <matan@nvidia.com>
> Subject: Re: [PATCH v4 2/4] mempool: add non-IO flag
> 
> External email: Use caution opening links or attachments
> 
> 
> On Fri, Oct 15, 2021 at 01:27:59PM +0000, Dmitry Kozlyuk wrote:
> > > [...]
> > > > +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?
> >
> > This can work with existing macros:
> >
> >       #define TEST_TRACE_FAILURE(...) goto fail
> >
> > Because of "trace" in the name it looks a bit like a hijacking.
> > Probably the macro should be named TEST_HANDLE_FAILURE
> > to suggest broader usages than just tracing,
> > but for now it looks the most neat way.
> 
> That would work for me.

Did so in v9.

> What about introducing another macro for this usage, that would
> be "return -1" by default and that can be overridden?

I like this suggestion by itself.
While implementing the solution with RTE_TEST_TRACE_FAILURE()
I didn't like the hustle with #ifdef/#pragma push/pop_macro.
At least some of them could be hidden, need to play with macros
before suggesting something clean.