linux-kernel - Re: [PATCH V7 1/5] asm-generic: ticket-lock: Remove unnecessary atomic

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, 28 Jun 2022 14:05:55 -0400
From:   Waiman Long <longman@...hat.com>
To:     guoren@...nel.org, palmer@...osinc.com, arnd@...db.de,
        mingo@...hat.com, will@...nel.org, boqun.feng@...il.com
Cc:     linux-riscv@...ts.infradead.org, linux-arch@...r.kernel.org,
        linux-kernel@...r.kernel.org, Guo Ren <guoren@...ux.alibaba.com>,
        Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH V7 1/5] asm-generic: ticket-lock: Remove unnecessary
 atomic_read

On 6/28/22 04:17, guoren@...nel.org wrote:
> From: Guo Ren <guoren@...ux.alibaba.com>
>
> Remove unnecessary atomic_read in arch_spin_value_unlocked(lock),
> because the value has been in lock. This patch could prevent
> arch_spin_value_unlocked contend spin_lock data again.
>
> Signed-off-by: Guo Ren <guoren@...ux.alibaba.com>
> Signed-off-by: Guo Ren <guoren@...nel.org>
> Cc: Peter Zijlstra (Intel) <peterz@...radead.org>
> Cc: Arnd Bergmann <arnd@...db.de>
> Cc: Palmer Dabbelt <palmer@...osinc.com>
> ---
>   include/asm-generic/spinlock.h | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/include/asm-generic/spinlock.h b/include/asm-generic/spinlock.h
> index fdfebcb050f4..f1e4fa100f5a 100644
> --- a/include/asm-generic/spinlock.h
> +++ b/include/asm-generic/spinlock.h
> @@ -84,7 +84,9 @@ static __always_inline int arch_spin_is_contended(arch_spinlock_t *lock)
>   
>   static __always_inline int arch_spin_value_unlocked(arch_spinlock_t lock)
>   {
> -	return !arch_spin_is_locked(&lock);
> +	u32 val = lock.counter;
> +
> +	return ((val >> 16) == (val & 0xffff));
>   }
>   
>   #include <asm/qrwlock.h>

lockref.c is the only current user of arch_spin_value_unlocked(). This 
change is probably OK with this particular use case. Do you have any 
performance data about the improvement due to this change?

You may have to document that we have to revisit that if another use 
case shows up.

Cheers,
Longman