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]

Message-ID: <YsRtaOnO9vlxJvLi@worktop.programming.kicks-ass.net>
Date:   Tue, 5 Jul 2022 18:57:12 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Nicholas Piggin <npiggin@...il.com>
Cc:     Ingo Molnar <mingo@...hat.com>, Will Deacon <will@...nel.org>,
        Waiman Long <longman@...hat.com>,
        Boqun Feng <boqun.feng@...il.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 02/13] locking/qspinlock: inline mcs_spinlock functions
 into qspinlock

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.