* Re: [PATCH v2 10/10] riscv: Add qspinlock support based on Zabha extension
@ 2024-07-07 2:20 ` Guo Ren
0 siblings, 0 replies; 25+ messages in thread
From: Guo Ren @ 2024-07-07 2:20 UTC (permalink / raw)
To: Alexandre Ghiti
Cc: Jonathan Corbet, Paul Walmsley, Palmer Dabbelt, Albert Ou,
Andrea Parri, Nathan Chancellor, Peter Zijlstra, Ingo Molnar,
Will Deacon, Waiman Long, Boqun Feng, Arnd Bergmann,
Leonardo Bras, linux-doc, linux-kernel, linux-riscv, linux-arch
On Wed, Jun 26, 2024 at 9:14 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
>
> In order to produce a generic kernel, a user can select
> CONFIG_COMBO_SPINLOCKS which will fallback at runtime to the ticket
> spinlock implementation if Zabha is not present.
>
> Note that we can't use alternatives here because the discovery of
> extensions is done too late and we need to start with the qspinlock
That's not true; we treat spinlock as qspinlock at first.
qspinlock_unlock would make the lock value zero (clean), but
ticket_lock would make a dirty one. (I've spent much time on this
mechanism, and you've preserved it in this patch.) So, making the
qspinlock -> ticket_lock change point safe until sched_init() is late
enough to make alternatives. The key problem of alternative
implementation is tough coding because you can't reuse the C code. The
whole ticket_lock must be rewritten in asm and include the qspinlock
fast path.
I think we should discuss some points before continuing the patch:
1. Using alternative mechanisms for combo spinlock
2. Using three Kconfigs for
ticket_spinlock/queune_spinlock/combo_spinlock during compile stage.
3. The forward progress guarantee requirement is written in
qspinlock.h comment. That is not about our CAS/BHA.
> implementation because the ticket spinlock implementation would pollute
> the spinlock value, so let's use static keys.
>
> This is largely based on Guo's work and Leonardo reviews at [1].
>
> Link: https://lore.kernel.org/linux-riscv/20231225125847.2778638-1-guoren@kernel.org/ [1]
> Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> ---
> .../locking/queued-spinlocks/arch-support.txt | 2 +-
> arch/riscv/Kconfig | 10 +++++
> arch/riscv/include/asm/Kbuild | 4 +-
> arch/riscv/include/asm/spinlock.h | 39 +++++++++++++++++++
> arch/riscv/kernel/setup.c | 21 ++++++++++
> include/asm-generic/qspinlock.h | 2 +
> include/asm-generic/ticket_spinlock.h | 2 +
> 7 files changed, 78 insertions(+), 2 deletions(-)
> create mode 100644 arch/riscv/include/asm/spinlock.h
>
> diff --git a/Documentation/features/locking/queued-spinlocks/arch-support.txt b/Documentation/features/locking/queued-spinlocks/arch-support.txt
> index 22f2990392ff..cf26042480e2 100644
> --- a/Documentation/features/locking/queued-spinlocks/arch-support.txt
> +++ b/Documentation/features/locking/queued-spinlocks/arch-support.txt
> @@ -20,7 +20,7 @@
> | openrisc: | ok |
> | parisc: | TODO |
> | powerpc: | ok |
> - | riscv: | TODO |
> + | riscv: | ok |
> | s390: | TODO |
> | sh: | TODO |
> | sparc: | ok |
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 0bbaec0444d0..c2ba673e58ca 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -72,6 +72,7 @@ config RISCV
> select ARCH_WANT_OPTIMIZE_HUGETLB_VMEMMAP
> select ARCH_WANTS_NO_INSTR
> select ARCH_WANTS_THP_SWAP if HAVE_ARCH_TRANSPARENT_HUGEPAGE
> + select ARCH_WEAK_RELEASE_ACQUIRE if ARCH_USE_QUEUED_SPINLOCKS
> select BINFMT_FLAT_NO_DATA_START_OFFSET if !MMU
> select BUILDTIME_TABLE_SORT if MMU
> select CLINT_TIMER if RISCV_M_MODE
> @@ -482,6 +483,15 @@ config NODES_SHIFT
> Specify the maximum number of NUMA Nodes available on the target
> system. Increases memory reserved to accommodate various tables.
>
> +config RISCV_COMBO_SPINLOCKS
> + bool "Using combo spinlock"
> + depends on SMP && MMU && TOOLCHAIN_HAS_ZABHA
> + select ARCH_USE_QUEUED_SPINLOCKS
> + default y
> + help
> + Embed both queued spinlock and ticket lock so that the spinlock
> + implementation can be chosen at runtime.
> +
COMBO SPINLOCK has side effects, which would expand spinlock code size
a lot. Ref: ARCH_INLINE_SPIN_LOCK
So, we shouldn't remove the three configs' selection.
+choice
+ prompt "RISC-V spinlock type"
+ default RISCV_COMBO_SPINLOCKS
+
+config RISCV_TICKET_SPINLOCKS
+ bool "Using ticket spinlock"
+
+config RISCV_QUEUED_SPINLOCKS
+ bool "Using queued spinlock"
+ depends on SMP && MMU
+ select ARCH_USE_QUEUED_SPINLOCKS
+ help
+ Make sure your micro arch give cmpxchg/xchg forward progress
+ guarantee. Otherwise, stay at ticket-lock.
+
+config RISCV_COMBO_SPINLOCKS
+ bool "Using combo spinlock"
+ depends on SMP && MMU
+ select ARCH_USE_QUEUED_SPINLOCKS
+ help
+ Select queued spinlock or ticket-lock by cmdline.
+endchoice
+
> config RISCV_ALTERNATIVE
> bool
> depends on !XIP_KERNEL
> diff --git a/arch/riscv/include/asm/Kbuild b/arch/riscv/include/asm/Kbuild
> index 504f8b7e72d4..ad72f2bd4cc9 100644
> --- a/arch/riscv/include/asm/Kbuild
> +++ b/arch/riscv/include/asm/Kbuild
> @@ -2,10 +2,12 @@
> generic-y += early_ioremap.h
> generic-y += flat.h
> generic-y += kvm_para.h
> +generic-y += mcs_spinlock.h
> generic-y += parport.h
> -generic-y += spinlock.h
> generic-y += spinlock_types.h
> +generic-y += ticket_spinlock.h
> generic-y += qrwlock.h
> generic-y += qrwlock_types.h
> +generic-y += qspinlock.h
> generic-y += user.h
> generic-y += vmlinux.lds.h
> diff --git a/arch/riscv/include/asm/spinlock.h b/arch/riscv/include/asm/spinlock.h
> new file mode 100644
> index 000000000000..4856d50006f2
> --- /dev/null
> +++ b/arch/riscv/include/asm/spinlock.h
> @@ -0,0 +1,39 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef __ASM_RISCV_SPINLOCK_H
> +#define __ASM_RISCV_SPINLOCK_H
> +
> +#ifdef CONFIG_RISCV_COMBO_SPINLOCKS
> +#define _Q_PENDING_LOOPS (1 << 9)
> +
> +#define __no_arch_spinlock_redefine
> +#include <asm/ticket_spinlock.h>
> +#include <asm/qspinlock.h>
> +#include <asm/alternative.h>
> +
> +DECLARE_STATIC_KEY_TRUE(qspinlock_key);
> +
> +#define SPINLOCK_BASE_DECLARE(op, type, type_lock) \
> +static __always_inline type arch_spin_##op(type_lock lock) \
> +{ \
> + if (static_branch_unlikely(&qspinlock_key)) \
> + return queued_spin_##op(lock); \
> + return ticket_spin_##op(lock); \
> +}
> +
> +SPINLOCK_BASE_DECLARE(lock, void, arch_spinlock_t *)
> +SPINLOCK_BASE_DECLARE(unlock, void, arch_spinlock_t *)
> +SPINLOCK_BASE_DECLARE(is_locked, int, arch_spinlock_t *)
> +SPINLOCK_BASE_DECLARE(is_contended, int, arch_spinlock_t *)
> +SPINLOCK_BASE_DECLARE(trylock, bool, arch_spinlock_t *)
> +SPINLOCK_BASE_DECLARE(value_unlocked, int, arch_spinlock_t)
> +
> +#else
> +
> +#include <asm/ticket_spinlock.h>
> +
> +#endif
> +
> +#include <asm/qrwlock.h>
> +
> +#endif /* __ASM_RISCV_SPINLOCK_H */
> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> index 4f73c0ae44b2..929bccd4c9e5 100644
> --- a/arch/riscv/kernel/setup.c
> +++ b/arch/riscv/kernel/setup.c
> @@ -244,6 +244,26 @@ static void __init parse_dtb(void)
> #endif
> }
>
> +DEFINE_STATIC_KEY_TRUE(qspinlock_key);
> +EXPORT_SYMBOL(qspinlock_key);
> +
> +static void __init riscv_spinlock_init(void)
> +{
> + 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);
The requirement of qspinlock concerns the forward progress guarantee
in micro-arch design, which includes cmpxchg_loop & xchg_tail. So, I
don't think these features have a relationship with Qspinlock.
If your machine doesn't have enough stickiness for a young exclusive
cacheline, fall back to ticket_lock.
> +
> +no_zacas:
> + static_branch_disable(&qspinlock_key);
> + pr_info("Ticket spinlock: enabled\n");
> +
> + return;
> +
> +qspinlock:
> + pr_info("Queued spinlock: enabled\n");
> +}
> +
> extern void __init init_rt_signal_env(void);
>
> void __init setup_arch(char **cmdline_p)
> @@ -295,6 +315,7 @@ void __init setup_arch(char **cmdline_p)
> riscv_set_dma_cache_alignment();
>
> riscv_user_isa_enable();
> + riscv_spinlock_init();
> }
>
> bool arch_cpu_is_hotpluggable(int cpu)
> diff --git a/include/asm-generic/qspinlock.h b/include/asm-generic/qspinlock.h
> index 0655aa5b57b2..bf47cca2c375 100644
> --- a/include/asm-generic/qspinlock.h
> +++ b/include/asm-generic/qspinlock.h
> @@ -136,6 +136,7 @@ static __always_inline bool virt_spin_lock(struct qspinlock *lock)
> }
> #endif
>
> +#ifndef __no_arch_spinlock_redefine
> /*
> * Remapping spinlock architecture specific functions to the corresponding
> * queued spinlock functions.
> @@ -146,5 +147,6 @@ static __always_inline bool virt_spin_lock(struct qspinlock *lock)
> #define arch_spin_lock(l) queued_spin_lock(l)
> #define arch_spin_trylock(l) queued_spin_trylock(l)
> #define arch_spin_unlock(l) queued_spin_unlock(l)
> +#endif
>
> #endif /* __ASM_GENERIC_QSPINLOCK_H */
> diff --git a/include/asm-generic/ticket_spinlock.h b/include/asm-generic/ticket_spinlock.h
> index cfcff22b37b3..325779970d8a 100644
> --- a/include/asm-generic/ticket_spinlock.h
> +++ b/include/asm-generic/ticket_spinlock.h
> @@ -89,6 +89,7 @@ static __always_inline int ticket_spin_is_contended(arch_spinlock_t *lock)
> return (s16)((val >> 16) - (val & 0xffff)) > 1;
> }
>
> +#ifndef __no_arch_spinlock_redefine
> /*
> * Remapping spinlock architecture specific functions to the corresponding
> * ticket spinlock functions.
> @@ -99,5 +100,6 @@ static __always_inline int ticket_spin_is_contended(arch_spinlock_t *lock)
> #define arch_spin_lock(l) ticket_spin_lock(l)
> #define arch_spin_trylock(l) ticket_spin_trylock(l)
> #define arch_spin_unlock(l) ticket_spin_unlock(l)
> +#endif
>
> #endif /* __ASM_GENERIC_TICKET_SPINLOCK_H */
> --
> 2.39.2
>
--
Best Regards
Guo Ren
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v2 10/10] riscv: Add qspinlock support based on Zabha extension
2024-07-07 2:20 ` Guo Ren
@ 2024-07-08 11:51 ` Guo Ren
-1 siblings, 0 replies; 25+ messages in thread
From: Guo Ren @ 2024-07-08 11:51 UTC (permalink / raw)
To: Alexandre Ghiti
Cc: Jonathan Corbet, Paul Walmsley, Palmer Dabbelt, Albert Ou,
Andrea Parri, Nathan Chancellor, Peter Zijlstra, Ingo Molnar,
Will Deacon, Waiman Long, Boqun Feng, Arnd Bergmann,
Leonardo Bras, linux-doc, linux-kernel, linux-riscv, linux-arch
On Sun, Jul 7, 2024 at 10:20 AM Guo Ren <guoren@kernel.org> wrote:
>
> On Wed, Jun 26, 2024 at 9:14 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
> >
> > In order to produce a generic kernel, a user can select
> > CONFIG_COMBO_SPINLOCKS which will fallback at runtime to the ticket
> > spinlock implementation if Zabha is not present.
> >
> > Note that we can't use alternatives here because the discovery of
> > extensions is done too late and we need to start with the qspinlock
> That's not true; we treat spinlock as qspinlock at first.
> qspinlock_unlock would make the lock value zero (clean), but
> ticket_lock would make a dirty one. (I've spent much time on this
> mechanism, and you've preserved it in this patch.) So, making the
> qspinlock -> ticket_lock change point safe until sched_init() is late
> enough to make alternatives. The key problem of alternative
> implementation is tough coding because you can't reuse the C code. The
> whole ticket_lock must be rewritten in asm and include the qspinlock
> fast path.
>
> I think we should discuss some points before continuing the patch:
> 1. Using alternative mechanisms for combo spinlock
> 2. Using three Kconfigs for
> ticket_spinlock/queune_spinlock/combo_spinlock during compile stage.
> 3. The forward progress guarantee requirement is written in
> qspinlock.h comment. That is not about our CAS/BHA.
>
> > implementation because the ticket spinlock implementation would pollute
> > the spinlock value, so let's use static keys.
> >
> > This is largely based on Guo's work and Leonardo reviews at [1].
> >
> > Link: https://lore.kernel.org/linux-riscv/20231225125847.2778638-1-guoren@kernel.org/ [1]
> > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> > ---
> > .../locking/queued-spinlocks/arch-support.txt | 2 +-
> > arch/riscv/Kconfig | 10 +++++
> > arch/riscv/include/asm/Kbuild | 4 +-
> > arch/riscv/include/asm/spinlock.h | 39 +++++++++++++++++++
> > arch/riscv/kernel/setup.c | 21 ++++++++++
> > include/asm-generic/qspinlock.h | 2 +
> > include/asm-generic/ticket_spinlock.h | 2 +
> > 7 files changed, 78 insertions(+), 2 deletions(-)
> > create mode 100644 arch/riscv/include/asm/spinlock.h
> >
> > diff --git a/Documentation/features/locking/queued-spinlocks/arch-support.txt b/Documentation/features/locking/queued-spinlocks/arch-support.txt
> > index 22f2990392ff..cf26042480e2 100644
> > --- a/Documentation/features/locking/queued-spinlocks/arch-support.txt
> > +++ b/Documentation/features/locking/queued-spinlocks/arch-support.txt
> > @@ -20,7 +20,7 @@
> > | openrisc: | ok |
> > | parisc: | TODO |
> > | powerpc: | ok |
> > - | riscv: | TODO |
> > + | riscv: | ok |
> > | s390: | TODO |
> > | sh: | TODO |
> > | sparc: | ok |
> > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > index 0bbaec0444d0..c2ba673e58ca 100644
> > --- a/arch/riscv/Kconfig
> > +++ b/arch/riscv/Kconfig
> > @@ -72,6 +72,7 @@ config RISCV
> > select ARCH_WANT_OPTIMIZE_HUGETLB_VMEMMAP
> > select ARCH_WANTS_NO_INSTR
> > select ARCH_WANTS_THP_SWAP if HAVE_ARCH_TRANSPARENT_HUGEPAGE
> > + select ARCH_WEAK_RELEASE_ACQUIRE if ARCH_USE_QUEUED_SPINLOCKS
> > select BINFMT_FLAT_NO_DATA_START_OFFSET if !MMU
> > select BUILDTIME_TABLE_SORT if MMU
> > select CLINT_TIMER if RISCV_M_MODE
> > @@ -482,6 +483,15 @@ config NODES_SHIFT
> > Specify the maximum number of NUMA Nodes available on the target
> > system. Increases memory reserved to accommodate various tables.
> >
> > +config RISCV_COMBO_SPINLOCKS
> > + bool "Using combo spinlock"
> > + depends on SMP && MMU && TOOLCHAIN_HAS_ZABHA
> > + select ARCH_USE_QUEUED_SPINLOCKS
> > + default y
> > + help
> > + Embed both queued spinlock and ticket lock so that the spinlock
> > + implementation can be chosen at runtime.
> > +
>
> COMBO SPINLOCK has side effects, which would expand spinlock code size
> a lot. Ref: ARCH_INLINE_SPIN_LOCK
>
> So, we shouldn't remove the three configs' selection.
>
> +choice
> + prompt "RISC-V spinlock type"
> + default RISCV_COMBO_SPINLOCKS
> +
> +config RISCV_TICKET_SPINLOCKS
> + bool "Using ticket spinlock"
> +
> +config RISCV_QUEUED_SPINLOCKS
> + bool "Using queued spinlock"
> + depends on SMP && MMU
> + select ARCH_USE_QUEUED_SPINLOCKS
> + help
> + Make sure your micro arch give cmpxchg/xchg forward progress
> + guarantee. Otherwise, stay at ticket-lock.
> +
> +config RISCV_COMBO_SPINLOCKS
> + bool "Using combo spinlock"
> + depends on SMP && MMU
> + select ARCH_USE_QUEUED_SPINLOCKS
> + help
> + Select queued spinlock or ticket-lock by cmdline.
> +endchoice
> +
>
>
>
> > config RISCV_ALTERNATIVE
> > bool
> > depends on !XIP_KERNEL
> > diff --git a/arch/riscv/include/asm/Kbuild b/arch/riscv/include/asm/Kbuild
> > index 504f8b7e72d4..ad72f2bd4cc9 100644
> > --- a/arch/riscv/include/asm/Kbuild
> > +++ b/arch/riscv/include/asm/Kbuild
> > @@ -2,10 +2,12 @@
> > generic-y += early_ioremap.h
> > generic-y += flat.h
> > generic-y += kvm_para.h
> > +generic-y += mcs_spinlock.h
> > generic-y += parport.h
> > -generic-y += spinlock.h
> > generic-y += spinlock_types.h
> > +generic-y += ticket_spinlock.h
> > generic-y += qrwlock.h
> > generic-y += qrwlock_types.h
> > +generic-y += qspinlock.h
> > generic-y += user.h
> > generic-y += vmlinux.lds.h
> > diff --git a/arch/riscv/include/asm/spinlock.h b/arch/riscv/include/asm/spinlock.h
> > new file mode 100644
> > index 000000000000..4856d50006f2
> > --- /dev/null
> > +++ b/arch/riscv/include/asm/spinlock.h
> > @@ -0,0 +1,39 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +#ifndef __ASM_RISCV_SPINLOCK_H
> > +#define __ASM_RISCV_SPINLOCK_H
> > +
> > +#ifdef CONFIG_RISCV_COMBO_SPINLOCKS
> > +#define _Q_PENDING_LOOPS (1 << 9)
> > +
> > +#define __no_arch_spinlock_redefine
> > +#include <asm/ticket_spinlock.h>
> > +#include <asm/qspinlock.h>
> > +#include <asm/alternative.h>
> > +
> > +DECLARE_STATIC_KEY_TRUE(qspinlock_key);
> > +
> > +#define SPINLOCK_BASE_DECLARE(op, type, type_lock) \
> > +static __always_inline type arch_spin_##op(type_lock lock) \
> > +{ \
> > + if (static_branch_unlikely(&qspinlock_key)) \
> > + return queued_spin_##op(lock); \
> > + return ticket_spin_##op(lock); \
> > +}
> > +
> > +SPINLOCK_BASE_DECLARE(lock, void, arch_spinlock_t *)
> > +SPINLOCK_BASE_DECLARE(unlock, void, arch_spinlock_t *)
> > +SPINLOCK_BASE_DECLARE(is_locked, int, arch_spinlock_t *)
> > +SPINLOCK_BASE_DECLARE(is_contended, int, arch_spinlock_t *)
> > +SPINLOCK_BASE_DECLARE(trylock, bool, arch_spinlock_t *)
> > +SPINLOCK_BASE_DECLARE(value_unlocked, int, arch_spinlock_t)
> > +
> > +#else
> > +
> > +#include <asm/ticket_spinlock.h>
> > +
> > +#endif
> > +
> > +#include <asm/qrwlock.h>
> > +
> > +#endif /* __ASM_RISCV_SPINLOCK_H */
> > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> > index 4f73c0ae44b2..929bccd4c9e5 100644
> > --- a/arch/riscv/kernel/setup.c
> > +++ b/arch/riscv/kernel/setup.c
> > @@ -244,6 +244,26 @@ static void __init parse_dtb(void)
> > #endif
> > }
> >
> > +DEFINE_STATIC_KEY_TRUE(qspinlock_key);
> > +EXPORT_SYMBOL(qspinlock_key);
> > +
> > +static void __init riscv_spinlock_init(void)
> > +{
> > + 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);
> The requirement of qspinlock concerns the forward progress guarantee
> in micro-arch design, which includes cmpxchg_loop & xchg_tail. So, I
> don't think these features have a relationship with Qspinlock.
>
> If your machine doesn't have enough stickiness for a young exclusive
> cacheline, fall back to ticket_lock.
Could we use "Ziccrse: Main memory supports forward progress on LR/SC
sequences" for qspinlock selection?
+ asm goto(ALTERNATIVE("nop", "j %[qspinlock]", 0,
RISCV_ISA_EXT_ZICCRSE, 1)
: : : : qspinlock);
[1] https://github.com/riscv/riscv-profiles/blob/main/src/rva23-profile.adoc
>
> > +
> > +no_zacas:
> > + static_branch_disable(&qspinlock_key);
> > + pr_info("Ticket spinlock: enabled\n");
> > +
> > + return;
> > +
> > +qspinlock:
> > + pr_info("Queued spinlock: enabled\n");
> > +}
> > +
> > extern void __init init_rt_signal_env(void);
> >
> > void __init setup_arch(char **cmdline_p)
> > @@ -295,6 +315,7 @@ void __init setup_arch(char **cmdline_p)
> > riscv_set_dma_cache_alignment();
> >
> > riscv_user_isa_enable();
> > + riscv_spinlock_init();
> > }
> >
> > bool arch_cpu_is_hotpluggable(int cpu)
> > diff --git a/include/asm-generic/qspinlock.h b/include/asm-generic/qspinlock.h
> > index 0655aa5b57b2..bf47cca2c375 100644
> > --- a/include/asm-generic/qspinlock.h
> > +++ b/include/asm-generic/qspinlock.h
> > @@ -136,6 +136,7 @@ static __always_inline bool virt_spin_lock(struct qspinlock *lock)
> > }
> > #endif
> >
> > +#ifndef __no_arch_spinlock_redefine
> > /*
> > * Remapping spinlock architecture specific functions to the corresponding
> > * queued spinlock functions.
> > @@ -146,5 +147,6 @@ static __always_inline bool virt_spin_lock(struct qspinlock *lock)
> > #define arch_spin_lock(l) queued_spin_lock(l)
> > #define arch_spin_trylock(l) queued_spin_trylock(l)
> > #define arch_spin_unlock(l) queued_spin_unlock(l)
> > +#endif
> >
> > #endif /* __ASM_GENERIC_QSPINLOCK_H */
> > diff --git a/include/asm-generic/ticket_spinlock.h b/include/asm-generic/ticket_spinlock.h
> > index cfcff22b37b3..325779970d8a 100644
> > --- a/include/asm-generic/ticket_spinlock.h
> > +++ b/include/asm-generic/ticket_spinlock.h
> > @@ -89,6 +89,7 @@ static __always_inline int ticket_spin_is_contended(arch_spinlock_t *lock)
> > return (s16)((val >> 16) - (val & 0xffff)) > 1;
> > }
> >
> > +#ifndef __no_arch_spinlock_redefine
> > /*
> > * Remapping spinlock architecture specific functions to the corresponding
> > * ticket spinlock functions.
> > @@ -99,5 +100,6 @@ static __always_inline int ticket_spin_is_contended(arch_spinlock_t *lock)
> > #define arch_spin_lock(l) ticket_spin_lock(l)
> > #define arch_spin_trylock(l) ticket_spin_trylock(l)
> > #define arch_spin_unlock(l) ticket_spin_unlock(l)
> > +#endif
> >
> > #endif /* __ASM_GENERIC_TICKET_SPINLOCK_H */
> > --
> > 2.39.2
> >
>
>
> --
> Best Regards
> Guo Ren
--
Best Regards
Guo Ren
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v2 10/10] riscv: Add qspinlock support based on Zabha extension
@ 2024-07-08 11:51 ` Guo Ren
0 siblings, 0 replies; 25+ messages in thread
From: Guo Ren @ 2024-07-08 11:51 UTC (permalink / raw)
To: Alexandre Ghiti
Cc: Jonathan Corbet, Paul Walmsley, Palmer Dabbelt, Albert Ou,
Andrea Parri, Nathan Chancellor, Peter Zijlstra, Ingo Molnar,
Will Deacon, Waiman Long, Boqun Feng, Arnd Bergmann,
Leonardo Bras, linux-doc, linux-kernel, linux-riscv, linux-arch
On Sun, Jul 7, 2024 at 10:20 AM Guo Ren <guoren@kernel.org> wrote:
>
> On Wed, Jun 26, 2024 at 9:14 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
> >
> > In order to produce a generic kernel, a user can select
> > CONFIG_COMBO_SPINLOCKS which will fallback at runtime to the ticket
> > spinlock implementation if Zabha is not present.
> >
> > Note that we can't use alternatives here because the discovery of
> > extensions is done too late and we need to start with the qspinlock
> That's not true; we treat spinlock as qspinlock at first.
> qspinlock_unlock would make the lock value zero (clean), but
> ticket_lock would make a dirty one. (I've spent much time on this
> mechanism, and you've preserved it in this patch.) So, making the
> qspinlock -> ticket_lock change point safe until sched_init() is late
> enough to make alternatives. The key problem of alternative
> implementation is tough coding because you can't reuse the C code. The
> whole ticket_lock must be rewritten in asm and include the qspinlock
> fast path.
>
> I think we should discuss some points before continuing the patch:
> 1. Using alternative mechanisms for combo spinlock
> 2. Using three Kconfigs for
> ticket_spinlock/queune_spinlock/combo_spinlock during compile stage.
> 3. The forward progress guarantee requirement is written in
> qspinlock.h comment. That is not about our CAS/BHA.
>
> > implementation because the ticket spinlock implementation would pollute
> > the spinlock value, so let's use static keys.
> >
> > This is largely based on Guo's work and Leonardo reviews at [1].
> >
> > Link: https://lore.kernel.org/linux-riscv/20231225125847.2778638-1-guoren@kernel.org/ [1]
> > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> > ---
> > .../locking/queued-spinlocks/arch-support.txt | 2 +-
> > arch/riscv/Kconfig | 10 +++++
> > arch/riscv/include/asm/Kbuild | 4 +-
> > arch/riscv/include/asm/spinlock.h | 39 +++++++++++++++++++
> > arch/riscv/kernel/setup.c | 21 ++++++++++
> > include/asm-generic/qspinlock.h | 2 +
> > include/asm-generic/ticket_spinlock.h | 2 +
> > 7 files changed, 78 insertions(+), 2 deletions(-)
> > create mode 100644 arch/riscv/include/asm/spinlock.h
> >
> > diff --git a/Documentation/features/locking/queued-spinlocks/arch-support.txt b/Documentation/features/locking/queued-spinlocks/arch-support.txt
> > index 22f2990392ff..cf26042480e2 100644
> > --- a/Documentation/features/locking/queued-spinlocks/arch-support.txt
> > +++ b/Documentation/features/locking/queued-spinlocks/arch-support.txt
> > @@ -20,7 +20,7 @@
> > | openrisc: | ok |
> > | parisc: | TODO |
> > | powerpc: | ok |
> > - | riscv: | TODO |
> > + | riscv: | ok |
> > | s390: | TODO |
> > | sh: | TODO |
> > | sparc: | ok |
> > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > index 0bbaec0444d0..c2ba673e58ca 100644
> > --- a/arch/riscv/Kconfig
> > +++ b/arch/riscv/Kconfig
> > @@ -72,6 +72,7 @@ config RISCV
> > select ARCH_WANT_OPTIMIZE_HUGETLB_VMEMMAP
> > select ARCH_WANTS_NO_INSTR
> > select ARCH_WANTS_THP_SWAP if HAVE_ARCH_TRANSPARENT_HUGEPAGE
> > + select ARCH_WEAK_RELEASE_ACQUIRE if ARCH_USE_QUEUED_SPINLOCKS
> > select BINFMT_FLAT_NO_DATA_START_OFFSET if !MMU
> > select BUILDTIME_TABLE_SORT if MMU
> > select CLINT_TIMER if RISCV_M_MODE
> > @@ -482,6 +483,15 @@ config NODES_SHIFT
> > Specify the maximum number of NUMA Nodes available on the target
> > system. Increases memory reserved to accommodate various tables.
> >
> > +config RISCV_COMBO_SPINLOCKS
> > + bool "Using combo spinlock"
> > + depends on SMP && MMU && TOOLCHAIN_HAS_ZABHA
> > + select ARCH_USE_QUEUED_SPINLOCKS
> > + default y
> > + help
> > + Embed both queued spinlock and ticket lock so that the spinlock
> > + implementation can be chosen at runtime.
> > +
>
> COMBO SPINLOCK has side effects, which would expand spinlock code size
> a lot. Ref: ARCH_INLINE_SPIN_LOCK
>
> So, we shouldn't remove the three configs' selection.
>
> +choice
> + prompt "RISC-V spinlock type"
> + default RISCV_COMBO_SPINLOCKS
> +
> +config RISCV_TICKET_SPINLOCKS
> + bool "Using ticket spinlock"
> +
> +config RISCV_QUEUED_SPINLOCKS
> + bool "Using queued spinlock"
> + depends on SMP && MMU
> + select ARCH_USE_QUEUED_SPINLOCKS
> + help
> + Make sure your micro arch give cmpxchg/xchg forward progress
> + guarantee. Otherwise, stay at ticket-lock.
> +
> +config RISCV_COMBO_SPINLOCKS
> + bool "Using combo spinlock"
> + depends on SMP && MMU
> + select ARCH_USE_QUEUED_SPINLOCKS
> + help
> + Select queued spinlock or ticket-lock by cmdline.
> +endchoice
> +
>
>
>
> > config RISCV_ALTERNATIVE
> > bool
> > depends on !XIP_KERNEL
> > diff --git a/arch/riscv/include/asm/Kbuild b/arch/riscv/include/asm/Kbuild
> > index 504f8b7e72d4..ad72f2bd4cc9 100644
> > --- a/arch/riscv/include/asm/Kbuild
> > +++ b/arch/riscv/include/asm/Kbuild
> > @@ -2,10 +2,12 @@
> > generic-y += early_ioremap.h
> > generic-y += flat.h
> > generic-y += kvm_para.h
> > +generic-y += mcs_spinlock.h
> > generic-y += parport.h
> > -generic-y += spinlock.h
> > generic-y += spinlock_types.h
> > +generic-y += ticket_spinlock.h
> > generic-y += qrwlock.h
> > generic-y += qrwlock_types.h
> > +generic-y += qspinlock.h
> > generic-y += user.h
> > generic-y += vmlinux.lds.h
> > diff --git a/arch/riscv/include/asm/spinlock.h b/arch/riscv/include/asm/spinlock.h
> > new file mode 100644
> > index 000000000000..4856d50006f2
> > --- /dev/null
> > +++ b/arch/riscv/include/asm/spinlock.h
> > @@ -0,0 +1,39 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +#ifndef __ASM_RISCV_SPINLOCK_H
> > +#define __ASM_RISCV_SPINLOCK_H
> > +
> > +#ifdef CONFIG_RISCV_COMBO_SPINLOCKS
> > +#define _Q_PENDING_LOOPS (1 << 9)
> > +
> > +#define __no_arch_spinlock_redefine
> > +#include <asm/ticket_spinlock.h>
> > +#include <asm/qspinlock.h>
> > +#include <asm/alternative.h>
> > +
> > +DECLARE_STATIC_KEY_TRUE(qspinlock_key);
> > +
> > +#define SPINLOCK_BASE_DECLARE(op, type, type_lock) \
> > +static __always_inline type arch_spin_##op(type_lock lock) \
> > +{ \
> > + if (static_branch_unlikely(&qspinlock_key)) \
> > + return queued_spin_##op(lock); \
> > + return ticket_spin_##op(lock); \
> > +}
> > +
> > +SPINLOCK_BASE_DECLARE(lock, void, arch_spinlock_t *)
> > +SPINLOCK_BASE_DECLARE(unlock, void, arch_spinlock_t *)
> > +SPINLOCK_BASE_DECLARE(is_locked, int, arch_spinlock_t *)
> > +SPINLOCK_BASE_DECLARE(is_contended, int, arch_spinlock_t *)
> > +SPINLOCK_BASE_DECLARE(trylock, bool, arch_spinlock_t *)
> > +SPINLOCK_BASE_DECLARE(value_unlocked, int, arch_spinlock_t)
> > +
> > +#else
> > +
> > +#include <asm/ticket_spinlock.h>
> > +
> > +#endif
> > +
> > +#include <asm/qrwlock.h>
> > +
> > +#endif /* __ASM_RISCV_SPINLOCK_H */
> > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> > index 4f73c0ae44b2..929bccd4c9e5 100644
> > --- a/arch/riscv/kernel/setup.c
> > +++ b/arch/riscv/kernel/setup.c
> > @@ -244,6 +244,26 @@ static void __init parse_dtb(void)
> > #endif
> > }
> >
> > +DEFINE_STATIC_KEY_TRUE(qspinlock_key);
> > +EXPORT_SYMBOL(qspinlock_key);
> > +
> > +static void __init riscv_spinlock_init(void)
> > +{
> > + 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);
> The requirement of qspinlock concerns the forward progress guarantee
> in micro-arch design, which includes cmpxchg_loop & xchg_tail. So, I
> don't think these features have a relationship with Qspinlock.
>
> If your machine doesn't have enough stickiness for a young exclusive
> cacheline, fall back to ticket_lock.
Could we use "Ziccrse: Main memory supports forward progress on LR/SC
sequences" for qspinlock selection?
+ asm goto(ALTERNATIVE("nop", "j %[qspinlock]", 0,
RISCV_ISA_EXT_ZICCRSE, 1)
: : : : qspinlock);
[1] https://github.com/riscv/riscv-profiles/blob/main/src/rva23-profile.adoc
>
> > +
> > +no_zacas:
> > + static_branch_disable(&qspinlock_key);
> > + pr_info("Ticket spinlock: enabled\n");
> > +
> > + return;
> > +
> > +qspinlock:
> > + pr_info("Queued spinlock: enabled\n");
> > +}
> > +
> > extern void __init init_rt_signal_env(void);
> >
> > void __init setup_arch(char **cmdline_p)
> > @@ -295,6 +315,7 @@ void __init setup_arch(char **cmdline_p)
> > riscv_set_dma_cache_alignment();
> >
> > riscv_user_isa_enable();
> > + riscv_spinlock_init();
> > }
> >
> > bool arch_cpu_is_hotpluggable(int cpu)
> > diff --git a/include/asm-generic/qspinlock.h b/include/asm-generic/qspinlock.h
> > index 0655aa5b57b2..bf47cca2c375 100644
> > --- a/include/asm-generic/qspinlock.h
> > +++ b/include/asm-generic/qspinlock.h
> > @@ -136,6 +136,7 @@ static __always_inline bool virt_spin_lock(struct qspinlock *lock)
> > }
> > #endif
> >
> > +#ifndef __no_arch_spinlock_redefine
> > /*
> > * Remapping spinlock architecture specific functions to the corresponding
> > * queued spinlock functions.
> > @@ -146,5 +147,6 @@ static __always_inline bool virt_spin_lock(struct qspinlock *lock)
> > #define arch_spin_lock(l) queued_spin_lock(l)
> > #define arch_spin_trylock(l) queued_spin_trylock(l)
> > #define arch_spin_unlock(l) queued_spin_unlock(l)
> > +#endif
> >
> > #endif /* __ASM_GENERIC_QSPINLOCK_H */
> > diff --git a/include/asm-generic/ticket_spinlock.h b/include/asm-generic/ticket_spinlock.h
> > index cfcff22b37b3..325779970d8a 100644
> > --- a/include/asm-generic/ticket_spinlock.h
> > +++ b/include/asm-generic/ticket_spinlock.h
> > @@ -89,6 +89,7 @@ static __always_inline int ticket_spin_is_contended(arch_spinlock_t *lock)
> > return (s16)((val >> 16) - (val & 0xffff)) > 1;
> > }
> >
> > +#ifndef __no_arch_spinlock_redefine
> > /*
> > * Remapping spinlock architecture specific functions to the corresponding
> > * ticket spinlock functions.
> > @@ -99,5 +100,6 @@ static __always_inline int ticket_spin_is_contended(arch_spinlock_t *lock)
> > #define arch_spin_lock(l) ticket_spin_lock(l)
> > #define arch_spin_trylock(l) ticket_spin_trylock(l)
> > #define arch_spin_unlock(l) ticket_spin_unlock(l)
> > +#endif
> >
> > #endif /* __ASM_GENERIC_TICKET_SPINLOCK_H */
> > --
> > 2.39.2
> >
>
>
> --
> Best Regards
> Guo Ren
--
Best Regards
Guo Ren
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v2 10/10] riscv: Add qspinlock support based on Zabha extension
2024-07-08 11:51 ` Guo Ren
@ 2024-07-15 7:33 ` Alexandre Ghiti
-1 siblings, 0 replies; 25+ messages in thread
From: Alexandre Ghiti @ 2024-07-15 7:33 UTC (permalink / raw)
To: Guo Ren
Cc: Jonathan Corbet, Paul Walmsley, Palmer Dabbelt, Albert Ou,
Andrea Parri, Nathan Chancellor, Peter Zijlstra, Ingo Molnar,
Will Deacon, Waiman Long, Boqun Feng, Arnd Bergmann,
Leonardo Bras, linux-doc, linux-kernel, linux-riscv, linux-arch
Hi Guo,
On Mon, Jul 8, 2024 at 1:51 PM Guo Ren <guoren@kernel.org> wrote:
>
> On Sun, Jul 7, 2024 at 10:20 AM Guo Ren <guoren@kernel.org> wrote:
> >
> > On Wed, Jun 26, 2024 at 9:14 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
> > >
> > > In order to produce a generic kernel, a user can select
> > > CONFIG_COMBO_SPINLOCKS which will fallback at runtime to the ticket
> > > spinlock implementation if Zabha is not present.
> > >
> > > Note that we can't use alternatives here because the discovery of
> > > extensions is done too late and we need to start with the qspinlock
> > That's not true; we treat spinlock as qspinlock at first.
> > qspinlock_unlock would make the lock value zero (clean), but
> > ticket_lock would make a dirty one. (I've spent much time on this
> > mechanism, and you've preserved it in this patch.) So, making the
> > qspinlock -> ticket_lock change point safe until sched_init() is late
> > enough to make alternatives. The key problem of alternative
> > implementation is tough coding because you can't reuse the C code. The
> > whole ticket_lock must be rewritten in asm and include the qspinlock
> > fast path.
> >
> > I think we should discuss some points before continuing the patch:
> > 1. Using alternative mechanisms for combo spinlock
> > 2. Using three Kconfigs for
> > ticket_spinlock/queune_spinlock/combo_spinlock during compile stage.
> > 3. The forward progress guarantee requirement is written in
> > qspinlock.h comment. That is not about our CAS/BHA.
> >
> > > implementation because the ticket spinlock implementation would pollute
> > > the spinlock value, so let's use static keys.
> > >
> > > This is largely based on Guo's work and Leonardo reviews at [1].
> > >
> > > Link: https://lore.kernel.org/linux-riscv/20231225125847.2778638-1-guoren@kernel.org/ [1]
> > > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> > > ---
> > > .../locking/queued-spinlocks/arch-support.txt | 2 +-
> > > arch/riscv/Kconfig | 10 +++++
> > > arch/riscv/include/asm/Kbuild | 4 +-
> > > arch/riscv/include/asm/spinlock.h | 39 +++++++++++++++++++
> > > arch/riscv/kernel/setup.c | 21 ++++++++++
> > > include/asm-generic/qspinlock.h | 2 +
> > > include/asm-generic/ticket_spinlock.h | 2 +
> > > 7 files changed, 78 insertions(+), 2 deletions(-)
> > > create mode 100644 arch/riscv/include/asm/spinlock.h
> > >
> > > diff --git a/Documentation/features/locking/queued-spinlocks/arch-support.txt b/Documentation/features/locking/queued-spinlocks/arch-support.txt
> > > index 22f2990392ff..cf26042480e2 100644
> > > --- a/Documentation/features/locking/queued-spinlocks/arch-support.txt
> > > +++ b/Documentation/features/locking/queued-spinlocks/arch-support.txt
> > > @@ -20,7 +20,7 @@
> > > | openrisc: | ok |
> > > | parisc: | TODO |
> > > | powerpc: | ok |
> > > - | riscv: | TODO |
> > > + | riscv: | ok |
> > > | s390: | TODO |
> > > | sh: | TODO |
> > > | sparc: | ok |
> > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > > index 0bbaec0444d0..c2ba673e58ca 100644
> > > --- a/arch/riscv/Kconfig
> > > +++ b/arch/riscv/Kconfig
> > > @@ -72,6 +72,7 @@ config RISCV
> > > select ARCH_WANT_OPTIMIZE_HUGETLB_VMEMMAP
> > > select ARCH_WANTS_NO_INSTR
> > > select ARCH_WANTS_THP_SWAP if HAVE_ARCH_TRANSPARENT_HUGEPAGE
> > > + select ARCH_WEAK_RELEASE_ACQUIRE if ARCH_USE_QUEUED_SPINLOCKS
> > > select BINFMT_FLAT_NO_DATA_START_OFFSET if !MMU
> > > select BUILDTIME_TABLE_SORT if MMU
> > > select CLINT_TIMER if RISCV_M_MODE
> > > @@ -482,6 +483,15 @@ config NODES_SHIFT
> > > Specify the maximum number of NUMA Nodes available on the target
> > > system. Increases memory reserved to accommodate various tables.
> > >
> > > +config RISCV_COMBO_SPINLOCKS
> > > + bool "Using combo spinlock"
> > > + depends on SMP && MMU && TOOLCHAIN_HAS_ZABHA
> > > + select ARCH_USE_QUEUED_SPINLOCKS
> > > + default y
> > > + help
> > > + Embed both queued spinlock and ticket lock so that the spinlock
> > > + implementation can be chosen at runtime.
> > > +
> >
> > COMBO SPINLOCK has side effects, which would expand spinlock code size
> > a lot. Ref: ARCH_INLINE_SPIN_LOCK
> >
> > So, we shouldn't remove the three configs' selection.
> >
> > +choice
> > + prompt "RISC-V spinlock type"
> > + default RISCV_COMBO_SPINLOCKS
> > +
> > +config RISCV_TICKET_SPINLOCKS
> > + bool "Using ticket spinlock"
> > +
> > +config RISCV_QUEUED_SPINLOCKS
> > + bool "Using queued spinlock"
> > + depends on SMP && MMU
> > + select ARCH_USE_QUEUED_SPINLOCKS
> > + help
> > + Make sure your micro arch give cmpxchg/xchg forward progress
> > + guarantee. Otherwise, stay at ticket-lock.
> > +
> > +config RISCV_COMBO_SPINLOCKS
> > + bool "Using combo spinlock"
> > + depends on SMP && MMU
> > + select ARCH_USE_QUEUED_SPINLOCKS
> > + help
> > + Select queued spinlock or ticket-lock by cmdline.
> > +endchoice
> > +
> >
> >
> >
> > > config RISCV_ALTERNATIVE
> > > bool
> > > depends on !XIP_KERNEL
> > > diff --git a/arch/riscv/include/asm/Kbuild b/arch/riscv/include/asm/Kbuild
> > > index 504f8b7e72d4..ad72f2bd4cc9 100644
> > > --- a/arch/riscv/include/asm/Kbuild
> > > +++ b/arch/riscv/include/asm/Kbuild
> > > @@ -2,10 +2,12 @@
> > > generic-y += early_ioremap.h
> > > generic-y += flat.h
> > > generic-y += kvm_para.h
> > > +generic-y += mcs_spinlock.h
> > > generic-y += parport.h
> > > -generic-y += spinlock.h
> > > generic-y += spinlock_types.h
> > > +generic-y += ticket_spinlock.h
> > > generic-y += qrwlock.h
> > > generic-y += qrwlock_types.h
> > > +generic-y += qspinlock.h
> > > generic-y += user.h
> > > generic-y += vmlinux.lds.h
> > > diff --git a/arch/riscv/include/asm/spinlock.h b/arch/riscv/include/asm/spinlock.h
> > > new file mode 100644
> > > index 000000000000..4856d50006f2
> > > --- /dev/null
> > > +++ b/arch/riscv/include/asm/spinlock.h
> > > @@ -0,0 +1,39 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +
> > > +#ifndef __ASM_RISCV_SPINLOCK_H
> > > +#define __ASM_RISCV_SPINLOCK_H
> > > +
> > > +#ifdef CONFIG_RISCV_COMBO_SPINLOCKS
> > > +#define _Q_PENDING_LOOPS (1 << 9)
> > > +
> > > +#define __no_arch_spinlock_redefine
> > > +#include <asm/ticket_spinlock.h>
> > > +#include <asm/qspinlock.h>
> > > +#include <asm/alternative.h>
> > > +
> > > +DECLARE_STATIC_KEY_TRUE(qspinlock_key);
> > > +
> > > +#define SPINLOCK_BASE_DECLARE(op, type, type_lock) \
> > > +static __always_inline type arch_spin_##op(type_lock lock) \
> > > +{ \
> > > + if (static_branch_unlikely(&qspinlock_key)) \
> > > + return queued_spin_##op(lock); \
> > > + return ticket_spin_##op(lock); \
> > > +}
> > > +
> > > +SPINLOCK_BASE_DECLARE(lock, void, arch_spinlock_t *)
> > > +SPINLOCK_BASE_DECLARE(unlock, void, arch_spinlock_t *)
> > > +SPINLOCK_BASE_DECLARE(is_locked, int, arch_spinlock_t *)
> > > +SPINLOCK_BASE_DECLARE(is_contended, int, arch_spinlock_t *)
> > > +SPINLOCK_BASE_DECLARE(trylock, bool, arch_spinlock_t *)
> > > +SPINLOCK_BASE_DECLARE(value_unlocked, int, arch_spinlock_t)
> > > +
> > > +#else
> > > +
> > > +#include <asm/ticket_spinlock.h>
> > > +
> > > +#endif
> > > +
> > > +#include <asm/qrwlock.h>
> > > +
> > > +#endif /* __ASM_RISCV_SPINLOCK_H */
> > > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> > > index 4f73c0ae44b2..929bccd4c9e5 100644
> > > --- a/arch/riscv/kernel/setup.c
> > > +++ b/arch/riscv/kernel/setup.c
> > > @@ -244,6 +244,26 @@ static void __init parse_dtb(void)
> > > #endif
> > > }
> > >
> > > +DEFINE_STATIC_KEY_TRUE(qspinlock_key);
> > > +EXPORT_SYMBOL(qspinlock_key);
> > > +
> > > +static void __init riscv_spinlock_init(void)
> > > +{
> > > + 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);
> > The requirement of qspinlock concerns the forward progress guarantee
> > in micro-arch design, which includes cmpxchg_loop & xchg_tail. So, I
> > don't think these features have a relationship with Qspinlock.
> >
> > If your machine doesn't have enough stickiness for a young exclusive
> > cacheline, fall back to ticket_lock.
>
> Could we use "Ziccrse: Main memory supports forward progress on LR/SC
> sequences" for qspinlock selection?
>
> + asm goto(ALTERNATIVE("nop", "j %[qspinlock]", 0,
> RISCV_ISA_EXT_ZICCRSE, 1)
> : : : : qspinlock);
>
> [1] https://github.com/riscv/riscv-profiles/blob/main/src/rva23-profile.adoc
Yes, I'll do that, thanks for the pointer!
Thanks,
Alex
>
> >
> > > +
> > > +no_zacas:
> > > + static_branch_disable(&qspinlock_key);
> > > + pr_info("Ticket spinlock: enabled\n");
> > > +
> > > + return;
> > > +
> > > +qspinlock:
> > > + pr_info("Queued spinlock: enabled\n");
> > > +}
> > > +
> > > extern void __init init_rt_signal_env(void);
> > >
> > > void __init setup_arch(char **cmdline_p)
> > > @@ -295,6 +315,7 @@ void __init setup_arch(char **cmdline_p)
> > > riscv_set_dma_cache_alignment();
> > >
> > > riscv_user_isa_enable();
> > > + riscv_spinlock_init();
> > > }
> > >
> > > bool arch_cpu_is_hotpluggable(int cpu)
> > > diff --git a/include/asm-generic/qspinlock.h b/include/asm-generic/qspinlock.h
> > > index 0655aa5b57b2..bf47cca2c375 100644
> > > --- a/include/asm-generic/qspinlock.h
> > > +++ b/include/asm-generic/qspinlock.h
> > > @@ -136,6 +136,7 @@ static __always_inline bool virt_spin_lock(struct qspinlock *lock)
> > > }
> > > #endif
> > >
> > > +#ifndef __no_arch_spinlock_redefine
> > > /*
> > > * Remapping spinlock architecture specific functions to the corresponding
> > > * queued spinlock functions.
> > > @@ -146,5 +147,6 @@ static __always_inline bool virt_spin_lock(struct qspinlock *lock)
> > > #define arch_spin_lock(l) queued_spin_lock(l)
> > > #define arch_spin_trylock(l) queued_spin_trylock(l)
> > > #define arch_spin_unlock(l) queued_spin_unlock(l)
> > > +#endif
> > >
> > > #endif /* __ASM_GENERIC_QSPINLOCK_H */
> > > diff --git a/include/asm-generic/ticket_spinlock.h b/include/asm-generic/ticket_spinlock.h
> > > index cfcff22b37b3..325779970d8a 100644
> > > --- a/include/asm-generic/ticket_spinlock.h
> > > +++ b/include/asm-generic/ticket_spinlock.h
> > > @@ -89,6 +89,7 @@ static __always_inline int ticket_spin_is_contended(arch_spinlock_t *lock)
> > > return (s16)((val >> 16) - (val & 0xffff)) > 1;
> > > }
> > >
> > > +#ifndef __no_arch_spinlock_redefine
> > > /*
> > > * Remapping spinlock architecture specific functions to the corresponding
> > > * ticket spinlock functions.
> > > @@ -99,5 +100,6 @@ static __always_inline int ticket_spin_is_contended(arch_spinlock_t *lock)
> > > #define arch_spin_lock(l) ticket_spin_lock(l)
> > > #define arch_spin_trylock(l) ticket_spin_trylock(l)
> > > #define arch_spin_unlock(l) ticket_spin_unlock(l)
> > > +#endif
> > >
> > > #endif /* __ASM_GENERIC_TICKET_SPINLOCK_H */
> > > --
> > > 2.39.2
> > >
> >
> >
> > --
> > Best Regards
> > Guo Ren
>
>
>
> --
> Best Regards
> Guo Ren
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v2 10/10] riscv: Add qspinlock support based on Zabha extension
@ 2024-07-15 7:33 ` Alexandre Ghiti
0 siblings, 0 replies; 25+ messages in thread
From: Alexandre Ghiti @ 2024-07-15 7:33 UTC (permalink / raw)
To: Guo Ren
Cc: Jonathan Corbet, Paul Walmsley, Palmer Dabbelt, Albert Ou,
Andrea Parri, Nathan Chancellor, Peter Zijlstra, Ingo Molnar,
Will Deacon, Waiman Long, Boqun Feng, Arnd Bergmann,
Leonardo Bras, linux-doc, linux-kernel, linux-riscv, linux-arch
Hi Guo,
On Mon, Jul 8, 2024 at 1:51 PM Guo Ren <guoren@kernel.org> wrote:
>
> On Sun, Jul 7, 2024 at 10:20 AM Guo Ren <guoren@kernel.org> wrote:
> >
> > On Wed, Jun 26, 2024 at 9:14 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
> > >
> > > In order to produce a generic kernel, a user can select
> > > CONFIG_COMBO_SPINLOCKS which will fallback at runtime to the ticket
> > > spinlock implementation if Zabha is not present.
> > >
> > > Note that we can't use alternatives here because the discovery of
> > > extensions is done too late and we need to start with the qspinlock
> > That's not true; we treat spinlock as qspinlock at first.
> > qspinlock_unlock would make the lock value zero (clean), but
> > ticket_lock would make a dirty one. (I've spent much time on this
> > mechanism, and you've preserved it in this patch.) So, making the
> > qspinlock -> ticket_lock change point safe until sched_init() is late
> > enough to make alternatives. The key problem of alternative
> > implementation is tough coding because you can't reuse the C code. The
> > whole ticket_lock must be rewritten in asm and include the qspinlock
> > fast path.
> >
> > I think we should discuss some points before continuing the patch:
> > 1. Using alternative mechanisms for combo spinlock
> > 2. Using three Kconfigs for
> > ticket_spinlock/queune_spinlock/combo_spinlock during compile stage.
> > 3. The forward progress guarantee requirement is written in
> > qspinlock.h comment. That is not about our CAS/BHA.
> >
> > > implementation because the ticket spinlock implementation would pollute
> > > the spinlock value, so let's use static keys.
> > >
> > > This is largely based on Guo's work and Leonardo reviews at [1].
> > >
> > > Link: https://lore.kernel.org/linux-riscv/20231225125847.2778638-1-guoren@kernel.org/ [1]
> > > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> > > ---
> > > .../locking/queued-spinlocks/arch-support.txt | 2 +-
> > > arch/riscv/Kconfig | 10 +++++
> > > arch/riscv/include/asm/Kbuild | 4 +-
> > > arch/riscv/include/asm/spinlock.h | 39 +++++++++++++++++++
> > > arch/riscv/kernel/setup.c | 21 ++++++++++
> > > include/asm-generic/qspinlock.h | 2 +
> > > include/asm-generic/ticket_spinlock.h | 2 +
> > > 7 files changed, 78 insertions(+), 2 deletions(-)
> > > create mode 100644 arch/riscv/include/asm/spinlock.h
> > >
> > > diff --git a/Documentation/features/locking/queued-spinlocks/arch-support.txt b/Documentation/features/locking/queued-spinlocks/arch-support.txt
> > > index 22f2990392ff..cf26042480e2 100644
> > > --- a/Documentation/features/locking/queued-spinlocks/arch-support.txt
> > > +++ b/Documentation/features/locking/queued-spinlocks/arch-support.txt
> > > @@ -20,7 +20,7 @@
> > > | openrisc: | ok |
> > > | parisc: | TODO |
> > > | powerpc: | ok |
> > > - | riscv: | TODO |
> > > + | riscv: | ok |
> > > | s390: | TODO |
> > > | sh: | TODO |
> > > | sparc: | ok |
> > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > > index 0bbaec0444d0..c2ba673e58ca 100644
> > > --- a/arch/riscv/Kconfig
> > > +++ b/arch/riscv/Kconfig
> > > @@ -72,6 +72,7 @@ config RISCV
> > > select ARCH_WANT_OPTIMIZE_HUGETLB_VMEMMAP
> > > select ARCH_WANTS_NO_INSTR
> > > select ARCH_WANTS_THP_SWAP if HAVE_ARCH_TRANSPARENT_HUGEPAGE
> > > + select ARCH_WEAK_RELEASE_ACQUIRE if ARCH_USE_QUEUED_SPINLOCKS
> > > select BINFMT_FLAT_NO_DATA_START_OFFSET if !MMU
> > > select BUILDTIME_TABLE_SORT if MMU
> > > select CLINT_TIMER if RISCV_M_MODE
> > > @@ -482,6 +483,15 @@ config NODES_SHIFT
> > > Specify the maximum number of NUMA Nodes available on the target
> > > system. Increases memory reserved to accommodate various tables.
> > >
> > > +config RISCV_COMBO_SPINLOCKS
> > > + bool "Using combo spinlock"
> > > + depends on SMP && MMU && TOOLCHAIN_HAS_ZABHA
> > > + select ARCH_USE_QUEUED_SPINLOCKS
> > > + default y
> > > + help
> > > + Embed both queued spinlock and ticket lock so that the spinlock
> > > + implementation can be chosen at runtime.
> > > +
> >
> > COMBO SPINLOCK has side effects, which would expand spinlock code size
> > a lot. Ref: ARCH_INLINE_SPIN_LOCK
> >
> > So, we shouldn't remove the three configs' selection.
> >
> > +choice
> > + prompt "RISC-V spinlock type"
> > + default RISCV_COMBO_SPINLOCKS
> > +
> > +config RISCV_TICKET_SPINLOCKS
> > + bool "Using ticket spinlock"
> > +
> > +config RISCV_QUEUED_SPINLOCKS
> > + bool "Using queued spinlock"
> > + depends on SMP && MMU
> > + select ARCH_USE_QUEUED_SPINLOCKS
> > + help
> > + Make sure your micro arch give cmpxchg/xchg forward progress
> > + guarantee. Otherwise, stay at ticket-lock.
> > +
> > +config RISCV_COMBO_SPINLOCKS
> > + bool "Using combo spinlock"
> > + depends on SMP && MMU
> > + select ARCH_USE_QUEUED_SPINLOCKS
> > + help
> > + Select queued spinlock or ticket-lock by cmdline.
> > +endchoice
> > +
> >
> >
> >
> > > config RISCV_ALTERNATIVE
> > > bool
> > > depends on !XIP_KERNEL
> > > diff --git a/arch/riscv/include/asm/Kbuild b/arch/riscv/include/asm/Kbuild
> > > index 504f8b7e72d4..ad72f2bd4cc9 100644
> > > --- a/arch/riscv/include/asm/Kbuild
> > > +++ b/arch/riscv/include/asm/Kbuild
> > > @@ -2,10 +2,12 @@
> > > generic-y += early_ioremap.h
> > > generic-y += flat.h
> > > generic-y += kvm_para.h
> > > +generic-y += mcs_spinlock.h
> > > generic-y += parport.h
> > > -generic-y += spinlock.h
> > > generic-y += spinlock_types.h
> > > +generic-y += ticket_spinlock.h
> > > generic-y += qrwlock.h
> > > generic-y += qrwlock_types.h
> > > +generic-y += qspinlock.h
> > > generic-y += user.h
> > > generic-y += vmlinux.lds.h
> > > diff --git a/arch/riscv/include/asm/spinlock.h b/arch/riscv/include/asm/spinlock.h
> > > new file mode 100644
> > > index 000000000000..4856d50006f2
> > > --- /dev/null
> > > +++ b/arch/riscv/include/asm/spinlock.h
> > > @@ -0,0 +1,39 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +
> > > +#ifndef __ASM_RISCV_SPINLOCK_H
> > > +#define __ASM_RISCV_SPINLOCK_H
> > > +
> > > +#ifdef CONFIG_RISCV_COMBO_SPINLOCKS
> > > +#define _Q_PENDING_LOOPS (1 << 9)
> > > +
> > > +#define __no_arch_spinlock_redefine
> > > +#include <asm/ticket_spinlock.h>
> > > +#include <asm/qspinlock.h>
> > > +#include <asm/alternative.h>
> > > +
> > > +DECLARE_STATIC_KEY_TRUE(qspinlock_key);
> > > +
> > > +#define SPINLOCK_BASE_DECLARE(op, type, type_lock) \
> > > +static __always_inline type arch_spin_##op(type_lock lock) \
> > > +{ \
> > > + if (static_branch_unlikely(&qspinlock_key)) \
> > > + return queued_spin_##op(lock); \
> > > + return ticket_spin_##op(lock); \
> > > +}
> > > +
> > > +SPINLOCK_BASE_DECLARE(lock, void, arch_spinlock_t *)
> > > +SPINLOCK_BASE_DECLARE(unlock, void, arch_spinlock_t *)
> > > +SPINLOCK_BASE_DECLARE(is_locked, int, arch_spinlock_t *)
> > > +SPINLOCK_BASE_DECLARE(is_contended, int, arch_spinlock_t *)
> > > +SPINLOCK_BASE_DECLARE(trylock, bool, arch_spinlock_t *)
> > > +SPINLOCK_BASE_DECLARE(value_unlocked, int, arch_spinlock_t)
> > > +
> > > +#else
> > > +
> > > +#include <asm/ticket_spinlock.h>
> > > +
> > > +#endif
> > > +
> > > +#include <asm/qrwlock.h>
> > > +
> > > +#endif /* __ASM_RISCV_SPINLOCK_H */
> > > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> > > index 4f73c0ae44b2..929bccd4c9e5 100644
> > > --- a/arch/riscv/kernel/setup.c
> > > +++ b/arch/riscv/kernel/setup.c
> > > @@ -244,6 +244,26 @@ static void __init parse_dtb(void)
> > > #endif
> > > }
> > >
> > > +DEFINE_STATIC_KEY_TRUE(qspinlock_key);
> > > +EXPORT_SYMBOL(qspinlock_key);
> > > +
> > > +static void __init riscv_spinlock_init(void)
> > > +{
> > > + 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);
> > The requirement of qspinlock concerns the forward progress guarantee
> > in micro-arch design, which includes cmpxchg_loop & xchg_tail. So, I
> > don't think these features have a relationship with Qspinlock.
> >
> > If your machine doesn't have enough stickiness for a young exclusive
> > cacheline, fall back to ticket_lock.
>
> Could we use "Ziccrse: Main memory supports forward progress on LR/SC
> sequences" for qspinlock selection?
>
> + asm goto(ALTERNATIVE("nop", "j %[qspinlock]", 0,
> RISCV_ISA_EXT_ZICCRSE, 1)
> : : : : qspinlock);
>
> [1] https://github.com/riscv/riscv-profiles/blob/main/src/rva23-profile.adoc
Yes, I'll do that, thanks for the pointer!
Thanks,
Alex
>
> >
> > > +
> > > +no_zacas:
> > > + static_branch_disable(&qspinlock_key);
> > > + pr_info("Ticket spinlock: enabled\n");
> > > +
> > > + return;
> > > +
> > > +qspinlock:
> > > + pr_info("Queued spinlock: enabled\n");
> > > +}
> > > +
> > > extern void __init init_rt_signal_env(void);
> > >
> > > void __init setup_arch(char **cmdline_p)
> > > @@ -295,6 +315,7 @@ void __init setup_arch(char **cmdline_p)
> > > riscv_set_dma_cache_alignment();
> > >
> > > riscv_user_isa_enable();
> > > + riscv_spinlock_init();
> > > }
> > >
> > > bool arch_cpu_is_hotpluggable(int cpu)
> > > diff --git a/include/asm-generic/qspinlock.h b/include/asm-generic/qspinlock.h
> > > index 0655aa5b57b2..bf47cca2c375 100644
> > > --- a/include/asm-generic/qspinlock.h
> > > +++ b/include/asm-generic/qspinlock.h
> > > @@ -136,6 +136,7 @@ static __always_inline bool virt_spin_lock(struct qspinlock *lock)
> > > }
> > > #endif
> > >
> > > +#ifndef __no_arch_spinlock_redefine
> > > /*
> > > * Remapping spinlock architecture specific functions to the corresponding
> > > * queued spinlock functions.
> > > @@ -146,5 +147,6 @@ static __always_inline bool virt_spin_lock(struct qspinlock *lock)
> > > #define arch_spin_lock(l) queued_spin_lock(l)
> > > #define arch_spin_trylock(l) queued_spin_trylock(l)
> > > #define arch_spin_unlock(l) queued_spin_unlock(l)
> > > +#endif
> > >
> > > #endif /* __ASM_GENERIC_QSPINLOCK_H */
> > > diff --git a/include/asm-generic/ticket_spinlock.h b/include/asm-generic/ticket_spinlock.h
> > > index cfcff22b37b3..325779970d8a 100644
> > > --- a/include/asm-generic/ticket_spinlock.h
> > > +++ b/include/asm-generic/ticket_spinlock.h
> > > @@ -89,6 +89,7 @@ static __always_inline int ticket_spin_is_contended(arch_spinlock_t *lock)
> > > return (s16)((val >> 16) - (val & 0xffff)) > 1;
> > > }
> > >
> > > +#ifndef __no_arch_spinlock_redefine
> > > /*
> > > * Remapping spinlock architecture specific functions to the corresponding
> > > * ticket spinlock functions.
> > > @@ -99,5 +100,6 @@ static __always_inline int ticket_spin_is_contended(arch_spinlock_t *lock)
> > > #define arch_spin_lock(l) ticket_spin_lock(l)
> > > #define arch_spin_trylock(l) ticket_spin_trylock(l)
> > > #define arch_spin_unlock(l) ticket_spin_unlock(l)
> > > +#endif
> > >
> > > #endif /* __ASM_GENERIC_TICKET_SPINLOCK_H */
> > > --
> > > 2.39.2
> > >
> >
> >
> > --
> > Best Regards
> > Guo Ren
>
>
>
> --
> Best Regards
> Guo Ren
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 10/10] riscv: Add qspinlock support based on Zabha extension
2024-07-07 2:20 ` Guo Ren
@ 2024-07-15 7:27 ` Alexandre Ghiti
-1 siblings, 0 replies; 25+ messages in thread
From: Alexandre Ghiti @ 2024-07-15 7:27 UTC (permalink / raw)
To: Guo Ren
Cc: Jonathan Corbet, Paul Walmsley, Palmer Dabbelt, Albert Ou,
Andrea Parri, Nathan Chancellor, Peter Zijlstra, Ingo Molnar,
Will Deacon, Waiman Long, Boqun Feng, Arnd Bergmann,
Leonardo Bras, linux-doc, linux-kernel, linux-riscv, linux-arch
Hi Guo,
On Sun, Jul 7, 2024 at 4:20 AM Guo Ren <guoren@kernel.org> wrote:
>
> On Wed, Jun 26, 2024 at 9:14 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
> >
> > In order to produce a generic kernel, a user can select
> > CONFIG_COMBO_SPINLOCKS which will fallback at runtime to the ticket
> > spinlock implementation if Zabha is not present.
> >
> > Note that we can't use alternatives here because the discovery of
> > extensions is done too late and we need to start with the qspinlock
> That's not true; we treat spinlock as qspinlock at first.
That's what I said: we have to use the qspinlock implementation at
first *because* we can't discover the extensions soon enough to use
the right spinlock implementation before the kernel uses a spinlock.
And since the spinlocks are used *before* the discovery of the
extensions, we cannot use the current alternative mechanism or we'd
need to extend it to add an "initial" value which does not depend on
the available extensions.
> qspinlock_unlock would make the lock value zero (clean), but
> ticket_lock would make a dirty one. (I've spent much time on this
> mechanism, and you've preserved it in this patch.) So, making the
> qspinlock -> ticket_lock change point safe until sched_init() is late
> enough to make alternatives. The key problem of alternative
> implementation is tough coding because you can't reuse the C code. The
> whole ticket_lock must be rewritten in asm and include the qspinlock
> fast path.
>
> I think we should discuss some points before continuing the patch:
> 1. Using alternative mechanisms for combo spinlock
We can easily get the extension string from the DT, and I have a PoC
that works with ACPI, that would make this possible.
> 2. Using three Kconfigs for
> ticket_spinlock/queune_spinlock/combo_spinlock during compile stage.
This makes sense, I'll do that.
> 3. The forward progress guarantee requirement is written in
> qspinlock.h comment. That is not about our CAS/BHA.
>
> > implementation because the ticket spinlock implementation would pollute
> > the spinlock value, so let's use static keys.
> >
> > This is largely based on Guo's work and Leonardo reviews at [1].
> >
> > Link: https://lore.kernel.org/linux-riscv/20231225125847.2778638-1-guoren@kernel.org/ [1]
> > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> > ---
> > .../locking/queued-spinlocks/arch-support.txt | 2 +-
> > arch/riscv/Kconfig | 10 +++++
> > arch/riscv/include/asm/Kbuild | 4 +-
> > arch/riscv/include/asm/spinlock.h | 39 +++++++++++++++++++
> > arch/riscv/kernel/setup.c | 21 ++++++++++
> > include/asm-generic/qspinlock.h | 2 +
> > include/asm-generic/ticket_spinlock.h | 2 +
> > 7 files changed, 78 insertions(+), 2 deletions(-)
> > create mode 100644 arch/riscv/include/asm/spinlock.h
> >
> > diff --git a/Documentation/features/locking/queued-spinlocks/arch-support.txt b/Documentation/features/locking/queued-spinlocks/arch-support.txt
> > index 22f2990392ff..cf26042480e2 100644
> > --- a/Documentation/features/locking/queued-spinlocks/arch-support.txt
> > +++ b/Documentation/features/locking/queued-spinlocks/arch-support.txt
> > @@ -20,7 +20,7 @@
> > | openrisc: | ok |
> > | parisc: | TODO |
> > | powerpc: | ok |
> > - | riscv: | TODO |
> > + | riscv: | ok |
> > | s390: | TODO |
> > | sh: | TODO |
> > | sparc: | ok |
> > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > index 0bbaec0444d0..c2ba673e58ca 100644
> > --- a/arch/riscv/Kconfig
> > +++ b/arch/riscv/Kconfig
> > @@ -72,6 +72,7 @@ config RISCV
> > select ARCH_WANT_OPTIMIZE_HUGETLB_VMEMMAP
> > select ARCH_WANTS_NO_INSTR
> > select ARCH_WANTS_THP_SWAP if HAVE_ARCH_TRANSPARENT_HUGEPAGE
> > + select ARCH_WEAK_RELEASE_ACQUIRE if ARCH_USE_QUEUED_SPINLOCKS
> > select BINFMT_FLAT_NO_DATA_START_OFFSET if !MMU
> > select BUILDTIME_TABLE_SORT if MMU
> > select CLINT_TIMER if RISCV_M_MODE
> > @@ -482,6 +483,15 @@ config NODES_SHIFT
> > Specify the maximum number of NUMA Nodes available on the target
> > system. Increases memory reserved to accommodate various tables.
> >
> > +config RISCV_COMBO_SPINLOCKS
> > + bool "Using combo spinlock"
> > + depends on SMP && MMU && TOOLCHAIN_HAS_ZABHA
> > + select ARCH_USE_QUEUED_SPINLOCKS
> > + default y
> > + help
> > + Embed both queued spinlock and ticket lock so that the spinlock
> > + implementation can be chosen at runtime.
> > +
>
> COMBO SPINLOCK has side effects, which would expand spinlock code size
> a lot. Ref: ARCH_INLINE_SPIN_LOCK
>
> So, we shouldn't remove the three configs' selection.
>
> +choice
> + prompt "RISC-V spinlock type"
> + default RISCV_COMBO_SPINLOCKS
> +
> +config RISCV_TICKET_SPINLOCKS
> + bool "Using ticket spinlock"
> +
> +config RISCV_QUEUED_SPINLOCKS
> + bool "Using queued spinlock"
> + depends on SMP && MMU
> + select ARCH_USE_QUEUED_SPINLOCKS
> + help
> + Make sure your micro arch give cmpxchg/xchg forward progress
> + guarantee. Otherwise, stay at ticket-lock.
> +
> +config RISCV_COMBO_SPINLOCKS
> + bool "Using combo spinlock"
> + depends on SMP && MMU
> + select ARCH_USE_QUEUED_SPINLOCKS
> + help
> + Select queued spinlock or ticket-lock by cmdline.
> +endchoice
> +
>
>
>
> > config RISCV_ALTERNATIVE
> > bool
> > depends on !XIP_KERNEL
> > diff --git a/arch/riscv/include/asm/Kbuild b/arch/riscv/include/asm/Kbuild
> > index 504f8b7e72d4..ad72f2bd4cc9 100644
> > --- a/arch/riscv/include/asm/Kbuild
> > +++ b/arch/riscv/include/asm/Kbuild
> > @@ -2,10 +2,12 @@
> > generic-y += early_ioremap.h
> > generic-y += flat.h
> > generic-y += kvm_para.h
> > +generic-y += mcs_spinlock.h
> > generic-y += parport.h
> > -generic-y += spinlock.h
> > generic-y += spinlock_types.h
> > +generic-y += ticket_spinlock.h
> > generic-y += qrwlock.h
> > generic-y += qrwlock_types.h
> > +generic-y += qspinlock.h
> > generic-y += user.h
> > generic-y += vmlinux.lds.h
> > diff --git a/arch/riscv/include/asm/spinlock.h b/arch/riscv/include/asm/spinlock.h
> > new file mode 100644
> > index 000000000000..4856d50006f2
> > --- /dev/null
> > +++ b/arch/riscv/include/asm/spinlock.h
> > @@ -0,0 +1,39 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +#ifndef __ASM_RISCV_SPINLOCK_H
> > +#define __ASM_RISCV_SPINLOCK_H
> > +
> > +#ifdef CONFIG_RISCV_COMBO_SPINLOCKS
> > +#define _Q_PENDING_LOOPS (1 << 9)
> > +
> > +#define __no_arch_spinlock_redefine
> > +#include <asm/ticket_spinlock.h>
> > +#include <asm/qspinlock.h>
> > +#include <asm/alternative.h>
> > +
> > +DECLARE_STATIC_KEY_TRUE(qspinlock_key);
> > +
> > +#define SPINLOCK_BASE_DECLARE(op, type, type_lock) \
> > +static __always_inline type arch_spin_##op(type_lock lock) \
> > +{ \
> > + if (static_branch_unlikely(&qspinlock_key)) \
> > + return queued_spin_##op(lock); \
> > + return ticket_spin_##op(lock); \
> > +}
> > +
> > +SPINLOCK_BASE_DECLARE(lock, void, arch_spinlock_t *)
> > +SPINLOCK_BASE_DECLARE(unlock, void, arch_spinlock_t *)
> > +SPINLOCK_BASE_DECLARE(is_locked, int, arch_spinlock_t *)
> > +SPINLOCK_BASE_DECLARE(is_contended, int, arch_spinlock_t *)
> > +SPINLOCK_BASE_DECLARE(trylock, bool, arch_spinlock_t *)
> > +SPINLOCK_BASE_DECLARE(value_unlocked, int, arch_spinlock_t)
> > +
> > +#else
> > +
> > +#include <asm/ticket_spinlock.h>
> > +
> > +#endif
> > +
> > +#include <asm/qrwlock.h>
> > +
> > +#endif /* __ASM_RISCV_SPINLOCK_H */
> > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> > index 4f73c0ae44b2..929bccd4c9e5 100644
> > --- a/arch/riscv/kernel/setup.c
> > +++ b/arch/riscv/kernel/setup.c
> > @@ -244,6 +244,26 @@ static void __init parse_dtb(void)
> > #endif
> > }
> >
> > +DEFINE_STATIC_KEY_TRUE(qspinlock_key);
> > +EXPORT_SYMBOL(qspinlock_key);
> > +
> > +static void __init riscv_spinlock_init(void)
> > +{
> > + 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);
> The requirement of qspinlock concerns the forward progress guarantee
> in micro-arch design, which includes cmpxchg_loop & xchg_tail. So, I
> don't think these features have a relationship with Qspinlock.
>
> If your machine doesn't have enough stickiness for a young exclusive
> cacheline, fall back to ticket_lock.
How riscv zacas/zabha implementation would not provide forward
progress guarantee when all other architecture's atomic memory
operations do?
>
> > +
> > +no_zacas:
> > + static_branch_disable(&qspinlock_key);
> > + pr_info("Ticket spinlock: enabled\n");
> > +
> > + return;
> > +
> > +qspinlock:
> > + pr_info("Queued spinlock: enabled\n");
> > +}
> > +
> > extern void __init init_rt_signal_env(void);
> >
> > void __init setup_arch(char **cmdline_p)
> > @@ -295,6 +315,7 @@ void __init setup_arch(char **cmdline_p)
> > riscv_set_dma_cache_alignment();
> >
> > riscv_user_isa_enable();
> > + riscv_spinlock_init();
> > }
> >
> > bool arch_cpu_is_hotpluggable(int cpu)
> > diff --git a/include/asm-generic/qspinlock.h b/include/asm-generic/qspinlock.h
> > index 0655aa5b57b2..bf47cca2c375 100644
> > --- a/include/asm-generic/qspinlock.h
> > +++ b/include/asm-generic/qspinlock.h
> > @@ -136,6 +136,7 @@ static __always_inline bool virt_spin_lock(struct qspinlock *lock)
> > }
> > #endif
> >
> > +#ifndef __no_arch_spinlock_redefine
> > /*
> > * Remapping spinlock architecture specific functions to the corresponding
> > * queued spinlock functions.
> > @@ -146,5 +147,6 @@ static __always_inline bool virt_spin_lock(struct qspinlock *lock)
> > #define arch_spin_lock(l) queued_spin_lock(l)
> > #define arch_spin_trylock(l) queued_spin_trylock(l)
> > #define arch_spin_unlock(l) queued_spin_unlock(l)
> > +#endif
> >
> > #endif /* __ASM_GENERIC_QSPINLOCK_H */
> > diff --git a/include/asm-generic/ticket_spinlock.h b/include/asm-generic/ticket_spinlock.h
> > index cfcff22b37b3..325779970d8a 100644
> > --- a/include/asm-generic/ticket_spinlock.h
> > +++ b/include/asm-generic/ticket_spinlock.h
> > @@ -89,6 +89,7 @@ static __always_inline int ticket_spin_is_contended(arch_spinlock_t *lock)
> > return (s16)((val >> 16) - (val & 0xffff)) > 1;
> > }
> >
> > +#ifndef __no_arch_spinlock_redefine
> > /*
> > * Remapping spinlock architecture specific functions to the corresponding
> > * ticket spinlock functions.
> > @@ -99,5 +100,6 @@ static __always_inline int ticket_spin_is_contended(arch_spinlock_t *lock)
> > #define arch_spin_lock(l) ticket_spin_lock(l)
> > #define arch_spin_trylock(l) ticket_spin_trylock(l)
> > #define arch_spin_unlock(l) ticket_spin_unlock(l)
> > +#endif
> >
> > #endif /* __ASM_GENERIC_TICKET_SPINLOCK_H */
> > --
> > 2.39.2
> >
>
>
> --
> Best Regards
> Guo Ren
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v2 10/10] riscv: Add qspinlock support based on Zabha extension
@ 2024-07-15 7:27 ` Alexandre Ghiti
0 siblings, 0 replies; 25+ messages in thread
From: Alexandre Ghiti @ 2024-07-15 7:27 UTC (permalink / raw)
To: Guo Ren
Cc: Jonathan Corbet, Paul Walmsley, Palmer Dabbelt, Albert Ou,
Andrea Parri, Nathan Chancellor, Peter Zijlstra, Ingo Molnar,
Will Deacon, Waiman Long, Boqun Feng, Arnd Bergmann,
Leonardo Bras, linux-doc, linux-kernel, linux-riscv, linux-arch
Hi Guo,
On Sun, Jul 7, 2024 at 4:20 AM Guo Ren <guoren@kernel.org> wrote:
>
> On Wed, Jun 26, 2024 at 9:14 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
> >
> > In order to produce a generic kernel, a user can select
> > CONFIG_COMBO_SPINLOCKS which will fallback at runtime to the ticket
> > spinlock implementation if Zabha is not present.
> >
> > Note that we can't use alternatives here because the discovery of
> > extensions is done too late and we need to start with the qspinlock
> That's not true; we treat spinlock as qspinlock at first.
That's what I said: we have to use the qspinlock implementation at
first *because* we can't discover the extensions soon enough to use
the right spinlock implementation before the kernel uses a spinlock.
And since the spinlocks are used *before* the discovery of the
extensions, we cannot use the current alternative mechanism or we'd
need to extend it to add an "initial" value which does not depend on
the available extensions.
> qspinlock_unlock would make the lock value zero (clean), but
> ticket_lock would make a dirty one. (I've spent much time on this
> mechanism, and you've preserved it in this patch.) So, making the
> qspinlock -> ticket_lock change point safe until sched_init() is late
> enough to make alternatives. The key problem of alternative
> implementation is tough coding because you can't reuse the C code. The
> whole ticket_lock must be rewritten in asm and include the qspinlock
> fast path.
>
> I think we should discuss some points before continuing the patch:
> 1. Using alternative mechanisms for combo spinlock
We can easily get the extension string from the DT, and I have a PoC
that works with ACPI, that would make this possible.
> 2. Using three Kconfigs for
> ticket_spinlock/queune_spinlock/combo_spinlock during compile stage.
This makes sense, I'll do that.
> 3. The forward progress guarantee requirement is written in
> qspinlock.h comment. That is not about our CAS/BHA.
>
> > implementation because the ticket spinlock implementation would pollute
> > the spinlock value, so let's use static keys.
> >
> > This is largely based on Guo's work and Leonardo reviews at [1].
> >
> > Link: https://lore.kernel.org/linux-riscv/20231225125847.2778638-1-guoren@kernel.org/ [1]
> > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> > ---
> > .../locking/queued-spinlocks/arch-support.txt | 2 +-
> > arch/riscv/Kconfig | 10 +++++
> > arch/riscv/include/asm/Kbuild | 4 +-
> > arch/riscv/include/asm/spinlock.h | 39 +++++++++++++++++++
> > arch/riscv/kernel/setup.c | 21 ++++++++++
> > include/asm-generic/qspinlock.h | 2 +
> > include/asm-generic/ticket_spinlock.h | 2 +
> > 7 files changed, 78 insertions(+), 2 deletions(-)
> > create mode 100644 arch/riscv/include/asm/spinlock.h
> >
> > diff --git a/Documentation/features/locking/queued-spinlocks/arch-support.txt b/Documentation/features/locking/queued-spinlocks/arch-support.txt
> > index 22f2990392ff..cf26042480e2 100644
> > --- a/Documentation/features/locking/queued-spinlocks/arch-support.txt
> > +++ b/Documentation/features/locking/queued-spinlocks/arch-support.txt
> > @@ -20,7 +20,7 @@
> > | openrisc: | ok |
> > | parisc: | TODO |
> > | powerpc: | ok |
> > - | riscv: | TODO |
> > + | riscv: | ok |
> > | s390: | TODO |
> > | sh: | TODO |
> > | sparc: | ok |
> > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > index 0bbaec0444d0..c2ba673e58ca 100644
> > --- a/arch/riscv/Kconfig
> > +++ b/arch/riscv/Kconfig
> > @@ -72,6 +72,7 @@ config RISCV
> > select ARCH_WANT_OPTIMIZE_HUGETLB_VMEMMAP
> > select ARCH_WANTS_NO_INSTR
> > select ARCH_WANTS_THP_SWAP if HAVE_ARCH_TRANSPARENT_HUGEPAGE
> > + select ARCH_WEAK_RELEASE_ACQUIRE if ARCH_USE_QUEUED_SPINLOCKS
> > select BINFMT_FLAT_NO_DATA_START_OFFSET if !MMU
> > select BUILDTIME_TABLE_SORT if MMU
> > select CLINT_TIMER if RISCV_M_MODE
> > @@ -482,6 +483,15 @@ config NODES_SHIFT
> > Specify the maximum number of NUMA Nodes available on the target
> > system. Increases memory reserved to accommodate various tables.
> >
> > +config RISCV_COMBO_SPINLOCKS
> > + bool "Using combo spinlock"
> > + depends on SMP && MMU && TOOLCHAIN_HAS_ZABHA
> > + select ARCH_USE_QUEUED_SPINLOCKS
> > + default y
> > + help
> > + Embed both queued spinlock and ticket lock so that the spinlock
> > + implementation can be chosen at runtime.
> > +
>
> COMBO SPINLOCK has side effects, which would expand spinlock code size
> a lot. Ref: ARCH_INLINE_SPIN_LOCK
>
> So, we shouldn't remove the three configs' selection.
>
> +choice
> + prompt "RISC-V spinlock type"
> + default RISCV_COMBO_SPINLOCKS
> +
> +config RISCV_TICKET_SPINLOCKS
> + bool "Using ticket spinlock"
> +
> +config RISCV_QUEUED_SPINLOCKS
> + bool "Using queued spinlock"
> + depends on SMP && MMU
> + select ARCH_USE_QUEUED_SPINLOCKS
> + help
> + Make sure your micro arch give cmpxchg/xchg forward progress
> + guarantee. Otherwise, stay at ticket-lock.
> +
> +config RISCV_COMBO_SPINLOCKS
> + bool "Using combo spinlock"
> + depends on SMP && MMU
> + select ARCH_USE_QUEUED_SPINLOCKS
> + help
> + Select queued spinlock or ticket-lock by cmdline.
> +endchoice
> +
>
>
>
> > config RISCV_ALTERNATIVE
> > bool
> > depends on !XIP_KERNEL
> > diff --git a/arch/riscv/include/asm/Kbuild b/arch/riscv/include/asm/Kbuild
> > index 504f8b7e72d4..ad72f2bd4cc9 100644
> > --- a/arch/riscv/include/asm/Kbuild
> > +++ b/arch/riscv/include/asm/Kbuild
> > @@ -2,10 +2,12 @@
> > generic-y += early_ioremap.h
> > generic-y += flat.h
> > generic-y += kvm_para.h
> > +generic-y += mcs_spinlock.h
> > generic-y += parport.h
> > -generic-y += spinlock.h
> > generic-y += spinlock_types.h
> > +generic-y += ticket_spinlock.h
> > generic-y += qrwlock.h
> > generic-y += qrwlock_types.h
> > +generic-y += qspinlock.h
> > generic-y += user.h
> > generic-y += vmlinux.lds.h
> > diff --git a/arch/riscv/include/asm/spinlock.h b/arch/riscv/include/asm/spinlock.h
> > new file mode 100644
> > index 000000000000..4856d50006f2
> > --- /dev/null
> > +++ b/arch/riscv/include/asm/spinlock.h
> > @@ -0,0 +1,39 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +#ifndef __ASM_RISCV_SPINLOCK_H
> > +#define __ASM_RISCV_SPINLOCK_H
> > +
> > +#ifdef CONFIG_RISCV_COMBO_SPINLOCKS
> > +#define _Q_PENDING_LOOPS (1 << 9)
> > +
> > +#define __no_arch_spinlock_redefine
> > +#include <asm/ticket_spinlock.h>
> > +#include <asm/qspinlock.h>
> > +#include <asm/alternative.h>
> > +
> > +DECLARE_STATIC_KEY_TRUE(qspinlock_key);
> > +
> > +#define SPINLOCK_BASE_DECLARE(op, type, type_lock) \
> > +static __always_inline type arch_spin_##op(type_lock lock) \
> > +{ \
> > + if (static_branch_unlikely(&qspinlock_key)) \
> > + return queued_spin_##op(lock); \
> > + return ticket_spin_##op(lock); \
> > +}
> > +
> > +SPINLOCK_BASE_DECLARE(lock, void, arch_spinlock_t *)
> > +SPINLOCK_BASE_DECLARE(unlock, void, arch_spinlock_t *)
> > +SPINLOCK_BASE_DECLARE(is_locked, int, arch_spinlock_t *)
> > +SPINLOCK_BASE_DECLARE(is_contended, int, arch_spinlock_t *)
> > +SPINLOCK_BASE_DECLARE(trylock, bool, arch_spinlock_t *)
> > +SPINLOCK_BASE_DECLARE(value_unlocked, int, arch_spinlock_t)
> > +
> > +#else
> > +
> > +#include <asm/ticket_spinlock.h>
> > +
> > +#endif
> > +
> > +#include <asm/qrwlock.h>
> > +
> > +#endif /* __ASM_RISCV_SPINLOCK_H */
> > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> > index 4f73c0ae44b2..929bccd4c9e5 100644
> > --- a/arch/riscv/kernel/setup.c
> > +++ b/arch/riscv/kernel/setup.c
> > @@ -244,6 +244,26 @@ static void __init parse_dtb(void)
> > #endif
> > }
> >
> > +DEFINE_STATIC_KEY_TRUE(qspinlock_key);
> > +EXPORT_SYMBOL(qspinlock_key);
> > +
> > +static void __init riscv_spinlock_init(void)
> > +{
> > + 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);
> The requirement of qspinlock concerns the forward progress guarantee
> in micro-arch design, which includes cmpxchg_loop & xchg_tail. So, I
> don't think these features have a relationship with Qspinlock.
>
> If your machine doesn't have enough stickiness for a young exclusive
> cacheline, fall back to ticket_lock.
How riscv zacas/zabha implementation would not provide forward
progress guarantee when all other architecture's atomic memory
operations do?
>
> > +
> > +no_zacas:
> > + static_branch_disable(&qspinlock_key);
> > + pr_info("Ticket spinlock: enabled\n");
> > +
> > + return;
> > +
> > +qspinlock:
> > + pr_info("Queued spinlock: enabled\n");
> > +}
> > +
> > extern void __init init_rt_signal_env(void);
> >
> > void __init setup_arch(char **cmdline_p)
> > @@ -295,6 +315,7 @@ void __init setup_arch(char **cmdline_p)
> > riscv_set_dma_cache_alignment();
> >
> > riscv_user_isa_enable();
> > + riscv_spinlock_init();
> > }
> >
> > bool arch_cpu_is_hotpluggable(int cpu)
> > diff --git a/include/asm-generic/qspinlock.h b/include/asm-generic/qspinlock.h
> > index 0655aa5b57b2..bf47cca2c375 100644
> > --- a/include/asm-generic/qspinlock.h
> > +++ b/include/asm-generic/qspinlock.h
> > @@ -136,6 +136,7 @@ static __always_inline bool virt_spin_lock(struct qspinlock *lock)
> > }
> > #endif
> >
> > +#ifndef __no_arch_spinlock_redefine
> > /*
> > * Remapping spinlock architecture specific functions to the corresponding
> > * queued spinlock functions.
> > @@ -146,5 +147,6 @@ static __always_inline bool virt_spin_lock(struct qspinlock *lock)
> > #define arch_spin_lock(l) queued_spin_lock(l)
> > #define arch_spin_trylock(l) queued_spin_trylock(l)
> > #define arch_spin_unlock(l) queued_spin_unlock(l)
> > +#endif
> >
> > #endif /* __ASM_GENERIC_QSPINLOCK_H */
> > diff --git a/include/asm-generic/ticket_spinlock.h b/include/asm-generic/ticket_spinlock.h
> > index cfcff22b37b3..325779970d8a 100644
> > --- a/include/asm-generic/ticket_spinlock.h
> > +++ b/include/asm-generic/ticket_spinlock.h
> > @@ -89,6 +89,7 @@ static __always_inline int ticket_spin_is_contended(arch_spinlock_t *lock)
> > return (s16)((val >> 16) - (val & 0xffff)) > 1;
> > }
> >
> > +#ifndef __no_arch_spinlock_redefine
> > /*
> > * Remapping spinlock architecture specific functions to the corresponding
> > * ticket spinlock functions.
> > @@ -99,5 +100,6 @@ static __always_inline int ticket_spin_is_contended(arch_spinlock_t *lock)
> > #define arch_spin_lock(l) ticket_spin_lock(l)
> > #define arch_spin_trylock(l) ticket_spin_trylock(l)
> > #define arch_spin_unlock(l) ticket_spin_unlock(l)
> > +#endif
> >
> > #endif /* __ASM_GENERIC_TICKET_SPINLOCK_H */
> > --
> > 2.39.2
> >
>
>
> --
> Best Regards
> Guo Ren
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v2 10/10] riscv: Add qspinlock support based on Zabha extension
2024-07-15 7:27 ` Alexandre Ghiti
@ 2024-07-15 19:30 ` Waiman Long
-1 siblings, 0 replies; 25+ messages in thread
From: Waiman Long @ 2024-07-15 19:30 UTC (permalink / raw)
To: Alexandre Ghiti, Guo Ren
Cc: Jonathan Corbet, Paul Walmsley, Palmer Dabbelt, Albert Ou,
Andrea Parri, Nathan Chancellor, Peter Zijlstra, Ingo Molnar,
Will Deacon, Boqun Feng, Arnd Bergmann, Leonardo Bras, linux-doc,
linux-kernel, linux-riscv, linux-arch
On 7/15/24 03:27, Alexandre Ghiti wrote:
> Hi Guo,
>
> On Sun, Jul 7, 2024 at 4:20 AM Guo Ren <guoren@kernel.org> wrote:
>> On Wed, Jun 26, 2024 at 9:14 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
>>> In order to produce a generic kernel, a user can select
>>> CONFIG_COMBO_SPINLOCKS which will fallback at runtime to the ticket
>>> spinlock implementation if Zabha is not present.
>>>
>>> Note that we can't use alternatives here because the discovery of
>>> extensions is done too late and we need to start with the qspinlock
>> That's not true; we treat spinlock as qspinlock at first.
> That's what I said: we have to use the qspinlock implementation at
> first *because* we can't discover the extensions soon enough to use
> the right spinlock implementation before the kernel uses a spinlock.
> And since the spinlocks are used *before* the discovery of the
> extensions, we cannot use the current alternative mechanism or we'd
> need to extend it to add an "initial" value which does not depend on
> the available extensions.
With qspinlock, the lock remains zero after a lock/unlock sequence. That
is not the case with ticket lock. Assuming that all the discovery will
be done before SMP boot, the qspinlock slowpath won't be activated and
so we don't need the presence of any extension. I believe that is the
main reason why qspinlock is used as the initial default and not ticket
lock.
Cheers,
Longman
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 10/10] riscv: Add qspinlock support based on Zabha extension
@ 2024-07-15 19:30 ` Waiman Long
0 siblings, 0 replies; 25+ messages in thread
From: Waiman Long @ 2024-07-15 19:30 UTC (permalink / raw)
To: Alexandre Ghiti, Guo Ren
Cc: Jonathan Corbet, Paul Walmsley, Palmer Dabbelt, Albert Ou,
Andrea Parri, Nathan Chancellor, Peter Zijlstra, Ingo Molnar,
Will Deacon, Boqun Feng, Arnd Bergmann, Leonardo Bras, linux-doc,
linux-kernel, linux-riscv, linux-arch
On 7/15/24 03:27, Alexandre Ghiti wrote:
> Hi Guo,
>
> On Sun, Jul 7, 2024 at 4:20 AM Guo Ren <guoren@kernel.org> wrote:
>> On Wed, Jun 26, 2024 at 9:14 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
>>> In order to produce a generic kernel, a user can select
>>> CONFIG_COMBO_SPINLOCKS which will fallback at runtime to the ticket
>>> spinlock implementation if Zabha is not present.
>>>
>>> Note that we can't use alternatives here because the discovery of
>>> extensions is done too late and we need to start with the qspinlock
>> That's not true; we treat spinlock as qspinlock at first.
> That's what I said: we have to use the qspinlock implementation at
> first *because* we can't discover the extensions soon enough to use
> the right spinlock implementation before the kernel uses a spinlock.
> And since the spinlocks are used *before* the discovery of the
> extensions, we cannot use the current alternative mechanism or we'd
> need to extend it to add an "initial" value which does not depend on
> the available extensions.
With qspinlock, the lock remains zero after a lock/unlock sequence. That
is not the case with ticket lock. Assuming that all the discovery will
be done before SMP boot, the qspinlock slowpath won't be activated and
so we don't need the presence of any extension. I believe that is the
main reason why qspinlock is used as the initial default and not ticket
lock.
Cheers,
Longman
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 10/10] riscv: Add qspinlock support based on Zabha extension
2024-07-15 19:30 ` Waiman Long
@ 2024-07-16 1:04 ` Guo Ren
-1 siblings, 0 replies; 25+ messages in thread
From: Guo Ren @ 2024-07-16 1:04 UTC (permalink / raw)
To: Waiman Long
Cc: Alexandre Ghiti, Jonathan Corbet, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Andrea Parri, Nathan Chancellor, Peter Zijlstra,
Ingo Molnar, Will Deacon, Boqun Feng, Arnd Bergmann,
Leonardo Bras, linux-doc, linux-kernel, linux-riscv, linux-arch
On Tue, Jul 16, 2024 at 3:30 AM Waiman Long <longman@redhat.com> wrote:
>
> On 7/15/24 03:27, Alexandre Ghiti wrote:
> > Hi Guo,
> >
> > On Sun, Jul 7, 2024 at 4:20 AM Guo Ren <guoren@kernel.org> wrote:
> >> On Wed, Jun 26, 2024 at 9:14 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
> >>> In order to produce a generic kernel, a user can select
> >>> CONFIG_COMBO_SPINLOCKS which will fallback at runtime to the ticket
> >>> spinlock implementation if Zabha is not present.
> >>>
> >>> Note that we can't use alternatives here because the discovery of
> >>> extensions is done too late and we need to start with the qspinlock
> >> That's not true; we treat spinlock as qspinlock at first.
> > That's what I said: we have to use the qspinlock implementation at
> > first *because* we can't discover the extensions soon enough to use
> > the right spinlock implementation before the kernel uses a spinlock.
> > And since the spinlocks are used *before* the discovery of the
> > extensions, we cannot use the current alternative mechanism or we'd
> > need to extend it to add an "initial" value which does not depend on
> > the available extensions.
>
> With qspinlock, the lock remains zero after a lock/unlock sequence. That
> is not the case with ticket lock. Assuming that all the discovery will
> be done before SMP boot, the qspinlock slowpath won't be activated and
> so we don't need the presence of any extension. I believe that is the
> main reason why qspinlock is used as the initial default and not ticket
> lock.
Thx Waiman,
Yes, qspinlock is a clean guy, but ticket lock is a dirty one.
Hi Alexandre,
Therefore, the switch point(before reset_init()) is late enough to
change the lock mechanism, and this satisfies the requirements of
apply_boot_alternatives(), apply_early_boot_alternatives(), and
apply_module_alternatives().
>
> Cheers,
> Longman
>
--
Best Regards
Guo Ren
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 10/10] riscv: Add qspinlock support based on Zabha extension
@ 2024-07-16 1:04 ` Guo Ren
0 siblings, 0 replies; 25+ messages in thread
From: Guo Ren @ 2024-07-16 1:04 UTC (permalink / raw)
To: Waiman Long
Cc: Alexandre Ghiti, Jonathan Corbet, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Andrea Parri, Nathan Chancellor, Peter Zijlstra,
Ingo Molnar, Will Deacon, Boqun Feng, Arnd Bergmann,
Leonardo Bras, linux-doc, linux-kernel, linux-riscv, linux-arch
On Tue, Jul 16, 2024 at 3:30 AM Waiman Long <longman@redhat.com> wrote:
>
> On 7/15/24 03:27, Alexandre Ghiti wrote:
> > Hi Guo,
> >
> > On Sun, Jul 7, 2024 at 4:20 AM Guo Ren <guoren@kernel.org> wrote:
> >> On Wed, Jun 26, 2024 at 9:14 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
> >>> In order to produce a generic kernel, a user can select
> >>> CONFIG_COMBO_SPINLOCKS which will fallback at runtime to the ticket
> >>> spinlock implementation if Zabha is not present.
> >>>
> >>> Note that we can't use alternatives here because the discovery of
> >>> extensions is done too late and we need to start with the qspinlock
> >> That's not true; we treat spinlock as qspinlock at first.
> > That's what I said: we have to use the qspinlock implementation at
> > first *because* we can't discover the extensions soon enough to use
> > the right spinlock implementation before the kernel uses a spinlock.
> > And since the spinlocks are used *before* the discovery of the
> > extensions, we cannot use the current alternative mechanism or we'd
> > need to extend it to add an "initial" value which does not depend on
> > the available extensions.
>
> With qspinlock, the lock remains zero after a lock/unlock sequence. That
> is not the case with ticket lock. Assuming that all the discovery will
> be done before SMP boot, the qspinlock slowpath won't be activated and
> so we don't need the presence of any extension. I believe that is the
> main reason why qspinlock is used as the initial default and not ticket
> lock.
Thx Waiman,
Yes, qspinlock is a clean guy, but ticket lock is a dirty one.
Hi Alexandre,
Therefore, the switch point(before reset_init()) is late enough to
change the lock mechanism, and this satisfies the requirements of
apply_boot_alternatives(), apply_early_boot_alternatives(), and
apply_module_alternatives().
>
> Cheers,
> Longman
>
--
Best Regards
Guo Ren
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 10/10] riscv: Add qspinlock support based on Zabha extension
2024-07-16 1:04 ` Guo Ren
@ 2024-07-16 6:43 ` Alexandre Ghiti
-1 siblings, 0 replies; 25+ messages in thread
From: Alexandre Ghiti @ 2024-07-16 6:43 UTC (permalink / raw)
To: Guo Ren
Cc: Waiman Long, Jonathan Corbet, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Andrea Parri, Nathan Chancellor, Peter Zijlstra,
Ingo Molnar, Will Deacon, Boqun Feng, Arnd Bergmann,
Leonardo Bras, linux-doc, linux-kernel, linux-riscv, linux-arch
Hi Guo, Waiman,
On Tue, Jul 16, 2024 at 3:05 AM Guo Ren <guoren@kernel.org> wrote:
>
> On Tue, Jul 16, 2024 at 3:30 AM Waiman Long <longman@redhat.com> wrote:
> >
> > On 7/15/24 03:27, Alexandre Ghiti wrote:
> > > Hi Guo,
> > >
> > > On Sun, Jul 7, 2024 at 4:20 AM Guo Ren <guoren@kernel.org> wrote:
> > >> On Wed, Jun 26, 2024 at 9:14 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
> > >>> In order to produce a generic kernel, a user can select
> > >>> CONFIG_COMBO_SPINLOCKS which will fallback at runtime to the ticket
> > >>> spinlock implementation if Zabha is not present.
> > >>>
> > >>> Note that we can't use alternatives here because the discovery of
> > >>> extensions is done too late and we need to start with the qspinlock
> > >> That's not true; we treat spinlock as qspinlock at first.
> > > That's what I said: we have to use the qspinlock implementation at
> > > first *because* we can't discover the extensions soon enough to use
> > > the right spinlock implementation before the kernel uses a spinlock.
> > > And since the spinlocks are used *before* the discovery of the
> > > extensions, we cannot use the current alternative mechanism or we'd
> > > need to extend it to add an "initial" value which does not depend on
> > > the available extensions.
> >
> > With qspinlock, the lock remains zero after a lock/unlock sequence. That
> > is not the case with ticket lock. Assuming that all the discovery will
> > be done before SMP boot, the qspinlock slowpath won't be activated and
> > so we don't need the presence of any extension. I believe that is the
> > main reason why qspinlock is used as the initial default and not ticket
> > lock.
> Thx Waiman,
> Yes, qspinlock is a clean guy, but ticket lock is a dirty one.
Guys, we all agree on that, that's why I kept this behaviour in this patchset.
>
> Hi Alexandre,
> Therefore, the switch point(before reset_init()) is late enough to
> change the lock mechanism, and this satisfies the requirements of
> apply_boot_alternatives(), apply_early_boot_alternatives(), and
> apply_module_alternatives().
I can't find reset_init().
The discovery of the extensions is done in riscv_fill_hwcap() called
from setup_arch()
https://elixir.bootlin.com/linux/latest/source/arch/riscv/kernel/setup.c#L288.
So *before* this point, we have no knowledge of the available
extensions on the platform. Let's imagine now that we use an
alternative for the qspinlock implementation, it will look like this
(I use only zabha here, that's an example):
--- a/arch/riscv/include/asm/spinlock.h
+++ b/arch/riscv/include/asm/spinlock.h
@@ -16,8 +16,12 @@ DECLARE_STATIC_KEY_TRUE(qspinlock_key);
#define SPINLOCK_BASE_DECLARE(op, type, type_lock) \
static __always_inline type arch_spin_##op(type_lock lock) \
{ \
- if (static_branch_unlikely(&qspinlock_key)) \
- return queued_spin_##op(lock); \
+ asm goto(ALTERNATIVE("j %[no_zabha]", "nop", 0, \
+ RISCV_ISA_EXT_ZABHA, 1) \
+ : : : : no_zabha); \
+ \
+ return queued_spin_##op(lock); \
+no_zabha: \
return ticket_spin_##op(lock); \
}
apply_early_boot_alternatives() can't be used to patch the above
alternative since it is called from setup_vm(), way before we know the
available extensions.
apply_boot_alternatives(), instead, is called after riscv_fill_hwcap()
and then will be able to patch the alternatives correctly
https://elixir.bootlin.com/linux/latest/source/arch/riscv/kernel/setup.c#L290.
But then, before apply_boot_alternatives(), any use of a spinlock
(printk does so) will use a ticket spinlock implementation, and not
the qspinlock implementation. How do you fix that?
>
> >
> > Cheers,
> > Longman
> >
>
>
> --
> Best Regards
> Guo Ren
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v2 10/10] riscv: Add qspinlock support based on Zabha extension
@ 2024-07-16 6:43 ` Alexandre Ghiti
0 siblings, 0 replies; 25+ messages in thread
From: Alexandre Ghiti @ 2024-07-16 6:43 UTC (permalink / raw)
To: Guo Ren
Cc: Waiman Long, Jonathan Corbet, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Andrea Parri, Nathan Chancellor, Peter Zijlstra,
Ingo Molnar, Will Deacon, Boqun Feng, Arnd Bergmann,
Leonardo Bras, linux-doc, linux-kernel, linux-riscv, linux-arch
Hi Guo, Waiman,
On Tue, Jul 16, 2024 at 3:05 AM Guo Ren <guoren@kernel.org> wrote:
>
> On Tue, Jul 16, 2024 at 3:30 AM Waiman Long <longman@redhat.com> wrote:
> >
> > On 7/15/24 03:27, Alexandre Ghiti wrote:
> > > Hi Guo,
> > >
> > > On Sun, Jul 7, 2024 at 4:20 AM Guo Ren <guoren@kernel.org> wrote:
> > >> On Wed, Jun 26, 2024 at 9:14 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
> > >>> In order to produce a generic kernel, a user can select
> > >>> CONFIG_COMBO_SPINLOCKS which will fallback at runtime to the ticket
> > >>> spinlock implementation if Zabha is not present.
> > >>>
> > >>> Note that we can't use alternatives here because the discovery of
> > >>> extensions is done too late and we need to start with the qspinlock
> > >> That's not true; we treat spinlock as qspinlock at first.
> > > That's what I said: we have to use the qspinlock implementation at
> > > first *because* we can't discover the extensions soon enough to use
> > > the right spinlock implementation before the kernel uses a spinlock.
> > > And since the spinlocks are used *before* the discovery of the
> > > extensions, we cannot use the current alternative mechanism or we'd
> > > need to extend it to add an "initial" value which does not depend on
> > > the available extensions.
> >
> > With qspinlock, the lock remains zero after a lock/unlock sequence. That
> > is not the case with ticket lock. Assuming that all the discovery will
> > be done before SMP boot, the qspinlock slowpath won't be activated and
> > so we don't need the presence of any extension. I believe that is the
> > main reason why qspinlock is used as the initial default and not ticket
> > lock.
> Thx Waiman,
> Yes, qspinlock is a clean guy, but ticket lock is a dirty one.
Guys, we all agree on that, that's why I kept this behaviour in this patchset.
>
> Hi Alexandre,
> Therefore, the switch point(before reset_init()) is late enough to
> change the lock mechanism, and this satisfies the requirements of
> apply_boot_alternatives(), apply_early_boot_alternatives(), and
> apply_module_alternatives().
I can't find reset_init().
The discovery of the extensions is done in riscv_fill_hwcap() called
from setup_arch()
https://elixir.bootlin.com/linux/latest/source/arch/riscv/kernel/setup.c#L288.
So *before* this point, we have no knowledge of the available
extensions on the platform. Let's imagine now that we use an
alternative for the qspinlock implementation, it will look like this
(I use only zabha here, that's an example):
--- a/arch/riscv/include/asm/spinlock.h
+++ b/arch/riscv/include/asm/spinlock.h
@@ -16,8 +16,12 @@ DECLARE_STATIC_KEY_TRUE(qspinlock_key);
#define SPINLOCK_BASE_DECLARE(op, type, type_lock) \
static __always_inline type arch_spin_##op(type_lock lock) \
{ \
- if (static_branch_unlikely(&qspinlock_key)) \
- return queued_spin_##op(lock); \
+ asm goto(ALTERNATIVE("j %[no_zabha]", "nop", 0, \
+ RISCV_ISA_EXT_ZABHA, 1) \
+ : : : : no_zabha); \
+ \
+ return queued_spin_##op(lock); \
+no_zabha: \
return ticket_spin_##op(lock); \
}
apply_early_boot_alternatives() can't be used to patch the above
alternative since it is called from setup_vm(), way before we know the
available extensions.
apply_boot_alternatives(), instead, is called after riscv_fill_hwcap()
and then will be able to patch the alternatives correctly
https://elixir.bootlin.com/linux/latest/source/arch/riscv/kernel/setup.c#L290.
But then, before apply_boot_alternatives(), any use of a spinlock
(printk does so) will use a ticket spinlock implementation, and not
the qspinlock implementation. How do you fix that?
>
> >
> > Cheers,
> > Longman
> >
>
>
> --
> Best Regards
> Guo Ren
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v2 10/10] riscv: Add qspinlock support based on Zabha extension
2024-07-16 6:43 ` Alexandre Ghiti
@ 2024-07-16 8:31 ` Guo Ren
-1 siblings, 0 replies; 25+ messages in thread
From: Guo Ren @ 2024-07-16 8:31 UTC (permalink / raw)
To: Alexandre Ghiti
Cc: Waiman Long, Jonathan Corbet, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Andrea Parri, Nathan Chancellor, Peter Zijlstra,
Ingo Molnar, Will Deacon, Boqun Feng, Arnd Bergmann,
Leonardo Bras, linux-doc, linux-kernel, linux-riscv, linux-arch
On Tue, Jul 16, 2024 at 2:43 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
>
> Hi Guo, Waiman,
>
> On Tue, Jul 16, 2024 at 3:05 AM Guo Ren <guoren@kernel.org> wrote:
> >
> > On Tue, Jul 16, 2024 at 3:30 AM Waiman Long <longman@redhat.com> wrote:
> > >
> > > On 7/15/24 03:27, Alexandre Ghiti wrote:
> > > > Hi Guo,
> > > >
> > > > On Sun, Jul 7, 2024 at 4:20 AM Guo Ren <guoren@kernel.org> wrote:
> > > >> On Wed, Jun 26, 2024 at 9:14 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
> > > >>> In order to produce a generic kernel, a user can select
> > > >>> CONFIG_COMBO_SPINLOCKS which will fallback at runtime to the ticket
> > > >>> spinlock implementation if Zabha is not present.
> > > >>>
> > > >>> Note that we can't use alternatives here because the discovery of
> > > >>> extensions is done too late and we need to start with the qspinlock
> > > >> That's not true; we treat spinlock as qspinlock at first.
> > > > That's what I said: we have to use the qspinlock implementation at
> > > > first *because* we can't discover the extensions soon enough to use
> > > > the right spinlock implementation before the kernel uses a spinlock.
> > > > And since the spinlocks are used *before* the discovery of the
> > > > extensions, we cannot use the current alternative mechanism or we'd
> > > > need to extend it to add an "initial" value which does not depend on
> > > > the available extensions.
> > >
> > > With qspinlock, the lock remains zero after a lock/unlock sequence. That
> > > is not the case with ticket lock. Assuming that all the discovery will
> > > be done before SMP boot, the qspinlock slowpath won't be activated and
> > > so we don't need the presence of any extension. I believe that is the
> > > main reason why qspinlock is used as the initial default and not ticket
> > > lock.
> > Thx Waiman,
> > Yes, qspinlock is a clean guy, but ticket lock is a dirty one.
>
> Guys, we all agree on that, that's why I kept this behaviour in this patchset.
>
> >
> > Hi Alexandre,
> > Therefore, the switch point(before reset_init()) is late enough to
> > change the lock mechanism, and this satisfies the requirements of
> > apply_boot_alternatives(), apply_early_boot_alternatives(), and
> > apply_module_alternatives().
>
> I can't find reset_init().
Sorry for the typo, rest_init()
>
> The discovery of the extensions is done in riscv_fill_hwcap() called
> from setup_arch()
> https://elixir.bootlin.com/linux/latest/source/arch/riscv/kernel/setup.c#L288.
> So *before* this point, we have no knowledge of the available
> extensions on the platform. Let's imagine now that we use an
> alternative for the qspinlock implementation, it will look like this
> (I use only zabha here, that's an example):
>
> --- a/arch/riscv/include/asm/spinlock.h
> +++ b/arch/riscv/include/asm/spinlock.h
> @@ -16,8 +16,12 @@ DECLARE_STATIC_KEY_TRUE(qspinlock_key);
> #define SPINLOCK_BASE_DECLARE(op, type, type_lock) \
> static __always_inline type arch_spin_##op(type_lock lock) \
> { \
> - if (static_branch_unlikely(&qspinlock_key)) \
> - return queued_spin_##op(lock); \
> + asm goto(ALTERNATIVE("j %[no_zabha]", "nop", 0, \
> + RISCV_ISA_EXT_ZABHA, 1) \
> + : : : : no_zabha); \
> + \
> + return queued_spin_##op(lock); \
> +no_zabha: \
> return ticket_spin_##op(lock); \
> }
>
> apply_early_boot_alternatives() can't be used to patch the above
> alternative since it is called from setup_vm(), way before we know the
> available extensions.
> apply_boot_alternatives(), instead, is called after riscv_fill_hwcap()
> and then will be able to patch the alternatives correctly
> https://elixir.bootlin.com/linux/latest/source/arch/riscv/kernel/setup.c#L290.
>
> But then, before apply_boot_alternatives(), any use of a spinlock
> (printk does so) will use a ticket spinlock implementation, and not
> the qspinlock implementation. How do you fix that?
Why "before apply_boot_alternatives(), any use of a spinlock (printk
does so) will use a ticket spinlock implementation" ?
We treat qspinlock as the initial spinlock forever, so there is only
qspinlock -> ticket_lock direction and no reverse.
>
> >
> > >
> > > Cheers,
> > > Longman
> > >
> >
> >
> > --
> > Best Regards
> > Guo Ren
--
Best Regards
Guo Ren
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v2 10/10] riscv: Add qspinlock support based on Zabha extension
@ 2024-07-16 8:31 ` Guo Ren
0 siblings, 0 replies; 25+ messages in thread
From: Guo Ren @ 2024-07-16 8:31 UTC (permalink / raw)
To: Alexandre Ghiti
Cc: Waiman Long, Jonathan Corbet, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Andrea Parri, Nathan Chancellor, Peter Zijlstra,
Ingo Molnar, Will Deacon, Boqun Feng, Arnd Bergmann,
Leonardo Bras, linux-doc, linux-kernel, linux-riscv, linux-arch
On Tue, Jul 16, 2024 at 2:43 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
>
> Hi Guo, Waiman,
>
> On Tue, Jul 16, 2024 at 3:05 AM Guo Ren <guoren@kernel.org> wrote:
> >
> > On Tue, Jul 16, 2024 at 3:30 AM Waiman Long <longman@redhat.com> wrote:
> > >
> > > On 7/15/24 03:27, Alexandre Ghiti wrote:
> > > > Hi Guo,
> > > >
> > > > On Sun, Jul 7, 2024 at 4:20 AM Guo Ren <guoren@kernel.org> wrote:
> > > >> On Wed, Jun 26, 2024 at 9:14 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
> > > >>> In order to produce a generic kernel, a user can select
> > > >>> CONFIG_COMBO_SPINLOCKS which will fallback at runtime to the ticket
> > > >>> spinlock implementation if Zabha is not present.
> > > >>>
> > > >>> Note that we can't use alternatives here because the discovery of
> > > >>> extensions is done too late and we need to start with the qspinlock
> > > >> That's not true; we treat spinlock as qspinlock at first.
> > > > That's what I said: we have to use the qspinlock implementation at
> > > > first *because* we can't discover the extensions soon enough to use
> > > > the right spinlock implementation before the kernel uses a spinlock.
> > > > And since the spinlocks are used *before* the discovery of the
> > > > extensions, we cannot use the current alternative mechanism or we'd
> > > > need to extend it to add an "initial" value which does not depend on
> > > > the available extensions.
> > >
> > > With qspinlock, the lock remains zero after a lock/unlock sequence. That
> > > is not the case with ticket lock. Assuming that all the discovery will
> > > be done before SMP boot, the qspinlock slowpath won't be activated and
> > > so we don't need the presence of any extension. I believe that is the
> > > main reason why qspinlock is used as the initial default and not ticket
> > > lock.
> > Thx Waiman,
> > Yes, qspinlock is a clean guy, but ticket lock is a dirty one.
>
> Guys, we all agree on that, that's why I kept this behaviour in this patchset.
>
> >
> > Hi Alexandre,
> > Therefore, the switch point(before reset_init()) is late enough to
> > change the lock mechanism, and this satisfies the requirements of
> > apply_boot_alternatives(), apply_early_boot_alternatives(), and
> > apply_module_alternatives().
>
> I can't find reset_init().
Sorry for the typo, rest_init()
>
> The discovery of the extensions is done in riscv_fill_hwcap() called
> from setup_arch()
> https://elixir.bootlin.com/linux/latest/source/arch/riscv/kernel/setup.c#L288.
> So *before* this point, we have no knowledge of the available
> extensions on the platform. Let's imagine now that we use an
> alternative for the qspinlock implementation, it will look like this
> (I use only zabha here, that's an example):
>
> --- a/arch/riscv/include/asm/spinlock.h
> +++ b/arch/riscv/include/asm/spinlock.h
> @@ -16,8 +16,12 @@ DECLARE_STATIC_KEY_TRUE(qspinlock_key);
> #define SPINLOCK_BASE_DECLARE(op, type, type_lock) \
> static __always_inline type arch_spin_##op(type_lock lock) \
> { \
> - if (static_branch_unlikely(&qspinlock_key)) \
> - return queued_spin_##op(lock); \
> + asm goto(ALTERNATIVE("j %[no_zabha]", "nop", 0, \
> + RISCV_ISA_EXT_ZABHA, 1) \
> + : : : : no_zabha); \
> + \
> + return queued_spin_##op(lock); \
> +no_zabha: \
> return ticket_spin_##op(lock); \
> }
>
> apply_early_boot_alternatives() can't be used to patch the above
> alternative since it is called from setup_vm(), way before we know the
> available extensions.
> apply_boot_alternatives(), instead, is called after riscv_fill_hwcap()
> and then will be able to patch the alternatives correctly
> https://elixir.bootlin.com/linux/latest/source/arch/riscv/kernel/setup.c#L290.
>
> But then, before apply_boot_alternatives(), any use of a spinlock
> (printk does so) will use a ticket spinlock implementation, and not
> the qspinlock implementation. How do you fix that?
Why "before apply_boot_alternatives(), any use of a spinlock (printk
does so) will use a ticket spinlock implementation" ?
We treat qspinlock as the initial spinlock forever, so there is only
qspinlock -> ticket_lock direction and no reverse.
>
> >
> > >
> > > Cheers,
> > > Longman
> > >
> >
> >
> > --
> > Best Regards
> > Guo Ren
--
Best Regards
Guo Ren
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v2 10/10] riscv: Add qspinlock support based on Zabha extension
2024-07-16 8:31 ` Guo Ren
@ 2024-07-17 6:19 ` Alexandre Ghiti
-1 siblings, 0 replies; 25+ messages in thread
From: Alexandre Ghiti @ 2024-07-17 6:19 UTC (permalink / raw)
To: Guo Ren
Cc: Waiman Long, Jonathan Corbet, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Andrea Parri, Nathan Chancellor, Peter Zijlstra,
Ingo Molnar, Will Deacon, Boqun Feng, Arnd Bergmann,
Leonardo Bras, linux-doc, linux-kernel, linux-riscv, linux-arch
On Tue, Jul 16, 2024 at 10:31 AM Guo Ren <guoren@kernel.org> wrote:
>
> On Tue, Jul 16, 2024 at 2:43 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
> >
> > Hi Guo, Waiman,
> >
> > On Tue, Jul 16, 2024 at 3:05 AM Guo Ren <guoren@kernel.org> wrote:
> > >
> > > On Tue, Jul 16, 2024 at 3:30 AM Waiman Long <longman@redhat.com> wrote:
> > > >
> > > > On 7/15/24 03:27, Alexandre Ghiti wrote:
> > > > > Hi Guo,
> > > > >
> > > > > On Sun, Jul 7, 2024 at 4:20 AM Guo Ren <guoren@kernel.org> wrote:
> > > > >> On Wed, Jun 26, 2024 at 9:14 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
> > > > >>> In order to produce a generic kernel, a user can select
> > > > >>> CONFIG_COMBO_SPINLOCKS which will fallback at runtime to the ticket
> > > > >>> spinlock implementation if Zabha is not present.
> > > > >>>
> > > > >>> Note that we can't use alternatives here because the discovery of
> > > > >>> extensions is done too late and we need to start with the qspinlock
> > > > >> That's not true; we treat spinlock as qspinlock at first.
> > > > > That's what I said: we have to use the qspinlock implementation at
> > > > > first *because* we can't discover the extensions soon enough to use
> > > > > the right spinlock implementation before the kernel uses a spinlock.
> > > > > And since the spinlocks are used *before* the discovery of the
> > > > > extensions, we cannot use the current alternative mechanism or we'd
> > > > > need to extend it to add an "initial" value which does not depend on
> > > > > the available extensions.
> > > >
> > > > With qspinlock, the lock remains zero after a lock/unlock sequence. That
> > > > is not the case with ticket lock. Assuming that all the discovery will
> > > > be done before SMP boot, the qspinlock slowpath won't be activated and
> > > > so we don't need the presence of any extension. I believe that is the
> > > > main reason why qspinlock is used as the initial default and not ticket
> > > > lock.
> > > Thx Waiman,
> > > Yes, qspinlock is a clean guy, but ticket lock is a dirty one.
> >
> > Guys, we all agree on that, that's why I kept this behaviour in this patchset.
> >
> > >
> > > Hi Alexandre,
> > > Therefore, the switch point(before reset_init()) is late enough to
> > > change the lock mechanism, and this satisfies the requirements of
> > > apply_boot_alternatives(), apply_early_boot_alternatives(), and
> > > apply_module_alternatives().
> >
> > I can't find reset_init().
> Sorry for the typo, rest_init()
> >
> > The discovery of the extensions is done in riscv_fill_hwcap() called
> > from setup_arch()
> > https://elixir.bootlin.com/linux/latest/source/arch/riscv/kernel/setup.c#L288.
> > So *before* this point, we have no knowledge of the available
> > extensions on the platform. Let's imagine now that we use an
> > alternative for the qspinlock implementation, it will look like this
> > (I use only zabha here, that's an example):
> >
> > --- a/arch/riscv/include/asm/spinlock.h
> > +++ b/arch/riscv/include/asm/spinlock.h
> > @@ -16,8 +16,12 @@ DECLARE_STATIC_KEY_TRUE(qspinlock_key);
> > #define SPINLOCK_BASE_DECLARE(op, type, type_lock) \
> > static __always_inline type arch_spin_##op(type_lock lock) \
> > { \
> > - if (static_branch_unlikely(&qspinlock_key)) \
> > - return queued_spin_##op(lock); \
> > + asm goto(ALTERNATIVE("j %[no_zabha]", "nop", 0, \
> > + RISCV_ISA_EXT_ZABHA, 1) \
> > + : : : : no_zabha); \
> > + \
> > + return queued_spin_##op(lock); \
> > +no_zabha: \
> > return ticket_spin_##op(lock); \
> > }
> >
> > apply_early_boot_alternatives() can't be used to patch the above
> > alternative since it is called from setup_vm(), way before we know the
> > available extensions.
> > apply_boot_alternatives(), instead, is called after riscv_fill_hwcap()
> > and then will be able to patch the alternatives correctly
> > https://elixir.bootlin.com/linux/latest/source/arch/riscv/kernel/setup.c#L290.
> >
> > But then, before apply_boot_alternatives(), any use of a spinlock
> > (printk does so) will use a ticket spinlock implementation, and not
> > the qspinlock implementation. How do you fix that?
> Why "before apply_boot_alternatives(), any use of a spinlock (printk
> does so) will use a ticket spinlock implementation" ?
> We treat qspinlock as the initial spinlock forever, so there is only
> qspinlock -> ticket_lock direction and no reverse.
Can you please provide an implementation of what you suggest using the
current alternatives tools? I'm about to send the v3 of this series,
you can use this as a base.
Thanks,
Alex
>
> >
> > >
> > > >
> > > > Cheers,
> > > > Longman
> > > >
> > >
> > >
> > > --
> > > Best Regards
> > > Guo Ren
>
>
>
> --
> Best Regards
> Guo Ren
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v2 10/10] riscv: Add qspinlock support based on Zabha extension
@ 2024-07-17 6:19 ` Alexandre Ghiti
0 siblings, 0 replies; 25+ messages in thread
From: Alexandre Ghiti @ 2024-07-17 6:19 UTC (permalink / raw)
To: Guo Ren
Cc: Waiman Long, Jonathan Corbet, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Andrea Parri, Nathan Chancellor, Peter Zijlstra,
Ingo Molnar, Will Deacon, Boqun Feng, Arnd Bergmann,
Leonardo Bras, linux-doc, linux-kernel, linux-riscv, linux-arch
On Tue, Jul 16, 2024 at 10:31 AM Guo Ren <guoren@kernel.org> wrote:
>
> On Tue, Jul 16, 2024 at 2:43 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
> >
> > Hi Guo, Waiman,
> >
> > On Tue, Jul 16, 2024 at 3:05 AM Guo Ren <guoren@kernel.org> wrote:
> > >
> > > On Tue, Jul 16, 2024 at 3:30 AM Waiman Long <longman@redhat.com> wrote:
> > > >
> > > > On 7/15/24 03:27, Alexandre Ghiti wrote:
> > > > > Hi Guo,
> > > > >
> > > > > On Sun, Jul 7, 2024 at 4:20 AM Guo Ren <guoren@kernel.org> wrote:
> > > > >> On Wed, Jun 26, 2024 at 9:14 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
> > > > >>> In order to produce a generic kernel, a user can select
> > > > >>> CONFIG_COMBO_SPINLOCKS which will fallback at runtime to the ticket
> > > > >>> spinlock implementation if Zabha is not present.
> > > > >>>
> > > > >>> Note that we can't use alternatives here because the discovery of
> > > > >>> extensions is done too late and we need to start with the qspinlock
> > > > >> That's not true; we treat spinlock as qspinlock at first.
> > > > > That's what I said: we have to use the qspinlock implementation at
> > > > > first *because* we can't discover the extensions soon enough to use
> > > > > the right spinlock implementation before the kernel uses a spinlock.
> > > > > And since the spinlocks are used *before* the discovery of the
> > > > > extensions, we cannot use the current alternative mechanism or we'd
> > > > > need to extend it to add an "initial" value which does not depend on
> > > > > the available extensions.
> > > >
> > > > With qspinlock, the lock remains zero after a lock/unlock sequence. That
> > > > is not the case with ticket lock. Assuming that all the discovery will
> > > > be done before SMP boot, the qspinlock slowpath won't be activated and
> > > > so we don't need the presence of any extension. I believe that is the
> > > > main reason why qspinlock is used as the initial default and not ticket
> > > > lock.
> > > Thx Waiman,
> > > Yes, qspinlock is a clean guy, but ticket lock is a dirty one.
> >
> > Guys, we all agree on that, that's why I kept this behaviour in this patchset.
> >
> > >
> > > Hi Alexandre,
> > > Therefore, the switch point(before reset_init()) is late enough to
> > > change the lock mechanism, and this satisfies the requirements of
> > > apply_boot_alternatives(), apply_early_boot_alternatives(), and
> > > apply_module_alternatives().
> >
> > I can't find reset_init().
> Sorry for the typo, rest_init()
> >
> > The discovery of the extensions is done in riscv_fill_hwcap() called
> > from setup_arch()
> > https://elixir.bootlin.com/linux/latest/source/arch/riscv/kernel/setup.c#L288.
> > So *before* this point, we have no knowledge of the available
> > extensions on the platform. Let's imagine now that we use an
> > alternative for the qspinlock implementation, it will look like this
> > (I use only zabha here, that's an example):
> >
> > --- a/arch/riscv/include/asm/spinlock.h
> > +++ b/arch/riscv/include/asm/spinlock.h
> > @@ -16,8 +16,12 @@ DECLARE_STATIC_KEY_TRUE(qspinlock_key);
> > #define SPINLOCK_BASE_DECLARE(op, type, type_lock) \
> > static __always_inline type arch_spin_##op(type_lock lock) \
> > { \
> > - if (static_branch_unlikely(&qspinlock_key)) \
> > - return queued_spin_##op(lock); \
> > + asm goto(ALTERNATIVE("j %[no_zabha]", "nop", 0, \
> > + RISCV_ISA_EXT_ZABHA, 1) \
> > + : : : : no_zabha); \
> > + \
> > + return queued_spin_##op(lock); \
> > +no_zabha: \
> > return ticket_spin_##op(lock); \
> > }
> >
> > apply_early_boot_alternatives() can't be used to patch the above
> > alternative since it is called from setup_vm(), way before we know the
> > available extensions.
> > apply_boot_alternatives(), instead, is called after riscv_fill_hwcap()
> > and then will be able to patch the alternatives correctly
> > https://elixir.bootlin.com/linux/latest/source/arch/riscv/kernel/setup.c#L290.
> >
> > But then, before apply_boot_alternatives(), any use of a spinlock
> > (printk does so) will use a ticket spinlock implementation, and not
> > the qspinlock implementation. How do you fix that?
> Why "before apply_boot_alternatives(), any use of a spinlock (printk
> does so) will use a ticket spinlock implementation" ?
> We treat qspinlock as the initial spinlock forever, so there is only
> qspinlock -> ticket_lock direction and no reverse.
Can you please provide an implementation of what you suggest using the
current alternatives tools? I'm about to send the v3 of this series,
you can use this as a base.
Thanks,
Alex
>
> >
> > >
> > > >
> > > > Cheers,
> > > > Longman
> > > >
> > >
> > >
> > > --
> > > Best Regards
> > > Guo Ren
>
>
>
> --
> Best Regards
> Guo Ren
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 25+ messages in thread