On Thu, Apr 07, 2022 at 11:32:12AM +0100, Bruce Richardson wrote:
> On Thu, Apr 07, 2022 at 11:26:53AM +0200, Morten Brørup wrote:
> > > From: Bruce Richardson [mailto:bruce.richard...@intel.com]
> > > Sent: Thursday, 7 April 2022 11.14
> > >
> > > On Thu, Apr 07, 2022 at 11:04:53AM +0200, Morten Brørup wrote:
> > > > > From: Morten Brørup [mailto:m...@smartsharesystems.com]
> > > > > Sent: Wednesday, 2 February 2022 11.34
> > > > >
> > > > > This patch fixes the rte_mempool_do_generic_put() caching
> > > algorithm,
> > > > > which was fundamentally wrong, causing multiple performance issues
> > > when
> > > > > flushing.
> > > > >
> > > >
> > > > [...]
> > > >
> > > > Olivier,
> > > >
> > > > Will you please consider this patch [1] and the other one [2].
> > > >
> > > > The primary bug here is this: When a mempool cache becomes full (i.e.
> > > exceeds the "flush threshold"), and is flushed to the backing ring, it
> > > is still full afterwards; but it should be empty afterwards. It is not
> > > flushed entirely, only the elements exceeding "size" are flushed.
> > > >
> > >
> > > I don't believe it should be flushed entirely, there should always be
> > > some
> > > elements left so that even after flush we can still allocate an
> > > additional
> > > burst. We want to avoid the situation where a flush of all elements is
> > > immediately followed by a refill of new elements. However, we can flush
> > > to
> > > maybe size/2, and improve things. In short, this not emptying is by
> > > design
> > > rather than a bug, though we can look to tweak the behaviour.
> > >
> >
> > I initially agreed with you about flushing to size/2.
> >
> > However, I did think further about it when I wrote the patch, and came to
> > this conclusion: If an application thread repeatedly puts objects into the
> > mempool, and does it so often that the cache overflows (i.e. reaches the
> > flush threshold) and needs to be flushed, it is far more likely that the
> > application thread will continue doing that, rather than start getting
> > objects from the mempool. This speaks for flushing the cache entirely.
> >
> > Both solutions are better than flushing to size, so if there is a
> > preference for keeping some objects in the cache after flushing, I can
> > update the patch accordingly.
> >
>
> Would it be worth looking at adding per-core hinting to the mempool?
> Indicate for a core that it allocates-only, i.e. RX thread, frees-only,
> i.e. TX-thread, or does both alloc and free (the default)? That hint could
> be used only on flush or refill to specify whether to flush all or partial,
> and similarly to refill to max possible or just to size.
>
Actually, taking the idea further, we could always track per-core whether a
core has ever done a flush/refill and use that as the hint instead. It
could even be done in a branch-free manner if we want. For example:
on flush:
keep_entries = (size >> 1) & (never_refills - 1);
which will set the entries to keep to be 0 if we have never had to refill, or
half of size, if the thread has previously done refills.
/Bruce
