From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from gandalf.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 45F5B5A027A for ; Thu, 15 Feb 2024 03:51:13 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1707965469; bh=Wq9yKB6OBgBEUVAOw+F+oKAj+od7eup7T9bwpCKkofM=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=HCo1B0XwlADwFFwfMlklTSpqbfUUvZLvr6+zm2sftFilq19amAuzE/PXkztwnw2Y2 pzl0afaIhWQeV4ph3ynMQFmK/V4M+zkmrwJoArD2NdLr+ID+fI889RmLKBXni3d4qc 7u/b1oLzptWyRymKr4dkEzkx1lihEpkHIHo0SVojLlPSRYM0Bg/57t1KE5oJKas+1A qIq424OOIzRK5e3WHGILNc1Y5Y2SnuGbTRjknt7QLusCCt+OCrtLAhfBATZQDpt9RX zZMdiVJd/W7a1AOQMSZKmuY3MdgSxVtYa4qT/ojFdbz2s/R0LEiztrVUjivotTlsPI MtcKSL092nALA== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4Tb01n408lz4wcN; Thu, 15 Feb 2024 13:51:09 +1100 (AEDT) Date: Thu, 15 Feb 2024 13:51:04 +1100 From: David Gibson To: Laurent Vivier Subject: Re: [PATCH v2 6/8] checksum: use csum_ip4_header() in udp.c and tcp.c Message-ID: References: <20240214085628.210783-1-lvivier@redhat.com> <20240214085628.210783-7-lvivier@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="EVvJp2KiJedqUQZw" Content-Disposition: inline In-Reply-To: <20240214085628.210783-7-lvivier@redhat.com> Message-ID-Hash: 42FQGZHT43PEY2ZH3YTYLKTTC5537A5H X-Message-ID-Hash: 42FQGZHT43PEY2ZH3YTYLKTTC5537A5H X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --EVvJp2KiJedqUQZw Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 >=20 > 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. >=20 > Signed-off-by: Laurent Vivier > --- >=20 > 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. >=20 > checksum.c | 16 ++++++++++++---- > checksum.h | 2 +- > tap.c | 2 +- > tcp.c | 22 +--------------------- > udp.c | 23 +++++------------------ > 5 files changed, 20 insertions(+), 45 deletions(-) >=20 > diff --git a/checksum.c b/checksum.c > index ac2bc49f7eb0..5613187a1c82 100644 > --- a/checksum.c > +++ b/checksum.c > @@ -57,6 +57,7 @@ > #include > =20 > #include "util.h" > +#include "ip.h" > =20 > /* 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); > =20 > /** > - * 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 =3D 0; > - ip4h->check =3D csum(ip4h, (size_t)ip4h->ihl * 4, 0); > + uint32_t sum =3D 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 +=3D ip4h->tot_len; > + sum +=3D (ip4h->saddr >> 16) & 0xffff; > + sum +=3D ip4h->saddr & 0xffff; > + sum +=3D (ip4h->daddr >> 16) & 0xffff; > + sum +=3D ip4h->daddr & 0xffff; > + > + return ~csum_fold(sum); > } > =20 > /** > 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 =3D proto; > ip4h->saddr =3D src.s_addr; > ip4h->daddr =3D dst.s_addr; > - csum_ip4_header(ip4h); > + ip4h->check =3D csum_ip4_header(ip4h); > return ip4h + 1; > } > =20 > 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); > } > =20 > -/** > - * 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 =3D L2_BUF_IP4_PSUM(IPPROTO_TCP); > - > - sum +=3D buf->iph.tot_len; > - sum +=3D (buf->iph.saddr >> 16) & 0xffff; > - sum +=3D buf->iph.saddr & 0xffff; > - sum +=3D (buf->iph.daddr >> 16) & 0xffff; > - sum +=3D buf->iph.daddr & 0xffff; > - > - buf->iph.check =3D (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 =3D a4->s_addr; > b->iph.daddr =3D c->ip4.addr_seen.s_addr; > =20 > - if (check) > - b->iph.check =3D *check; > - else > - tcp_update_check_ip4(b); > + b->iph.check =3D check ? *check : csum_ip4_header(&b->iph); > =20 > SET_TCP_HEADER_COMMON_V4_V6(b, conn, seq); > =20 > 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) > } > } > =20 > -/** > - * 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 =3D L2_BUF_IP4_PSUM(IPPROTO_UDP); > - > - sum +=3D buf->iph.tot_len; > - sum +=3D (buf->iph.saddr >> 16) & 0xffff; > - sum +=3D buf->iph.saddr & 0xffff; > - sum +=3D (buf->iph.daddr >> 16) & 0xffff; > - sum +=3D buf->iph.daddr & 0xffff; > - > - buf->iph.check =3D (uint16_t)~csum_fold(sum); > -} > - > /** > * udp_update_l2_buf() - Update L2 buffers with Ethernet and IPv4 addres= ses > * @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 dstp= ort, > 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 =3D b->s_in.sin_addr.s_addr; > } > =20 > - udp_update_check4(b); > + b->iph.check =3D csum_ip4_header(&b->iph); > b->uh.source =3D b->s_in.sin_port; > b->uh.dest =3D htons(dstport); > b->uh.len =3D htons(udp4_l2_mh_sock[n].msg_len + sizeof(b->uh)); > =20 > return tap_iov_len(c, &b->taph, ip_len); > } > +#pragma GCC diagnostic pop > =20 > /** > * udp_update_hdr6() - Update headers for one IPv6 datagram --=20 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 --EVvJp2KiJedqUQZw Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmXNfBcACgkQzQJF27ox 2GdYsA/7BAkz1nR5WzUSdYIpvGYe5dv7PrMclGhEx50MNGTe9AqfzMlHM6oMX6wc ooJnoRQPxqWSX2G8aTiH+JVLlosf6FyXWwrK/OAVN3AYjE0hi5BcmOzZO1NV7flg YzR+tmw05uZIoRquIM+DjNU70bw+NezgH1cgnwhxcrZ/m+NY0WcrIqOrtBwhui8x DTxY+YthXk7vP7hdDa6m/sOAF46YJt2FqFoS3jbH7fc64ADnyFBqvzuzwlb7ugKX /g6F1wZSZdHUM2ByUfQwX9pQugqE41pkgXSeYaq7NRfaL1f4GuIWAt+ziRlhh5/u 4qCuHBLNecTVuHaO8sm48yG/E3LOztAs4JCI1ITUpmtjyXOjwTYoCaTf24k+1LHK u4XoIDQZpOVXZEWZ57IWM8ueMC98Mhl0+lDtcU1UOMYxPz/nPrgLj3PmdqmHq0E0 lJmNDT383vN5kzfU2lkfisITW7w6ZGVqI3XkDZ5aRQRe0XqbEKTIGQTqr17X8nj2 XbGD6W2F1jpy8rQ/WNs+dR7GnNF3ZZhizhWvm1doqggUVi4T4PFY4/t+POyDcHrB jz6wT9m4ilzNdXZA5l/5ti8MnC4PfKZAhXWXdty235b386Nt4Eyh32TcVhpGz7cs WX3FaYbbzzCaWleAfGWKFgtxelYJCFExndlJJx7ikP4PaqJJNhI= =CpZA -----END PGP SIGNATURE----- --EVvJp2KiJedqUQZw--