[PATCH] ip: Use regular htons() for non-constant protocol number in L2_BUF_IP4_PSUM

Stefano Brivio

6 Mar 2024 6 Mar '24

8:08 a.m.

instead of htons_constant(), which is for... constants. Fixes: 5bf200ae8a1a ("tcp, udp: Don't include destination address in partially precomputed csums") Signed-off-by: Stefano Brivio --- ip.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ip.h b/ip.h index 9be4778..b9aedf6 100644 --- a/ip.h +++ b/ip.h @@ -38,7 +38,7 @@ .daddr = 0, \ } #define L2_BUF_IP4_PSUM(proto) ((uint32_t)htons_constant(0x4500) + \ - (uint32_t)htons_constant(0xff00 | (proto))) + (uint32_t)htons(0xff00 | (proto))) #define L2_BUF_IP6_INIT(proto) \ { \ -- 2.39.2

Show replies by date

David Gibson

8 Mar 8 Mar

1:55 a.m.

On Wed, Mar 06, 2024 at 08:08:36AM +0100, Stefano Brivio wrote:

...

instead of htons_constant(), which is for... constants.

Fixes: 5bf200ae8a1a ("tcp, udp: Don't include destination address in partially precomputed csums") Signed-off-by: Stefano Brivio

Reviewed-by: David Gibson It seems to get the job done, at what's probably negligible practical cost. My perfectionist side has some misgivings: AIUI, the point of htons_constant() isn't so much that it *requires* a constant, but that because it's open-coded in plain arithmetic operations the compiler will be able to constant fold it, if it is invoked with a constant parameter. Since htons() is a library function, it probably can't be elided in that way. The cost of htons_constant() is that it may be a less optimal implementation when the calculation really does need to be done at runtime. I'm still a bit baffled at that Coverity warning: I can't see why it doesn't preclude any situation where you want to write out calculations for clarity, even though you know the result will be a constant (and you expect the compiler to fold it).

...

--- ip.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ip.h b/ip.h index 9be4778..b9aedf6 100644 --- a/ip.h +++ b/ip.h @@ -38,7 +38,7 @@ .daddr = 0, \ } #define L2_BUF_IP4_PSUM(proto) ((uint32_t)htons_constant(0x4500) + \ - (uint32_t)htons_constant(0xff00 | (proto))) + (uint32_t)htons(0xff00 | (proto)))

#define L2_BUF_IP6_INIT(proto) \ { \

-- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson

Stefano Brivio

10:28 a.m.

On Fri, 8 Mar 2024 11:55:32 +1100 David Gibson wrote:

...

On Wed, Mar 06, 2024 at 08:08:36AM +0100, Stefano Brivio wrote:

...
instead of htons_constant(), which is for... constants.

Fixes: 5bf200ae8a1a ("tcp, udp: Don't include destination address in partially precomputed csums") Signed-off-by: Stefano Brivio

Reviewed-by: David Gibson

It seems to get the job done, at what's probably negligible practical cost. My perfectionist side has some misgivings:

AIUI, the point of htons_constant() isn't so much that it *requires* a constant, but that because it's open-coded in plain arithmetic operations the compiler will be able to constant fold it, if it is invoked with a constant parameter.

Right, yes, it doesn't require a constant. Still, I'd argue it's meant for constants. :)

...

Since htons() is a library function, it probably can't be elided in that way. The cost of htons_constant() is that it may be a less optimal implementation when the calculation really does need to be done at runtime.

I'm still a bit baffled at that Coverity warning: I can't see why it doesn't preclude any situation where you want to write out calculations for clarity, even though you know the result will be a constant (and you expect the compiler to fold it).

...maybe it actually does preclude any situation like that? This is the only example we have with __bswap_constant16(), and variables mixed (ORed) with constants. Other usages of __bswap_constant16() have just a variable as argument (no "problem" with that, of course), and we use __bswap_constant_32() with constants only. -- Stefano

David Gibson

14 Mar 14 Mar

6:02 a.m.

On Fri, Mar 08, 2024 at 10:28:38AM +0100, Stefano Brivio wrote:

...

On Fri, 8 Mar 2024 11:55:32 +1100 David Gibson wrote:

...
On Wed, Mar 06, 2024 at 08:08:36AM +0100, Stefano Brivio wrote:

...
instead of htons_constant(), which is for... constants.

Fixes: 5bf200ae8a1a ("tcp, udp: Don't include destination address in partially precomputed csums") Signed-off-by: Stefano Brivio

Reviewed-by: David Gibson

It seems to get the job done, at what's probably negligible practical cost. My perfectionist side has some misgivings:

AIUI, the point of htons_constant() isn't so much that it *requires* a constant, but that because it's open-coded in plain arithmetic operations the compiler will be able to constant fold it, if it is invoked with a constant parameter.

Right, yes, it doesn't require a constant. Still, I'd argue it's meant for constants. :)

...
Since htons() is a library function, it probably can't be elided in that way. The cost of htons_constant() is that it may be a less optimal implementation when the calculation really does need to be done at runtime.

I'm still a bit baffled at that Coverity warning: I can't see why it doesn't preclude any situation where you want to write out calculations for clarity, even though you know the result will be a constant (and you expect the compiler to fold it).

...maybe it actually does preclude any situation like that? This is the only example we have with __bswap_constant16(), and variables mixed (ORed) with constants.

Right, but AFAICT there's nothing that should be specific to bswap_constant here, since that's just expanding to some shifts and masks.

...

Other usages of __bswap_constant16() have just a variable as argument (no "problem" with that, of course), and we use __bswap_constant_32() with constants only.

-- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson

469

Age (days ago)

477

Last active (days ago)

List overview

Download

3 comments

2 participants

participants (2)

David Gibson
Stefano Brivio