08/11/2022 12:25, Morten Brørup:
> From: Morten Brørup
> Sent: Tuesday, 8 November 2022 12.22
>
> > From: Konstantin Ananyev [mailto:konstantin.anan...@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 <m...@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.anan...@huawei.com>
>
> @Thomas, this series should be ready to apply... it now has been:
> Reviewed-by: (mempool maintainer) Andrew Rybchenko
> <andrew.rybche...@oktetlabs.ru>
> Reviewed-By: Mattias Rönnblom <mattias.ronnb...@ericsson.com>
> Acked-by: Konstantin Ananyev <konstantin.anan...@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".
