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. > > 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. > >> 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). >
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). > 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? > > 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
