[PATCH 11/17] powerpc/qspinlock: allow propagation of yield CPU down the queue

[Nicholas Piggin]

On Thu Nov 10, 2022 at 10:42 AM AEST, Jordan Niethe wrote:
> On Thu, 2022-07-28 at 16:31 +1000, Nicholas Piggin wrote:
> [resend as utf-8, not utf-7]
> > Having all CPUs poll the lock word for the owner CPU that should be
> > yielded to defeats most of the purpose of using MCS queueing for
> > scalability. Yet it may be desirable for queued waiters to to yield
> > to a preempted owner.
> > 
> > s390 addreses this problem by having queued waiters sample the lock
> > word to find the owner much less frequently. In this approach, the
> > waiters never sample it directly, but the queue head propagates the
> > owner CPU back to the next waiter if it ever finds the owner has
> > been preempted. Queued waiters then subsequently propagate the owner
> > CPU back to the next waiter, and so on.
> > 
> > Disable this option by default for now, i.e., no logical change.
> > ---
> >  arch/powerpc/lib/qspinlock.c | 85 +++++++++++++++++++++++++++++++++++-
> >  1 file changed, 84 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/powerpc/lib/qspinlock.c b/arch/powerpc/lib/qspinlock.c
> > index 94f007f66942..28c85a2d5635 100644
> > --- a/arch/powerpc/lib/qspinlock.c
> > +++ b/arch/powerpc/lib/qspinlock.c
> > @@ -12,6 +12,7 @@
> >  struct qnode {
> >  	struct qnode	*next;
> >  	struct qspinlock *lock;
> > +	int		yield_cpu;
> >  	u8		locked; /* 1 if lock acquired */
> >  };
> >  
> > @@ -28,6 +29,7 @@ static int HEAD_SPINS __read_mostly = (1<<8);
> >  static bool pv_yield_owner __read_mostly = true;
> >  static bool pv_yield_allow_steal __read_mostly = false;
> >  static bool pv_yield_prev __read_mostly = true;
> > +static bool pv_yield_propagate_owner __read_mostly = true;
>
> This also seems to be enabled by default.
>
> >  
> >  static DEFINE_PER_CPU_ALIGNED(struct qnodes, qnodes);
> >  
> > @@ -257,13 +259,66 @@ static __always_inline void yield_head_to_locked_owner(struct qspinlock *lock, u
> >  	__yield_to_locked_owner(lock, val, paravirt, clear_mustq);
> >  }
> >  
> > +static __always_inline void propagate_yield_cpu(struct qnode *node, u32 val, int *set_yield_cpu, bool paravirt)
> > +{
> > +	struct qnode *next;
> > +	int owner;
> > +
> > +	if (!paravirt)
> > +		return;
> > +	if (!pv_yield_propagate_owner)
> > +		return;
> > +
> > +	owner = get_owner_cpu(val);
> > +	if (*set_yield_cpu == owner)
> > +		return;
> > +
> > +	next = READ_ONCE(node->next);
> > +	if (!next)
> > +		return;
> > +
> > +	if (vcpu_is_preempted(owner)) {
>
> Is there a difference about using vcpu_is_preempted() here
> vs checking bit 0 in other places?

Yeah we weren't yielding here so didn't have to load the yield count
explicitly.

> > +		next->yield_cpu = owner;
> > +		*set_yield_cpu = owner;
> > +	} else if (*set_yield_cpu != -1) {
>
> It might be worth giving the -1 CPU a #define.

It's fairly standard to use -1 as a null value for cpu.

>
> > +		next->yield_cpu = owner;
> > +		*set_yield_cpu = owner;
> > +	}
> > +}
>
> Does this need to pass set_yield_cpu by reference? Couldn't it's new value be
> returned? To me it makes it more clear the function is used to change
> set_yield_cpu. I think this would work:
>
> int set_yield_cpu = -1;
>
> static __always_inline int propagate_yield_cpu(struct qnode *node, u32 val, int set_yield_cpu, bool paravirt)
> {
> 	struct qnode *next;
> 	int owner;
>
> 	if (!paravirt)
> 		goto out;
> 	if (!pv_yield_propagate_owner)
> 		goto out;
>
> 	owner = get_owner_cpu(val);
> 	if (set_yield_cpu == owner)
> 		goto out;
>
> 	next = READ_ONCE(node->next);
> 	if (!next)
> 		goto out;
>
> 	if (vcpu_is_preempted(owner)) {
> 		next->yield_cpu = owner;
> 		return owner;
> 	} else if (set_yield_cpu != -1) {
> 		next->yield_cpu = owner;
> 		return owner;
> 	}
>
> out:
> 	return set_yield_cpu;
> }
>
> set_yield_cpu = propagate_yield_cpu(...  set_yield_cpu ...);

I think I prefer as is, because the caller doesn't use it anywhere. It
looks more like some temporary storage to be used by the function over
multiple calls this way.

Thanks,
Nick