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

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

[Stefano Brivio]

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 <sbrivio@redhat.com>
---
 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)						\
 	{								\
-- 
@@ -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

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

[David Gibson]

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

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 <sbrivio@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Stefano Brivio]

On Fri, 8 Mar 2024 11:55:32 +1100
David Gibson <david@gibson.dropbear.id.au> 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 <sbrivio@redhat.com>  
> 
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> 
> 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

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

[David Gibson]

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

On Fri, Mar 08, 2024 at 10:28:38AM +0100, Stefano Brivio wrote:
> On Fri, 8 Mar 2024 11:55:32 +1100
> David Gibson <david@gibson.dropbear.id.au> 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 <sbrivio@redhat.com>  
> > 
> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > 
> > 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]