Re: [PATCH v2 6/8] checksum: use csum_ip4_header() in udp.c and tcp.c

[David Gibson <david@gibson.dropbear.id.au>]

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

On Wed, Feb 14, 2024 at 09:56:26AM +0100, Laurent Vivier wrote:
> We can find the same function to compute the IPv4 header
> checksum in tcp.c, udp.c and tap.c
> 
> Use the function defined for tap.c, csum_ip4_header(), but
> with the code used in tcp.c and udp.c as it doesn't need a fully
> initialiazed IPv4 header, only protocol, tot_len, saddr and daddr.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
> 
> Notes:
>     v2:
>       - use csum_ip4_header() from checksum.c
>       - use code from tcp.c and udp.c in csum_ip4_header()
>       - use "const struct iphfr *", check is not updated by the
>         function but by the caller.
> 
>  checksum.c | 16 ++++++++++++----
>  checksum.h |  2 +-
>  tap.c      |  2 +-
>  tcp.c      | 22 +---------------------
>  udp.c      | 23 +++++------------------
>  5 files changed, 20 insertions(+), 45 deletions(-)
> 
> diff --git a/checksum.c b/checksum.c
> index ac2bc49f7eb0..5613187a1c82 100644
> --- a/checksum.c
> +++ b/checksum.c
> @@ -57,6 +57,7 @@
>  #include <linux/icmpv6.h>
>  
>  #include "util.h"
> +#include "ip.h"
>  
>  /* Checksums are optional for UDP over IPv4, so we usually just set
>   * them to 0.  Change this to 1 to calculate real UDP over IPv4
> @@ -115,13 +116,20 @@ uint16_t csum_fold(uint32_t sum)
>  uint16_t csum(const void *buf, size_t len, uint32_t init);
>  
>  /**
> - * csum_ip4_header() - Calculate and set IPv4 header checksum
> + * csum_ip4_header() - Calculate IPv4 header checksum
>   * @ip4h:	IPv4 header
>   */
> -void csum_ip4_header(struct iphdr *ip4h)
> +uint16_t csum_ip4_header(const struct iphdr *ip4h)
>  {
> -	ip4h->check = 0;
> -	ip4h->check = csum(ip4h, (size_t)ip4h->ihl * 4, 0);
> +	uint32_t sum = L2_BUF_IP4_PSUM(ip4h->protocol);

Hrm, it's probably not a huge deal, but this change has more
consequences than might be immediately apparent.

In the existing use cases, I was expecting L2_BUF_IP4_PSUM() to be
evaluated at compile time, because it's always passed a constant.
With this new formulation the setting of ip4h->protocol is far
separated from this checksum, so I doubt the compiler will be able to
deduce it always has the same value.  As well as extra computation
that could be an extra memory access, which is more significant.  Als,
the macro uses htons_constant(), which I guess works for
non-constants, but probably isn't ideal.

So, although it seems technically redundant, I'd suggest passing in
the protocol rather than reading it from the header, to preserve that
ability to constant fold where the protocol is statically known.
Well.. assuming the compiler inlines enough to propagate the constant
across the function call, which given we don't have a separate link
pass is possible.

Or, maybe we should rework this to take the addresses as parameters
too.  That does have a few advantages:

  * It makes it obvious exactly what this function requires, rather
    than having assumptions about what fields of the header must
    already be initialised
  * It should avoid the #pragma nonsense to avoid the unaligned
    warning
  * For at least some of the callsites, the addresses are probably
    already in registers, so it might save a couple of memory accesses

> +	sum += ip4h->tot_len;
> +	sum += (ip4h->saddr >> 16) & 0xffff;
> +	sum += ip4h->saddr & 0xffff;
> +	sum += (ip4h->daddr >> 16) & 0xffff;
> +	sum += ip4h->daddr & 0xffff;
> +
> +	return ~csum_fold(sum);
>  }
>  
>  /**
> diff --git a/checksum.h b/checksum.h
> index 6a20297a5826..b87ecd720df5 100644
> --- a/checksum.h
> +++ b/checksum.h
> @@ -13,7 +13,7 @@ struct icmp6hdr;
>  uint32_t sum_16b(const void *buf, size_t len);
>  uint16_t csum_fold(uint32_t sum);
>  uint16_t csum_unaligned(const void *buf, size_t len, uint32_t init);
> -void csum_ip4_header(struct iphdr *ip4h);
> +uint16_t csum_ip4_header(const struct iphdr *ip4h);
>  void csum_udp4(struct udphdr *udp4hr,
>  	       struct in_addr saddr, struct in_addr daddr,
>  	       const void *payload, size_t len);
> diff --git a/tap.c b/tap.c
> index 3ea03f720d6d..70f36a55314f 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -160,7 +160,7 @@ static void *tap_push_ip4h(char *buf, struct in_addr src, struct in_addr dst,
>  	ip4h->protocol = proto;
>  	ip4h->saddr = src.s_addr;
>  	ip4h->daddr = dst.s_addr;
> -	csum_ip4_header(ip4h);
> +	ip4h->check = csum_ip4_header(ip4h);
>  	return ip4h + 1;
>  }
>  
> diff --git a/tcp.c b/tcp.c
> index 45ef5146729a..35e240f4ffc3 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -934,23 +934,6 @@ static void tcp_sock_set_bufsize(const struct ctx *c, int s)
>  		trace("TCP: failed to set SO_SNDBUF to %i", v);
>  }
>  
> -/**
> - * tcp_update_check_ip4() - Update IPv4 with variable parts from stored one
> - * @buf:	L2 packet buffer with final IPv4 header
> - */
> -static void tcp_update_check_ip4(struct tcp4_l2_buf_t *buf)
> -{
> -	uint32_t sum = L2_BUF_IP4_PSUM(IPPROTO_TCP);
> -
> -	sum += buf->iph.tot_len;
> -	sum += (buf->iph.saddr >> 16) & 0xffff;
> -	sum += buf->iph.saddr & 0xffff;
> -	sum += (buf->iph.daddr >> 16) & 0xffff;
> -	sum += buf->iph.daddr & 0xffff;
> -
> -	buf->iph.check = (uint16_t)~csum_fold(sum);
> -}
> -
>  /**
>   * tcp_update_check_tcp4() - Update TCP checksum from stored one
>   * @buf:	L2 packet buffer with final IPv4 header
> @@ -1393,10 +1376,7 @@ do {									\
>  		b->iph.saddr = a4->s_addr;
>  		b->iph.daddr = c->ip4.addr_seen.s_addr;
>  
> -		if (check)
> -			b->iph.check = *check;
> -		else
> -			tcp_update_check_ip4(b);
> +		b->iph.check = check ? *check : csum_ip4_header(&b->iph);
>  
>  		SET_TCP_HEADER_COMMON_V4_V6(b, conn, seq);
>  
> diff --git a/udp.c b/udp.c
> index d514c864ab5b..e645c800a823 100644
> --- a/udp.c
> +++ b/udp.c
> @@ -270,23 +270,6 @@ static void udp_invert_portmap(struct udp_port_fwd *fwd)
>  	}
>  }
>  
> -/**
> - * udp_update_check4() - Update checksum with variable parts from stored one
> - * @buf:	L2 packet buffer with final IPv4 header
> - */
> -static void udp_update_check4(struct udp4_l2_buf_t *buf)
> -{
> -	uint32_t sum = L2_BUF_IP4_PSUM(IPPROTO_UDP);
> -
> -	sum += buf->iph.tot_len;
> -	sum += (buf->iph.saddr >> 16) & 0xffff;
> -	sum += buf->iph.saddr & 0xffff;
> -	sum += (buf->iph.daddr >> 16) & 0xffff;
> -	sum += buf->iph.daddr & 0xffff;
> -
> -	buf->iph.check = (uint16_t)~csum_fold(sum);
> -}
> -
>  /**
>   * udp_update_l2_buf() - Update L2 buffers with Ethernet and IPv4 addresses
>   * @eth_d:	Ethernet destination address, NULL if unchanged
> @@ -579,6 +562,9 @@ static void udp_splice_sendfrom(const struct ctx *c, unsigned start, unsigned n,
>   *
>   * Return: size of tap frame with headers
>   */
> +#pragma GCC diagnostic push
> +/* ignore unaligned pointer value warning for &b->iph */
> +#pragma GCC diagnostic ignored "-Waddress-of-packed-member"
>  static size_t udp_update_hdr4(const struct ctx *c, int n, in_port_t dstport,
>  			      const struct timespec *now)
>  {
> @@ -614,13 +600,14 @@ static size_t udp_update_hdr4(const struct ctx *c, int n, in_port_t dstport,
>  		b->iph.saddr = b->s_in.sin_addr.s_addr;
>  	}
>  
> -	udp_update_check4(b);
> +	b->iph.check = csum_ip4_header(&b->iph);
>  	b->uh.source = b->s_in.sin_port;
>  	b->uh.dest = htons(dstport);
>  	b->uh.len = htons(udp4_l2_mh_sock[n].msg_len + sizeof(b->uh));
>  
>  	return tap_iov_len(c, &b->taph, ip_len);
>  }
> +#pragma GCC diagnostic pop
>  
>  /**
>   * udp_update_hdr6() - Update headers for one IPv6 datagram

-- 
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 --]