| Message ID | 20220728063120.2867508-17-npiggin@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | powerpc: alternate queued spinlock implementation | expand |
On Thu, 2022-07-28 at 16:31 +1000, Nicholas Piggin wrote: > Allow for a reduction in the number of times a CPU from a different > node than the owner can attempt to steal the lock before queueing. > This could bias the transfer behaviour of the lock across the > machine and reduce NUMA crossings. > --- > arch/powerpc/lib/qspinlock.c | 34 +++++++++++++++++++++++++++++++--- > 1 file changed, 31 insertions(+), 3 deletions(-) > > diff --git a/arch/powerpc/lib/qspinlock.c b/arch/powerpc/lib/qspinlock.c > index d4594c701f7d..24f68bd71e2b 100644 > --- a/arch/powerpc/lib/qspinlock.c > +++ b/arch/powerpc/lib/qspinlock.c > @@ -4,6 +4,7 @@ > #include <linux/export.h> > #include <linux/percpu.h> > #include <linux/smp.h> > +#include <linux/topology.h> > #include <asm/qspinlock.h> > #include <asm/paravirt.h> > > @@ -24,6 +25,7 @@ struct qnodes { > > /* Tuning parameters */ > static int STEAL_SPINS __read_mostly = (1<<5); > +static int REMOTE_STEAL_SPINS __read_mostly = (1<<2); > #if _Q_SPIN_TRY_LOCK_STEAL == 1 > static const bool MAYBE_STEALERS = true; > #else > @@ -39,9 +41,13 @@ static bool pv_prod_head __read_mostly = false; > > static DEFINE_PER_CPU_ALIGNED(struct qnodes, qnodes); > > -static __always_inline int get_steal_spins(bool paravirt) > +static __always_inline int get_steal_spins(bool paravirt, bool remote) > { > - return STEAL_SPINS; > + if (remote) { > + return REMOTE_STEAL_SPINS; > + } else { > + return STEAL_SPINS; > + } > } > > static __always_inline int get_head_spins(bool paravirt) > @@ -380,8 +386,13 @@ static __always_inline bool try_to_steal_lock(struct qspinlock *lock, bool parav > > iters++; > > - if (iters >= get_steal_spins(paravirt)) > + if (iters >= get_steal_spins(paravirt, false)) > break; > + if (iters >= get_steal_spins(paravirt, true)) { There's no indication of what true and false mean here which is hard to read. To me it feels like two separate functions would be more clear. > + int cpu = get_owner_cpu(val); > + if (numa_node_id() != cpu_to_node(cpu)) What about using node_distance() instead? > + break; > + } > } > spin_end(); > > @@ -588,6 +599,22 @@ static int steal_spins_get(void *data, u64 *val) > > DEFINE_SIMPLE_ATTRIBUTE(fops_steal_spins, steal_spins_get, steal_spins_set, "%llu\n"); > > +static int remote_steal_spins_set(void *data, u64 val) > +{ > + REMOTE_STEAL_SPINS = val; REMOTE_STEAL_SPINS is int not u64. > + > + return 0; > +} > + > +static int remote_steal_spins_get(void *data, u64 *val) > +{ > + *val = REMOTE_STEAL_SPINS; > + > + return 0; > +} > + > +DEFINE_SIMPLE_ATTRIBUTE(fops_remote_steal_spins, remote_steal_spins_get, remote_steal_spins_set, "%llu\n"); > + > static int head_spins_set(void *data, u64 val) > { > HEAD_SPINS = val; > @@ -687,6 +714,7 @@ DEFINE_SIMPLE_ATTRIBUTE(fops_pv_prod_head, pv_prod_head_get, pv_prod_head_set, " > static __init int spinlock_debugfs_init(void) > { > debugfs_create_file("qspl_steal_spins", 0600, arch_debugfs_dir, NULL, &fops_steal_spins); > + debugfs_create_file("qspl_remote_steal_spins", 0600, arch_debugfs_dir, NULL, &fops_remote_steal_spins); > debugfs_create_file("qspl_head_spins", 0600, arch_debugfs_dir, NULL, &fops_head_spins); > if (is_shared_processor()) { > debugfs_create_file("qspl_pv_yield_owner", 0600, arch_debugfs_dir, NULL, &fops_pv_yield_owner);
On Thu, 2022-07-28 at 16:31 +1000, Nicholas Piggin wrote: [resend as utf-8, not utf-7] > Allow for a reduction in the number of times a CPU from a different > node than the owner can attempt to steal the lock before queueing. > This could bias the transfer behaviour of the lock across the > machine and reduce NUMA crossings. > --- > arch/powerpc/lib/qspinlock.c | 34 +++++++++++++++++++++++++++++++--- > 1 file changed, 31 insertions(+), 3 deletions(-) > > diff --git a/arch/powerpc/lib/qspinlock.c b/arch/powerpc/lib/qspinlock.c > index d4594c701f7d..24f68bd71e2b 100644 > --- a/arch/powerpc/lib/qspinlock.c > +++ b/arch/powerpc/lib/qspinlock.c > @@ -4,6 +4,7 @@ > #include <linux/export.h> > #include <linux/percpu.h> > #include <linux/smp.h> > +#include <linux/topology.h> > #include <asm/qspinlock.h> > #include <asm/paravirt.h> > > @@ -24,6 +25,7 @@ struct qnodes { > > /* Tuning parameters */ > static int STEAL_SPINS __read_mostly = (1<<5); > +static int REMOTE_STEAL_SPINS __read_mostly = (1<<2); > #if _Q_SPIN_TRY_LOCK_STEAL == 1 > static const bool MAYBE_STEALERS = true; > #else > @@ -39,9 +41,13 @@ static bool pv_prod_head __read_mostly = false; > > static DEFINE_PER_CPU_ALIGNED(struct qnodes, qnodes); > > -static __always_inline int get_steal_spins(bool paravirt) > +static __always_inline int get_steal_spins(bool paravirt, bool remote) > { > - return STEAL_SPINS; > + if (remote) { > + return REMOTE_STEAL_SPINS; > + } else { > + return STEAL_SPINS; > + } > } > > static __always_inline int get_head_spins(bool paravirt) > @@ -380,8 +386,13 @@ static __always_inline bool try_to_steal_lock(struct qspinlock *lock, bool parav > > iters++; > > - if (iters >= get_steal_spins(paravirt)) > + if (iters >= get_steal_spins(paravirt, false)) > break; > + if (iters >= get_steal_spins(paravirt, true)) { There's no indication of what true and false mean here which is hard to read. To me it feels like two separate functions would be more clear. > + int cpu = get_owner_cpu(val); > + if (numa_node_id() != cpu_to_node(cpu)) What about using node_distance() instead? > + break; > + } > } > spin_end(); > > @@ -588,6 +599,22 @@ static int steal_spins_get(void *data, u64 *val) > > DEFINE_SIMPLE_ATTRIBUTE(fops_steal_spins, steal_spins_get, steal_spins_set, "%llu\n"); > > +static int remote_steal_spins_set(void *data, u64 val) > +{ > + REMOTE_STEAL_SPINS = val; REMOTE_STEAL_SPINS is int not u64. > + > + return 0; > +} > + > +static int remote_steal_spins_get(void *data, u64 *val) > +{ > + *val = REMOTE_STEAL_SPINS; > + > + return 0; > +} > + > +DEFINE_SIMPLE_ATTRIBUTE(fops_remote_steal_spins, remote_steal_spins_get, remote_steal_spins_set, "%llu\n"); > + > static int head_spins_set(void *data, u64 val) > { > HEAD_SPINS = val; > @@ -687,6 +714,7 @@ DEFINE_SIMPLE_ATTRIBUTE(fops_pv_prod_head, pv_prod_head_get, pv_prod_head_set, " > static __init int spinlock_debugfs_init(void) > { > debugfs_create_file("qspl_steal_spins", 0600, arch_debugfs_dir, NULL, &fops_steal_spins); > + debugfs_create_file("qspl_remote_steal_spins", 0600, arch_debugfs_dir, NULL, &fops_remote_steal_spins); > debugfs_create_file("qspl_head_spins", 0600, arch_debugfs_dir, NULL, &fops_head_spins); > if (is_shared_processor()) { > debugfs_create_file("qspl_pv_yield_owner", 0600, arch_debugfs_dir, NULL, &fops_pv_yield_owner);
On Thu Nov 10, 2022 at 10:43 AM AEST, Jordan Niethe wrote: > On Thu, 2022-07-28 at 16:31 +1000, Nicholas Piggin wrote: > [resend as utf-8, not utf-7] > > Allow for a reduction in the number of times a CPU from a different > > node than the owner can attempt to steal the lock before queueing. > > This could bias the transfer behaviour of the lock across the > > machine and reduce NUMA crossings. > > --- > > arch/powerpc/lib/qspinlock.c | 34 +++++++++++++++++++++++++++++++--- > > 1 file changed, 31 insertions(+), 3 deletions(-) > > > > diff --git a/arch/powerpc/lib/qspinlock.c b/arch/powerpc/lib/qspinlock.c > > index d4594c701f7d..24f68bd71e2b 100644 > > --- a/arch/powerpc/lib/qspinlock.c > > +++ b/arch/powerpc/lib/qspinlock.c > > @@ -4,6 +4,7 @@ > > #include <linux/export.h> > > #include <linux/percpu.h> > > #include <linux/smp.h> > > +#include <linux/topology.h> > > #include <asm/qspinlock.h> > > #include <asm/paravirt.h> > > > > @@ -24,6 +25,7 @@ struct qnodes { > > > > /* Tuning parameters */ > > static int STEAL_SPINS __read_mostly = (1<<5); > > +static int REMOTE_STEAL_SPINS __read_mostly = (1<<2); > > #if _Q_SPIN_TRY_LOCK_STEAL == 1 > > static const bool MAYBE_STEALERS = true; > > #else > > @@ -39,9 +41,13 @@ static bool pv_prod_head __read_mostly = false; > > > > static DEFINE_PER_CPU_ALIGNED(struct qnodes, qnodes); > > > > -static __always_inline int get_steal_spins(bool paravirt) > > +static __always_inline int get_steal_spins(bool paravirt, bool remote) > > { > > - return STEAL_SPINS; > > + if (remote) { > > + return REMOTE_STEAL_SPINS; > > + } else { > > + return STEAL_SPINS; > > + } > > } > > > > static __always_inline int get_head_spins(bool paravirt) > > @@ -380,8 +386,13 @@ static __always_inline bool try_to_steal_lock(struct qspinlock *lock, bool parav > > > > iters++; > > > > - if (iters >= get_steal_spins(paravirt)) > > + if (iters >= get_steal_spins(paravirt, false)) > > break; > > + if (iters >= get_steal_spins(paravirt, true)) { > > There's no indication of what true and false mean here which is hard to read. > To me it feels like two separate functions would be more clear. Good point. I reworked this a bit. > > > > + int cpu = get_owner_cpu(val); > > + if (numa_node_id() != cpu_to_node(cpu)) > > What about using node_distance() instead? We don't really need the distance, just whether or not it's the same or not. I think distance not only has to look up the nodes, but then has to look up a matrix to get a number. Thanks, Nick
diff --git a/arch/powerpc/lib/qspinlock.c b/arch/powerpc/lib/qspinlock.c index d4594c701f7d..24f68bd71e2b 100644 --- a/arch/powerpc/lib/qspinlock.c +++ b/arch/powerpc/lib/qspinlock.c @@ -4,6 +4,7 @@ #include <linux/export.h> #include <linux/percpu.h> #include <linux/smp.h> +#include <linux/topology.h> #include <asm/qspinlock.h> #include <asm/paravirt.h> @@ -24,6 +25,7 @@ struct qnodes { /* Tuning parameters */ static int STEAL_SPINS __read_mostly = (1<<5); +static int REMOTE_STEAL_SPINS __read_mostly = (1<<2); #if _Q_SPIN_TRY_LOCK_STEAL == 1 static const bool MAYBE_STEALERS = true; #else @@ -39,9 +41,13 @@ static bool pv_prod_head __read_mostly = false; static DEFINE_PER_CPU_ALIGNED(struct qnodes, qnodes); -static __always_inline int get_steal_spins(bool paravirt) +static __always_inline int get_steal_spins(bool paravirt, bool remote) { - return STEAL_SPINS; + if (remote) { + return REMOTE_STEAL_SPINS; + } else { + return STEAL_SPINS; + } } static __always_inline int get_head_spins(bool paravirt) @@ -380,8 +386,13 @@ static __always_inline bool try_to_steal_lock(struct qspinlock *lock, bool parav iters++; - if (iters >= get_steal_spins(paravirt)) + if (iters >= get_steal_spins(paravirt, false)) break; + if (iters >= get_steal_spins(paravirt, true)) { + int cpu = get_owner_cpu(val); + if (numa_node_id() != cpu_to_node(cpu)) + break; + } } spin_end(); @@ -588,6 +599,22 @@ static int steal_spins_get(void *data, u64 *val) DEFINE_SIMPLE_ATTRIBUTE(fops_steal_spins, steal_spins_get, steal_spins_set, "%llu\n"); +static int remote_steal_spins_set(void *data, u64 val) +{ + REMOTE_STEAL_SPINS = val; + + return 0; +} + +static int remote_steal_spins_get(void *data, u64 *val) +{ + *val = REMOTE_STEAL_SPINS; + + return 0; +} + +DEFINE_SIMPLE_ATTRIBUTE(fops_remote_steal_spins, remote_steal_spins_get, remote_steal_spins_set, "%llu\n"); + static int head_spins_set(void *data, u64 val) { HEAD_SPINS = val; @@ -687,6 +714,7 @@ DEFINE_SIMPLE_ATTRIBUTE(fops_pv_prod_head, pv_prod_head_get, pv_prod_head_set, " static __init int spinlock_debugfs_init(void) { debugfs_create_file("qspl_steal_spins", 0600, arch_debugfs_dir, NULL, &fops_steal_spins); + debugfs_create_file("qspl_remote_steal_spins", 0600, arch_debugfs_dir, NULL, &fops_remote_steal_spins); debugfs_create_file("qspl_head_spins", 0600, arch_debugfs_dir, NULL, &fops_head_spins); if (is_shared_processor()) { debugfs_create_file("qspl_pv_yield_owner", 0600, arch_debugfs_dir, NULL, &fops_pv_yield_owner);