Re: [PATCH v6 3/4] mempool: fix cache flushing algorithm

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

Hi Morten, Andrew,

On Sun, Oct 09, 2022 at 05:08:39PM +0200, Morten Brørup wrote:
> > From: Andrew Rybchenko [mailto:andrew.rybchenko@oktetlabs.ru]
> > Sent: Sunday, 9 October 2022 16.52
> > 
> > On 10/9/22 17:31, Morten Brørup wrote:
> > >> From: Andrew Rybchenko [mailto:andrew.rybchenko@oktetlabs.ru]
> > >> Sent: Sunday, 9 October 2022 15.38
> > >>
> > >> From: Morten Brørup <mb@smartsharesystems.com>
> > >>
> 
> [...]

I finally took a couple of hours to carefully review the mempool-related
series (including the ones that have already been pushed).

The new behavior looks better to me in all situations I can think about.

> 
> > >> --- a/lib/mempool/rte_mempool.h
> > >> +++ b/lib/mempool/rte_mempool.h
> > >> @@ -90,7 +90,7 @@ struct rte_mempool_cache {
> > >>   	 * Cache is allocated to this size to allow it to overflow in
> > >> certain
> > >>   	 * cases to avoid needless emptying of cache.
> > >>   	 */
> > >> -	void *objs[RTE_MEMPOOL_CACHE_MAX_SIZE * 3]; /**< Cache objects */
> > >> +	void *objs[RTE_MEMPOOL_CACHE_MAX_SIZE * 2]; /**< Cache objects */
> > >>   } __rte_cache_aligned;
> > >
> > > How much are we allowed to break the ABI here?
> > >
> > > This patch reduces the size of the structure by removing a now unused
> > part at the end, which should be harmless.

It is an ABI breakage: an existing application will use the new 22.11
function to create the mempool (with a smaller cache), but will use the
old inlined get/put that can exceed MAX_SIZE x 2 will remain.

But this is a nice memory consumption improvement, in my opinion we
should accept it for 22.11 with an entry in the release note.


> > >
> > > If we may also move the position of the objs array, I would add
> > __rte_cache_aligned to the objs array. It makes no difference in the
> > general case, but if get/put operations are always 32 objects, it will
> > reduce the number of memory (or last level cache) accesses from five to
> > four 64 B cache lines for every get/put operation.

Will it really be the case? Since cache->len has to be accessed too,
I don't think it would make a difference.


> > >
> > > 	uint32_t len;	      /**< Current cache count */
> > > -	/*
> > > -	 * Cache is allocated to this size to allow it to overflow in
> > certain
> > > -	 * cases to avoid needless emptying of cache.
> > > -	 */
> > > -	void *objs[RTE_MEMPOOL_CACHE_MAX_SIZE * 3]; /**< Cache objects */
> > > +	/**
> > > +	 * Cache objects
> > > +	 *
> > > +	 * Cache is allocated to this size to allow it to overflow in
> > certain
> > > +	 * cases to avoid needless emptying of cache.
> > > +	 */
> > > +	void *objs[RTE_MEMPOOL_CACHE_MAX_SIZE * 2] __rte_cache_aligned;
> > > } __rte_cache_aligned;
> > 
> > I think aligning objs on cacheline should be a separate patch.
> 
> Good point. I'll let you do it. :-)
> 
> PS: Thank you for following up on this patch series, Andrew!

Many thanks for this rework.

Acked-by: Olivier Matz <olivier.matz@6wind.com>