linux-kernel - Re: [PATCH 05/13] locking/qspinlock: be less clever with the preprocessor

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: <9bb0e3b3-288f-31d2-ef82-684386265b3e@redhat.com>
Date:   Tue, 5 Jul 2022 16:02:05 -0400
From:   Waiman Long <longman@...hat.com>
To:     Nicholas Piggin <npiggin@...il.com>,
        Peter Zijlstra <peterz@...radead.org>
Cc:     Ingo Molnar <mingo@...hat.com>, Will Deacon <will@...nel.org>,
        Boqun Feng <boqun.feng@...il.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 05/13] locking/qspinlock: be less clever with the
 preprocessor

On 7/4/22 10:38, Nicholas Piggin wrote:
> Stop qspinlock.c including itself and avoid most of the function
> renaming with the preprocessor.
>
> This is mostly done by having the common slowpath code take a 'bool
> paravirt' argument and adjusting code based on that.
>
> Signed-off-by: Nicholas Piggin <npiggin@...il.com>
> ---
>   kernel/locking/qspinlock.c          | 116 ++++++++++++----------------
>   kernel/locking/qspinlock_paravirt.h |  10 +--
>   2 files changed, 52 insertions(+), 74 deletions(-)
>
> diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c
> index 8f2173e22479..b96c58ca51de 100644
> --- a/kernel/locking/qspinlock.c
> +++ b/kernel/locking/qspinlock.c
> @@ -11,8 +11,6 @@
>    *          Peter Zijlstra <peterz@...radead.org>
>    */
>   
> -#ifndef _GEN_PV_LOCK_SLOWPATH
> -
>   #include <linux/smp.h>
>   #include <linux/bug.h>
>   #include <linux/cpumask.h>
> @@ -285,35 +283,21 @@ static __always_inline void set_locked(struct qspinlock *lock)
>   	WRITE_ONCE(lock->locked, _Q_LOCKED_VAL);
>   }
>   
> -
> -/*
> - * Generate the native code for queued_spin_unlock_slowpath(); provide NOPs for
> - * all the PV callbacks.
> - */
> -
> -static __always_inline void __pv_init_node(struct qnode *node) { }
> -static __always_inline void __pv_wait_node(struct qnode *node,
> -					   struct qnode *prev) { }
> -static __always_inline void __pv_kick_node(struct qspinlock *lock,
> -					   struct qnode *node) { }
> -static __always_inline u32  __pv_wait_head_or_lock(struct qspinlock *lock,
> -						   struct qnode *node)
> -						   { return 0; }
> -
> -#define pv_enabled()		false
> -
> -#define pv_init_node		__pv_init_node
> -#define pv_wait_node		__pv_wait_node
> -#define pv_kick_node		__pv_kick_node
> -#define pv_wait_head_or_lock	__pv_wait_head_or_lock
> -
>   #ifdef CONFIG_PARAVIRT_SPINLOCKS
> -#define queued_spin_lock_slowpath	native_queued_spin_lock_slowpath
> -#endif
> -
> -#endif /* _GEN_PV_LOCK_SLOWPATH */
> +#include "qspinlock_paravirt.h"
> +#else /* CONFIG_PARAVIRT_SPINLOCKS */
> +static __always_inline void pv_init_node(struct qnode *node) { }
> +static __always_inline void pv_wait_node(struct qnode *node,
> +					 struct qnode *prev) { }
> +static __always_inline void pv_kick_node(struct qspinlock *lock,
> +					 struct qnode *node) { }
> +static __always_inline u32  pv_wait_head_or_lock(struct qspinlock *lock,
> +						 struct qnode *node)
> +						   { return 0; }
> +static __always_inline bool pv_hybrid_queued_unfair_trylock(struct qspinlock *lock) { BUILD_BUG(); }
> +#endif /* CONFIG_PARAVIRT_SPINLOCKS */
>   
> -static inline void queued_spin_lock_mcs_queue(struct qspinlock *lock)
> +static inline void queued_spin_lock_mcs_queue(struct qspinlock *lock, bool paravirt)

Using "const bool paravirt" may help the compiler generating better code 
by eliminating dead one, if it is not doing that already.

>   {
>   	struct qnode *prev, *next, *node;
>   	u32 val, old, tail;
> @@ -338,8 +322,13 @@ static inline void queued_spin_lock_mcs_queue(struct qspinlock *lock)
>   	 */
>   	if (unlikely(idx >= MAX_NODES)) {
>   		lockevent_inc(lock_no_node);
> -		while (!queued_spin_trylock(lock))
> -			cpu_relax();
> +		if (paravirt) {
> +			while (!pv_hybrid_queued_unfair_trylock(lock))
> +				cpu_relax();
> +		} else {
> +			while (!queued_spin_trylock(lock))
> +				cpu_relax();
> +		}

The code will look a bit better if you add the following helper function 
and use it instead.

static inline bool queued_spin_trylock_common(struct qspinlock *lock, 
const bool paravirt)
{
         if (paravirt)
                 return pv_hybrid_queued_unfair_trylock(lock);
         else
                 return queued_spin_trylock(lock);
}

Cheers,
Longman