Re: [PATCH v1 1/1] x86/percpu: Cast -1 to argument type when comparing in percpu_add_op()

["Christoph Lameter (Ampere)" <cl@gentwo.org>]

[-- Attachment #1: Type: text/plain, Size: 327 bytes --]

On Tue, 22 Oct 2024, Dave Hansen wrote:

> So I think Peter's version was the best.  It shuts up clang and also
> preserves the existing (good) gcc 'sub' behavior.  I'll send it out for
> real in a bit, but I'm thinking of something like the attached patch.

The desired behavior is a "dec". "sub" has a longer op code AFAICT.

[-- Attachment #2: Type: text/x-patch, Size: 3180 bytes --]

From d63bcd350e1a3ba6196dadb26cb2f36f0ba1e182 Mon Sep 17 00:00:00 2001
From: Dave Hansen <dave.hansen@linux.intel.com>
Date: Fri, 18 Oct 2024 11:07:47 -0700
Subject: [PATCH] x86/percpu: Avoid comparing unsigned types to -1

clang warns when comparing an unsinged type to -1 since the comparison
is always false.

This can be quickly reproduced by setting CONFIG_WERROR=y and running:

	make W=1 CC=clang-14 net/ipv4/tcp_output.o

net/ipv4/tcp_output.c:187:3: error: result of comparison of constant -1 with expression of type 'u8' (aka 'unsigned char') is always false [-Werror,-Wtautological-constant-out-of-range-compare]
  187 |                 NET_ADD_STATS(sock_net(sk), LINUX_MIB_TCPACKCOMPRESSED,
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  188 |                               tp->compressed_ack);
      |                               ~~~~~~~~~~~~~~~~~~~
...
arch/x86/include/asm/percpu.h:238:31: note: expanded from macro 'percpu_add_op'
  238 |                               ((val) == 1 || (val) == -1)) ?            \
      |                                              ~~~~~ ^  ~~

Fix this by avoiding a comparison of an uncast -1 to 'val'.

Doing this in addition to the existing 'pao_ID__' calculation would make it
even more unreadable. Remove 'pao_ID__' and replace it with the three
components of its calculation.

This preserves some unintuitive but useful behavior.  For instance, gcc sees:

	percpu_add_op(..., var, (u8)-1);

and can transform that into a "dec".  Clang, on the other hand, sees the 'u8'
type and assumes that "(val) == -1" is false, which was the root of the
warning.

This is useful gcc behavior because:

	#define this_cpu_sub(pcp, val)  this_cpu_add(pcp, -(typeof(pcp))(val))

so any code that does:

	this_cpu_sub(A, 1)

where 'A' is an unsigned type generates a "dec".  Clang, on the other
hand generates a less-efficient "add".

Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
---
 arch/x86/include/asm/percpu.h | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/percpu.h b/arch/x86/include/asm/percpu.h
index c55a79d5feae..57d9759c692e 100644
--- a/arch/x86/include/asm/percpu.h
+++ b/arch/x86/include/asm/percpu.h
@@ -234,18 +234,19 @@ do {									\
  */
 #define percpu_add_op(size, qual, var, val)				\
 do {									\
-	const int pao_ID__ = (__builtin_constant_p(val) &&		\
-			      ((val) == 1 || (val) == -1)) ?		\
-				(int)(val) : 0;				\
+	const int pao_const__ = __builtin_constant_p(val);		\
+	const int pao_inc__   = (val) == 1;				\
+	const int pao_dec__   = (typeof(var))(val) ==			\
+				(typeof(var))-1;			\
 									\
 	if (0) {							\
 		typeof(var) pao_tmp__;					\
 		pao_tmp__ = (val);					\
 		(void)pao_tmp__;					\
 	}								\
-	if (pao_ID__ == 1)						\
+	if (pao_const__ && pao_inc__)					\
 		percpu_unary_op(size, qual, "inc", var);		\
-	else if (pao_ID__ == -1)					\
+	else if (pao_const__ && pao_dec__)				\
 		percpu_unary_op(size, qual, "dec", var);		\
 	else								\
 		percpu_binary_op(size, qual, "add", var, val);		\
-- 
2.34.1