> From: Olivier Matz [mailto:olivier.m...@6wind.com]
> Sent: Friday, 14 October 2022 21.51
>
> On Fri, Oct 14, 2022 at 05:57:39PM +0200, Morten Brørup wrote:
> > > From: Olivier Matz [mailto:olivier.m...@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.rybche...@oktetlabs.ru]
> > > > > Sent: Sunday, 9 October 2022 16.52
> > > > >
> > > > > On 10/9/22 17:31, Morten Brørup wrote:
> > > > > >> From: Andrew Rybchenko
> [mailto:andrew.rybche...@oktetlabs.ru]
> > > > > >> Sent: Sunday, 9 October 2022 15.38
> > > > > >>
> > > > > >> From: Morten Brørup <m...@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?
Accessing the objects at the bottom of the mempool cache is a special case,
where cache line0 is also used for objects.
Consider the next burst (and any following bursts):
Current:
┌────────┐
│len │
cache │ │
line0 │ │
│ │
├────────┤
│ │
cache │ │
line1 │ │
│ │
├────────┤
│ │
cache │********│---
line2 │********│ ^
│********│ |
├────────┤ | 16 objects
│********│ | 128B
cache │********│ |
line3 │********│ |
│********│ |
├────────┤ |
│********│_v_
cache │ │
line4 │ │
│ │
└────────┘
4 cache lines touched, incl. line0 for len.
With the proposed alignment:
┌────────┐
│len │
cache │ │
line0 │ │
│ │
├────────┤
│ │
cache │ │
line1 │ │
│ │
├────────┤
│ │
cache │ │
line2 │ │
│ │
├────────┤
│********│---
cache │********│ ^
line3 │********│ |
│********│ | 16 objects
├────────┤ | 128B
│********│ |
cache │********│ |
line4 │********│ |
│********│_v_
└────────┘
Only 3 cache lines touched, incl. line0 for len.
>
> >
> > >
> > >
> > > > > >
> > > > > > 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.m...@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
