[PATCH 0/6] riscv: Add helpers use_alternative_{likely,unlikely}

[PATCH 0/6] riscv: Add helpers use_alternative_{likely,unlikely}

[Vivian Wang]

There are about a dozen uses of asm goto in arch/riscv just to select
between two code paths with the alternative mechanism. Introduce helpers
similar to arm64's alternative_has_cap_{likely,unlikely} for this, and
convert the existing code to use it.

I did not use the name alternative_has_cap_{likely,unlikely} since riscv
alternatives are not all CPU capabilities.

In each case, I have tried to preserve the existing logic while picking
between "likely" and "unlikely".

These patches are also available at:

https://github.com/dramforever/linux/tree/riscv/altn-helper/v1

---
Vivian Wang (6):
      riscv: Introduce use_alternative_{likely,unlikely}
      riscv: pgtable: Convert to use_alternative_unlikely
      riscv: checksum: Convert to use_alternative_likely
      riscv: hweight: Convert to use_alternative_likely
      riscv: bitops: Convert to use_alternative_likely
      riscv: cmpxchg: Convert to use_alternative_likely

 arch/riscv/include/asm/alternative-macros.h |  73 ++++++++++++++++
 arch/riscv/include/asm/arch_hweight.h       |  42 ++++------
 arch/riscv/include/asm/bitops.h             | 112 +++++++++++--------------
 arch/riscv/include/asm/checksum.h           |  13 +--
 arch/riscv/include/asm/cmpxchg.h            | 125 ++++++++++++++--------------
 arch/riscv/include/asm/pgtable.h            |  15 ++--
 arch/riscv/lib/csum.c                       |  65 ++++++---------
 arch/riscv/mm/pgtable.c                     |  22 +++--
 8 files changed, 247 insertions(+), 220 deletions(-)
---
base-commit: 062b3e4a1f880f104a8d4b90b767788786aa7b78
change-id: 20250820-riscv-altn-helper-wip-00af3a552c37

Best regards,
-- 
Vivian "dramforever" Wang


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

[PATCH 1/6] riscv: Introduce use_alternative_{likely,unlikely}

[Vivian Wang]

Introduce convenience helpers use_alternative_likely() and
use_alternative_unlikely() to implement the pattern of using asm goto to
check if an alternative is selected. Existing code will be converted in
subsequent patches.

Similar to arm64 alternative_has_cap_{likely,unlikely}, but for riscv,
alternatives are not all CPU capabilities.

Suggested-by: Aydın Mercan <aydin@mercan.dev>
Signed-off-by: Vivian Wang <wangruikang@iscas.ac.cn>
---
 arch/riscv/include/asm/alternative-macros.h | 73 +++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)

diff --git a/arch/riscv/include/asm/alternative-macros.h b/arch/riscv/include/asm/alternative-macros.h
index 231d777d936c2d29c858decaa9a3fa5f172efbb8..be9835b5e4eba03d76db3a73da19ac9e2981c4db 100644
--- a/arch/riscv/include/asm/alternative-macros.h
+++ b/arch/riscv/include/asm/alternative-macros.h
@@ -158,4 +158,77 @@
 	_ALTERNATIVE_CFG_2(old_content, new_content_1, vendor_id_1, patch_id_1, CONFIG_k_1,	\
 					new_content_2, vendor_id_2, patch_id_2, CONFIG_k_2)
 
+/*
+ * use_alternative_{likely,unlikely}() returns true if the alternative is
+ * applied and false otherwise, but in a way where the compiler can optimize
+ * this check down to a nop instruction that's patched into a jump, or vice
+ * versa.
+ *
+ * Always returns false if the alternatives mechanism is not available.
+ *
+ * Usage example:
+ *   if (use_alternative_likely(0, RISCV_ISA_ZBB))
+ *
+ * Similar to static keys, "likely" means use a nop if the alternative is
+ * selected, and jump if unselected; "unlikely" is the other way around.
+ */
+
+#ifndef __ASSEMBLER__
+
+#include <linux/types.h>
+
+#ifdef CONFIG_RISCV_ALTERNATIVE
+
+static __always_inline bool use_alternative_likely(u16 vendor_id, u32 patch_id)
+{
+	BUILD_BUG_ON(!__builtin_constant_p(vendor_id));
+	BUILD_BUG_ON(!__builtin_constant_p(patch_id));
+
+	asm goto(ALTERNATIVE("j %l[no_alt]", "nop", %[vendor_id], %[patch_id], 1)
+		 :
+		 : [vendor_id] "i"(vendor_id),
+		   [patch_id] "i"(patch_id)
+		 :
+		 : no_alt);
+
+	return true;
+
+no_alt:
+	return false;
+}
+
+static __always_inline bool use_alternative_unlikely(u16 vendor_id, u32 patch_id)
+{
+	BUILD_BUG_ON(!__builtin_constant_p(vendor_id));
+	BUILD_BUG_ON(!__builtin_constant_p(patch_id));
+
+	asm goto(ALTERNATIVE("nop", "j %l[alt]", %[vendor_id], %[patch_id], 1)
+		 :
+		 : [vendor_id] "i"(vendor_id),
+		   [patch_id] "i"(patch_id)
+		 :
+		 : alt);
+
+	return false;
+
+alt:
+	return true;
+}
+
+#else
+
+static inline bool use_alternative_likely(u16 vendor_id, u32 patch_id)
+{
+	return false;
+}
+
+static inline bool use_alternative_unlikely(u16 vendor_id, u32 patch_id)
+{
+	return false;
+}
+
+#endif /* CONFIG_RISCV_ALTERNATIVE */
+
+#endif /* __ASSEMBLER__ */
+
 #endif

-- 
2.50.1


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

[PATCH 2/6] riscv: pgtable: Convert to use_alternative_unlikely

[Vivian Wang]

Use use_alternative_unlikely() to check for RISCV_ISA_EXT_SVVPTC,
replacing the use of asm goto with ALTERNATIVE.

The "unlikely" variant is used to match the behavior of the original
implementation using ALTERNATIVE("nop", "j %l[svvptc]", ...).

Signed-off-by: Vivian Wang <wangruikang@iscas.ac.cn>
---
 arch/riscv/include/asm/pgtable.h | 15 +++++++--------
 arch/riscv/mm/pgtable.c          | 22 ++++++++++------------
 2 files changed, 17 insertions(+), 20 deletions(-)

diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
index 91697fbf1f9013005800f713797e4b6b1fc8d312..81eb386da837f064c7372530e2f2227575a703d3 100644
--- a/arch/riscv/include/asm/pgtable.h
+++ b/arch/riscv/include/asm/pgtable.h
@@ -495,8 +495,13 @@ static inline void update_mmu_cache_range(struct vm_fault *vmf,
 		struct vm_area_struct *vma, unsigned long address,
 		pte_t *ptep, unsigned int nr)
 {
-	asm goto(ALTERNATIVE("nop", "j %l[svvptc]", 0, RISCV_ISA_EXT_SVVPTC, 1)
-		 : : : : svvptc);
+	/*
+	 * Svvptc guarantees that the new valid pte will be visible within
+	 * a bounded timeframe, so when the uarch does not cache invalid
+	 * entries, we don't have to do anything.
+	 */
+	if (use_alternative_unlikely(0, RISCV_ISA_EXT_SVVPTC))
+		return;
 
 	/*
 	 * The kernel assumes that TLBs don't cache invalid entries, but
@@ -508,12 +513,6 @@ static inline void update_mmu_cache_range(struct vm_fault *vmf,
 	while (nr--)
 		local_flush_tlb_page(address + nr * PAGE_SIZE);
 
-svvptc:;
-	/*
-	 * Svvptc guarantees that the new valid pte will be visible within
-	 * a bounded timeframe, so when the uarch does not cache invalid
-	 * entries, we don't have to do anything.
-	 */
 }
 #define update_mmu_cache(vma, addr, ptep) \
 	update_mmu_cache_range(NULL, vma, addr, ptep, 1)
diff --git a/arch/riscv/mm/pgtable.c b/arch/riscv/mm/pgtable.c
index 8b6c0a112a8db4e91de54c3bd3bd527a605a6197..e0c414fa0d433fdc39c80ec390c467ca59a9a334 100644
--- a/arch/riscv/mm/pgtable.c
+++ b/arch/riscv/mm/pgtable.c
@@ -9,8 +9,16 @@ int ptep_set_access_flags(struct vm_area_struct *vma,
 			  unsigned long address, pte_t *ptep,
 			  pte_t entry, int dirty)
 {
-	asm goto(ALTERNATIVE("nop", "j %l[svvptc]", 0, RISCV_ISA_EXT_SVVPTC, 1)
-		 : : : : svvptc);
+	if (use_alternative_unlikely(0, RISCV_ISA_EXT_SVVPTC)) {
+		if (!pte_same(ptep_get(ptep), entry)) {
+			__set_pte_at(vma->vm_mm, ptep, entry);
+			/* Here only not svadu is impacted */
+			flush_tlb_page(vma, address);
+			return true;
+		}
+
+		return false;
+	}
 
 	if (!pte_same(ptep_get(ptep), entry))
 		__set_pte_at(vma->vm_mm, ptep, entry);
@@ -19,16 +27,6 @@ int ptep_set_access_flags(struct vm_area_struct *vma,
 	 * the case that the PTE changed and the spurious fault case.
 	 */
 	return true;
-
-svvptc:
-	if (!pte_same(ptep_get(ptep), entry)) {
-		__set_pte_at(vma->vm_mm, ptep, entry);
-		/* Here only not svadu is impacted */
-		flush_tlb_page(vma, address);
-		return true;
-	}
-
-	return false;
 }
 
 int ptep_test_and_clear_young(struct vm_area_struct *vma,

-- 
2.50.1


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

[PATCH 3/6] riscv: checksum: Convert to use_alternative_likely

[Vivian Wang]

Use use_alternative_likely() to check for RISCV_ISA_EXT_ZBB, replacing
the use of asm goto with ALTERNATIVE.

The "likely" variant is used to match the behavior of the original
implementation using ALTERNATIVE("j %l[no_zbb]", "nop", ...).

Signed-off-by: Vivian Wang <wangruikang@iscas.ac.cn>
---
 arch/riscv/include/asm/checksum.h | 13 +++-----
 arch/riscv/lib/csum.c             | 65 +++++++++++++++------------------------
 2 files changed, 28 insertions(+), 50 deletions(-)

diff --git a/arch/riscv/include/asm/checksum.h b/arch/riscv/include/asm/checksum.h
index da378856f1d590e22271b90e803c7e55e8dd22e3..14f3942007b55646a0feb82bf73351323350347e 100644
--- a/arch/riscv/include/asm/checksum.h
+++ b/arch/riscv/include/asm/checksum.h
@@ -49,16 +49,11 @@ static inline __sum16 ip_fast_csum(const void *iph, unsigned int ihl)
 	 * ZBB only saves three instructions on 32-bit and five on 64-bit so not
 	 * worth checking if supported without Alternatives.
 	 */
-	if (IS_ENABLED(CONFIG_RISCV_ISA_ZBB) && IS_ENABLED(CONFIG_TOOLCHAIN_HAS_ZBB)) {
+	if (IS_ENABLED(CONFIG_RISCV_ISA_ZBB) &&
+	    IS_ENABLED(CONFIG_TOOLCHAIN_HAS_ZBB) &&
+	    use_alternative_likely(0, RISCV_ISA_EXT_ZBB)) {
 		unsigned long fold_temp;
 
-		asm goto(ALTERNATIVE("j %l[no_zbb]", "nop", 0,
-					      RISCV_ISA_EXT_ZBB, 1)
-		    :
-		    :
-		    :
-		    : no_zbb);
-
 		if (IS_ENABLED(CONFIG_32BIT)) {
 			asm(".option push				\n\
 			.option arch,+zbb				\n\
@@ -81,7 +76,7 @@ static inline __sum16 ip_fast_csum(const void *iph, unsigned int ihl)
 		}
 		return (__force __sum16)(csum >> 16);
 	}
-no_zbb:
+
 #ifndef CONFIG_32BIT
 	csum += ror64(csum, 32);
 	csum >>= 32;
diff --git a/arch/riscv/lib/csum.c b/arch/riscv/lib/csum.c
index 9408f50ca59a8901f7cfbcf3297d1492172c6ea2..659d3b51d0db1010deda042134bdef1f79cf8b42 100644
--- a/arch/riscv/lib/csum.c
+++ b/arch/riscv/lib/csum.c
@@ -40,20 +40,15 @@ __sum16 csum_ipv6_magic(const struct in6_addr *saddr,
 	uproto = (__force unsigned int)htonl(proto);
 	sum += uproto;
 
-	if (IS_ENABLED(CONFIG_RISCV_ISA_ZBB) && IS_ENABLED(CONFIG_TOOLCHAIN_HAS_ZBB)) {
+	/*
+	 * Zbb is likely available when the kernel is compiled with Zbb
+	 * support.
+	 */
+	if (IS_ENABLED(CONFIG_RISCV_ISA_ZBB) &&
+	    IS_ENABLED(CONFIG_TOOLCHAIN_HAS_ZBB) &&
+	    use_alternative_likely(0, RISCV_ISA_EXT_ZBB)) {
 		unsigned long fold_temp;
 
-		/*
-		 * Zbb is likely available when the kernel is compiled with Zbb
-		 * support, so nop when Zbb is available and jump when Zbb is
-		 * not available.
-		 */
-		asm goto(ALTERNATIVE("j %l[no_zbb]", "nop", 0,
-					      RISCV_ISA_EXT_ZBB, 1)
-				  :
-				  :
-				  :
-				  : no_zbb);
 		asm(".option push					\n\
 		.option arch,+zbb					\n\
 			rori	%[fold_temp], %[sum], 32		\n\
@@ -66,7 +61,7 @@ __sum16 csum_ipv6_magic(const struct in6_addr *saddr,
 		: [sum] "+r" (sum), [fold_temp] "=&r" (fold_temp));
 		return (__force __sum16)(sum >> 16);
 	}
-no_zbb:
+
 	sum += ror64(sum, 32);
 	sum >>= 32;
 	return csum_fold((__force __wsum)sum);
@@ -151,22 +146,16 @@ do_csum_with_alignment(const unsigned char *buff, int len)
 	end = (const unsigned long *)(buff + len);
 	csum = do_csum_common(ptr, end, data);
 
+	/*
+	 * Zbb is likely available when the kernel is compiled with Zbb
+	 * support.
+	 */
 #ifdef CC_HAS_ASM_GOTO_TIED_OUTPUT
-	if (IS_ENABLED(CONFIG_RISCV_ISA_ZBB) && IS_ENABLED(CONFIG_TOOLCHAIN_HAS_ZBB)) {
+	if (IS_ENABLED(CONFIG_RISCV_ISA_ZBB) &&
+	    IS_ENABLED(CONFIG_TOOLCHAIN_HAS_ZBB) &&
+	    use_alternative_likely(0, RISCV_ISA_EXT_ZBB)) {
 		unsigned long fold_temp;
 
-		/*
-		 * Zbb is likely available when the kernel is compiled with Zbb
-		 * support, so nop when Zbb is available and jump when Zbb is
-		 * not available.
-		 */
-		asm goto(ALTERNATIVE("j %l[no_zbb]", "nop", 0,
-					      RISCV_ISA_EXT_ZBB, 1)
-				  :
-				  :
-				  :
-				  : no_zbb);
-
 #ifdef CONFIG_32BIT
 		asm_goto_output(".option push			\n\
 		.option arch,+zbb				\n\
@@ -204,7 +193,7 @@ do_csum_with_alignment(const unsigned char *buff, int len)
 end:
 		return csum >> 16;
 	}
-no_zbb:
+
 #endif /* CC_HAS_ASM_GOTO_TIED_OUTPUT */
 #ifndef CONFIG_32BIT
 	csum += ror64(csum, 32);
@@ -234,21 +223,15 @@ do_csum_no_alignment(const unsigned char *buff, int len)
 	end = (const unsigned long *)(buff + len);
 	csum = do_csum_common(ptr, end, data);
 
-	if (IS_ENABLED(CONFIG_RISCV_ISA_ZBB) && IS_ENABLED(CONFIG_TOOLCHAIN_HAS_ZBB)) {
+	/*
+	 * Zbb is likely available when the kernel is compiled with Zbb
+	 * support.
+	 */
+	if (IS_ENABLED(CONFIG_RISCV_ISA_ZBB) &&
+	    IS_ENABLED(CONFIG_TOOLCHAIN_HAS_ZBB) &&
+	    use_alternative_likely(0, RISCV_ISA_EXT_ZBB)) {
 		unsigned long fold_temp;
 
-		/*
-		 * Zbb is likely available when the kernel is compiled with Zbb
-		 * support, so nop when Zbb is available and jump when Zbb is
-		 * not available.
-		 */
-		asm goto(ALTERNATIVE("j %l[no_zbb]", "nop", 0,
-					      RISCV_ISA_EXT_ZBB, 1)
-				  :
-				  :
-				  :
-				  : no_zbb);
-
 #ifdef CONFIG_32BIT
 		asm (".option push				\n\
 		.option arch,+zbb				\n\
@@ -274,7 +257,7 @@ do_csum_no_alignment(const unsigned char *buff, int len)
 #endif /* !CONFIG_32BIT */
 		return csum >> 16;
 	}
-no_zbb:
+
 #ifndef CONFIG_32BIT
 	csum += ror64(csum, 32);
 	csum >>= 32;

-- 
2.50.1


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

[PATCH 4/6] riscv: hweight: Convert to use_alternative_likely

[Vivian Wang]

Use use_alternative_likely() to check for RISCV_ISA_EXT_ZBB, replacing
the use of asm goto with ALTERNATIVE.

The "likely" variant is used to match the behavior of the original
implementation using ALTERNATIVE("j %l[legacy]", "nop", ...).

Signed-off-by: Vivian Wang <wangruikang@iscas.ac.cn>
---
 arch/riscv/include/asm/arch_hweight.h | 42 +++++++++++++++--------------------
 1 file changed, 18 insertions(+), 24 deletions(-)

diff --git a/arch/riscv/include/asm/arch_hweight.h b/arch/riscv/include/asm/arch_hweight.h
index 0e7cdbbec8efd3c293da2fa96a8c6d0a93faf56f..58ed7a3b2d7882f6a7913c4bfdb9bce4a4394956 100644
--- a/arch/riscv/include/asm/arch_hweight.h
+++ b/arch/riscv/include/asm/arch_hweight.h
@@ -20,20 +20,17 @@
 static __always_inline unsigned int __arch_hweight32(unsigned int w)
 {
 #if defined(CONFIG_RISCV_ISA_ZBB) && defined(CONFIG_TOOLCHAIN_HAS_ZBB)
-	asm goto(ALTERNATIVE("j %l[legacy]", "nop", 0,
-				      RISCV_ISA_EXT_ZBB, 1)
-			  : : : : legacy);
+	if (use_alternative_likely(0, RISCV_ISA_EXT_ZBB)) {
+		asm (".option push\n"
+		     ".option arch,+zbb\n"
+		     CPOPW "%0, %1\n"
+		     ".option pop\n"
+		     : "=r" (w) : "r" (w) :);
 
-	asm (".option push\n"
-	     ".option arch,+zbb\n"
-	     CPOPW "%0, %1\n"
-	     ".option pop\n"
-	     : "=r" (w) : "r" (w) :);
-
-	return w;
-
-legacy:
+		return w;
+	}
 #endif
+
 	return __sw_hweight32(w);
 }
 
@@ -51,20 +48,17 @@ static inline unsigned int __arch_hweight8(unsigned int w)
 static __always_inline unsigned long __arch_hweight64(__u64 w)
 {
 #if defined(CONFIG_RISCV_ISA_ZBB) && defined(CONFIG_TOOLCHAIN_HAS_ZBB)
-	asm goto(ALTERNATIVE("j %l[legacy]", "nop", 0,
-				      RISCV_ISA_EXT_ZBB, 1)
-			  : : : : legacy);
+	if (use_alternative_likely(0, RISCV_ISA_EXT_ZBB)) {
+		asm (".option push\n"
+		     ".option arch,+zbb\n"
+		     "cpop %0, %1\n"
+		     ".option pop\n"
+		     : "=r" (w) : "r" (w) :);
 
-	asm (".option push\n"
-	     ".option arch,+zbb\n"
-	     "cpop %0, %1\n"
-	     ".option pop\n"
-	     : "=r" (w) : "r" (w) :);
-
-	return w;
-
-legacy:
+		return w;
+	}
 #endif
+
 	return __sw_hweight64(w);
 }
 #else /* BITS_PER_LONG == 64 */

-- 
2.50.1


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

[PATCH 5/6] riscv: bitops: Convert to use_alternative_likely

[Vivian Wang]

Use use_alternative_likely() to check for RISCV_ISA_EXT_ZBB, replacing
the use of asm goto with ALTERNATIVE.

The "likely" variant is used to match the behavior of the original
implementation using ALTERNATIVE("j %l[legacy]", "nop", ...).

Signed-off-by: Vivian Wang <wangruikang@iscas.ac.cn>
---
 arch/riscv/include/asm/bitops.h | 112 ++++++++++++++++++----------------------
 1 file changed, 50 insertions(+), 62 deletions(-)

diff --git a/arch/riscv/include/asm/bitops.h b/arch/riscv/include/asm/bitops.h
index d59310f74c2ba70caeb7b9b0e9221882117583f5..0257d547a96293909d90b017c8a48b508d0fd642 100644
--- a/arch/riscv/include/asm/bitops.h
+++ b/arch/riscv/include/asm/bitops.h
@@ -47,20 +47,17 @@
 
 static __always_inline unsigned long variable__ffs(unsigned long word)
 {
-	asm goto(ALTERNATIVE("j %l[legacy]", "nop", 0,
-				      RISCV_ISA_EXT_ZBB, 1)
-			  : : : : legacy);
-
-	asm volatile (".option push\n"
-		      ".option arch,+zbb\n"
-		      "ctz %0, %1\n"
-		      ".option pop\n"
-		      : "=r" (word) : "r" (word) :);
-
-	return word;
-
-legacy:
-	return generic___ffs(word);
+	if (use_alternative_likely(0, RISCV_ISA_EXT_ZBB)) {
+		asm volatile (".option push\n"
+			      ".option arch,+zbb\n"
+			      "ctz %0, %1\n"
+			      ".option pop\n"
+			      : "=r" (word) : "r" (word) :);
+
+		return word;
+	} else {
+		return generic___ffs(word);
+	}
 }
 
 /**
@@ -76,20 +73,17 @@ static __always_inline unsigned long variable__ffs(unsigned long word)
 
 static __always_inline unsigned long variable__fls(unsigned long word)
 {
-	asm goto(ALTERNATIVE("j %l[legacy]", "nop", 0,
-				      RISCV_ISA_EXT_ZBB, 1)
-			  : : : : legacy);
-
-	asm volatile (".option push\n"
-		      ".option arch,+zbb\n"
-		      "clz %0, %1\n"
-		      ".option pop\n"
-		      : "=r" (word) : "r" (word) :);
-
-	return BITS_PER_LONG - 1 - word;
-
-legacy:
-	return generic___fls(word);
+	if (use_alternative_likely(0, RISCV_ISA_EXT_ZBB)) {
+		asm volatile (".option push\n"
+			      ".option arch,+zbb\n"
+			      "clz %0, %1\n"
+			      ".option pop\n"
+			      : "=r" (word) : "r" (word) :);
+
+		return BITS_PER_LONG - 1 - word;
+	} else {
+		return generic___fls(word);
+	}
 }
 
 /**
@@ -105,23 +99,20 @@ static __always_inline unsigned long variable__fls(unsigned long word)
 
 static __always_inline int variable_ffs(int x)
 {
-	asm goto(ALTERNATIVE("j %l[legacy]", "nop", 0,
-				      RISCV_ISA_EXT_ZBB, 1)
-			  : : : : legacy);
-
-	if (!x)
-		return 0;
-
-	asm volatile (".option push\n"
-		      ".option arch,+zbb\n"
-		      CTZW "%0, %1\n"
-		      ".option pop\n"
-		      : "=r" (x) : "r" (x) :);
-
-	return x + 1;
-
-legacy:
-	return generic_ffs(x);
+	if (use_alternative_likely(0, RISCV_ISA_EXT_ZBB)) {
+		if (!x)
+			return 0;
+
+		asm volatile (".option push\n"
+			      ".option arch,+zbb\n"
+			      CTZW "%0, %1\n"
+			      ".option pop\n"
+			      : "=r" (x) : "r" (x) :);
+
+		return x + 1;
+	} else {
+		return generic_ffs(x);
+	}
 }
 
 /**
@@ -137,23 +128,20 @@ static __always_inline int variable_ffs(int x)
 
 static __always_inline int variable_fls(unsigned int x)
 {
-	asm goto(ALTERNATIVE("j %l[legacy]", "nop", 0,
-				      RISCV_ISA_EXT_ZBB, 1)
-			  : : : : legacy);
-
-	if (!x)
-		return 0;
-
-	asm volatile (".option push\n"
-		      ".option arch,+zbb\n"
-		      CLZW "%0, %1\n"
-		      ".option pop\n"
-		      : "=r" (x) : "r" (x) :);
-
-	return 32 - x;
-
-legacy:
-	return generic_fls(x);
+	if (use_alternative_likely(0, RISCV_ISA_EXT_ZBB)) {
+		if (!x)
+			return 0;
+
+		asm volatile (".option push\n"
+			      ".option arch,+zbb\n"
+			      CLZW "%0, %1\n"
+			      ".option pop\n"
+			      : "=r" (x) : "r" (x) :);
+
+		return 32 - x;
+	} else {
+		return generic_fls(x);
+	}
 }
 
 /**

-- 
2.50.1


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

[PATCH 6/6] riscv: cmpxchg: Convert to use_alternative_likely

[Vivian Wang]

Use use_alternative_likely() to check for RISCV_ISA_EXT_ZAWRS, replacing
the use of asm goto with ALTERNATIVE.

The "likely" variant is used to match the behavior of the original
implementation using ALTERNATIVE("j %l[no_zawrs]", "nop", ...).

Signed-off-by: Vivian Wang <wangruikang@iscas.ac.cn>
---
 arch/riscv/include/asm/cmpxchg.h | 125 +++++++++++++++++++--------------------
 1 file changed, 61 insertions(+), 64 deletions(-)

diff --git a/arch/riscv/include/asm/cmpxchg.h b/arch/riscv/include/asm/cmpxchg.h
index 0b749e7102162477432f7cf9a34768fbdf2e8cc7..1ef6e9de5f6d2721d325fa07f2e636ebc951dc7e 100644
--- a/arch/riscv/include/asm/cmpxchg.h
+++ b/arch/riscv/include/asm/cmpxchg.h
@@ -370,74 +370,71 @@ static __always_inline void __cmpwait(volatile void *ptr,
 	u32 *__ptr32b;
 	ulong __s, __val, __mask;
 
-	asm goto(ALTERNATIVE("j %l[no_zawrs]", "nop",
-			     0, RISCV_ISA_EXT_ZAWRS, 1)
-		 : : : : no_zawrs);
-
-	switch (size) {
-	case 1:
-		__ptr32b = (u32 *)((ulong)(ptr) & ~0x3);
-		__s = ((ulong)(ptr) & 0x3) * BITS_PER_BYTE;
-		__val = val << __s;
-		__mask = 0xff << __s;
-
-		asm volatile(
-		"	lr.w	%0, %1\n"
-		"	and	%0, %0, %3\n"
-		"	xor	%0, %0, %2\n"
-		"	bnez	%0, 1f\n"
-			ZAWRS_WRS_NTO "\n"
-		"1:"
-		: "=&r" (tmp), "+A" (*(__ptr32b))
-		: "r" (__val), "r" (__mask)
-		: "memory");
-		break;
-	case 2:
-		__ptr32b = (u32 *)((ulong)(ptr) & ~0x3);
-		__s = ((ulong)(ptr) & 0x2) * BITS_PER_BYTE;
-		__val = val << __s;
-		__mask = 0xffff << __s;
-
-		asm volatile(
-		"	lr.w	%0, %1\n"
-		"	and	%0, %0, %3\n"
-		"	xor	%0, %0, %2\n"
-		"	bnez	%0, 1f\n"
-			ZAWRS_WRS_NTO "\n"
-		"1:"
-		: "=&r" (tmp), "+A" (*(__ptr32b))
-		: "r" (__val), "r" (__mask)
-		: "memory");
-		break;
-	case 4:
-		asm volatile(
-		"	lr.w	%0, %1\n"
-		"	xor	%0, %0, %2\n"
-		"	bnez	%0, 1f\n"
-			ZAWRS_WRS_NTO "\n"
-		"1:"
-		: "=&r" (tmp), "+A" (*(u32 *)ptr)
-		: "r" (val));
-		break;
+	if (use_alternative_likely(0, RISCV_ISA_EXT_ZAWRS)) {
+		switch (size) {
+		case 1:
+			__ptr32b = (u32 *)((ulong)(ptr) & ~0x3);
+			__s = ((ulong)(ptr) & 0x3) * BITS_PER_BYTE;
+			__val = val << __s;
+			__mask = 0xff << __s;
+
+			asm volatile(
+			"	lr.w	%0, %1\n"
+			"	and	%0, %0, %3\n"
+			"	xor	%0, %0, %2\n"
+			"	bnez	%0, 1f\n"
+				ZAWRS_WRS_NTO "\n"
+			"1:"
+			: "=&r" (tmp), "+A" (*(__ptr32b))
+			: "r" (__val), "r" (__mask)
+			: "memory");
+			break;
+		case 2:
+			__ptr32b = (u32 *)((ulong)(ptr) & ~0x3);
+			__s = ((ulong)(ptr) & 0x2) * BITS_PER_BYTE;
+			__val = val << __s;
+			__mask = 0xffff << __s;
+
+			asm volatile(
+			"	lr.w	%0, %1\n"
+			"	and	%0, %0, %3\n"
+			"	xor	%0, %0, %2\n"
+			"	bnez	%0, 1f\n"
+				ZAWRS_WRS_NTO "\n"
+			"1:"
+			: "=&r" (tmp), "+A" (*(__ptr32b))
+			: "r" (__val), "r" (__mask)
+			: "memory");
+			break;
+		case 4:
+			asm volatile(
+			"	lr.w	%0, %1\n"
+			"	xor	%0, %0, %2\n"
+			"	bnez	%0, 1f\n"
+				ZAWRS_WRS_NTO "\n"
+			"1:"
+			: "=&r" (tmp), "+A" (*(u32 *)ptr)
+			: "r" (val));
+			break;
 #if __riscv_xlen == 64
-	case 8:
-		asm volatile(
-		"	lr.d	%0, %1\n"
-		"	xor	%0, %0, %2\n"
-		"	bnez	%0, 1f\n"
-			ZAWRS_WRS_NTO "\n"
-		"1:"
-		: "=&r" (tmp), "+A" (*(u64 *)ptr)
-		: "r" (val));
-		break;
+		case 8:
+			asm volatile(
+			"	lr.d	%0, %1\n"
+			"	xor	%0, %0, %2\n"
+			"	bnez	%0, 1f\n"
+				ZAWRS_WRS_NTO "\n"
+			"1:"
+			: "=&r" (tmp), "+A" (*(u64 *)ptr)
+			: "r" (val));
+			break;
 #endif
-	default:
-		BUILD_BUG();
-	}
+		default:
+			BUILD_BUG();
+		}
 
-	return;
+		return;
+	}
 
-no_zawrs:
 	asm volatile(RISCV_PAUSE : : : "memory");
 }
 

-- 
2.50.1


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

Re: [PATCH 1/6] riscv: Introduce use_alternative_{likely,unlikely}

[Yury Norov]

On Wed, Aug 20, 2025 at 09:44:45PM +0800, Vivian Wang wrote:
> Introduce convenience helpers use_alternative_likely() and
> use_alternative_unlikely() to implement the pattern of using asm goto to
> check if an alternative is selected. Existing code will be converted in
> subsequent patches.
> 
> Similar to arm64 alternative_has_cap_{likely,unlikely}, but for riscv,
> alternatives are not all CPU capabilities.
> 
> Suggested-by: Aydın Mercan <aydin@mercan.dev>
> Signed-off-by: Vivian Wang <wangruikang@iscas.ac.cn>
> ---
>  arch/riscv/include/asm/alternative-macros.h | 73 +++++++++++++++++++++++++++++
>  1 file changed, 73 insertions(+)
> 
> diff --git a/arch/riscv/include/asm/alternative-macros.h b/arch/riscv/include/asm/alternative-macros.h
> index 231d777d936c2d29c858decaa9a3fa5f172efbb8..be9835b5e4eba03d76db3a73da19ac9e2981c4db 100644
> --- a/arch/riscv/include/asm/alternative-macros.h
> +++ b/arch/riscv/include/asm/alternative-macros.h
> @@ -158,4 +158,77 @@
>  	_ALTERNATIVE_CFG_2(old_content, new_content_1, vendor_id_1, patch_id_1, CONFIG_k_1,	\
>  					new_content_2, vendor_id_2, patch_id_2, CONFIG_k_2)
>  
> +/*
> + * use_alternative_{likely,unlikely}() returns true if the alternative is
> + * applied and false otherwise, but in a way where the compiler can optimize
> + * this check down to a nop instruction that's patched into a jump, or vice
> + * versa.
> + *
> + * Always returns false if the alternatives mechanism is not available.
> + *
> + * Usage example:
> + *   if (use_alternative_likely(0, RISCV_ISA_ZBB))
> + *
> + * Similar to static keys, "likely" means use a nop if the alternative is
> + * selected, and jump if unselected; "unlikely" is the other way around.
> + */
> +
> +#ifndef __ASSEMBLER__
> +
> +#include <linux/types.h>
> +
> +#ifdef CONFIG_RISCV_ALTERNATIVE
> +
> +static __always_inline bool use_alternative_likely(u16 vendor_id, u32 patch_id)
> +{
> +	BUILD_BUG_ON(!__builtin_constant_p(vendor_id));
> +	BUILD_BUG_ON(!__builtin_constant_p(patch_id));
> +
> +	asm goto(ALTERNATIVE("j %l[no_alt]", "nop", %[vendor_id], %[patch_id], 1)
> +		 :
> +		 : [vendor_id] "i"(vendor_id),
> +		   [patch_id] "i"(patch_id)
> +		 :
> +		 : no_alt);
> +
> +	return true;
> +
> +no_alt:
> +	return false;
> +}

Apart from those BUILD_BUG_ON()s, it looks similar to
__riscv_has_extension_likely(). Can you make sure you don't duplicate
it?

If so, can you describe what's the difference between those two in the
commit message?

> +static __always_inline bool use_alternative_unlikely(u16 vendor_id, u32 patch_id)
> +{
> +	BUILD_BUG_ON(!__builtin_constant_p(vendor_id));
> +	BUILD_BUG_ON(!__builtin_constant_p(patch_id));
> +
> +	asm goto(ALTERNATIVE("nop", "j %l[alt]", %[vendor_id], %[patch_id], 1)
> +		 :
> +		 : [vendor_id] "i"(vendor_id),
> +		   [patch_id] "i"(patch_id)
> +		 :
> +		 : alt);
> +
> +	return false;
> +
> +alt:
> +	return true;
> +}

This 'unlikely' version is just an negation of 'likely' one, and it
looks like an attempt to save on one negation. On the other hand, the
function is __always_inline, which means that compiler should normally
take care of it. Can you prove with objdump that it really works as
intended? I mean that 

        if (use_alternative_unlikely())
                do_something();

generates a better code than 
        
        if (!use_alternative_likely())
                do_something();

Thanks,
Yury

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

Re: [PATCH 5/6] riscv: bitops: Convert to use_alternative_likely

[Yury Norov]

On Wed, Aug 20, 2025 at 09:44:49PM +0800, Vivian Wang wrote:
> Use use_alternative_likely() to check for RISCV_ISA_EXT_ZBB, replacing
> the use of asm goto with ALTERNATIVE.
> 
> The "likely" variant is used to match the behavior of the original
> implementation using ALTERNATIVE("j %l[legacy]", "nop", ...).
> 
> Signed-off-by: Vivian Wang <wangruikang@iscas.ac.cn>
> ---
>  arch/riscv/include/asm/bitops.h | 112 ++++++++++++++++++----------------------
>  1 file changed, 50 insertions(+), 62 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/bitops.h b/arch/riscv/include/asm/bitops.h
> index d59310f74c2ba70caeb7b9b0e9221882117583f5..0257d547a96293909d90b017c8a48b508d0fd642 100644
> --- a/arch/riscv/include/asm/bitops.h
> +++ b/arch/riscv/include/asm/bitops.h
> @@ -47,20 +47,17 @@
>  
>  static __always_inline unsigned long variable__ffs(unsigned long word)
>  {
> -	asm goto(ALTERNATIVE("j %l[legacy]", "nop", 0,
> -				      RISCV_ISA_EXT_ZBB, 1)
> -			  : : : : legacy);
> -
> -	asm volatile (".option push\n"
> -		      ".option arch,+zbb\n"
> -		      "ctz %0, %1\n"
> -		      ".option pop\n"
> -		      : "=r" (word) : "r" (word) :);
> -
> -	return word;
> -
> -legacy:
> -	return generic___ffs(word);
> +	if (use_alternative_likely(0, RISCV_ISA_EXT_ZBB)) {

I don't think that 'likely' is used properly here. The likely/unlikely
wording has a meaning of a hint to the compiler:

        if (unlikely(WARN_ON(cond))
                goto err;

In your case, it's just meaningless, because whatever is 'likely' for
one CPU, will be always 'unlikely' for another. 


> +		asm volatile (".option push\n"
> +			      ".option arch,+zbb\n"
> +			      "ctz %0, %1\n"
> +			      ".option pop\n"
> +			      : "=r" (word) : "r" (word) :);
> +
> +		return word;
> +	} else {
> +		return generic___ffs(word);
> +	}
>  }

This tabs wipe most of the history. Can you reorganize your patch
such that it preserves as much history as you can?

        if (use_alternative_unlikely(...))
                return generic___ffs();

        asm volatile (".option push\n"
                      ".option arch,+zbb\n"
                      "ctz %0, %1\n"
                      ".option pop\n"
                      : "=r" (word) : "r" (word) :);

        return word;

And so on.

Thanks,
Yury

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

Re: [PATCH 1/6] riscv: Introduce use_alternative_{likely,unlikely}

[Vivian Wang]

Hi Yury,

Thanks for the review.

On 8/20/25 22:56, Yury Norov wrote:
> On Wed, Aug 20, 2025 at 09:44:45PM +0800, Vivian Wang wrote:
>> Introduce convenience helpers use_alternative_likely() and
>> use_alternative_unlikely() to implement the pattern of using asm goto to
>> check if an alternative is selected. Existing code will be converted in
>> subsequent patches.
>>
>> Similar to arm64 alternative_has_cap_{likely,unlikely}, but for riscv,
>> alternatives are not all CPU capabilities.
>>
>> Suggested-by: Aydın Mercan <aydin@mercan.dev>
>> Signed-off-by: Vivian Wang <wangruikang@iscas.ac.cn>
>> ---
>>  arch/riscv/include/asm/alternative-macros.h | 73 +++++++++++++++++++++++++++++
>>  1 file changed, 73 insertions(+)
>>
>> diff --git a/arch/riscv/include/asm/alternative-macros.h b/arch/riscv/include/asm/alternative-macros.h
>> index 231d777d936c2d29c858decaa9a3fa5f172efbb8..be9835b5e4eba03d76db3a73da19ac9e2981c4db 100644
>> --- a/arch/riscv/include/asm/alternative-macros.h
>> +++ b/arch/riscv/include/asm/alternative-macros.h
>> @@ -158,4 +158,77 @@
>>  	_ALTERNATIVE_CFG_2(old_content, new_content_1, vendor_id_1, patch_id_1, CONFIG_k_1,	\
>>  					new_content_2, vendor_id_2, patch_id_2, CONFIG_k_2)
>>  
>> +/*
>> + * use_alternative_{likely,unlikely}() returns true if the alternative is
>> + * applied and false otherwise, but in a way where the compiler can optimize
>> + * this check down to a nop instruction that's patched into a jump, or vice
>> + * versa.
>> + *
>> + * Always returns false if the alternatives mechanism is not available.
>> + *
>> + * Usage example:
>> + *   if (use_alternative_likely(0, RISCV_ISA_ZBB))
>> + *
>> + * Similar to static keys, "likely" means use a nop if the alternative is
>> + * selected, and jump if unselected; "unlikely" is the other way around.
>> + */
>> +
>> +#ifndef __ASSEMBLER__
>> +
>> +#include <linux/types.h>
>> +
>> +#ifdef CONFIG_RISCV_ALTERNATIVE
>> +
>> +static __always_inline bool use_alternative_likely(u16 vendor_id, u32 patch_id)
>> +{
>> +	BUILD_BUG_ON(!__builtin_constant_p(vendor_id));
>> +	BUILD_BUG_ON(!__builtin_constant_p(patch_id));
>> +
>> +	asm goto(ALTERNATIVE("j %l[no_alt]", "nop", %[vendor_id], %[patch_id], 1)
>> +		 :
>> +		 : [vendor_id] "i"(vendor_id),
>> +		   [patch_id] "i"(patch_id)
>> +		 :
>> +		 : no_alt);
>> +
>> +	return true;
>> +
>> +no_alt:
>> +	return false;
>> +}
> Apart from those BUILD_BUG_ON()s, it looks similar to
> __riscv_has_extension_likely(). Can you make sure you don't duplicate
> it?
>
> If so, can you describe what's the difference between those two in the
> commit message?

Whoops, *completely* missed that. Thanks for the catch.

It turns out I was trying to find uses of this pattern by searching for
"j<space>%l[...]". The block in __riscv_has_extension_{likely,unlikely}
uses "j<tab>%l[...]".

I'll just use __riscv_has_extension_{likely,unlikely} in v2 and drop this.

>> +static __always_inline bool use_alternative_unlikely(u16 vendor_id, u32 patch_id)
>> +{
>> +	BUILD_BUG_ON(!__builtin_constant_p(vendor_id));
>> +	BUILD_BUG_ON(!__builtin_constant_p(patch_id));
>> +
>> +	asm goto(ALTERNATIVE("nop", "j %l[alt]", %[vendor_id], %[patch_id], 1)
>> +		 :
>> +		 : [vendor_id] "i"(vendor_id),
>> +		   [patch_id] "i"(patch_id)
>> +		 :
>> +		 : alt);
>> +
>> +	return false;
>> +
>> +alt:
>> +	return true;
>> +}
> This 'unlikely' version is just an negation of 'likely' one, and it
> looks like an attempt to save on one negation. On the other hand, the
> function is __always_inline, which means that compiler should normally
> take care of it. Can you prove with objdump that it really works as
> intended? I mean that 
>
>         if (use_alternative_unlikely())
>                 do_something();
>
> generates a better code than 
>         
>         if (!use_alternative_likely())
>                 do_something();

use_alternative_likely() and use_alternative_unlikely() are not
negations of each other and in fact should be functionally equivalent. I
also briefly explained the difference in the comment, but the difference
is which case is nop i.e. fallthrough, and which case requires a jump
instruction. The likely case should get a "nop", and the unlikely case
should get a "j %l[...]". This choice does work as intended [1].

I don't think it is possible to give both options to the compiler, so at
least for now AIUI users have to pick one.

The same applies to __riscv_has_extension_{likely,unlikely}.

Vivian "dramforever" Wang

[1]: https://godbolt.org/z/v8zTEhzTx



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

Re: [PATCH 5/6] riscv: bitops: Convert to use_alternative_likely

[Vivian Wang]

On 8/20/25 23:04, Yury Norov wrote:

> On Wed, Aug 20, 2025 at 09:44:49PM +0800, Vivian Wang wrote:
>> Use use_alternative_likely() to check for RISCV_ISA_EXT_ZBB, replacing
>> the use of asm goto with ALTERNATIVE.
>>
>> The "likely" variant is used to match the behavior of the original
>> implementation using ALTERNATIVE("j %l[legacy]", "nop", ...).
>>
>> Signed-off-by: Vivian Wang <wangruikang@iscas.ac.cn>
>> ---
>>  arch/riscv/include/asm/bitops.h | 112 ++++++++++++++++++----------------------
>>  1 file changed, 50 insertions(+), 62 deletions(-)
>>
>> diff --git a/arch/riscv/include/asm/bitops.h b/arch/riscv/include/asm/bitops.h
>> index d59310f74c2ba70caeb7b9b0e9221882117583f5..0257d547a96293909d90b017c8a48b508d0fd642 100644
>> --- a/arch/riscv/include/asm/bitops.h
>> +++ b/arch/riscv/include/asm/bitops.h
>> @@ -47,20 +47,17 @@
>>  
>>  static __always_inline unsigned long variable__ffs(unsigned long word)
>>  {
>> -	asm goto(ALTERNATIVE("j %l[legacy]", "nop", 0,
>> -				      RISCV_ISA_EXT_ZBB, 1)
>> -			  : : : : legacy);
>> -
>> -	asm volatile (".option push\n"
>> -		      ".option arch,+zbb\n"
>> -		      "ctz %0, %1\n"
>> -		      ".option pop\n"
>> -		      : "=r" (word) : "r" (word) :);
>> -
>> -	return word;
>> -
>> -legacy:
>> -	return generic___ffs(word);
>> +	if (use_alternative_likely(0, RISCV_ISA_EXT_ZBB)) {
> I don't think that 'likely' is used properly here. The likely/unlikely
> wording has a meaning of a hint to the compiler:
>
>         if (unlikely(WARN_ON(cond))
>                 goto err;
>
> In your case, it's just meaningless, because whatever is 'likely' for
> one CPU, will be always 'unlikely' for another. 

As mentioned in the reply for patch 1, I do believe that unfortunately
we currently have no way of leaving the decision up to the compiler, so
we have to pick one. The situation is similar to
static_branch_{likely,unlikely}.

So I have preserved the original choice made here: The original asm goto
uses the "likely" pattern, so I kept it as "likely".

>> +		asm volatile (".option push\n"
>> +			      ".option arch,+zbb\n"
>> +			      "ctz %0, %1\n"
>> +			      ".option pop\n"
>> +			      : "=r" (word) : "r" (word) :);
>> +
>> +		return word;
>> +	} else {
>> +		return generic___ffs(word);
>> +	}
>>  }
> This tabs wipe most of the history. Can you reorganize your patch
> such that it preserves as much history as you can?
>
>         if (use_alternative_unlikely(...))
>                 return generic___ffs();
>
>         asm volatile (".option push\n"
>                       ".option arch,+zbb\n"
>                       "ctz %0, %1\n"
>                       ".option pop\n"
>                       : "=r" (word) : "r" (word) :);
>
>         return word;
>
> And so on.

Thanks for the tip, I'll give it a go at minimizing the diff for v2.

Vivian "dramforever" Wang


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

Re: [PATCH 1/6] riscv: Introduce use_alternative_{likely,unlikely}

[Yury Norov]

> > This 'unlikely' version is just an negation of 'likely' one, and it
> > looks like an attempt to save on one negation. On the other hand, the
> > function is __always_inline, which means that compiler should normally
> > take care of it. Can you prove with objdump that it really works as
> > intended? I mean that 
> >
> >         if (use_alternative_unlikely())
> >                 do_something();
> >
> > generates a better code than 
> >         
> >         if (!use_alternative_likely())
> >                 do_something();
> 
> use_alternative_likely() and use_alternative_unlikely() are not
> negations of each other and in fact should be functionally equivalent. I
> also briefly explained the difference in the comment, but the difference
> is which case is nop i.e. fallthrough, and which case requires a jump
> instruction. The likely case should get a "nop", and the unlikely case
> should get a "j %l[...]". This choice does work as intended [1].
> 
> I don't think it is possible to give both options to the compiler, so at
> least for now AIUI users have to pick one.
> 
> The same applies to __riscv_has_extension_{likely,unlikely}.
> 
> Vivian "dramforever" Wang
> 
> [1]: https://godbolt.org/z/v8zTEhzTx

I realize that likely and unlikely versions generate different code,
I'm just not convinced that

1. it works in real kernel as intended, not only in the godbold; and
2. has any measurable impact.

That's why I asked you to share objdump and possibly perf tests.

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

Re: [PATCH 1/6] riscv: Introduce use_alternative_{likely,unlikely}

[Vivian Wang]

On 8/20/25 23:43, Yury Norov wrote:
>>> This 'unlikely' version is just an negation of 'likely' one, and it
>>> looks like an attempt to save on one negation. On the other hand, the
>>> function is __always_inline, which means that compiler should normally
>>> take care of it. Can you prove with objdump that it really works as
>>> intended? I mean that 
>>>
>>>         if (use_alternative_unlikely())
>>>                 do_something();
>>>
>>> generates a better code than 
>>>         
>>>         if (!use_alternative_likely())
>>>                 do_something();
>> use_alternative_likely() and use_alternative_unlikely() are not
>> negations of each other and in fact should be functionally equivalent. I
>> also briefly explained the difference in the comment, but the difference
>> is which case is nop i.e. fallthrough, and which case requires a jump
>> instruction. The likely case should get a "nop", and the unlikely case
>> should get a "j %l[...]". This choice does work as intended [1].
>>
>> I don't think it is possible to give both options to the compiler, so at
>> least for now AIUI users have to pick one.
>>
>> The same applies to __riscv_has_extension_{likely,unlikely}.
>>
>> Vivian "dramforever" Wang
>>
>> [1]: https://godbolt.org/z/v8zTEhzTx
> I realize that likely and unlikely versions generate different code,
> I'm just not convinced that
>
> 1. it works in real kernel as intended, not only in the godbold; and
> 2. has any measurable impact.
>
> That's why I asked you to share objdump and possibly perf tests.

Ah, that makes sense. I had considered my patch to only be refactoring,
so I only sought to preserve the original logic rather than to achieve
an optimization.

I don't have concrete performance benchmark results, but since it is a
mere refactoring, the performance should not be worse than what's
already in v6.17-rc1.

Having said that, I am also fairly certain that the selection works in a
real kernel. I have put two objdump examples at the bottom of this message.

Vivian "dramforever" Wang

------------------------------

I grabbed v6.17-rc1 with this series applied, and built a defconfig then
mod2noconfig then DEBUG_INFO_DWARF5=y kernel. The compiler is
riscv64-unknown-linux-gnu-gcc (GCC) 14.3.0. Then I looked for random
uses of Zbb instructions.

Here is an example in register_pidns_sysctls(), where it
calls num_possible_cpus(), which uses hweight_long(), which can use a
cpop instruction with Zbb extension, and falls back to __sw_hweight64()
otherwise. Here's the code:

        pidns->pid_max = min(pid_max_max, max_t(int, pidns->pid_max,
ffffffff8004ee38:       892a                    mv      s2,a0
ffffffff8004ee3a:       0444aa83                lw      s5,68(s1)
ffffffff8004ee3e:       9781aa03                lw      s4,-1672(gp) # ffffffff81814258 <pid_max_max>
                return hweight_long(*src & BITMAP_LAST_WORD_MASK(nbits));
ffffffff8004ee42:       012ef517                auipc   a0,0x12ef
ffffffff8004ee46:       e2653503                ld      a0,-474(a0) # ffffffff8133dc68 <__cpu_possible_mask>
ffffffff8004ee4a:       05c0006f                j       ffffffff8004eea6 <register_pidns_sysctls+0xc2>
                                                ^~~~~~~~~~~~~~~~~~~~~~~~ Jump to "unlikely" non-Zbb fallback

"Has Zbb" is "likely" here, and in that case this jump gets patched into
a nop and falls through to the cpop here:

                asm (".option push\n"
ffffffff8004ee4e:       60251793                cpop    a5,a0
                                                ^~~~~~~~~~~~~ Zbb implementation

ffffffff8004ee52:       00a7979b                slliw   a5,a5,0xa
ffffffff8004ee56:       873e                    mv      a4,a5
ffffffff8004ee58:       0157d363                bge     a5,s5,ffffffff8004ee5e <register_pidns_sysctls+0x7a>
ffffffff8004ee5c:       8756                    mv      a4,s5
ffffffff8004ee5e:       2701                    sext.w  a4,a4
ffffffff8004ee60:       87ba                    mv      a5,a4
ffffffff8004ee62:       00ea5363                bge     s4,a4,ffffffff8004ee68 <register_pidns_sysctls+0x84>
ffffffff8004ee66:       87d2                    mv      a5,s4
ffffffff8004ee68:       c0fc                    sw      a5,68(s1)
                             PIDS_PER_CPU_DEFAULT * num_possible_cpus()));
                                                ...

Later comes the fallback code that calls __sw_hweight64() and jumps back:

        return __sw_hweight64(w);
ffffffff8004eea6:       004f2097                auipc   ra,0x4f2
ffffffff8004eeaa:       006080e7                jalr    6(ra) # ffffffff80540eac <__sw_hweight64>
ffffffff8004eeae:       87aa                    mv      a5,a0
ffffffff8004eeb0:       b74d                    j       ffffffff8004ee52 <register_pidns_sysctls+0x6e>

------------------------------

Here's another example ip_fast_csum() which has a Zbb implementation and
a non-Zbb one. The asm goto line seems to have been preserved in debug
information more nicely:

static __always_inline bool use_alternative_likely(u16 vendor_id, u32 patch_id)
{
        BUILD_BUG_ON(!__builtin_constant_p(vendor_id));
        BUILD_BUG_ON(!__builtin_constant_p(patch_id));
        asm goto(ALTERNATIVE("j %l[no_alt]", "nop", %[vendor_id], %[patch_id], 1)
ffffffff8000f952:       01e0006f                j       ffffffff8000f970 <ip_fast_csum+0x40>
                                                ^~~~~~~~~~~~~~~~~~~~~~~~ Jump to "unlikely" non-Zbb fallback

                                rori    %[csum], %[csum], 16            \n\
                                sub     %[csum], %[fold_temp], %[csum]  \n\
                        .option pop"
                        : [csum] "+r" (csum), [fold_temp] "=&r" (fold_temp));
                } else {
                        asm(".option push                               \n\
ffffffff8000f956:       6207d713                rori    a4,a5,0x20
ffffffff8000f95a:       97ba                    add     a5,a5,a4
ffffffff8000f95c:       9381                    srli    a5,a5,0x20
ffffffff8000f95e:       fff7c713                not     a4,a5
ffffffff8000f962:       6107d79b                roriw   a5,a5,0x10
ffffffff8000f966:       40f707bb                subw    a5,a4,a5
                                                ^~~~~~~~~~~~~~~~~~ This block is the Zbb implementation

                                roriw   %[csum], %[csum], 16            \n\
                                subw    %[csum], %[fold_temp], %[csum]  \n\
                        .option pop"
                        : [csum] "+r" (csum), [fold_temp] "=&r" (fold_temp));
                }
                return (__force __sum16)(csum >> 16);
ffffffff8000f96a:       0107d51b                srliw   a0,a5,0x10
ffffffff8000f96e:       a015                    j       ffffffff8000f992 <ip_fast_csum+0x62>

... and then it jumps further to more code. Then comes the non-Zbb
implementation, which starts with a rotate operation as well but has to
use three instructions for it

 * @word: value to rotate
 * @shift: bits to roll
 */
static inline __u64 ror64(__u64 word, unsigned int shift)
{
        return (word >> (shift & 63)) | (word << ((-shift) & 63));
ffffffff8000f970:       0207d693                srli    a3,a5,0x20
ffffffff8000f974:       02079713                slli    a4,a5,0x20
ffffffff8000f978:       8f55                    or      a4,a4,a3
                                                ...

And the non-Zbb implementation goes on...



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