Re: [PATCH 14/17] powerpc/qspinlock: use spin_begin/end API

[Jordan Niethe <jniethe5@gmail.com>]

On Thu, 2022-07-28 at 16:31 +1000, Nicholas Piggin wrote:
[resend as utf-8, not utf-7]
> Use the spin_begin/spin_cpu_relax/spin_end APIs in qspinlock, which helps
> to prevent threads issuing a lot of expensive priority nops which may not
> have much effect due to immediately executing low then medium priority.

Just a general comment regarding the spin_{begin,end} API, more complicated
than something like

	spin_begin()
	for(;;)
		spin_cpu_relax()
	spin_end()

it becomes difficult to keep track of. Unfortunately, I don't have any good
suggestions how to improve it. Hopefully with P10s wait instruction we can
maybe try and move away from this.

It might be useful to comment the functions pre and post conditions regarding
expectations about spin_begin() and spin_end().

> ---
>  arch/powerpc/lib/qspinlock.c | 35 +++++++++++++++++++++++++++++++----
>  1 file changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/powerpc/lib/qspinlock.c b/arch/powerpc/lib/qspinlock.c
> index 277aef1fab0a..d4594c701f7d 100644
> --- a/arch/powerpc/lib/qspinlock.c
> +++ b/arch/powerpc/lib/qspinlock.c
> @@ -233,6 +233,8 @@ static __always_inline void __yield_to_locked_owner(struct qspinlock *lock, u32
>  	if ((yield_count & 1) == 0)
>  		goto relax; /* owner vcpu is running */
>  
> +	spin_end();
> +
>  	/*
>  	 * Read the lock word after sampling the yield count. On the other side
>  	 * there may a wmb because the yield count update is done by the
> @@ -248,11 +250,13 @@ static __always_inline void __yield_to_locked_owner(struct qspinlock *lock, u32
>  		yield_to_preempted(owner, yield_count);
>  		if (clear_mustq)
>  			lock_set_mustq(lock);
> +		spin_begin();
>  		/* Don't relax if we yielded. Maybe we should? */
>  		return;
>  	}
> +	spin_begin();
>  relax:
> -	cpu_relax();
> +	spin_cpu_relax();
>  }
>  
>  static __always_inline void yield_to_locked_owner(struct qspinlock *lock, u32 val, bool paravirt)
> @@ -315,14 +319,18 @@ static __always_inline void yield_to_prev(struct qspinlock *lock, struct qnode *
>  	if ((yield_count & 1) == 0)
>  		goto yield_prev; /* owner vcpu is running */
>  
> +	spin_end();
> +
>  	smp_rmb();
>  
>  	if (yield_cpu == node->yield_cpu) {
>  		if (node->next && node->next->yield_cpu != yield_cpu)
>  			node->next->yield_cpu = yield_cpu;
>  		yield_to_preempted(yield_cpu, yield_count);
> +		spin_begin();
>  		return;
>  	}
> +	spin_begin();
>  
>  yield_prev:
>  	if (!pv_yield_prev)
> @@ -332,15 +340,19 @@ static __always_inline void yield_to_prev(struct qspinlock *lock, struct qnode *
>  	if ((yield_count & 1) == 0)
>  		goto relax; /* owner vcpu is running */
>  
> +	spin_end();
> +
>  	smp_rmb(); /* See yield_to_locked_owner comment */
>  
>  	if (!node->locked) {
>  		yield_to_preempted(prev_cpu, yield_count);
> +		spin_begin();
>  		return;
>  	}
> +	spin_begin();
>  
>  relax:
> -	cpu_relax();
> +	spin_cpu_relax();
>  }
>  
>  
> @@ -349,6 +361,7 @@ static __always_inline bool try_to_steal_lock(struct qspinlock *lock, bool parav
>  	int iters;
>  
>  	/* Attempt to steal the lock */
> +	spin_begin();
>  	for (;;) {
>  		u32 val = READ_ONCE(lock->val);
>  
> @@ -356,8 +369,10 @@ static __always_inline bool try_to_steal_lock(struct qspinlock *lock, bool parav
>  			break;
>  
>  		if (unlikely(!(val & _Q_LOCKED_VAL))) {
> +			spin_end();
>  			if (trylock_with_tail_cpu(lock, val))
>  				return true;
> +			spin_begin();
>  			continue;
>  		}
>  
> @@ -368,6 +383,7 @@ static __always_inline bool try_to_steal_lock(struct qspinlock *lock, bool parav
>  		if (iters >= get_steal_spins(paravirt))
>  			break;
>  	}
> +	spin_end();
>  
>  	return false;
>  }
> @@ -418,8 +434,10 @@ static __always_inline void queued_spin_lock_mcs_queue(struct qspinlock *lock, b
>  		WRITE_ONCE(prev->next, node);
>  
>  		/* Wait for mcs node lock to be released */
> +		spin_begin();
>  		while (!node->locked)
>  			yield_to_prev(lock, node, prev_cpu, paravirt);
> +		spin_end();
>  
>  		/* Clear out stale propagated yield_cpu */
>  		if (paravirt && pv_yield_propagate_owner && node->yield_cpu != -1)
> @@ -432,10 +450,12 @@ static __always_inline void queued_spin_lock_mcs_queue(struct qspinlock *lock, b
>  		int set_yield_cpu = -1;
>  
>  		/* We're at the head of the waitqueue, wait for the lock. */
> +		spin_begin();
>  		while ((val = READ_ONCE(lock->val)) & _Q_LOCKED_VAL) {
>  			propagate_yield_cpu(node, val, &set_yield_cpu, paravirt);
>  			yield_head_to_locked_owner(lock, val, paravirt, false);
>  		}
> +		spin_end();
>  
>  		/* If we're the last queued, must clean up the tail. */
>  		if ((val & _Q_TAIL_CPU_MASK) == tail) {
> @@ -453,6 +473,7 @@ static __always_inline void queued_spin_lock_mcs_queue(struct qspinlock *lock, b
>  
>  again:
>  		/* We're at the head of the waitqueue, wait for the lock. */
> +		spin_begin();
>  		while ((val = READ_ONCE(lock->val)) & _Q_LOCKED_VAL) {
>  			propagate_yield_cpu(node, val, &set_yield_cpu, paravirt);
>  			yield_head_to_locked_owner(lock, val, paravirt,
> @@ -465,6 +486,7 @@ static __always_inline void queued_spin_lock_mcs_queue(struct qspinlock *lock, b
>  				val |= _Q_MUST_Q_VAL;
>  			}
>  		}
> +		spin_end();
>  
>  		/* If we're the last queued, must clean up the tail. */
>  		if ((val & _Q_TAIL_CPU_MASK) == tail) {
> @@ -480,8 +502,13 @@ static __always_inline void queued_spin_lock_mcs_queue(struct qspinlock *lock, b
>  
>  unlock_next:
>  	/* contended path; must wait for next != NULL (MCS protocol) */
> -	while (!(next = READ_ONCE(node->next)))
> -		cpu_relax();
> +	next = READ_ONCE(node->next);
> +	if (!next) {
> +		spin_begin();
> +		while (!(next = READ_ONCE(node->next)))
> +			cpu_relax();
> +		spin_end();
> +	}
>  
>  	/*
>  	 * Unlock the next mcs waiter node. Release barrier is not required