Re: [PATCH 13/17] powerpc/qspinlock: trylock and initial lock attempt may steal

[Jordan Niethe <jniethe5@gmail.com>]

On Thu, 2022-07-28 at 16:31 +1000, Nicholas Piggin wrote:
[resend as utf-8, not utf-7]
> This gives trylock slightly more strength, and it also gives most
> of the benefit of passing 'val' back through the slowpath without
> the complexity.
> ---
>  arch/powerpc/include/asm/qspinlock.h | 39 +++++++++++++++++++++++++++-
>  arch/powerpc/lib/qspinlock.c         |  9 +++++++
>  2 files changed, 47 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/include/asm/qspinlock.h b/arch/powerpc/include/asm/qspinlock.h
> index 44601b261e08..d3d2039237b2 100644
> --- a/arch/powerpc/include/asm/qspinlock.h
> +++ b/arch/powerpc/include/asm/qspinlock.h
> @@ -5,6 +5,8 @@
>  #include <linux/compiler.h>
>  #include <asm/qspinlock_types.h>
>  
> +#define _Q_SPIN_TRY_LOCK_STEAL 1

Would this be a config option?

> +
>  static __always_inline int queued_spin_is_locked(struct qspinlock *lock)
>  {
>  	return READ_ONCE(lock->val);
> @@ -26,11 +28,12 @@ static __always_inline u32 queued_spin_get_locked_val(void)
>  	return _Q_LOCKED_VAL | (smp_processor_id() << _Q_OWNER_CPU_OFFSET);
>  }
>  
> -static __always_inline int queued_spin_trylock(struct qspinlock *lock)
> +static __always_inline int __queued_spin_trylock_nosteal(struct qspinlock *lock)
>  {
>  	u32 new = queued_spin_get_locked_val();
>  	u32 prev;
>  
> +	/* Trylock succeeds only when unlocked and no queued nodes */
>  	asm volatile(
>  "1:	lwarx	%0,0,%1,%3	# queued_spin_trylock			\n"

s/queued_spin_trylock/__queued_spin_trylock_nosteal

>  "	cmpwi	0,%0,0							\n"
> @@ -49,6 +52,40 @@ static __always_inline int queued_spin_trylock(struct qspinlock *lock)
>  	return 0;
>  }
>  
> +static __always_inline int __queued_spin_trylock_steal(struct qspinlock *lock)
> +{
> +	u32 new = queued_spin_get_locked_val();
> +	u32 prev, tmp;
> +
> +	/* Trylock may get ahead of queued nodes if it finds unlocked */
> +	asm volatile(
> +"1:	lwarx	%0,0,%2,%5	# queued_spin_trylock			\n"

s/queued_spin_trylock/__queued_spin_trylock_steal

> +"	andc.	%1,%0,%4						\n"
> +"	bne-	2f							\n"
> +"	and	%1,%0,%4						\n"
> +"	or	%1,%1,%3						\n"
> +"	stwcx.	%1,0,%2							\n"
> +"	bne-	1b							\n"
> +"\t"	PPC_ACQUIRE_BARRIER "						\n"
> +"2:									\n"

Just because there's a little bit more going on here...

Q_TAIL_CPU_MASK = 0xFFFE0000
~Q_TAIL_CPU_MASK = 0x1FFFF


1:	lwarx	prev, 0, &lock->val, IS_ENABLED_PPC64
	andc.	tmp, prev, _Q_TAIL_CPU_MASK 	(tmp = prev & ~_Q_TAIL_CPU_MASK)
	bne-	2f 				(exit if locked)
	and	tmp, prev, _Q_TAIL_CPU_MASK 	(tmp = prev & _Q_TAIL_CPU_MASK)
	or	tmp, tmp, new			(tmp |= new)					
	stwcx.	tmp, 0, &lock->val					
		
	bne-	1b							
	PPC_ACQUIRE_BARRIER		
2:

... which seems correct.


> +	: "=&r" (prev), "=&r" (tmp)
> +	: "r" (&lock->val), "r" (new), "r" (_Q_TAIL_CPU_MASK),
> +	  "i" (IS_ENABLED(CONFIG_PPC64) ? 1 : 0)
> +	: "cr0", "memory");
> +
> +	if (likely(!(prev & ~_Q_TAIL_CPU_MASK)))
> +		return 1;
> +	return 0;
> +}
> +
> +static __always_inline int queued_spin_trylock(struct qspinlock *lock)
> +{
> +	if (!_Q_SPIN_TRY_LOCK_STEAL)
> +		return __queued_spin_trylock_nosteal(lock);
> +	else
> +		return __queued_spin_trylock_steal(lock);
> +}
> +
>  void queued_spin_lock_slowpath(struct qspinlock *lock);
>  
>  static __always_inline void queued_spin_lock(struct qspinlock *lock)
> diff --git a/arch/powerpc/lib/qspinlock.c b/arch/powerpc/lib/qspinlock.c
> index 3b10e31bcf0a..277aef1fab0a 100644
> --- a/arch/powerpc/lib/qspinlock.c
> +++ b/arch/powerpc/lib/qspinlock.c
> @@ -24,7 +24,11 @@ struct qnodes {
>  
>  /* Tuning parameters */
>  static int STEAL_SPINS __read_mostly = (1<<5);
> +#if _Q_SPIN_TRY_LOCK_STEAL == 1
> +static const bool MAYBE_STEALERS = true;
> +#else
>  static bool MAYBE_STEALERS __read_mostly = true;
> +#endif
>  static int HEAD_SPINS __read_mostly = (1<<8);
>  
>  static bool pv_yield_owner __read_mostly = true;
> @@ -522,6 +526,10 @@ void pv_spinlocks_init(void)
>  #include <linux/debugfs.h>
>  static int steal_spins_set(void *data, u64 val)
>  {
> +#if _Q_SPIN_TRY_LOCK_STEAL == 1
> +	/* MAYBE_STEAL remains true */
> +	STEAL_SPINS = val;
> +#else
>  	static DEFINE_MUTEX(lock);
>  
>  	mutex_lock(&lock);
> @@ -539,6 +547,7 @@ static int steal_spins_set(void *data, u64 val)
>  		STEAL_SPINS = val;
>  	}
>  	mutex_unlock(&lock);
> +#endif
>  
>  	return 0;
>  }