All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 10/10] riscv: Add qspinlock support based on Zabha extension
  2024-06-26 13:03 [PATCH v2 00/10] Zacas/Zabha support and qspinlocks Alexandre Ghiti
@ 2024-06-26 13:03   ` Alexandre Ghiti
  0 siblings, 0 replies; 25+ messages in thread
From: Alexandre Ghiti @ 2024-06-26 13:03 UTC (permalink / raw)
  To: 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, Guo Ren, linux-doc, linux-kernel, linux-riscv,
	linux-arch
  Cc: Alexandre Ghiti

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
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.
+
 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);
+
+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


^ permalink raw reply related	[flat|nested] 25+ messages in thread
* [PATCH v2 10/10] riscv: Add qspinlock support based on Zabha extension
@ 2024-06-26 13:03   ` Alexandre Ghiti
  0 siblings, 0 replies; 25+ messages in thread
From: Alexandre Ghiti @ 2024-06-26 13:03 UTC (permalink / raw)
  To: 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, Guo Ren, linux-doc, linux-kernel, linux-riscv,
	linux-arch
  Cc: Alexandre Ghiti

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
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.
+
 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);
+
+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


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

^ permalink raw reply related	[flat|nested] 25+ messages in thread
* Re: [PATCH v2 10/10] riscv: Add qspinlock support based on Zabha extension
@ 2024-06-27  7:21 kernel test robot
  0 siblings, 0 replies; 25+ messages in thread
From: kernel test robot @ 2024-06-27  7:21 UTC (permalink / raw)
  To: oe-kbuild; +Cc: lkp

:::::: 
:::::: Manual check reason: "low confidence bisect report"
:::::: 

BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20240626130347.520750-11-alexghiti@rivosinc.com>
References: <20240626130347.520750-11-alexghiti@rivosinc.com>
TO: Alexandre Ghiti <alexghiti@rivosinc.com>
TO: Jonathan Corbet <corbet@lwn.net>
TO: Paul Walmsley <paul.walmsley@sifive.com>
TO: Palmer Dabbelt <palmer@dabbelt.com>
TO: Albert Ou <aou@eecs.berkeley.edu>
TO: Andrea Parri <parri.andrea@gmail.com>
TO: Nathan Chancellor <nathan@kernel.org>
TO: Peter Zijlstra <peterz@infradead.org>
TO: Ingo Molnar <mingo@redhat.com>
TO: Will Deacon <will@kernel.org>
TO: Waiman Long <longman@redhat.com>
TO: Boqun Feng <boqun.feng@gmail.com>
TO: Arnd Bergmann <arnd@arndb.de>
TO: Leonardo Bras <leobras@redhat.com>
TO: Guo Ren <guoren@kernel.org>
TO: linux-doc@vger.kernel.org
TO: linux-kernel@vger.kernel.org
TO: linux-riscv@lists.infradead.org
TO: linux-arch@vger.kernel.org
CC: Alexandre Ghiti <alexghiti@rivosinc.com>

Hi Alexandre,

kernel test robot noticed the following build warnings:

[auto build test WARNING on soc/for-next]
[also build test WARNING on linus/master v6.10-rc5]
[cannot apply to arnd-asm-generic/master robh/for-next tip/locking/core next-20240626]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Alexandre-Ghiti/riscv-Implement-cmpxchg32-64-using-Zacas/20240627-034946
base:   https://git.kernel.org/pub/scm/linux/kernel/git/soc/soc.git for-next
patch link:    https://lore.kernel.org/r/20240626130347.520750-11-alexghiti%40rivosinc.com
patch subject: [PATCH v2 10/10] riscv: Add qspinlock support based on Zabha extension
:::::: branch date: 11 hours ago
:::::: commit date: 11 hours ago
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/r/202406271442.V29TPfzy-lkp@intel.com/

includecheck warnings: (new ones prefixed by >>)
>> arch/riscv/include/asm/spinlock.h: asm/ticket_spinlock.h is included more than once.

vim +10 arch/riscv/include/asm/spinlock.h

     8	
     9	#define __no_arch_spinlock_redefine
  > 10	#include <asm/ticket_spinlock.h>
    11	#include <asm/qspinlock.h>
    12	#include <asm/alternative.h>
    13	
    14	DECLARE_STATIC_KEY_TRUE(qspinlock_key);
    15	
    16	#define SPINLOCK_BASE_DECLARE(op, type, type_lock)			\
    17	static __always_inline type arch_spin_##op(type_lock lock)		\
    18	{									\
    19		if (static_branch_unlikely(&qspinlock_key))			\
    20			return queued_spin_##op(lock);				\
    21		return ticket_spin_##op(lock);					\
    22	}
    23	
    24	SPINLOCK_BASE_DECLARE(lock, void, arch_spinlock_t *)
    25	SPINLOCK_BASE_DECLARE(unlock, void, arch_spinlock_t *)
    26	SPINLOCK_BASE_DECLARE(is_locked, int, arch_spinlock_t *)
    27	SPINLOCK_BASE_DECLARE(is_contended, int, arch_spinlock_t *)
    28	SPINLOCK_BASE_DECLARE(trylock, bool, arch_spinlock_t *)
    29	SPINLOCK_BASE_DECLARE(value_unlocked, int, arch_spinlock_t)
    30	
    31	#else
    32	
  > 33	#include <asm/ticket_spinlock.h>
    34	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 25+ messages in thread
* Re: [PATCH v2 10/10] riscv: Add qspinlock support based on Zabha extension
  2024-06-26 13:03   ` Alexandre Ghiti
@ 2024-06-27 15:19     ` Andrea Parri
  -1 siblings, 0 replies; 25+ messages in thread
From: Andrea Parri @ 2024-06-27 15:19 UTC (permalink / raw)
  To: Alexandre Ghiti
  Cc: Jonathan Corbet, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Nathan Chancellor, Peter Zijlstra, Ingo Molnar, Will Deacon,
	Waiman Long, Boqun Feng, Arnd Bergmann, Leonardo Bras, Guo Ren,
	linux-doc, linux-kernel, linux-riscv, linux-arch

> This is largely based on Guo's work and Leonardo reviews at [1].

Guo, could/should this have your Co-developed-by:/Signed-off-by:?

(disclaimer: I haven't looked at the last three patches of this submission
with due calm and probably won't before ~mid-July...)


> Link: https://lore.kernel.org/linux-riscv/20231225125847.2778638-1-guoren@kernel.org/ [1]

There seems to be a distinct lack of experimental results, compared to the
previous/cited submission (and numbers are good to have!!  ;-)). Maybe Guo
/others can provide some? to confirm this is going in the right direction.

  Andrea

^ permalink raw reply	[flat|nested] 25+ messages in thread
* Re: [PATCH v2 10/10] riscv: Add qspinlock support based on Zabha extension
@ 2024-06-27 15:19     ` Andrea Parri
  0 siblings, 0 replies; 25+ messages in thread
From: Andrea Parri @ 2024-06-27 15:19 UTC (permalink / raw)
  To: Alexandre Ghiti
  Cc: Jonathan Corbet, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Nathan Chancellor, Peter Zijlstra, Ingo Molnar, Will Deacon,
	Waiman Long, Boqun Feng, Arnd Bergmann, Leonardo Bras, Guo Ren,
	linux-doc, linux-kernel, linux-riscv, linux-arch

> This is largely based on Guo's work and Leonardo reviews at [1].

Guo, could/should this have your Co-developed-by:/Signed-off-by:?

(disclaimer: I haven't looked at the last three patches of this submission
with due calm and probably won't before ~mid-July...)


> Link: https://lore.kernel.org/linux-riscv/20231225125847.2778638-1-guoren@kernel.org/ [1]

There seems to be a distinct lack of experimental results, compared to the
previous/cited submission (and numbers are good to have!!  ;-)). Maybe Guo
/others can provide some? to confirm this is going in the right direction.

  Andrea

_______________________________________________
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-06-27 15:19     ` Andrea Parri
@ 2024-07-04 17:33       ` Alexandre Ghiti
  -1 siblings, 0 replies; 25+ messages in thread
From: Alexandre Ghiti @ 2024-07-04 17:33 UTC (permalink / raw)
  To: Andrea Parri, Alexandre Ghiti
  Cc: Jonathan Corbet, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Nathan Chancellor, Peter Zijlstra, Ingo Molnar, Will Deacon,
	Waiman Long, Boqun Feng, Arnd Bergmann, Leonardo Bras, Guo Ren,
	linux-doc, linux-kernel, linux-riscv, linux-arch


On 27/06/2024 17:19, Andrea Parri wrote:
>> This is largely based on Guo's work and Leonardo reviews at [1].
> Guo, could/should this have your Co-developed-by:/Signed-off-by:?


Indeed, I'll add a SoB from Guo.


>
> (disclaimer: I haven't looked at the last three patches of this submission
> with due calm and probably won't before ~mid-July...)
>
>
>> Link: https://lore.kernel.org/linux-riscv/20231225125847.2778638-1-guoren@kernel.org/ [1]
> There seems to be a distinct lack of experimental results, compared to the
> previous/cited submission (and numbers are good to have!!  ;-)). Maybe Guo
> /others can provide some? to confirm this is going in the right direction.
>
>    Andrea
>
> _______________________________________________
> 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-04 17:33       ` Alexandre Ghiti
  0 siblings, 0 replies; 25+ messages in thread
From: Alexandre Ghiti @ 2024-07-04 17:33 UTC (permalink / raw)
  To: Andrea Parri, Alexandre Ghiti
  Cc: Jonathan Corbet, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Nathan Chancellor, Peter Zijlstra, Ingo Molnar, Will Deacon,
	Waiman Long, Boqun Feng, Arnd Bergmann, Leonardo Bras, Guo Ren,
	linux-doc, linux-kernel, linux-riscv, linux-arch


On 27/06/2024 17:19, Andrea Parri wrote:
>> This is largely based on Guo's work and Leonardo reviews at [1].
> Guo, could/should this have your Co-developed-by:/Signed-off-by:?


Indeed, I'll add a SoB from Guo.


>
> (disclaimer: I haven't looked at the last three patches of this submission
> with due calm and probably won't before ~mid-July...)
>
>
>> Link: https://lore.kernel.org/linux-riscv/20231225125847.2778638-1-guoren@kernel.org/ [1]
> There seems to be a distinct lack of experimental results, compared to the
> previous/cited submission (and numbers are good to have!!  ;-)). Maybe Guo
> /others can provide some? to confirm this is going in the right direction.
>
>    Andrea
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

_______________________________________________
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-06-26 13:03   ` Alexandre Ghiti
@ 2024-07-07  2:20     ` Guo Ren
  -1 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

^ 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
  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-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-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-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
end of thread, other threads:[~2024-07-17  6:20 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-06-27  7:21 [PATCH v2 10/10] riscv: Add qspinlock support based on Zabha extension kernel test robot
  -- strict thread matches above, loose matches on Subject: below --
2024-06-26 13:03 [PATCH v2 00/10] Zacas/Zabha support and qspinlocks Alexandre Ghiti
2024-06-26 13:03 ` [PATCH v2 10/10] riscv: Add qspinlock support based on Zabha extension Alexandre Ghiti
2024-06-26 13:03   ` Alexandre Ghiti
2024-06-27 15:19   ` Andrea Parri
2024-06-27 15:19     ` Andrea Parri
2024-07-04 17:33     ` Alexandre Ghiti
2024-07-04 17:33       ` Alexandre Ghiti
2024-07-07  2:20   ` Guo Ren
2024-07-07  2:20     ` Guo Ren
2024-07-08 11:51     ` Guo Ren
2024-07-08 11:51       ` Guo Ren
2024-07-15  7:33       ` Alexandre Ghiti
2024-07-15  7:33         ` Alexandre Ghiti
2024-07-15  7:27     ` Alexandre Ghiti
2024-07-15  7:27       ` Alexandre Ghiti
2024-07-15 19:30       ` Waiman Long
2024-07-15 19:30         ` Waiman Long
2024-07-16  1:04         ` Guo Ren
2024-07-16  1:04           ` Guo Ren
2024-07-16  6:43           ` Alexandre Ghiti
2024-07-16  6:43             ` Alexandre Ghiti
2024-07-16  8:31             ` Guo Ren
2024-07-16  8:31               ` Guo Ren
2024-07-17  6:19               ` Alexandre Ghiti
2024-07-17  6:19                 ` Alexandre Ghiti

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.