Re: Bug in rte_mempool_do_generic_get?

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

On Mon, Feb 27, 2023 at 10:48:09AM +0100, Morten Brørup wrote:
> > From: Olivier Matz [mailto:olivier.matz@6wind.com]
> > Sent: Monday, 27 February 2023 10.10
> > 
> > Hi,
> > 
> > On Sun, Feb 26, 2023 at 11:45:48AM +0100, Morten Brørup wrote:
> > > > From: Harris, James R [mailto:james.r.harris@intel.com]
> > > > Sent: Friday, 24 February 2023 17.43
> > > >
> > > > > On Feb 24, 2023, at 6:56 AM, Honnappa Nagarahalli
> > > > <Honnappa.Nagarahalli@arm.com> wrote:
> > > > >
> > > > >> From: Morten Brørup <mb@smartsharesystems.com>
> > > > >> Sent: Friday, February 24, 2023 6:13 AM
> > > > >> To: Harris, James R <james.r.harris@intel.com>; dev@dpdk.org
> > > > >> Subject: RE: Bug in rte_mempool_do_generic_get?
> > > > >>
> > > > >>> If you have a mempool with 2048 objects, shouldn't 4 cores each be
> > > > able to do a 512 buffer bulk get, regardless of the configured cache
> > > > size?
> > > > >>
> > > > >> No, the scenario you described above is the expected behavior. I
> > > > think it is
> > > > >> documented somewhere that objects in the caches are unavailable for
> > > > other
> > > > >> cores, but now I cannot find where this is documented.
> > > > >>
> > > >
> > > > Thanks Morten.
> > > >
> > > > Yeah, I think it is documented somewhere, but I also couldn’t find it.
> > > > I was aware of cores not being able to allocate from another core’s
> > > > cache.  My surprise was that in a pristine new mempool, that 4 cores
> > > > could not each do one initial 512 buffer bulk get.  But I also see that
> > > > even before the a2833ecc5 patch, the cache would get populated on gets
> > > > less than cache size, in addition to the buffers requested by the user.
> > > > So if cache size is 256, and bulk get is for 128 buffers, it pulls 384
> > > > buffers from backing pool - 128 for the caller, another 256 to prefill
> > > > the cache.  Your patch makes this cache filling consistent between less-
> > > > than-cache-size and greater-than-or-equal-to-cache-size cases.
> > >
> > > Yeah... I have tried hard to clean up some of the mess introduced by the
> > <insert curse words here> patch [1] that increased the effective cache size by
> > factor 1.5.
> > >
> > > [1]
> > http://git.dpdk.org/dpdk/commit/lib/librte_mempool/rte_mempool.h?id=ea5dd2744b
> > 90b330f07fd10f327ab99ef55c7266
> > >
> > > E.g., somewhat related to your use case, I have argued against the design
> > goal of trying to keep the mempool caches full at all times, but have not yet
> > been able to convince the community against it.
> > >
> > > Overall, this is one of the absolute core DPDK libraries, so the support for
> > the changes I would like to see is near zero - even minor adjustments are
> > considered very high risk, which I must admit has quite a lot of merit.
> > >
> > > So instead of fixing the library's implementation to make it behave as
> > reasonably expected according to its documentation, the library's
> > implementation is considered the reference. And as a consequence, the
> > library's internals, such as the cache size multiplier, is getting promoted to
> > be part of the public API, e.g. for applications to implement mempool sizing
> > calculations like the one below.
> > >
> > > I recall running into problems once myself, when I had sized a mempool with
> > a specific cache size for a specific number of cores, because 50 % additional
> > objects got stuck in the caches. If you ask me, this bug is so fundamental
> > that it should be fixed at the root, not by trying to document the weird
> > behavior of allocating 50 % more than specified.
> > 
> > I think we should document it: even in the case the 1.5 factor is
> > removed, it is helpful to document that a user needs to reserve more
> > objects when using a cache, to ensure that all cores can allocate their
> > objects.
> > 
> > Morten, what would you think about this change below?
> 
> A big improvement! Only a few comments, inline below.
> 
> > 
> >   --- a/doc/guides/prog_guide/mempool_lib.rst
> >   +++ b/doc/guides/prog_guide/mempool_lib.rst
> >   @@ -89,9 +89,13 @@ In this way, each core has full access to its own cache
> > (with locks) of free obj
> >    only when the cache fills does the core need to shuffle some of the free
> > objects back to the pools ring or
> >    obtain more objects when the cache is empty.
> > 
> >   -While this may mean a number of buffers may sit idle on some core's cache,
> >   +While this may mean a number of buffers may sit idle on some core's cache
> > (up to ``1.5 * cache_length``),
> 
> Typo: cache_length -> cache_size
> 
> Looking at this, I noticed another detail: Chapter 10.4 uses the term "buffers", but it should use the term "objects", because a mempool may hold other objects than packet buffers. (Using the term "packet buffers" in the example in chapter 10.3 is correct.)
> 
> >    the speed at which a core can access its own cache for a specific memory
> > pool without locks provides performance gains.
> > 
> >   +However, to ensure that all cores can get objects when a cache is used, it
> > is required to allocate
> >   +more objects: if N objects are needed, allocate a mempool with ``N +
> > (number_of_active_cores *
> >   +cache_size * 1.5)`` objects.
> >   +
> 
> Perhaps add here that the factor 1.5 stems from the cache being allowed to exceed its configured size by 50 %.
> 
> >    The cache is composed of a small, per-core table of pointers and its length
> > (used as a stack).
> >    This internal cache can be enabled or disabled at creation of the pool.
> > 
> 
> If something similar is added to the description of the "cache_size" parameter to the "rte_mempool_create()" function, the most obvious places to look for documentation are covered.
> 
> A lengthy description might be redundant here, so perhaps only mention that the actual cache size is 1.5 * cache_size, because the cache is allowed to exceed its configured size by 50 %.

Thanks for the quick feedback, I'll submit a patch for this.

> 
> > 
> > >
> > > >
> > > > >> Furthermore, since the effective per-core cache size is 1.5 *
> > > > configured cache
> > > > >> size, a configured cache size of 256 may leave up to 384 objects in
> > > > each per-
> > > > >> core cache.
> > > > >>
> > > > >> With 4 cores, you can expect up to 3 * 384 = 1152 objects sitting in
> > > > the
> > > > >> caches of other cores. If you want to be able to pull 512 objects
> > > > with each
> > > > >> core, the pool size should be 4 * 512 + 1152 objects.
> > > > > May be we should document this in mempool library?
> > > > >
> > > >
> > > > Maybe.  But this case I described here is a bit wonky - SPDK should
> > > > never have been specifying a non-zero cache in this case.  We only
> > > > noticed this change in behavior because we were creating the mempool
> > > > with a cache when we shouldn’t have.
> > >
> > > So, looking at the positive side, this regression revealed a "bug" in SPDK.
> > ;-)
> > >
> > > PS: I assume that you are aware that mempools generally perform better with
> > cache, so I assume that you know what you are doing when you say that you
> > don't need the cache.
> > >
> > > PPS: Sorry about venting here. If nothing else, I hope it had some
> > entertainment value. :-)
> > >
>