linux-kernel - Re: [PATCH v3 11/11] riscv: Add qspinlock support

Open Source and information security mailing list archives

Message-ID: <2c227c58-d414-4994-9382-11feb80bb818@ghiti.fr>
Date: Thu, 18 Jul 2024 15:08:39 +0200
From: Alexandre Ghiti <alex@...ti.fr>
To: Andrea Parri <parri.andrea@...il.com>,
 Alexandre Ghiti <alexghiti@...osinc.com>
Cc: Jonathan Corbet <corbet@....net>, Paul Walmsley
 <paul.walmsley@...ive.com>, Palmer Dabbelt <palmer@...belt.com>,
 Albert Ou <aou@...s.berkeley.edu>, Conor Dooley <conor@...nel.org>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Nathan Chancellor <nathan@...nel.org>, Peter Zijlstra
 <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
 Will Deacon <will@...nel.org>, Waiman Long <longman@...hat.com>,
 Boqun Feng <boqun.feng@...il.com>, Arnd Bergmann <arnd@...db.de>,
 Leonardo Bras <leobras@...hat.com>, Guo Ren <guoren@...nel.org>,
 linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-riscv@...ts.infradead.org, linux-arch@...r.kernel.org
Subject: Re: [PATCH v3 11/11] riscv: Add qspinlock support

Hi Andrea,

On 17/07/2024 18:29, Andrea Parri wrote:
>> +config RISCV_QUEUED_SPINLOCKS
> I'm seeing the following warnings with CONFIG_RISCV_QUEUED_SPINLOCKS=y:
>
> In file included from ./arch/riscv/include/generated/asm/qspinlock.h:1,
>                   from kernel/locking/qspinlock.c:24:
> ./include/asm-generic/qspinlock.h:144:9: warning: "arch_spin_is_locked" redefined
>    144 | #define arch_spin_is_locked(l)          queued_spin_is_locked(l)
>        |         ^~~~~~~~~~~~~~~~~~~
> In file included from ./arch/riscv/include/generated/asm/ticket_spinlock.h:1,
>                   from ./arch/riscv/include/asm/spinlock.h:33,
>                   from ./include/linux/spinlock.h:95,
>                   from ./include/linux/sched.h:2142,
>                   from ./include/linux/percpu.h:13,
>                   from kernel/locking/qspinlock.c:19:
> ./include/asm-generic/ticket_spinlock.h:97:9: note: this is the location of the previous definition
>     97 | #define arch_spin_is_locked(l)          ticket_spin_is_locked(l)
>        |         ^~~~~~~~~~~~~~~~~~~
> ./include/asm-generic/qspinlock.h:145:9: warning: "arch_spin_is_contended" redefined
>    145 | #define arch_spin_is_contended(l)       queued_spin_is_contended(l)
>        |         ^~~~~~~~~~~~~~~~~~~~~~
> ./include/asm-generic/ticket_spinlock.h:98:9: note: this is the location of the previous definition
>     98 | #define arch_spin_is_contended(l)       ticket_spin_is_contended(l)
>        |         ^~~~~~~~~~~~~~~~~~~~~~
> ./include/asm-generic/qspinlock.h:146:9: warning: "arch_spin_value_unlocked" redefined
>    146 | #define arch_spin_value_unlocked(l)     queued_spin_value_unlocked(l)
>        |         ^~~~~~~~~~~~~~~~~~~~~~~~
> ./include/asm-generic/ticket_spinlock.h:99:9: note: this is the location of the previous definition
>     99 | #define arch_spin_value_unlocked(l)     ticket_spin_value_unlocked(l)
>        |         ^~~~~~~~~~~~~~~~~~~~~~~~
> ./include/asm-generic/qspinlock.h:147:9: warning: "arch_spin_lock" redefined
>    147 | #define arch_spin_lock(l)               queued_spin_lock(l)
>        |         ^~~~~~~~~~~~~~
> ./include/asm-generic/ticket_spinlock.h:100:9: note: this is the location of the previous definition
>    100 | #define arch_spin_lock(l)               ticket_spin_lock(l)
>        |         ^~~~~~~~~~~~~~
> ./include/asm-generic/qspinlock.h:148:9: warning: "arch_spin_trylock" redefined
>    148 | #define arch_spin_trylock(l)            queued_spin_trylock(l)
>        |         ^~~~~~~~~~~~~~~~~
> ./include/asm-generic/ticket_spinlock.h:101:9: note: this is the location of the previous definition
>    101 | #define arch_spin_trylock(l)            ticket_spin_trylock(l)
>        |         ^~~~~~~~~~~~~~~~~
> ./include/asm-generic/qspinlock.h:149:9: warning: "arch_spin_unlock" redefined
>    149 | #define arch_spin_unlock(l)             queued_spin_unlock(l)
>        |         ^~~~~~~~~~~~~~~~
> ./include/asm-generic/ticket_spinlock.h:102:9: note: this is the location of the previous definition
>    102 | #define arch_spin_unlock(l)             ticket_spin_unlock(l)
>
>
> The following diff resolves them for me (please double check):
>
> diff --git a/arch/riscv/include/asm/spinlock.h b/arch/riscv/include/asm/spinlock.h
> index 4856d50006f28..2d59f56a9e2d1 100644
> --- a/arch/riscv/include/asm/spinlock.h
> +++ b/arch/riscv/include/asm/spinlock.h
> @@ -30,7 +30,11 @@ SPINLOCK_BASE_DECLARE(value_unlocked, int, arch_spinlock_t)
>   
>   #else
>   
> +#if defined(CONFIG_RISCV_TICKET_SPINLOCKS)
>   #include <asm/ticket_spinlock.h>
> +#elif defined(CONFIG_RISCV_QUEUED_SPINLOCKS)
> +#include <asm/qspinlock.h>
> +#endif
>   
>   #endif

Thanks for testing this config (when I did not...)!

I came up with something slightly different, but same fix in the end, 
thanks!

>
>> +DEFINE_STATIC_KEY_TRUE(qspinlock_key);
>> +EXPORT_SYMBOL(qspinlock_key);
>> +
>> +static void __init riscv_spinlock_init(void)
>> +{
>> +	char *using_ext;
>> +
>> +	if (IS_ENABLED(CONFIG_RISCV_ISA_ZACAS) &&
>> +	    IS_ENABLED(CONFIG_RISCV_ISA_ZABHA)) {
>> +		using_ext = "using Zabha";
>> +
>> +		asm goto(ALTERNATIVE("j %[no_zacas]", "nop", 0, RISCV_ISA_EXT_ZACAS, 1)
>> +			 : : : : no_zacas);
>> +		asm goto(ALTERNATIVE("nop", "j %[qspinlock]", 0, RISCV_ISA_EXT_ZABHA, 1)
>> +			 : : : : qspinlock);
>> +	}
>> +
>> +no_zacas:
>> +	using_ext = "using Ziccrse";
>> +	asm goto(ALTERNATIVE("nop", "j %[qspinlock]", 0,
>> +			     RISCV_ISA_EXT_ZICCRSE, 1)
>> +		 : : : : qspinlock);
>> +
>> +	static_branch_disable(&qspinlock_key);
>> +	pr_info("Ticket spinlock: enabled\n");
>> +
>> +	return;
>> +
>> +qspinlock:
>> +	pr_info("Queued spinlock %s: enabled\n", using_ext);
>> +}
>> +
> Your commit message suggests that riscv_spinlock_init() doesn't need to
> do anything if CONFIG_RISCV_COMBO_SPINLOCKS=n:
>
> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> index d7c31c9b8ead2..b2be1b0b700d2 100644
> --- a/arch/riscv/kernel/setup.c
> +++ b/arch/riscv/kernel/setup.c
> @@ -244,6 +244,7 @@ static void __init parse_dtb(void)
>   #endif
>   }
>   
> +#if defined(CONFIG_RISCV_COMBO_SPINLOCKS)
>   DEFINE_STATIC_KEY_TRUE(qspinlock_key);
>   EXPORT_SYMBOL(qspinlock_key);
>   
> @@ -275,6 +276,11 @@ static void __init riscv_spinlock_init(void)
>   qspinlock:
>   	pr_info("Queued spinlock %s: enabled\n", using_ext);
>   }
> +#else
> +static void __init riscv_spinlock_init(void)
> +{
> +}
> +#endif
>   
>   extern void __init init_rt_signal_env(void);
>
>
> Makes sense?  What am I missing?

Totally makes sense, I completely overlooked this when I added the 
ticket/queued configs, thanks for taking the time to look into it.

That will be fixed in the next version.

Thanks again,

Alex

>
>    Andrea
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

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