Re: [PATCH 10/17] powerpc/qspinlock: allow stealing when head of queue yields

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

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]
> > If the head of queue is preventing stealing but it finds the owner vCPU
> > is preempted, it will yield its cycles to the owner which could cause it
> > to become preempted. Add an option to re-allow stealers before yielding,
> > and disallow them again after returning from the yield.
> > 
> > Disable this option by default for now, i.e., no logical change.
> > ---
> >  arch/powerpc/lib/qspinlock.c | 56 ++++++++++++++++++++++++++++++++++--
> >  1 file changed, 53 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/powerpc/lib/qspinlock.c b/arch/powerpc/lib/qspinlock.c
> > index b39f8c5b329c..94f007f66942 100644
> > --- a/arch/powerpc/lib/qspinlock.c
> > +++ b/arch/powerpc/lib/qspinlock.c
> > @@ -26,6 +26,7 @@ static bool MAYBE_STEALERS __read_mostly = true;
> >  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;
>
> To me this one does read as a boolean, but if you go with those other changes
> I'd make it pv_yield_steal_enable to be consistent.
>
> >  static bool pv_yield_prev __read_mostly = true;
> >  
> >  static DEFINE_PER_CPU_ALIGNED(struct qnodes, qnodes);
> > @@ -173,6 +174,23 @@ static __always_inline u32 lock_set_mustq(struct qspinlock *lock)
> >  	return prev;
> >  }
> >  
> > +static __always_inline u32 lock_clear_mustq(struct qspinlock *lock)
> > +{
> > +	u32 new = _Q_MUST_Q_VAL;
> > +	u32 prev;
> > +
> > +	asm volatile(
> > +"1:	lwarx	%0,0,%1		# lock_clear_mustq			\n"
> > +"	andc	%0,%0,%2						\n"
> > +"	stwcx.	%0,0,%1							\n"
> > +"	bne-	1b							\n"
> > +	: "=&r" (prev)
> > +	: "r" (&lock->val), "r" (new)
> > +	: "cr0", "memory");
> > +
>
> This is pretty similar to the DEFINE_TESTOP() pattern again with the same llong caveat.
>
>
> > +	return prev;
> > +}
> > +
> >  static struct qnode *get_tail_qnode(struct qspinlock *lock, u32 val)
> >  {
> >  	int cpu = get_tail_cpu(val);
> > @@ -188,7 +206,7 @@ static struct qnode *get_tail_qnode(struct qspinlock *lock, u32 val)
> >  	BUG();
> >  }
> >  
> > -static __always_inline void yield_to_locked_owner(struct qspinlock *lock, u32 val, bool paravirt)
> > +static __always_inline void __yield_to_locked_owner(struct qspinlock *lock, u32 val, bool paravirt, bool clear_mustq)
>
>  /* See yield_to_locked_owner comment */ comment needs to be updated now.

Yep.

> >  {
> >  	int owner;
> >  	u32 yield_count;
> > @@ -217,7 +235,11 @@ static __always_inline void yield_to_locked_owner(struct qspinlock *lock, u32 va
> >  	smp_rmb();
> >  
> >  	if (READ_ONCE(lock->val) == val) {
> > +		if (clear_mustq)
> > +			lock_clear_mustq(lock);
> >  		yield_to_preempted(owner, yield_count);
> > +		if (clear_mustq)
> > +			lock_set_mustq(lock);
> >  		/* Don't relax if we yielded. Maybe we should? */
> >  		return;
> >  	}
> > @@ -225,6 +247,16 @@ static __always_inline void yield_to_locked_owner(struct qspinlock *lock, u32 va
> >  	cpu_relax();
> >  }
> >  
> > +static __always_inline void yield_to_locked_owner(struct qspinlock *lock, u32 val, bool paravirt)
> > +{
> > +	__yield_to_locked_owner(lock, val, paravirt, false);
> > +}
> > +
> > +static __always_inline void yield_head_to_locked_owner(struct qspinlock *lock, u32 val, bool paravirt, bool clear_mustq)
> > +{
>
> The check for pv_yield_allow_steal seems like it could go here instead of
> being done by the caller.
> __yield_to_locked_owner() checks for pv_yield_owner so it seems more
>   consistent.

Yeah that worked and is probably an improvement.

Thanks,
Nick