From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from gandalf.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id 03DE65A0272 for ; Thu, 8 Feb 2024 00:54:56 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1707350091; bh=kvp1U0rNJ6H1WG/oIiZXrOwhTW9TcEjkYvwhUKUyyBk=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=nvuT/emqY1fra+wF+sSduTQCoAocoZCyypszoOpkyDJXSxFgAF8ahz54SZpKjUoWY 2C9liUShcTt7wN7R50LH/98CvJ3L0VudFwXzy/0+sj5oFJzDAmxesfQ2WwHAsaZacr pc5OoMyJlEStnDTvPFtrSSgQ4AafSM4A2axwTBpSBtAsWWtZWd4QPJCb5tS3XE5F03 JosUrRgaVfseY7Psjj+0ceg5Vz6WiIAHSHBIRPceXgs87JdaHanS50TadE7d0jRxrt MkAcXTWo9RUo0XvK24Wwc4wDzc1WgQqKOBRfjIaTnnPUuO18HdK7AYsSwHLhKgUHW1 7sOrjSFzncyRw== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4TVcRb6gM2z4wys; Thu, 8 Feb 2024 10:54:51 +1100 (AEDT) Date: Thu, 8 Feb 2024 10:43:35 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH 06/24] ip: move duplicate IPv4 checksum function to ip.h Message-ID: References: <20240202141151.3762941-1-lvivier@redhat.com> <20240202141151.3762941-7-lvivier@redhat.com> <20240207114040.78e7081f@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="p/UtwYCK84SFmH0o" Content-Disposition: inline In-Reply-To: <20240207114040.78e7081f@elisabeth> Message-ID-Hash: T3RZXZAWGARJUAMNQTUV2NJLWWMIZE72 X-Message-ID-Hash: T3RZXZAWGARJUAMNQTUV2NJLWWMIZE72 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: Laurent Vivier , 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: --p/UtwYCK84SFmH0o Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Feb 07, 2024 at 11:40:40AM +0100, Stefano Brivio wrote: > On Fri, 2 Feb 2024 15:11:33 +0100 > Laurent Vivier wrote: >=20 > > We can find the same function to compute the IPv4 header > > checksum in tcp.c and udp.c > >=20 > > Signed-off-by: Laurent Vivier > > --- > > ip.h | 14 ++++++++++++++ > > tcp.c | 23 ++--------------------- > > udp.c | 19 +------------------ > > 3 files changed, 17 insertions(+), 39 deletions(-) > >=20 > > diff --git a/ip.h b/ip.h > > index b2e08bc049f3..ff7902c45a95 100644 > > --- a/ip.h > > +++ b/ip.h > > @@ -9,6 +9,8 @@ > > #include > > #include > > =20 > > +#include "checksum.h" > > + > > #define IN4_IS_ADDR_UNSPECIFIED(a) \ > > ((a)->s_addr =3D=3D htonl_constant(INADDR_ANY)) > > #define IN4_IS_ADDR_BROADCAST(a) \ > > @@ -83,4 +85,16 @@ struct ipv6_opt_hdr { > > =20 > > char *ipv6_l4hdr(const struct pool *p, int idx, size_t offset, uint8_t= *proto, > > size_t *dlen); > > +static inline uint16_t ipv4_hdr_checksum(struct iphdr *iph, int proto) >=20 > A function comment would be nice. A couple of doubts: >=20 > - why is this an inline in ip.h, instead of a function in checksum.c? > That would be more natural, I think >=20 > - this would be the first Layer-4 protocol number passed as int: we use > uint8_t elsewhere. Now, socket(2) and similar all take an int, but > using uint8_t internally keeps large arrays such as tap4_l4 a bit > smaller. >=20 > The only value defined in Linux UAPI exceeding eight bits is > IPPROTO_MPTCP, 262, because that's never on the wire (the TCP > protocol number is used instead). And we won't meet that either. Right. This is pretty much explicitly the IP protocol number, which is an 8-bit field in the IPv4 header. > In practice, it doesn't matter what we use here, but still uint8_t > would be consistent. >=20 > > +{ > > + uint32_t sum =3D L2_BUF_IP4_PSUM(proto); > > + > > + sum +=3D iph->tot_len; > > + sum +=3D (iph->saddr >> 16) & 0xffff; > > + sum +=3D iph->saddr & 0xffff; > > + sum +=3D (iph->daddr >> 16) & 0xffff; > > + sum +=3D iph->daddr & 0xffff; > > + > > + return ~csum_fold(sum); > > +} > > #endif /* IP_H */ > > diff --git a/tcp.c b/tcp.c > > index 4c9c5fb51c60..293ab12d8c21 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 store= d 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,8 @@ 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 : > > + ipv4_hdr_checksum(&b->iph, IPPROTO_TCP); > > =20 > > SET_TCP_HEADER_COMMON_V4_V6(b, conn, seq); > > =20 > > diff --git a/udp.c b/udp.c > > index d514c864ab5b..6f867df81c05 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 stor= ed 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 addr= esses > > * @eth_d: Ethernet destination address, NULL if unchanged > > @@ -614,7 +597,7 @@ 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 ipv4_hdr_checksum(&b->iph, IPPROTO_UDP); > > 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 --=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 --p/UtwYCK84SFmH0o Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmXEFZYACgkQzQJF27ox 2GcyoQ//RRfyNQvnSNRk+VrhLkH8iv6Mt6WxWjUkRStJ655W/WcsFmOUBWskfgQ9 bhyj/BKatvJ0DFg0/FUX78tvwVpvmIGDdyguVjyCW+auUY8C7k91tuxfRmBs4AC5 l7RDCy/GYwJ7jAvUWgYa8IFInRHfsnECaVkIBOh2j1MQxEgGQBmGONQy2Jr2gGXk BNzkH4Y9iblvXjOrQcpJyRo//oTiaNtL5wde7sOALE0gJHpOZ/yHyP48xS7c0zJ9 ODxLVBR31Xgsy9AsbA0SUivf9oot9VvH6PjGmh2QlKa13jdUjVxE95pUqouDPO2X X4YsxeaPNgxO2fPRCVTz6IsSln/1ljETiX7fLjgE6l3Xl8bR5k3FGnruhC7+mzZJ rYQVdFIe+XjxZGhxXW1sHo+BRm61DTw9fJPhOSRdJHnikze6tOxbb+zMHlTXyob7 zW0tPzu1AtjGFBtvEPHM3YAnENyyuX9CaHpqZ8wF/aDHqhh8riHWw0q8rk0GdsXt OD3FxGB1+pUSxlFCsg+0jhQQm17xRAwFmU2gm+lrBh8XHZEZUDaN+TPXJ6SSKANh 29AkLyrHx/RNayD+7cdQplqqdFQLk9VOv0DvNstIVuNh/x7htkJnMg3ROsajR/Gb scq8055N38XFD+ak02HNlTFJJkrimFCL9Ow9cn/904BFlT94nZ4= =e+NX -----END PGP SIGNATURE----- --p/UtwYCK84SFmH0o--