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

[Dave Hansen <dave.hansen@intel.com>]

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

On 10/17/24 11:18, Peter Zijlstra wrote:
> On Wed, Oct 16, 2024 at 12:44:18PM -0700, Dave Hansen wrote:
> 
>> Would anybody hate if we broke this up a bit, like:
>>
>>         const typeof(var) _val = val;
>>         const int paoconst = __builtin_constant_p(val);
>>         const int paoinc   = paoconst && ((_val) == 1);
>>         const int paodec   = paoconst && ((_val) == (typeof(var))-1);
>>
>> and then did
>>
>> 	if (paoinc)
>> 		percpu_unary_op(size, qual, "inc", var);
>> 	...
> I think that is an overall improvement. Proceed! 🙂

I poked at this a bit:

> https://git.kernel.org/pub/scm/linux/kernel/git/daveh/devel.git/commit/?h=testme&id=30e0899c6ab7fe1134e4b96db963f0be89b1dd5a

I believe it functions fine.  But it surprised me with a few things.
Here's one.  I assumed that doing an add((unsigned)-1) would be rare.
It's not.  It's actually pretty common because this:

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

ends up causing problems when 'pcp' is an unsigned type.  For example,
in this chain:

	mem_cgroup_exit ->
	obj_cgroup_put ->
	percpu_ref_put ->
	percpu_ref_put_many(ref, 1) ->
	this_cpu_sub

the compiler can see the '1' constant.  It effectively expands to:

	this_cpu_add(pcp, -(unsigned long)(1))

With the old code, gcc manages to generate a 'dec'.  Clang generates an
'add'.  With my hack above both compilers generate an 'add'.  This
actually matters in some code that seems potentially rather performance
sensitive:

add/remove: 0/0 grow/shrink: 219/9 up/down: 755/-141 (614)
Function                                     old     new   delta
flush_end_io                                 905    1070    +165
x86_pmu_cancel_txn                           242     338     +96
lru_add                                      554     594     +40
mlock_folio_batch                           3264    3300     +36
compaction_alloc                            3813    3838     +25
tcp_leave_memory_pressure                     86     110     +24
account_guest_time                           270     287     +17
...

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.


[-- Attachment #2: 0001-x86-percpu-Avoid-comparing-unsigned-types-to-1.patch --]
[-- 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