On Wed, Aug 16, 2023 at 03:35:47PM +0200, Morten Brørup wrote: > > From: Olivier Matz [mailto:olivier.m...@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.march...@redhat.com> wrote: > > > > > > > > > > On Wed, Aug 16, 2023 at 1:02 PM Bruce Richardson > > > > > <bruce.richard...@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. > Yep. David has kindly done up a patch to fix this, and I'm rolling it into the v6 of this set.
/Bruce
