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

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

On Fri, Oct 14, 2022 at 05:57:39PM +0200, Morten Brørup wrote:
> > From: Olivier Matz [mailto:olivier.matz@6wind.com]
> > Sent: Friday, 14 October 2022 16.01
> > 
> > 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.
> 
> Extreme care is required when touching a core library like the mempool.
> 
> Thank you, Olivier.
> 
> > 
> > >
> > > > >> --- 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.
> 
> Yes, the first cache line, containing cache->len, will be accessed always. I forgot to count that; so the improvement by aligning cache->objs will be five cache line accesses instead of six.
> 
> Let me try to explain the scenario in other words:
> 
> In an application where a mempool cache is only accessed in bursts of 32 objects (256 B), it matters if those 256 B accesses in the mempool cache start at a cache line aligned address or not. If cache line aligned, accessing those 256 B in the mempool cache will only touch 4 cache lines; if not, 5 cache lines will be touched. (For architectures with 128 B cache line, it will be 2 instead of 3 touched cache lines per mempool cache get/put operation in applications using only bursts of 32 objects.)
> 
> If we cache line align cache->objs, those bursts of 32 objects (256 B) will be cache line aligned: Any address at cache->objs[N * 32 objects] is cache line aligned if objs->objs[0] is cache line aligned.
> 
> Currently, the cache->objs directly follows cache->len, which makes cache->objs[0] cache line unaligned.
> 
> If we decide to break the mempool cache ABI, we might as well include my suggested cache line alignment performance improvement. It doesn't degrade performance for mempool caches not only accessed in bursts of 32 objects.

I don't follow you. Currently, with 16 objects (128B), we access to 3
cache lines:

      ┌────────┐
      │len     │
cache │********│---
line0 │********│ ^
      │********│ |
      ├────────┤ | 16 objects
      │********│ | 128B
cache │********│ |
line1 │********│ |
      │********│ |
      ├────────┤ |
      │********│_v_
cache │        │
line2 │        │
      │        │
      └────────┘

With the alignment, it is also 3 cache lines:

      ┌────────┐
      │len     │
cache │        │
line0 │        │
      │        │
      ├────────┤---
      │********│ ^
cache │********│ |
line1 │********│ |
      │********│ |
      ├────────┤ | 16 objects
      │********│ | 128B
cache │********│ |
line2 │********│ |
      │********│ v
      └────────┘---


Am I missing something?

> 
> > 
> > 
> > > > >
> > > > > 	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>
> 
> Perhaps Reviewed-by would be appropriate?

I was thinking that "Acked-by" was commonly used by maintainers, and
"Reviewed-by" for reviews by community members. After reading the
documentation again, it's not that clear now in my mind :)

Thanks,
Olivier