Re: [PATCH 15/17] powerpc/qspinlock: reduce remote node steal spins

["Nicholas Piggin" <npiggin@gmail.com>]

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