> On 2022-11-09 06:03, Morten Brørup wrote: > >> From: Konstantin Ananyev [mailto:konstantin.anan...@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:tho...@monjalon.net] > >>>>>> 08/11/2022 12:25, Morten Brørup: > >>>>>>> From: Morten Brørup > >>>>>>>> From: Konstantin Ananyev > >> [mailto:konstantin.anan...@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.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. > >>>>> > >>>>> 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? > > 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? 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. 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. > 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. > > 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. > > 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
