Re: [PATCH 02/13] locking/qspinlock: inline mcs_spinlock functions into qspinlock

[Peter Zijlstra <peterz@infradead.org>]

On Tue, Jul 05, 2022 at 12:38:09AM +1000, Nicholas Piggin wrote:
> qspinlock uses mcs_spinlock for the struct type (.next, .locked, and the
> misplaced .count), and arch_mcs_spin_{un}lock_contended(). These can be
> trivially inlined into qspinlock, and the unused mcs_spinlock code
> removed.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---

>  arch/arm/include/asm/mcs_spinlock.h |  24 ------

> --- a/arch/arm/include/asm/mcs_spinlock.h
> +++ /dev/null
> @@ -1,24 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0 */
> -#ifndef __ASM_MCS_LOCK_H
> -#define __ASM_MCS_LOCK_H
> -
> -#ifdef CONFIG_SMP
> -#include <asm/spinlock.h>
> -
> -/* MCS spin-locking. */
> -#define arch_mcs_spin_lock_contended(lock)				\
> -do {									\
> -	/* Ensure prior stores are observed before we enter wfe. */	\
> -	smp_mb();							\
> -	while (!(smp_load_acquire(lock)))				\
> -		wfe();							\
> -} while (0)								\
> -
> -#define arch_mcs_spin_unlock_contended(lock)				\
> -do {									\
> -	smp_store_release(lock, 1);					\
> -	dsb_sev();							\
> -} while (0)
> -
> -#endif	/* CONFIG_SMP */
> -#endif	/* __ASM_MCS_LOCK_H */

> @@ -475,7 +476,8 @@ void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val)
>  		WRITE_ONCE(prev->next, node);
>  
>  		pv_wait_node(node, prev);
> -		arch_mcs_spin_lock_contended(&node->locked);
> +		/* Wait for mcs node lock to be released */
> +		smp_cond_load_acquire(&node->locked, VAL);
>  
>  		/*
>  		 * While waiting for the MCS lock, the next pointer may have
> @@ -554,7 +556,7 @@ void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val)
>  	if (!next)
>  		next = smp_cond_load_relaxed(&node->next, (VAL));
>  
> -	arch_mcs_spin_unlock_contended(&next->locked);
> +	smp_store_release(&next->locked, 1); /* unlock the mcs node lock */

These are not equivalent. Now it so happens that ARM doesn't use
qspinlock and the other mcs lock users are gone by now, but something
like this should at least have been called out in the Changelog I think.