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.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ