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

[Thomas Monjalon <thomas@monjalon.net>]

08/11/2022 15:30, Morten Brørup:
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 08/11/2022 12:25, Morten Brørup:
> > > From: Morten Brørup
> > > > From: Konstantin Ananyev [mailto:konstantin.ananyev@huawei.com]
> > > > Sent: Tuesday, 8 November 2022 10.20
> > > > > +#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.
> 
> I understand that the RFC/v1 of this series was formally too late to make it in 22.11, so I will not complain loudly if you choose to omit it for 22.11.
> 
> With two independent reviews, including from a mempool maintainer, I still have some hope. Also considering the risk assessment below. ;-)
> 
> > Please tell what is the benefit for 22.11 (before/after and condition).
> 
> Short version: With this series, mempool statistics can be used in production. Without it, the performance cost (mempool_perf_autotest: -74 %) is prohibitive!
> 
> Long version:
> 
> The patch series provides significantly higher performance for mempool statistics, which are readable through rte_mempool_dump(FILE *f, struct rte_mempool *mp).
> 
> Without this series, you have to set RTE_LIBRTE_MEMPOOL_DEBUG at build time to get mempool statistics. RTE_LIBRTE_MEMPOOL_DEBUG also enables protective cookies before and after each mempool object, which are all verified on get/put from the mempool. According to mempool_perf_autotest, the performance cost of mempool statistics (by setting RTE_LIBRTE_MEMPOOL_DEBUG) is a 74 % decrease in rate_persec for mempools with cache (i.e. mbuf pools). Prohibitive for use in production!
> 
> With this series, the performance cost of mempool statistics (by setting RTE_LIBRTE_MEMPOOL_STATS) in mempool_perf_autotest is only 6.7 %, so mempool statistics can be used in production.
> 
> > Note there is a real risk doing such change that late.
> 
> Risk assessment:
> 
> The patch series has zero effect unless either RTE_LIBRTE_MEMPOOL_DEBUG or RTE_LIBRTE_MEMPOOL_STATS are set when building. They are not set in the default build.

If theses build flags are not set, there is no risk and no benefit.
But if they are set, there is a risk of regression,
for the benefit of an increased performance of a debug feature.
I would say it is better to avoid any functional regression in a debug feature
at this stage.
Any other opinion?