RE: FW: [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: Wednesday, 9 November 2022 11.20
> 
> > On 2022-11-09 06:03, Morten Brørup wrote:
> > >> From: Konstantin Ananyev [mailto:konstantin.ananyev@huawei.com]
> > >> Sent: Tuesday, 8 November 2022 18.38
> > >>>
> > >>> On Tue, Nov 08, 2022 at 04:51:11PM +0100, Thomas Monjalon wrote:
> > >>>> 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)-
> > >>>>>>> https://protect2.fireeye.com/v1/url?k=31323334-501d5122-
> 313273af-454445555731-27a859e7ec13035a&q=1&e=a120e28e-
> > caa7-4783-9686-5868c871553d&u=http%3A%2F%2Fstats.name%2F += n
> > >>>>>>>>
> > >>>>>>>> As Andrew already pointed, it needs to be: ((cache)-
> > >>> https://protect2.fireeye.com/v1/url?k=31323334-501d5122-313273af-
> 454445555731-27a859e7ec13035a&q=1&e=a120e28e-caa7-
> > 4783-9686-5868c871553d&u=http%3A%2F%2Fstats.name%2F +=
> > >>>>>> (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?
> > >>>>
> > >>> While I agree that we should avoid any functional regression, I
> > >> wonder how
> > >>> widely used the debug feature is, and how big the risk of a
> > >> regression is?
> > >>> Even if there is one, having a regression in a debug feature is a
> lot
> > >> less
> > >>> serious than having one in something which goes into production.
> > >>>
> > >>
> > >> Unless it introduces an ABI breakage (as I understand it doesn't),
> I'll
> > >> wait till 23.03.
> > >> Just in case.
> > >
> > > If built (both before and after this series) without
> RTE_LIBRTE_MEMPOOL_DEBUG (and without RTE_LIBRTE_MEMPOOL_STATS,
> > which is introduced by the series), there is no ABI breakage.
> > >
> > > If built (both before and after this series) with
> RTE_LIBRTE_MEMPOOL_DEBUG (and without RTE_LIBRTE_MEMPOOL_STATS), the
> > ABI differs between before and after this series: The stats array
> disappears from struct rte_mempool, and the output from
> > rte_mempool_dump() does not include the statistics.
> > >
> 
> Can we probably always enable RTE_LIBRTE_MEMPOOL_STATS when
> RTE_LIBRTE_MEMPOOL_DEBUG is on?

That would fix the rte_mempool_dump() API breakage, yes.

But since it's only a partial fix, I don't think we should do it.

> 
> > > If built (both before and after this series) with
> RTE_LIBRTE_MEMPOOL_DEBUG (and with RTE_LIBRTE_MEMPOOL_STATS), the ABI
> > also differs between before and after this series: The size of the
> stats array in struct rte_mempool grows by one element.
> 
> Ah yes, missed that one.
> So the question is then - does it count as formal ABI breakage or not?

Yes, this is a key question!

However, performance improvements are not accepted as LTS patches, so not including it in -rc3 will make it useless until 23.11. (At least for users deploying only LTS releases.)

> If yes, then probably better to go ahead with these changes for 22.11
> (it sounds too prohibitive to wait for an year here).
> Or at least take in the part that introduce the ABI breakage.

Without the 3rd patch in the series, the performance is not fully optimized. (Remember: Only relevant when built with RTE_LIBRTE_MEMPOOL_STATS.)

> If not, probably not bit deal to wait till 23.03.
> 
> > >> BTW, as a side thought - if the impact is really that small now,
> would
> > >> it make sense to make
> > >> it run-time option, instead of compile-time one?
> > >
> > > The mempool get/put functions are very lean when built without
> STATS or DEBUG. With a runtime option, the resulting code would
> > be slightly longer, and only one additional conditional would be hit
> in the common case (i.e. when the objects don't miss the mempool
> > cache). So with stats disabled (at runtime), it would only add a very
> small performance cost. However, checking the value of the
> > enabled/disabled variable can cause a CPU cache miss, which has a
> performance cost. And the enabled/disabled variable should
> > definitely be global - if it is per mempool, it will cause many CPU
> cache misses (because the common case doesn't touch the mempool
> > structure, only the mempool cache structure).
> > >
> 
> Yes, either a global one, or put it into both structs:
> rte_mempool_cache and rte_mempool.

That is doable. There is room for it in rte_mempool_cache, and it can be a flag in rte_mempool.

> 
> > It's not totally obvious that a conditional is better than just
> always
> > performing these simple arithmetic operations, even if you don't know
> if
> > you need the result (i.e., if stats is enabled or not), especially
> since
> > they operate on a cache line that is very likely already owned by the
> > core running the core (since the 'len' fields is frequently used).
> 
> Yep, that's another option - always update the cache part.

Yes, always updating the rte_mempool_cache stats to avoid the conditional in the likely code path seems like a viable concept.

(And for now, we can ignore that 64 bit stats are somewhat more costly on 32 bit architectures if tearing must be avoided. I say that we can ignore it for now, because this kind of tearing is ignored for 64 bit stats everywhere in DPDK, so it should be ignorable here too.)

> 
> > > Also, checking the runtime option should have unlikely(), so the
> performance cost of the stats (when enabled at runtime) is also
> > higher than with a build time option. (Yes, dynamic branch prediction
> will alleviate most of this, but it will consume entries in the
> > branch predictor table - these are inlined functions. Just like we
> always try to avoid cache misses in DPDK, we should also try to
> > conserve branch predictor table entries. I hate the argument that
> branch prediction fixes conditionals, especially if they are weird or
> > could have been avoided.)
> > >
> > > In the cost/benefit analysis, we need to consider that these
> statistics are not fill/emptiness level status or similar, but only
> debug
> > counters (number of get/put transactions and objects), so we need to
> ask ourselves this question: How many users are interested in
> > these statistics for production and are unable to build their
> application with RTE_LIBRTE_MEMPOOL_STATS?
> 
> Obviously, I don't have such stats.
> From my perspective - I am ok to spend few extra cycles to avoid
> building separate binary.
> Again, I guess that  with global switch the impact will be negligible.
> But anyway, it will require even more changes and another ABI breakage
> (as stats should always be included),
> so it definitely out of scope for this release.

Agree. This series can be tweaked further, as the discussion clearly shows.

However, time is about to run out on. If we want to include it 22.11, we should take it into -rc3 without any of the discussed modifications (except fixing the macro to satisfy checkpatch).

Furthermore, the discussed modifications - various ways of handling a runtime option - will introduce some performance degradation with the default build configuration. Regardless how small we think the performance degradation is, I would hesitate to introduce a performance degradation in -rc3.

The current patch series has zero performance effect with the default build configuration.

> 
> > > For example, we (SmartShare Systems) are only interested in them
> for application profiling purposes... trying to improve the
> > performance by striving for a higher number of objects per burst in
> every pipeline stage.
> > >
> > >> Konstantin