linux-kernel - Re: [PATCH 02/13] locking/qspinlock: inline mcs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 12 Jul 2022 10:06:52 +1000
From:   Nicholas Piggin <npiggin@...il.com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Boqun Feng <boqun.feng@...il.com>, linux-kernel@...r.kernel.org,
        Waiman Long <longman@...hat.com>,
        Ingo Molnar <mingo@...hat.com>, Will Deacon <will@...nel.org>
Subject: Re: [PATCH 02/13] locking/qspinlock: inline mcs_spinlock functions
 into qspinlock

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@...il.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.
> 

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

Thanks,
Nick