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

[Thomas Monjalon <thomas@monjalon.net>]

08/11/2022 12:25, Morten Brørup:
> From: Morten Brørup 
> Sent: Tuesday, 8 November 2022 12.22
> 
> > 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>

Being acked does not mean it is good to apply in -rc3.
Please tell what is the benefit for 22.11 (before/after and condition).
Note there is a real risk doing such change that late.

> 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

Would be easier if you fix it.

> @Thomas/@David: I changed the state of this patch series to Awaiting Upstream in patchwork. Is that helpful, or should I change them to some other state?

You should keep it as "New".