Excerpts from Davidlohr Bueso's message of March 9, 2021 11:59 am: > 49a7d46a06c3 (powerpc: Implement smp_cond_load_relaxed()) added > busy-waiting pausing with a preferred SMT priority pattern, lowering > the priority (reducing decode cycles) during the whole loop slowpath. > > However, data shows that while this pattern works well with simple > spinlocks, queued spinlocks benefit more being kept in medium priority, > with a cpu_relax() instead, being a low+medium combo on powerpc.
Thanks for tracking this down and the comprehensive results, great work. It's only a relatively recent patch, so I think the revert is a good idea (i.e., don't keep it around for possibly other code to hit problems with). One request, could you add a comment in place that references smp_cond_load_relaxed() so this commit can be found again if someone looks at it? Something like this /* * smp_cond_load_relaxed was found to have performance problems if * implemented with spin_begin()/spin_end(). */ I wonder if it should have a Fixes: tag to the original commit as well. Otherwise, Acked-by: Nicholas Piggin <npig...@gmail.com> Thanks, Nick > > Data is from three benchmarks on a Power9: 9008-22L 64 CPUs with > 2 sockets and 8 threads per core. > > 1. locktorture. > > This is data for the lowest and most artificial/pathological level, > with increasing thread counts pounding on the lock. Metrics are total > ops/minute. Despite some small hits in the 4-8 range, scenarios are > either neutral or favorable to this patch. > > +=========+==========+==========+=======+ > | # tasks | vanilla | dirty | %diff | > +=========+==========+==========+=======+ > | 2 | 46718565 | 48751350 | 4.35 | > +---------+----------+----------+-------+ > | 4 | 51740198 | 50369082 | -2.65 | > +---------+----------+----------+-------+ > | 8 | 63756510 | 62568821 | -1.86 | > +---------+----------+----------+-------+ > | 16 | 67824531 | 70966546 | 4.63 | > +---------+----------+----------+-------+ > | 32 | 53843519 | 61155508 | 13.58 | > +---------+----------+----------+-------+ > | 64 | 53005778 | 53104412 | 0.18 | > +---------+----------+----------+-------+ > | 128 | 53331980 | 54606910 | 2.39 | > +=========+==========+==========+=======+ > > 2. sockperf (tcp throughput) > > Here a client will do one-way throughput tests to a localhost server, with > increasing message sizes, dealing with the sk_lock. This patch shows to put > the performance of the qspinlock back to par with that of the simple lock: > > simple-spinlock vanilla dirty > Hmean 14 73.50 ( 0.00%) 54.44 * -25.93%* 73.45 * > -0.07%* > Hmean 100 654.47 ( 0.00%) 385.61 * -41.08%* 771.43 * > 17.87%* > Hmean 300 2719.39 ( 0.00%) 2181.67 * -19.77%* 2666.50 * > -1.94%* > Hmean 500 4400.59 ( 0.00%) 3390.77 * -22.95%* 4322.14 * > -1.78%* > Hmean 850 6726.21 ( 0.00%) 5264.03 * -21.74%* 6863.12 * > 2.04%* > > 3. dbench (tmpfs) > > Configured to run with up to ncpusx8 clients, it shows both latency and > throughput metrics. For the latency, with the exception of the 64 case, > there is really nothing to go by: > vanilla dirty > Amean latency-1 1.67 ( 0.00%) 1.67 * 0.09%* > Amean latency-2 2.15 ( 0.00%) 2.08 * 3.36%* > Amean latency-4 2.50 ( 0.00%) 2.56 * -2.27%* > Amean latency-8 2.49 ( 0.00%) 2.48 * 0.31%* > Amean latency-16 2.69 ( 0.00%) 2.72 * -1.37%* > Amean latency-32 2.96 ( 0.00%) 3.04 * -2.60%* > Amean latency-64 7.78 ( 0.00%) 8.17 * -5.07%* > Amean latency-512 186.91 ( 0.00%) 186.41 * 0.27%* > > For the dbench4 Throughput (misleading but traditional) there's a small > but rather constant improvement: > > vanilla dirty > Hmean 1 849.13 ( 0.00%) 851.51 * 0.28%* > Hmean 2 1664.03 ( 0.00%) 1663.94 * -0.01%* > Hmean 4 3073.70 ( 0.00%) 3104.29 * 1.00%* > Hmean 8 5624.02 ( 0.00%) 5694.16 * 1.25%* > Hmean 16 9169.49 ( 0.00%) 9324.43 * 1.69%* > Hmean 32 11969.37 ( 0.00%) 12127.09 * 1.32%* > Hmean 64 15021.12 ( 0.00%) 15243.14 * 1.48%* > Hmean 512 14891.27 ( 0.00%) 15162.11 * 1.82%* > > Measuring the dbench4 Per-VFS Operation latency, shows some very minor > differences within the noise level, around the 0-1% ranges. > > Signed-off-by: Davidlohr Bueso <dbu...@suse.de> > --- > arch/powerpc/include/asm/barrier.h | 16 ---------------- > 1 file changed, 16 deletions(-) > > diff --git a/arch/powerpc/include/asm/barrier.h > b/arch/powerpc/include/asm/barrier.h > index aecfde829d5d..7ae29cfb06c0 100644 > --- a/arch/powerpc/include/asm/barrier.h > +++ b/arch/powerpc/include/asm/barrier.h > @@ -80,22 +80,6 @@ do { > \ > ___p1; \ > }) > > -#ifdef CONFIG_PPC64 > -#define smp_cond_load_relaxed(ptr, cond_expr) ({ \ > - typeof(ptr) __PTR = (ptr); \ > - __unqual_scalar_typeof(*ptr) VAL; \ > - VAL = READ_ONCE(*__PTR); \ > - if (unlikely(!(cond_expr))) { \ > - spin_begin(); \ > - do { \ > - VAL = READ_ONCE(*__PTR); \ > - } while (!(cond_expr)); \ > - spin_end(); \ > - } \ > - (typeof(*ptr))VAL; \ > -}) > -#endif > - > #ifdef CONFIG_PPC_BOOK3S_64 > #define NOSPEC_BARRIER_SLOT nop > #elif defined(CONFIG_PPC_FSL_BOOK3E) > -- > 2.26.2 > >