RE: [PATCH v5 05/10] app/test: define unit tests suites based on test macros

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> Sent: Wednesday, 16 August 2023 15.17
> 
> Hi,
> 
> On Wed, Aug 16, 2023 at 01:33:46PM +0100, Bruce Richardson wrote:
> > On Wed, Aug 16, 2023 at 01:40:41PM +0200, David Marchand wrote:
> > > On Wed, Aug 16, 2023 at 1:15 PM David Marchand
> > > <david.marchand@redhat.com> wrote:
> > > >
> > > > On Wed, Aug 16, 2023 at 1:02 PM Bruce Richardson
> > > > <bruce.richardson@intel.com> wrote:
> > > > > These lines here seem to be exposing a bug in the mempool unit tests
> for
> > > > > shared builds, which is why we have a CI failure.
> > > > >
> > > > > The mempool unit tests use the mempool "create_empty" API, and then
> call
> > > > > the populate APIs to finish setting up the mempool. However, the
> > > > > create_empty API does not explicitly configure a particular set of
> mempool
> > > > > ops for the new mempool, leaving the ops field at 0. This means that
> unless
> > > > > the app explicitly sets other ops, the mempool will use the ops from
> > > > > whatever driver registers itself first. This occurs even when the
> driver is
> > > > > unsuitable, e.g. on my Intel system, the dpaa2 is first on the list,
> > > > > leading to failures in setting up and using the mempool.
> > > >
> > > > Hum, it sounds like a bug to me.
> > > > The dpaa2 driver should not be registered as the default (or here,
> > > > default platform) mempool.
> > > > Other mempool drivers ensure that required hw is available, I guess
> > > > dpaa2 is missing some check.
> > >
> > > Ok, re-reading your last comment, I agree it looks like an issue in
> > > the unit test itself.
> > > Copying Olivier.
> > >
> > No, I think it's not a bug in the test, but rather in the mempool.
> > Unfortunately, I think the concept of the "default" driver for mempools
> > seems to exist only when using the mbuf library to create mempools. Even
> > then, the default mempool is different from what the first entry in the
> > list is. That's the fundamental issue here, we are using the zero-eth entry
> > in the ops list, rather than a default one.
> 
> Yes, Bruce is right.
> 
> As discussed off-list with David, moving rte_mempool_set_ops_byname() from
> rte_mempool_create() to rte_mempool_create_empty() would ensure that the ring
> driver is the default (and taking flags into account).

I took a look at the mempool library source code, and reached the same conclusion.

Your suggested fix is also supported by the documentation of the "flags" parameter to rte_mempool_create_empty(), which - by referring to the "flags" parameter to rte_mempool_create() - says that the mempool ops will be set depending on the RTE_MEMPOOL_F_[S|M][P_PUT|C_GET] flags.

It should probably be flagged as a bug and backported to stable releases.