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=gln0pVGJ; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id A69715A0262 for ; Tue, 27 Aug 2024 08:05:09 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202408; t=1724738705; bh=jy4I0UhKxrR3L7fE3pWwsjogv52OV10eD9RhhBf4iaQ=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=gln0pVGJBxzoZs9BFnvoMxYxRJrrNTvDpkls37yqAdAbZePWBna9d+Rdl6AsevkiX r9e6nFddLuzuFhki+phrVTtEoDW0BJmB1LkfUnHeufxkw3VREmIo0qPt8pgLazMhVP Xmmc3jmeJ58vIg7zbnFYSb+RzMhu8oKEnahzlUqzdRUyTviUg67QK2I6gBq3eQehHQ nkM/DbnsmO+nvoOYH+E7d5WDU/mXaa6tIvhsAd7yBOQ/+iPl78eKFY5Hf5kbZmryoB SfqdbXBdnXoW6cHVXDCOlM59AkKRjcO2bP/f8posGoBp6maHl7DLzoGZx05h7A8NR6 wUG5NXOCrsRVg== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4WtH816Xm1z4x8C; Tue, 27 Aug 2024 16:05:05 +1000 (AEST) Date: Tue, 27 Aug 2024 16:04:59 +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> <20240827073329.565765e3@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="sObcwzSsIlJd+N7u" Content-Disposition: inline In-Reply-To: <20240827073329.565765e3@elisabeth> Message-ID-Hash: 47G2DTKO2WPHGQY73ZR3ZKXIMY6HIH34 X-Message-ID-Hash: 47G2DTKO2WPHGQY73ZR3ZKXIMY6HIH34 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: --sObcwzSsIlJd+N7u Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Aug 27, 2024 at 07:33:29AM +0200, Stefano Brivio wrote: > On Tue, 27 Aug 2024 11:12:46 +1000 > David Gibson wrote: >=20 > > 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 str= uctures > > > > in udp.c by merging them with each other. One significant one rema= ins: > > > > udp[46]_mh_recv. This was a bit awkward to remove because of a sub= tle > > > > interaction. We initialise the msg_namelen fields to represent the= total > > > > size we have for a socket address, but when we receive into the arr= ays > > > > those are modified to the actual length of the sockaddr we received. > > > >=20 > > > > That meant that naively merging the arrays meant that if we receive= d IPv4 > > > > 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. Findin= g the > > > > flow is the only thing that might use the actual sockaddr length, a= lthough > > > > 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_ep= oll_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 socke= ts */ > > > > 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. Oth= erwise, > > > > - * an IPv4 recv() will alter msg_namelen, so we'd have to reset i= t every > > > > - * 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, union 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 > > >=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); = =20 > >=20 > > It's not redundant per se, because the later assignment only occurs > > for i > 0, so the first one is for slot 0. >=20 > I still don't see how: the second assignment (out of three) is done > before i is incremented, so that should cover i =3D=3D 0 as well, right? >=20 > > It would, however, be > > possible to move to a single assignment in the loop body before i is > > incremented. > >=20 > > 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. > >=20 > > 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". > >=20 > > That was my reasoning, anyway. I'm happy enough to change it if you > > have a preferred approach. >=20 > No, no, this all makes sense. But you add three assignments here, and I > don't understand why #1 is needed if we have #2 and #3, or why #2 is > needed if we have #1 and #3. Oh, bother, somehow I missed #2 when reading your comments. It's a leftover from an earlier draft and is not supposed to be there. New spin shortly. --=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 --sObcwzSsIlJd+N7u Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmbNbIYACgkQzQJF27ox 2Gf+BRAAqG4Yw22AWdkS4oYRcsjyEOsXnUbATzPWIeD0OdpPKsnvDZ6roZ3fIS6W sXRS04tPziENtgE+sxPqdntGeHdKAfvkdp4D1iVW78p3npqKEnkRxxY4/SUcyYXb B7k5rodINz7ABiMtrFB9GEf8tM2u+e3lFDk41o3Sn3GUURGkElH7PEEIjAB1BQml g6kN9ZlWC2+2w5A5xRfylsEnfWoOc+Ryt3mxI7UyVIgJEgc/OrECwIN6A7dzfT/K +/dekpQ5Wny7UrwAFan8t9jEYwZ/XPhZboAYftEDYIuLti+Iw7YcHRWv2YbnKQuC pxzO92M0V5eqlxxpdfAHl7iOAJB4JR9BoEs9iz6Pvlb3iYJW+3CBWvk+/5a0Bn2Q rz0+DGPMalI/84gZ/wHf0JMGrtXB7Eg1+7F5WXqNzrDG/pdiLrQO2AwBrVsZebLn M/oo+CwsCvJ2/DthQEllnPWfgH4ZgZ9oi5nUp9Lh/2I4bkFKbDqI1UNEZMSa7fLX mYQHVxuZWSIs3n9mDhrTHYg6p/qDfJS1NeZ6WEpmVCIzvjIR00JgBC4eGkodUw0h vpb+JUEPYgI7kO3lj5ItSYI2vbwKeMhXmiKPzUMuxW+ZxbB2pGIy4nq+JRlBcyaz mzB2WGqr+8CMJJWtW9vxJiNJ6eJjF/RoFDUy0chZQBDm3EgWZ6Q= =Oj9c -----END PGP SIGNATURE----- --sObcwzSsIlJd+N7u--