From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202408 header.b=T97vrIWD; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id BA6095A004C for ; Tue, 27 Aug 2024 03:13:00 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202408; t=1724721173; bh=k4PYKLr+cASBY+VtTTrKaUKROCNQI5rToCFrLrbkuKI=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=T97vrIWDBHcwNSGp859sW1IdkWvKKgk5CIbIUphGG9jZ7b0kZtT1wRVrcAvVxS4Xs h/oF5mWRzyx1SvGQimEHCAx6W5G8Oq1RpP8uapevu4jCHHlPRyQk7DqBhd0yF+i2SI rYYV8nTw4TtBVCEQW8viMKSMlaU4NuhQ6Axy8WInsUes2J0UBNWqbomLgskDHjrmud r4kxgyd2DO0p4dnOc+sydFdPvnz+EOyLmaamq8UUJUCjyB1iFzs2HYY8VWFb86hk6+ h/sPoUNyxD3LRggx4xGK1idbO8HC0/thTUcf79MPlcKan8VBbKvg7woaSFNbjMNzvT f1qRWARl0i1iw== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4Wt8fs3Zr5z4wcF; Tue, 27 Aug 2024 11:12:53 +1000 (AEST) Date: Tue, 27 Aug 2024 11:12:46 +1000 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH 1/3] udp: Merge udp[46]_mh_recv arrays Message-ID: References: <20240826093716.1925064-1-david@gibson.dropbear.id.au> <20240826093716.1925064-2-david@gibson.dropbear.id.au> <20240826213255.769242da@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="Yag3EQA04RXmhEGf" Content-Disposition: inline In-Reply-To: <20240826213255.769242da@elisabeth> Message-ID-Hash: PYNJ77PGFAZAPD234ARRXZX3DRMCVX3D X-Message-ID-Hash: PYNJ77PGFAZAPD234ARRXZX3DRMCVX3D 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: --Yag3EQA04RXmhEGf Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Aug 26, 2024 at 09:32:55PM +0200, Stefano Brivio wrote: > On Mon, 26 Aug 2024 19:37:14 +1000 > David Gibson wrote: >=20 > > We've already gotten rid of most of the IPv4/IPv6 specific data structu= res > > in udp.c by merging them with each other. One significant one remains: > > udp[46]_mh_recv. This was a bit awkward to remove because of a subtle > > interaction. We initialise the msg_namelen fields to represent the tot= al > > size we have for a socket address, but when we receive into the arrays > > those are modified to the actual length of the sockaddr we received. > >=20 > > That meant that naively merging the arrays meant that if we received IP= v4 > > datagrams, then IPv6 datagrams, the addresses for the latter would be > > truncated. In this patch address that by resetting the received > > msg_namelen as soon as we've found a flow for the datagram. Finding the > > flow is the only thing that might use the actual sockaddr length, altho= ugh > > we in fact don't need it for the time being. > >=20 > > This also removes the last use of the 'v6' field from udp_listen_epoll_= ref, > > so remove that as well. > >=20 > > Signed-off-by: David Gibson > > --- > > udp.c | 57 ++++++++++++++++++++------------------------------------- > > udp.h | 2 -- > > 2 files changed, 20 insertions(+), 39 deletions(-) > >=20 > > diff --git a/udp.c b/udp.c > > index 8a93aad6..6638c22b 100644 > > --- a/udp.c > > +++ b/udp.c > > @@ -178,8 +178,7 @@ enum udp_iov_idx { > > =20 > > /* IOVs and msghdr arrays for receiving datagrams from sockets */ > > static struct iovec udp_iov_recv [UDP_MAX_FRAMES]; > > -static struct mmsghdr udp4_mh_recv [UDP_MAX_FRAMES]; > > -static struct mmsghdr udp6_mh_recv [UDP_MAX_FRAMES]; > > +static struct mmsghdr udp_mh_recv [UDP_MAX_FRAMES]; > > =20 > > /* IOVs and msghdr arrays for sending "spliced" datagrams to sockets */ > > static union sockaddr_inany udp_splice_to; > > @@ -222,6 +221,7 @@ 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_t *payload =3D &udp_payload[i]; > > + struct msghdr *mh =3D &udp_mh_recv[i].msg_hdr; > > struct udp_meta_t *meta =3D &udp_meta[i]; > > struct iovec *siov =3D &udp_iov_recv[i]; > > struct iovec *tiov =3D udp_l2_iov[i]; > > @@ -236,27 +236,10 @@ static void udp_iov_init_one(const struct ctx *c,= size_t i) > > tiov[UDP_IOV_TAP] =3D tap_hdr_iov(c, &meta->taph); > > tiov[UDP_IOV_PAYLOAD].iov_base =3D payload; > > =20 > > - /* It's useful to have separate msghdr arrays for receiving. Otherwi= se, > > - * an IPv4 recv() will alter msg_namelen, so we'd have to reset it ev= ery > > - * time or risk truncating the address on future IPv6 recv()s. > > - */ > > - if (c->ifi4) { > > - struct msghdr *mh =3D &udp4_mh_recv[i].msg_hdr; > > - > > - mh->msg_name =3D &meta->s_in; > > - mh->msg_namelen =3D sizeof(struct sockaddr_in); > > - mh->msg_iov =3D siov; > > - mh->msg_iovlen =3D 1; > > - } > > - > > - if (c->ifi6) { > > - struct msghdr *mh =3D &udp6_mh_recv[i].msg_hdr; > > - > > - mh->msg_name =3D &meta->s_in; > > - mh->msg_namelen =3D sizeof(struct sockaddr_in6); > > - mh->msg_iov =3D siov; > > - mh->msg_iovlen =3D 1; > > - } > > + mh->msg_name =3D &meta->s_in; > > + mh->msg_namelen =3D sizeof(meta->s_in); > > + mh->msg_iov =3D siov; > > + mh->msg_iovlen =3D 1; > > } > > =20 > > /** > > @@ -506,10 +489,10 @@ static int udp_sock_recv(const struct ctx *c, int= s, uint32_t events, > > void udp_listen_sock_handler(const struct ctx *c, union epoll_ref ref, > > uint32_t events, const struct timespec *now) > > { > > - struct mmsghdr *mmh_recv =3D ref.udp.v6 ? udp6_mh_recv : udp4_mh_recv; > > + const socklen_t sasize =3D sizeof(udp_meta[0].s_in); > > int n, i; > > =20 > > - if ((n =3D udp_sock_recv(c, ref.fd, events, mmh_recv)) <=3D 0) > > + if ((n =3D udp_sock_recv(c, ref.fd, events, udp_mh_recv)) <=3D 0) > > return; > > =20 > > /* We divide datagrams into batches based on how we need to send them, > > @@ -518,6 +501,7 @@ void udp_listen_sock_handler(const struct ctx *c, u= nion epoll_ref ref, > > * populate it one entry *ahead* of the loop counter. > > */ > > udp_meta[0].tosidx =3D udp_flow_from_sock(c, ref, &udp_meta[0].s_in, = now); > > + udp_mh_recv[0].msg_hdr.msg_namelen =3D sasize; >=20 > I don't understand why you need this assignment. To me it looks > redundant with: >=20 > udp_mh_recv[i].msg_hdr.msg_namelen =3D sizeof(udp_meta[i].s_in); It's not redundant per se, because the later assignment only occurs for i > 0, so the first one is for slot 0. It would, however, be possible to move to a single assignment in the loop body before i is incremented. I did it this way, because I found it easier to reason about. At least theoretically the value of msg_namelen written by recvmmsg() could be important, although we don't use yet (we rely on the sa_family field instead). But because of that it felt wrong to overwrite that value before we've "consumed" it. Logically that happens in udp_flow_from_sock() which is what takes the address in msg_name / msg_namelen and converts it into the long-term form (as part of the flowside). Hence, clearing msg_namelen immediately after each call to udp_flow_from_sock() made sense to me. I did consider changing udp_flow_from_sock() to take a socklen_t * which it clears after using. That seemed slightly abstraction violationy to me: clearing msg_namelen only makes sense because the address is part of a re-used mmsghdr array, and that's not something udp_flow_from_sock() "knows". That was my reasoning, anyway. I'm happy enough to change it if you have a preferred approach. > later (because n > 0), and: >=20 > > for (i =3D 0; i < n; ) { > > flow_sidx_t batchsidx =3D udp_meta[i].tosidx; > > uint8_t batchpif =3D pif_at_sidx(batchsidx); > > @@ -525,18 +509,22 @@ void udp_listen_sock_handler(const struct ctx *c,= union epoll_ref ref, > > =20 > > do { > > if (pif_is_socket(batchpif)) { > > - udp_splice_prepare(mmh_recv, i); > > + udp_splice_prepare(udp_mh_recv, i); > > } else if (batchpif =3D=3D PIF_TAP) { > > - udp_tap_prepare(mmh_recv, i, > > + udp_tap_prepare(udp_mh_recv, i, > > flowside_at_sidx(batchsidx)); > > } > > =20 > > + /* Restore sockaddr length clobbered by recvmsg() */ > > + udp_mh_recv[i].msg_hdr.msg_namelen =3D sizeof(udp_meta[i].s_in); >=20 > what is the difference between assigning sizeof(udp_meta[i].s_in); and > sasize? I thought it would be the same quantity. It is. The only purpose of sasize is to avoid some over-long lines. > > + > > if (++i >=3D n) > > break; > > =20 > > udp_meta[i].tosidx =3D udp_flow_from_sock(c, ref, > > &udp_meta[i].s_in, > > now); > > + udp_mh_recv[i].msg_hdr.msg_namelen =3D sasize; > > } while (flow_sidx_eq(udp_meta[i].tosidx, batchsidx)); > > =20 > > if (pif_is_socket(batchpif)) { >=20 --=20 David Gibson (he or they) | 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 --Yag3EQA04RXmhEGf Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmbNKA0ACgkQzQJF27ox 2GfX7g//YRpiyfVSohp1NmCQPT5OK159UDMD7jEcLAtDbpOSq56H0jM8VIye5L/G pG+vfoH6V1TGQhGqtG2psq2IyFJ8+IzjeTWRb/3QFgEkHzyl8Dswi4LnYr35lHO1 wISyXQqIE7KQRXHcryWDJFEh0HVHzetYN+ce2j+4hifptuykqn91ahBRs0ezr5G0 Emv6qNiepZbsKi+F7Xg5W2eInDwYRH+I5iqM2aA/TQk3fuGm0QRtAbsXB7kTGcqw E+Ep8XT8BzExVP5cJEngj3hZNHBSPjNoF2LKXruMpVm/17N3sujxPSRnqveujQCI uNBwKdIIB+eiLNHTmyoe1yrH5eIozPhD07nDafTaCA/G5+dgD+sWBVCZJPHR6xyv 5ostA6R0JNc/yvb00D6qPO4AJvxpXMl4/a9kFwRQWvRMEH8660rreXc1oHNw+E5q Q9R1zOCIJeXK17W2pMzk3t4JbrZuSYrFvStySHFoABkAYWpviSZbJhRTqvnBdfIQ ykelfo06cNiq9g9d63bSdip16fPdEa8nvzD9htfKDImRV/FyyFM3PyNv2Wi4vMe0 cgoq2k+cyfA8dlI8SrMWNxKxMS8iAHaEKQ+WaaZ94Q1JtfXslGSV3cLbLuFInBts No6jbjAMrx6hAKHwC9zNmfGqEISbe5MpxdiYfHpssETbyhuVI8g= =l5XC -----END PGP SIGNATURE----- --Yag3EQA04RXmhEGf--