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

[Nicholas Piggin]

Excerpts from Peter Zijlstra's message of July 6, 2022 2:57 am:
> 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@xxxxxxxxx>
>> ---
> 
>>  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.
> 

Yeah I could mention it in the changelog, I just didn't because it is
practically equivalent.

Thanks,
Nick