Re: [PATCH 5/9] ipv4: tcp_output: avoid warning about NET_ADD_STATS

From: Eric Dumazet
Date: Thu Mar 28 2024 - 10:38:26 EST


On Thu, Mar 28, 2024 at 3:31 PM Arnd Bergmann <arnd@xxxxxxxxxx> wrote:
>
> From: Arnd Bergmann <arnd@xxxxxxxx>
>
> Clang warns about a range check in percpu_add_op() being impossible
> to hit for an u8 variable:
>
> net/ipv4/tcp_output.c:188: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]
> NET_ADD_STATS(sock_net(sk), LINUX_MIB_TCPACKCOMPRESSED,
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> include/net/ip.h:291:41: note: expanded from macro 'NET_ADD_STATS'
> #define NET_ADD_STATS(net, field, adnd) SNMP_ADD_STATS((net)->mib.net_statistics, field, adnd)
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> include/net/snmp.h:143:4: note: expanded from macro 'SNMP_ADD_STATS'
> this_cpu_add(mib->mibs[field], addend)
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> include/linux/percpu-defs.h:509:33: note: expanded from macro 'this_cpu_add'
> #define this_cpu_add(pcp, val) __pcpu_size_call(this_cpu_add_, pcp, val)
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> note: (skipping 1 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all)
> <scratch space>:187:1: note: expanded from here
> this_cpu_add_8
> ^
> arch/x86/include/asm/percpu.h:326:35: note: expanded from macro 'this_cpu_add_8'
> #define this_cpu_add_8(pcp, val) percpu_add_op(8, volatile, (pcp), val)
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> arch/x86/include/asm/percpu.h:127:31: note: expanded from macro 'percpu_add_op'
> ((val) == 1 || (val) == -1)) ? \
> ~~~~~ ^ ~~
>

This seems like a bug in the macro or the compiler, because val is not
a constant ?

__builtin_constant_p(val) should return false ???

+#define percpu_add_op(size, qual, var, val) \
+do { \
+ const int pao_ID__ = (__builtin_constant_p(val) && \
+ ((val) == 1 || (val) == -1)) ? \
+ (int)(val) : 0; \
+ if (0) { \
+ typeof(var) pao_tmp__; \
+ pao_tmp__ = (val); \
+ (void)pao_tmp__; \
+ } \
+ if (pao_ID__ == 1) \
+ percpu_unary_op(size, qual, "inc", var); \
+ else if (pao_ID__ == -1) \
+ percpu_unary_op(size, qual, "dec", var); \
+ else \
+ percpu_to_op(size, qual, "add", var, val); \
+} while (0)
+



> Avoid this warning with a cast to a signed 'int'.
>
> Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx>
> ---
> net/ipv4/tcp_output.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index e3167ad96567..dbe54fceee08 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -183,7 +183,7 @@ static inline void tcp_event_ack_sent(struct sock *sk, u32 rcv_nxt)
>
> if (unlikely(tp->compressed_ack)) {
> NET_ADD_STATS(sock_net(sk), LINUX_MIB_TCPACKCOMPRESSED,
> - tp->compressed_ack);
> + (int)tp->compressed_ack);
> tp->compressed_ack = 0;
> if (hrtimer_try_to_cancel(&tp->compressed_ack_timer) == 1)
> __sock_put(sk);
> --
> 2.39.2
>