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 3AFEE5A02EC for ; Wed, 1 May 2024 04:54:45 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1714532080; bh=5glqzRz7nZMOzQUFlNhGj1iV0awOsex2BnFhPBM3LxU=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=JwZpW7akzBx5VdG3IXe8v2azg3nd23zBdpUcewGAmalFWbjGErK6mtyprTKPESWpt kedTcyXmk+7X3jHfaEOtFguWYLcabBRWE8KdfEs803B0K4IfeHU4X4618CgRyzFSXt D3hCR4PluECsMUvuZsymMTJ+KZMlLvbTkuLmQ4TFU9m8uaAKXebZsFFXtMWZf/QSeY RW5HyGnZD6SorbaakISYLHirzA/4QFzGX1RtsTGtm+38RJ47k5t3J53wiKIqbghJVR BOJTVsv9tXC2y06reYHH7YG6GxptTYjMCZMEGwfwgE3o2onVp91fgk1w67h6+7aZ4x vMon29pWADB+w== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4VThVm4x38z4x10; Wed, 1 May 2024 12:54:40 +1000 (AEST) Date: Wed, 1 May 2024 12:46:10 +1000 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH 5/7] udp: Share payload buffers between IPv4 and IPv6 Message-ID: References: <20240430100541.381350-1-david@gibson.dropbear.id.au> <20240430100541.381350-6-david@gibson.dropbear.id.au> <20240430221604.247fb543@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="DZ3ys5FoyyqDG1yu" Content-Disposition: inline In-Reply-To: <20240430221604.247fb543@elisabeth> Message-ID-Hash: O5RDWJBYTN52SW7737SU6G6JE5EIEKMW X-Message-ID-Hash: O5RDWJBYTN52SW7737SU6G6JE5EIEKMW 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: --DZ3ys5FoyyqDG1yu Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Apr 30, 2024 at 10:16:04PM +0200, Stefano Brivio wrote: > On Tue, 30 Apr 2024 20:05:39 +1000 > David Gibson wrote: >=20 > > Currently the IPv4 and IPv6 paths unnecessarily use different buffers f= or > > the UDP payload. Now that we're handling the various pieces of the UDP > > packets with an iov, we can split the payload part of the buffers off i= nto > > its own array shared between IPv4 and IPv6. As well as saving a little > > memory, this allows the payload buffers to be neatly page aligned. > >=20 > > With the buffers merged, udp[46]_l2_iov_sock contain exactly the same t= hing > > as each other and can also be merged. Likewise udp[46]_iov_splice can = be > > merged together. > >=20 > > Signed-off-by: David Gibson > > --- > > udp.c | 127 ++++++++++++++++++++++++++++++---------------------------- > > 1 file changed, 65 insertions(+), 62 deletions(-) > >=20 > > diff --git a/udp.c b/udp.c > > index 86d0419f..8c726056 100644 > > --- a/udp.c > > +++ b/udp.c > > @@ -171,14 +171,22 @@ static uint8_t udp_act[IP_VERSIONS][UDP_ACT_TYPE_= MAX][DIV_ROUND_UP(NUM_PORTS, 8) > > =20 > > /* Static buffers */ > > =20 > > +static struct udp_payload { >=20 > As we're defining a new struct, the usual comment format would be nice, > and I would also keep it symmetric with tcp_payload_t (udp_payload_t), > say: >=20 > /** > * struct udp_payload_t - UDP header and data for inbound messages > * @uh: UDP header > * @data: UDP data > */ >=20 > > + struct udphdr uh; > > + char data[USHRT_MAX - sizeof(struct udphdr)]; > > +#ifdef __AVX2__ > > +} __attribute__ ((packed, aligned(32))) > > +#else > > +} __attribute__ ((packed, aligned(__alignof__(unsigned int)))) > > +#endif > > +udp_payload_buf[UDP_MAX_FRAMES]; >=20 > ...and this could be 'udp_payload'. Amended as suggested. > > + > > /** > > * udp4_l2_buf_t - Pre-cooked IPv4 packet buffers for tap connections > > * @s_in: Source socket address, filled in by recvmmsg() > > * @taph: Tap backend specific header > > * @eh: Prefilled ethernet header > > * @iph: Pre-filled IP header (except for tot_len and saddr) > > - * @uh: Headroom for UDP header > > - * @data: Storage for UDP payload > > */ > > static struct udp4_l2_buf_t { > > struct sockaddr_in s_in; > > @@ -186,9 +194,6 @@ static struct udp4_l2_buf_t { > > struct tap_hdr taph; > > struct ethhdr eh; > > struct iphdr iph; > > - struct udphdr uh; > > - uint8_t data[USHRT_MAX - > > - (sizeof(struct iphdr) + sizeof(struct udphdr))]; > > } __attribute__ ((packed, aligned(__alignof__(unsigned int)))) > > udp4_l2_buf[UDP_MAX_FRAMES]; > > =20 > > @@ -198,8 +203,6 @@ udp4_l2_buf[UDP_MAX_FRAMES]; > > * @taph: Tap backend specific header > > * @eh: Pre-filled ethernet header > > * @ip6h: Pre-filled IP header (except for payload_len and addresses) > > - * @uh: Headroom for UDP header > > - * @data: Storage for UDP payload > > */ > > struct udp6_l2_buf_t { > > struct sockaddr_in6 s_in6; > > @@ -212,9 +215,6 @@ struct udp6_l2_buf_t { > > struct tap_hdr taph; > > struct ethhdr eh; > > struct ipv6hdr ip6h; > > - struct udphdr uh; > > - uint8_t data[USHRT_MAX - > > - (sizeof(struct ipv6hdr) + sizeof(struct udphdr))]; > > #ifdef __AVX2__ > > } __attribute__ ((packed, aligned(32))) > > #else > > @@ -239,8 +239,7 @@ enum udp_iov_idx { > > }; > > =20 > > /* recvmmsg()/sendmmsg() data for tap */ > > -static struct iovec udp4_l2_iov_sock [UDP_MAX_FRAMES]; > > -static struct iovec udp6_l2_iov_sock [UDP_MAX_FRAMES]; > > +static struct iovec udp_l2_iov_sock [UDP_MAX_FRAMES]; > > =20 > > static struct iovec udp4_l2_iov_tap [UDP_MAX_FRAMES][UDP_NUM_IOVS]; > > static struct iovec udp6_l2_iov_tap [UDP_MAX_FRAMES][UDP_NUM_IOVS]; > > @@ -249,8 +248,7 @@ static struct mmsghdr udp4_l2_mh_sock [UDP_MAX_FRA= MES]; > > static struct mmsghdr udp6_l2_mh_sock [UDP_MAX_FRAMES]; > > =20 > > /* recvmmsg()/sendmmsg() data for "spliced" connections */ > > -static struct iovec udp4_iov_splice [UDP_MAX_FRAMES]; > > -static struct iovec udp6_iov_splice [UDP_MAX_FRAMES]; > > +static struct iovec udp_iov_splice [UDP_MAX_FRAMES]; > > =20 > > static struct sockaddr_in udp4_localname =3D { > > .sin_family =3D AF_INET, > > @@ -261,8 +259,8 @@ static struct sockaddr_in6 udp6_localname =3D { > > .sin6_addr =3D IN6ADDR_LOOPBACK_INIT, > > }; > > =20 > > -static struct mmsghdr udp4_mh_splice [UDP_MAX_FRAMES]; > > -static struct mmsghdr udp6_mh_splice [UDP_MAX_FRAMES]; > > +static struct mmsghdr udp4_mh_splice [UDP_MAX_FRAMES]; > > +static struct mmsghdr udp6_mh_splice [UDP_MAX_FRAMES]; > > =20 > > /** > > * udp_portmap_clear() - Clear UDP port map before configuration > > @@ -322,10 +320,14 @@ void udp_update_l2_buf(const unsigned char *eth_d= , const unsigned char *eth_s) > > */ > > static void udp_iov_init_one(const struct ctx *c, size_t i) > > { > > + struct udp_payload *payload =3D &udp_payload_buf[i]; > > + struct iovec *siov =3D &udp_l2_iov_sock[i]; > > + > > + *siov =3D IOV_OF_LVALUE(payload->data); > > + > > if (c->ifi4) { > > struct msghdr *mh =3D &udp4_l2_mh_sock[i].msg_hdr; > > struct udp4_l2_buf_t *buf =3D &udp4_l2_buf[i]; > > - struct iovec *siov =3D &udp4_l2_iov_sock[i]; > > struct iovec *tiov =3D udp4_l2_iov_tap[i]; > > =20 > > *buf =3D (struct udp4_l2_buf_t) { > > @@ -333,8 +335,6 @@ static void udp_iov_init_one(const struct ctx *c, s= ize_t i) > > .iph =3D L2_BUF_IP4_INIT(IPPROTO_UDP) > > }; > > =20 > > - *siov =3D IOV_OF_LVALUE(buf->data); > > - > > mh->msg_name =3D &buf->s_in; > > mh->msg_namelen =3D sizeof(buf->s_in); > > mh->msg_iov =3D siov; > > @@ -343,13 +343,12 @@ static void udp_iov_init_one(const struct ctx *c,= size_t i) > > tiov[UDP_IOV_TAP] =3D tap_hdr_iov(c, &buf->taph); > > tiov[UDP_IOV_ETH] =3D IOV_OF_LVALUE(buf->eh); > > tiov[UDP_IOV_IP] =3D IOV_OF_LVALUE(buf->iph); > > - tiov[UDP_IOV_PAYLOAD].iov_base =3D &buf->uh; > > + tiov[UDP_IOV_PAYLOAD].iov_base =3D payload; > > } > > =20 > > if (c->ifi6) { > > struct msghdr *mh =3D &udp6_l2_mh_sock[i].msg_hdr; > > struct udp6_l2_buf_t *buf =3D &udp6_l2_buf[i]; > > - struct iovec *siov =3D &udp6_l2_iov_sock[i]; > > struct iovec *tiov =3D udp6_l2_iov_tap[i]; > > =20 > > *buf =3D (struct udp6_l2_buf_t) { > > @@ -357,8 +356,6 @@ static void udp_iov_init_one(const struct ctx *c, s= ize_t i) > > .ip6h =3D L2_BUF_IP6_INIT(IPPROTO_UDP) > > }; > > =20 > > - *siov =3D IOV_OF_LVALUE(buf->data); > > - > > mh->msg_name =3D &buf->s_in6; > > mh->msg_namelen =3D sizeof(buf->s_in6); > > mh->msg_iov =3D siov; > > @@ -367,7 +364,7 @@ static void udp_iov_init_one(const struct ctx *c, s= ize_t i) > > tiov[UDP_IOV_TAP] =3D tap_hdr_iov(c, &buf->taph); > > tiov[UDP_IOV_ETH] =3D IOV_OF_LVALUE(buf->eh); > > tiov[UDP_IOV_IP] =3D IOV_OF_LVALUE(buf->ip6h); > > - tiov[UDP_IOV_PAYLOAD].iov_base =3D &buf->uh; > > + tiov[UDP_IOV_PAYLOAD].iov_base =3D payload; > > } > > } > > =20 > > @@ -581,22 +578,24 @@ static void udp_splice_sendfrom(const struct ctx = *c, unsigned start, unsigned n, > > /** > > * udp_update_hdr4() - Update headers for one IPv4 datagram > > * @c: Execution context > > - * @b: Pointer to udp4_l2_buf to update > > + * @bh: Pointer to udp4_l2_buf to update > > + * @bp: Pointer to udp_payload to update > > * @dstport: Destination port number > > * @plen: Length of UDP payload > > * @now: Current timestamp > > * > > * Return: size of IPv4 payload (UDP header + data) > > */ > > -static size_t udp_update_hdr4(const struct ctx *c, struct udp4_l2_buf_= t *b, > > +static size_t udp_update_hdr4(const struct ctx *c, > > + struct udp4_l2_buf_t *bh, struct udp_payload *bp, > > in_port_t dstport, size_t plen, > > const struct timespec *now) > > { > > + in_port_t srcport =3D ntohs(bh->s_in.sin_port); > > const struct in_addr dst =3D c->ip4.addr_seen; > > - size_t l4len =3D plen + sizeof(b->uh); > > - size_t l3len =3D l4len + sizeof(b->iph); > > - in_port_t srcport =3D ntohs(b->s_in.sin_port); > > - struct in_addr src =3D b->s_in.sin_addr; > > + struct in_addr src =3D bh->s_in.sin_addr; > > + size_t l4len =3D plen + sizeof(bp->uh); > > + size_t l3len =3D l4len + sizeof(bh->iph); > > =20 > > if (!IN4_IS_ADDR_UNSPECIFIED(&c->ip4.dns_match) && > > IN4_ARE_ADDR_EQUAL(&src, &c->ip4.dns_host) && srcport =3D=3D 53 && > > @@ -617,39 +616,41 @@ static size_t udp_update_hdr4(const struct ctx *c= , struct udp4_l2_buf_t *b, > > src =3D c->ip4.gw; > > } > > =20 > > - b->iph.tot_len =3D htons(l3len); > > - b->iph.daddr =3D dst.s_addr; > > - b->iph.saddr =3D src.s_addr; > > - b->iph.check =3D csum_ip4_header(b->iph.tot_len, IPPROTO_UDP, > > - src, c->ip4.addr_seen); > > + bh->iph.tot_len =3D htons(l3len); > > + bh->iph.daddr =3D c->ip4.addr_seen.s_addr; >=20 > This is still the same as dst.s_addr (existing assignment), right? Ah, yes, I think that's a leftover from some interim version. Spurious change removed. --=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 --DZ3ys5FoyyqDG1yu Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmYxrPEACgkQzQJF27ox 2Gfn9g//XQYSGZr0FkMtbI8+gi1Kw22XlDTasP9Z2UJP2L5hbSP+d3ijejyOhpCK eaRHObcOiIxpZpfp755MbKrNfC4GfdOXPC0F4tiWK4TmaDaP2CHiZ1uJTk4G+TR4 FgFhrlPM+2pxWuf8ucstCiPX6ydOhbndFQBodPuyKcN1T9XyY9Sx7pN7+4P9tnu0 Xk9jsJJg/C+bmWaq1f9kcvbVtd9MjMt6Kupf0Lkvdz0Xc/ReM6s+6AVFtsFQVHVM PZm7rGGVQyx3jhI6mZL3It2S2dFTKPLKePu6NNNtYICgYr/NWa7vObByMy7aXAMD vgd+wBw4hT+ZXSt7mSSROoR7Dme8W3SDZdnjytRhiD1A2b6MXtb4XKRSBhnyIsS7 s7JL7MJ+N7gL4IRG21XMxev9wwEW/UewXz/95oYnjjiSSo017ax5sST9GW+6kBWn wKCbmuHfA/Xtkj7an6/SIGkZcXJfTfXOJc9EyuR60F2DzJ6AUG5Ym0b/Gz7sTsFs s5f9dFJ4FL0zA9SrI36BC6GnzbaVazhhC64LF715c5mC2ciOZDlfMChwQNx6aDCk fXu0ML3BSfswVRUZxS7NNGvaG0Qtk+RMonE9y2ZnMsqu3nUtupqkxZJ2ui/fNsoi 8rowpZXIZdUZ1EUQ6zVBvNQoYGxXpwqnC86yos6qUklwNC85Abo= =zbEh -----END PGP SIGNATURE----- --DZ3ys5FoyyqDG1yu--