RE: [PATCH v4 3/3] mempool: use cache for frequently updated stats

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Konstantin Ananyev [mailto:konstantin.ananyev@huawei.com]
> Sent: Tuesday, 8 November 2022 10.20
> 
> > When built with stats enabled (RTE_LIBRTE_MEMPOOL_STATS defined), the
> > performance of mempools with caches is improved as follows.
> >
> > When accessing objects in the mempool, either the put_bulk and
> put_objs or
> > the get_success_bulk and get_success_objs statistics counters are
> likely
> > to be incremented.
> >
> > By adding an alternative set of these counters to the mempool cache
> > structure, accessing the dedicated statistics structure is avoided in
> the
> > likely cases where these counters are incremented.
> >
> > The trick here is that the cache line holding the mempool cache
> structure
> > is accessed anyway, in order to access the 'len' or 'flushthresh'
> fields.
> > Updating some statistics counters in the same cache line has lower
> > performance cost than accessing the statistics counters in the
> dedicated
> > statistics structure, which resides in another cache line.
> >
> > mempool_perf_autotest with this patch shows the following
> improvements in
> > rate_persec.
> >
> > The cost of enabling mempool stats (without debug) after this patch:
> > -6.8 % and -6.7 %, respectively without and with cache.
> >
> > v4:
> > * Fix checkpatch warnings:
> >   A couple of typos in the patch description.
> >   The macro to add to a mempool cache stat variable should not use
> >   do {} while (0). Personally, I would tend to disagree with this,
> but
> >   whatever keeps the CI happy.
> > v3:
> > * Don't update the description of the RTE_MEMPOOL_STAT_ADD macro.
> >   This change belongs in the first patch of the series.
> > v2:
> > * Move the statistics counters into a stats structure.
> >
> > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > ---

[...]

> > +/**
> > + * @internal When stats is enabled, store some statistics.
> > + *
> > + * @param cache
> > + *   Pointer to the memory pool cache.
> > + * @param name
> > + *   Name of the statistics field to increment in the memory pool
> cache.
> > + * @param n
> > + *   Number to add to the statistics.
> > + */
> > +#ifdef RTE_LIBRTE_MEMPOOL_STATS
> > +#define RTE_MEMPOOL_CACHE_STAT_ADD(cache, name, n) (cache)->stats.name += n
> 
> As Andrew already pointed, it needs to be: ((cache)->stats.name += (n))
> Apart from that, LGTM.
> Series-Acked-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>

@Thomas, this series should be ready to apply... it now has been:
Reviewed-by: (mempool maintainer) Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Reviewed-By: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
Acked-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>

Please fix the RTE_MEMPOOL_CACHE_STAT_ADD macro while merging, to satisfy checkpatch. ;-)

It should be:

+#ifdef RTE_LIBRTE_MEMPOOL_STATS
+#define RTE_MEMPOOL_CACHE_STAT_ADD(cache, name, n) ((cache)->stats.name += (n))
+#else
+#define RTE_MEMPOOL_CACHE_STAT_ADD(cache, name, n) do {} while (0)
+#endif