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=202502 header.b=EyoFe/2Y; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id BE3535A0008 for ; Thu, 27 Mar 2025 00:23:01 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202502; t=1743031377; bh=rEk4MgBnVNA6aV9xxL2RjLJFHx6tBJIQYW2L6Ax6bwE=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=EyoFe/2YGy5hzxsZ42U0NRvtQWzAinJ5DceYxYgypDW31L/BDHmEcCpuj4LTCdQO0 juQfEsKlkrTaj1lVblxFU0cwcYHJoCqWVU7NC5QtI62BQUwhLIrH3MpnQbWu1P4wOk fbFzwsARytB5ZnPdB6e41p34A74Z8bCIZqEJgFOplIfSAcgJPhlaAJQo4SRzSnK8iF QbPHxCQ4ZN3CbBAtM5TyU7/mOup4BqslcUKnW7JQdpGVH99qYPJWq6DqEH+JN2pvr9 WXCHOMkMDKJdLM3MfQBt+9kM+M7SkygpVyadfpZERcVZ0DhiEoTyVOcZl1Uafh26qv rbjxuw89jTqug== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4ZNNB90nVsz4x8Z; Thu, 27 Mar 2025 10:22:57 +1100 (AEDT) Date: Thu, 27 Mar 2025 10:11:48 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH v2 4/7] udp: Share more logic between vu and non-vu reply socket paths Message-ID: References: <20250326034407.2240846-1-david@gibson.dropbear.id.au> <20250326034407.2240846-5-david@gibson.dropbear.id.au> <20250326231433.06d4b263@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="fYEQtpAuk2NKqjig" Content-Disposition: inline In-Reply-To: <20250326231433.06d4b263@elisabeth> Message-ID-Hash: E7VPD6N3E6OBHX75UNEO7D2QC75SUDCH X-Message-ID-Hash: E7VPD6N3E6OBHX75UNEO7D2QC75SUDCH 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: --fYEQtpAuk2NKqjig Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Mar 26, 2025 at 11:14:33PM +0100, Stefano Brivio wrote: > On Wed, 26 Mar 2025 14:44:04 +1100 > David Gibson wrote: >=20 > > Share some additional miscellaneous logic between the vhost-user and "b= uf" > > paths for data on udp reply sockets. The biggest piece is error handli= ng > > of cases where we can't forward between the two pifs of the flow. We a= lso > > make common some more simple logic locating the correct flow and its > > parameters. > >=20 > > This adds some lines of code due to extra comment lines, but nonetheless > > reduces logic duplication. > >=20 > > Signed-off-by: David Gibson > > --- > > udp.c | 41 ++++++++++++++++++++++++++--------------- > > udp_vu.c | 26 +++++++++++--------------- > > udp_vu.h | 3 ++- > > 3 files changed, 39 insertions(+), 31 deletions(-) > >=20 > > diff --git a/udp.c b/udp.c > > index 55021ac4..4258812e 100644 > > --- a/udp.c > > +++ b/udp.c > > @@ -754,24 +754,25 @@ void udp_listen_sock_handler(const struct ctx *c, > > /** > > * udp_buf_reply_sock_data() - Handle new data from flow specific sock= et > > * @c: Execution context > > - * @ref: epoll reference > > + * @s: Socket to read data from > > + * @tosidx: Flow & side to forward data from @s to > > * @now: Current timestamp > > * > > + * Return: true on success, false if can't forward from socket to flow= 's pif >=20 > "on success"... >=20 > > + * > > * #syscalls recvmmsg > > */ > > -static void udp_buf_reply_sock_data(const struct ctx *c, union epoll_r= ef ref, > > +static bool udp_buf_reply_sock_data(const struct ctx *c, > > + int s, flow_sidx_t tosidx, > > const struct timespec *now) > > { > > - flow_sidx_t tosidx =3D flow_sidx_opposite(ref.flowside); > > const struct flowside *toside =3D flowside_at_sidx(tosidx); > > - struct udp_flow *uflow =3D udp_at_sidx(ref.flowside); > > + struct udp_flow *uflow =3D udp_at_sidx(tosidx); > > uint8_t topif =3D pif_at_sidx(tosidx); > > - int n, i, from_s; > > - > > - from_s =3D uflow->s[ref.flowside.sidei]; > > + int n, i; > > =20 > > - if ((n =3D udp_sock_recv(c, from_s, udp_mh_recv)) <=3D 0) > > - return; > > + if ((n =3D udp_sock_recv(c, s, udp_mh_recv)) <=3D 0) > > + return true; >=20 > is not exactly accurate: 0 means either no datagrams (should never > happen) or error on recvmmsg(). But I think it's clear enough, and > you'll have follow-ups anyway, so I went ahead and applied this. >=20 > Maybe: >=20 > * Return: false if we can't forward from socket to flow's pif, true othe= rwise >=20 > ? Actually, looking at the caller: Yeah. While writing subsequent patches, I think I spotted a better way to structure this. I'll put some fixups in the next series. > > flow_trace(uflow, "Received %d datagrams on reply socket", n); > > uflow->ts =3D now->tv_sec; > > @@ -790,11 +791,10 @@ static void udp_buf_reply_sock_data(const struct = ctx *c, union epoll_ref ref, > > } else if (topif =3D=3D PIF_TAP) { > > tap_send_frames(c, &udp_l2_iov[0][0], UDP_NUM_IOVS, n); > > } else { > > - uint8_t frompif =3D pif_at_sidx(ref.flowside); > > - > > - flow_err(uflow, "No support for forwarding UDP from %s to %s", > > - pif_name(frompif), pif_name(topif)); > > + return false; > > } > > + > > + return true; > > } > > =20 > > /** > > @@ -821,10 +821,21 @@ void udp_reply_sock_handler(const struct ctx *c, = union epoll_ref ref, > > } > > =20 > > if (events & EPOLLIN) { > > + flow_sidx_t tosidx =3D flow_sidx_opposite(ref.flowside); > > + int s =3D ref.fd; > > + bool ret; > > + > > if (c->mode =3D=3D MODE_VU) > > - udp_vu_reply_sock_data(c, ref, now); > > + ret =3D udp_vu_reply_sock_data(c, s, tosidx, now); > > else > > - udp_buf_reply_sock_data(c, ref, now); > > + ret =3D udp_buf_reply_sock_data(c, s, tosidx, now); > > + > > + if (!ret) { >=20 > the opposite would probably be less surprising, if (ret) flow_err(...). >=20 > > + flow_err(uflow, > > + "No support for forwarding UDP from %s to %s", > > + pif_name(pif_at_sidx(ref.flowside)), > > + pif_name(pif_at_sidx(tosidx))); > > + } > > } > > } > > =20 > > diff --git a/udp_vu.c b/udp_vu.c > > index 6e1823a9..06bdeae6 100644 > > --- a/udp_vu.c > > +++ b/udp_vu.c > > @@ -273,38 +273,32 @@ void udp_vu_listen_sock_data(const struct ctx *c,= union epoll_ref ref, > > /** > > * udp_vu_reply_sock_data() - Handle new data from flow specific socket > > * @c: Execution context > > - * @ref: epoll reference > > + * @s: Socket to read data from > > + * @tosidx: Flow & side to forward data from @s to > > * @now: Current timestamp > > + * > > + * Return: true on success, false if can't forward from socket to flow= 's pif > > */ > > -void udp_vu_reply_sock_data(const struct ctx *c, union epoll_ref ref, > > +bool udp_vu_reply_sock_data(const struct ctx *c, int s, flow_sidx_t to= sidx, > > const struct timespec *now) > > { > > - flow_sidx_t tosidx =3D flow_sidx_opposite(ref.flowside); > > const struct flowside *toside =3D flowside_at_sidx(tosidx); > > bool v6 =3D !(inany_v4(&toside->eaddr) && inany_v4(&toside->oaddr)); > > - struct udp_flow *uflow =3D udp_at_sidx(ref.flowside); > > - int from_s =3D uflow->s[ref.flowside.sidei]; > > + struct udp_flow *uflow =3D udp_at_sidx(tosidx); > > struct vu_dev *vdev =3D c->vdev; > > struct vu_virtq *vq =3D &vdev->vq[VHOST_USER_RX_QUEUE]; > > - uint8_t topif =3D pif_at_sidx(tosidx); > > int i; > > =20 > > ASSERT(uflow); > > =20 > > - if (topif !=3D PIF_TAP) { > > - uint8_t frompif =3D pif_at_sidx(ref.flowside); > > - > > - flow_err(uflow, > > - "No support for forwarding UDP from %s to %s", > > - pif_name(frompif), pif_name(topif)); > > - return; > > - } > > + if (pif_at_sidx(tosidx) !=3D PIF_TAP) > > + return false; > > =20 > > for (i =3D 0; i < UDP_MAX_FRAMES; i++) { > > ssize_t dlen; > > int iov_used; > > =20 > > - iov_used =3D udp_vu_sock_recv(c, from_s, v6, &dlen); > > + iov_used =3D udp_vu_sock_recv(c, s, v6, &dlen); > > if (iov_used <=3D 0) > > break; > > flow_trace(uflow, "Received 1 datagram on reply socket"); > > @@ -318,4 +312,6 @@ void udp_vu_reply_sock_data(const struct ctx *c, un= ion epoll_ref ref, > > } > > vu_flush(vdev, vq, elem, iov_used); > > } > > + > > + return true; > > } > > diff --git a/udp_vu.h b/udp_vu.h > > index 4f2262d0..2299b51f 100644 > > --- a/udp_vu.h > > +++ b/udp_vu.h > > @@ -8,6 +8,7 @@ > > =20 > > void udp_vu_listen_sock_data(const struct ctx *c, union epoll_ref ref, > > const struct timespec *now); > > -void udp_vu_reply_sock_data(const struct ctx *c, union epoll_ref ref, > > +bool udp_vu_reply_sock_data(const struct ctx *c, int s, flow_sidx_t to= sidx, > > const struct timespec *now); > > + > > #endif /* UDP_VU_H */ >=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 --fYEQtpAuk2NKqjig Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmfkibAACgkQzQJF27ox 2GfsMQ/+O00lNC94XupZ0af5iRmmefJ4Oj4hYVT+LpSpxfVAOeO6/PxLsv+Y7+c1 xXVThpeDk8FXu76CIYzYZP7ZEcNNSFle/4PqiG3erqajIbHECCdowa+plA8yE1Lm SlcDtrFH9/A8FZN/ZwxMU1ZAV67jMyc2b1/BkG4JKaWrVyH13wNWFHWNVYiLo1yT Kf52fDXqFIzLiJu/EPy68/rU4Jz9zw3/grKFDGuJcyYSKiV46QP4m5GIL/TgQ/m+ zfzEsfkXp55Dwf1RUVl419TY/ayipbOYNpZZQ9t4no6u3ODMB1GN/RKHqX8skd/9 U7PIdKJakcrFRy+wsthcLnvNFsJb8ipukLjxk01FG/+U1k0Vnq0oAZwWYV0I0v9X 2tiB3AT2pOmNL/f22zykmZvCbKxE93LcvYCh7RcLIP7PcoIER3G7sby/MMKYHKGk vhHeuJ5MqdCogHS6rxiPMkcZkYMq8yqFazc7LjSDLOSlvFtU4QaeeeghGD4Fvp// T8Jlf5E2T8+a2v/OrB/PUxitAOpTbSVTFbTw0raWgu+xQ2LKoYDIyzJ1HTEAiBHL +q//JBai3q+wByfffbMui6ApxeC4qPJKqLFKEqAkpYrkxaNNsZjpiNOczQf0NbQe cW48fg7XjN/RBjdsxjYjvU0E43RVgyQDEi3xNJI/DMeoRduZKME= =lbdd -----END PGP SIGNATURE----- --fYEQtpAuk2NKqjig--