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=202608 header.b=fftuaz+m; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id F35955A0272 for ; Thu, 13 Aug 2026 04:09:46 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202608; t=1786586982; bh=oKtx1CPga2pgrGIAR4/77LZRotrqRW8FFlO5Xw2kjrk=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=fftuaz+m3WUGfll4MwK6esqfhZgNKbxo1WpckdObk9F94i3zrRzMVFvTEXlSZfugI SiUnEk+slD0E+uBYFLzEbtfWspJ+ESDtpC6QUTqjByAPB/lkCqkofVA43SaI6JiAR+ aZtQ0A61+OkT2WeKXVnl34KxTG/wbO2GZ5wyAemVciYDkrnhcRf1E7C+ZMgSOZvf+e 2CvjKH8mJE7GCFS/PKWBNNeOMR9WoTW8r6NLIEC7zd1bH4WtgE/IgETvt5iz0PyK6C WCfx34rW+698Zc13d2Z4E2HJ+ggAsM+zn/71oxtl1RacGZ1eENv4UKhVa+02WeLTZn SaER26Q2zzb6A== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4hL81y1lm8z4wGx; Thu, 13 Aug 2026 12:09:42 +1000 (AEST) Date: Thu, 13 Aug 2026 12:01:59 +1000 From: David Gibson To: Ammar Yasser Subject: Re: [RFC v3 7/8] pasta: Implement pasta vhost TX (pasta->guest) prerequisites Message-ID: References: <20260802132155.870796-1-aerosound161@gmail.com> <20260802132155.870796-8-aerosound161@gmail.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="G87F1jl0m5ypDhL0" Content-Disposition: inline In-Reply-To: Message-ID-Hash: 25K2HPEO7SCJ7QNLS3YONPVK4GVBWVGV X-Message-ID-Hash: 25K2HPEO7SCJ7QNLS3YONPVK4GVBWVGV 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, eperezma@redhat.com 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: --G87F1jl0m5ypDhL0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Aug 12, 2026 at 09:39:33PM +0300, Ammar Yasser wrote: > On Mon Aug 10, 2026 at 12:01 PM EEST, David Gibson wrote: > > On Sun, Aug 02, 2026 at 01:21:54PM +0000, Ammar Yasser wrote: > >> - tap_send_single was changed to directly call > >> tap_send_frames_passt/pasta instead of relying on tap_send_frames > >> because protocols that use tap_send_single won't use vhost > >> acceleration, only tcp and udp will. The function was also moved low= er > >> in the file to by the time its called tap_send_frames_* functions are > >> defined and a forward declaration isn't needed > > > > Sorry, I'm not really following why the vhost change requires > > tap_send_single() to bypass tap_send_frames(). >=20 > In a previous review from you on the original work by Eugenio you said > you don't want tap_send_frames to take a vhost boolean parameter because > its a pasta only value to a function that is used by passt and pasta. > If tap_send_single relies on tap_send_frames (assuming it takes no vhost > bool), and deducing whether to use vhost or no is based on c->vhost then > arp, icmp, ndp.. and other single send protos end up using vhost as > well. which won't work since those protos don't have a concrete memory > region i can make the kernel aware of. Ah, I see. This is the key point the commit message needs to explain: some of the users of tap_send_single() *can't* use the vhost path, because of the shared buffers. > And obvioiusly doing a protocol > check inside tap_send_frames is not super cool either. >=20 > Let me know your opinion on this! >=20 > > > >> - extend udp_meta_t to instead of always carrying a tap_hdr, to carry a > >> union of a tap_hdr and a virtio_net header because if vhost is used > >> the will the latter type of header and then build an iov from this > >> virtio net header in udp.c > > > > In fact, the intended role of tap_hdr itself was to be a union of > > whatever "below L2" headers we might need for all our backends. It > > was never very obvious because there was just the vnet_len (for the > > qemu -net socket protocol) and nothing at all (for tuntap via the > > chardev). With vhost-user, in theory it should have gained a > > virtio_net_mrg_rxbuf branch, but the vhost-user paths are different > > enough that we never quite needed it. > > > > Here for vhost-kernel, though, we should add the virtio_net header > > inside tap_hdr, rather than making a union including tap_hdr. > > tap_hdr_iov() and tap_hdr_update() should be updated to handle that > > case as well. >=20 > To make sure i understand, you're saying that tap_hdr itself should be a > union of vnet_len and virtio_net_mrg_rxbuf ? Yes. > i don't disagree in > principal, (English usage nit: in this context it's "principle", not "principal") > but will need to check that frame length accounting doesn't go > wrong in any area as a consequence of this. Right, doing this will certainly need some updating in places to handle it. I don't think it should be super hard, since it was introduced with a future extension like this in mind. Note that in a sense you can already think of tap_hdr as a union between vnet_len (passt) and an empty structure (pasta). > >> +/** > >> + * tap_send_single() - Send a single frame > >> + * @c: Execution context > >> + * @data: Packet buffer > >> + * @l2len: Total L2 packet length > >> + */ > >> +void tap_send_single(const struct ctx *c, const void *data, size_t l2= len) > >> +{ > >> + uint8_t padded[ETH_ZLEN] =3D { 0 }; > >> + struct iovec iov[2]; > >> + size_t iovcnt =3D 0; > >> + int m =3D 0; > >> + uint32_t vnet_len; > >> + > >> + if (l2len < ETH_ZLEN) { > >> + memcpy(padded, data, l2len); > >> + data =3D padded; > >> + l2len =3D ETH_ZLEN; > >> + } > >> + > >> + vnet_len =3D htonl(l2len); > >> + switch (c->mode) { > >> + case MODE_PASST: > >> + /* create an iov for the length */ > >> + iov[iovcnt] =3D IOV_OF_LVALUE(vnet_len); > >> + iovcnt++; > >> + /* create the data iov */ > >> + iov[iovcnt].iov_base =3D (void *)data; > >> + iov[iovcnt].iov_len =3D l2len; > >> + iovcnt++; > >> + m =3D tap_send_frames_passt(c, iov, iovcnt, 1); > >> + break; > >> + case MODE_PASTA: > >> + /* don't create a length iov in the case of pasta */ > >> + iov[iovcnt].iov_base =3D (void *)data; > >> + iov[iovcnt].iov_len =3D l2len; > >> + iovcnt++; > >> + > >> + m =3D tap_send_frames_pasta(c, iov, iovcnt, 1, false); > > > > You explicitly disable vhost here, even if available. As I've said > > before, tap_send_single() is a slow path that doesn't really need > > vhost acceleration. However, there's also not really a reason *not* > > to use vhost unless it simplifies things. So far this seems to be > > adding (slightly) complexity to avoid using vhost, and it's not clear > > to me if there's a simplification elsewhere that makes it worth it. >=20 > As per above comment, its a correctness problem. single send protocols > won't work with vhost in the current state, thats why i went the extra > mile to avoid it. let me know if i am misunderstanding anything Right, I missed that constraint. Might be worth putting it in a comment, since it's not necessarily obvious from this point in the code. >=20 > > > >> + break; > >> + case MODE_VU: > >> + m =3D vu_send_single(c, data, l2len); > >> + break; > >> + } > >> + > >> + if (m < 1) > >> + debug("tap: failed to send a single frame"); > >> + > >> + pcap_multiple(iov, iovcnt, m, > >> + c->mode =3D=3D MODE_PASST ? sizeof(uint32_t) : > >> + (c->fd_vhost !=3D -1) ? VNET_HLEN : 0); > >> +} > >> + > >> /** > >> * tap_send_frames() - Send out multiple prepared frames > >> * @c: Execution context > >> @@ -523,7 +537,8 @@ size_t tap_send_frames(const struct ctx *c, const = struct iovec *iov, > >> =20 > >> switch (c->mode) { > >> case MODE_PASTA: > >> - m =3D tap_send_frames_pasta(c, iov, bufs_per_frame, nframes); > >> + /* use vhost in pasta sending only if the vhost setup was actually = successful, indicated by a non-zero fd_vhost */ > >> + m =3D tap_send_frames_pasta(c, iov, bufs_per_frame, nframes, ((c->f= d_vhost !=3D -1))); > >> break; > >> case MODE_PASST: > >> m =3D tap_send_frames_passt(c, iov, bufs_per_frame, nframes); > >> @@ -539,7 +554,8 @@ size_t tap_send_frames(const struct ctx *c, const = struct iovec *iov, > >> nframes - m, nframes); > >> =20 > >> pcap_multiple(iov, bufs_per_frame, m, > >> - c->mode =3D=3D MODE_PASST ? sizeof(uint32_t) : 0); > >> + c->mode =3D=3D MODE_PASST ? sizeof(uint32_t) : > >> + (c->fd_vhost !=3D -1) ? VNET_HLEN : 0); > > > > Rather than this nested ?: expression we should make a helper that > > gives the necessary length to exclude tap_hdr. tap_hdr_iov() should > > then use the same helper. >=20 > Noted. will do >=20 > =20 > >> +static inline struct iovec iov_from_virtio_net_hdr(struct virtio_net_= hdr_mrg_rxbuf *hdr) > >> +{ > >> + return (struct iovec){ > >> + .iov_base =3D hdr, > >> + .iov_len =3D sizeof(*hdr), > >> + }; > >> +} > > > > As noted, this should become a new branch of tap_hdr_iov()... > > > >> /** > >> * tcp_sock_iov_init() - Initialise scatter-gather L2 buffers for IPv= 4 sockets > >> * @c: Execution context > >> @@ -89,7 +98,14 @@ void tcp_sock_iov_init(const struct ctx *c) > >> for (i =3D 0; i < TCP_FRAMES_MEM; i++) { > >> struct iovec *iov =3D tcp_l2_iov[i]; > >> =20 > >> - iov[TCP_IOV_TAP] =3D tap_hdr_iov(c, &tcp_payload_tap_hdr[i]); > >> + /* If we are using pasta with vhost acceleration, the first entry i= n the tcp buffers > >> + * should point to a virtio_net header, otherwise a tap header=20 > >> + */ =09 > >> + if ((c->fd_vhost !=3D -1)) > >> + iov[TCP_IOV_TAP] =3D iov_from_virtio_net_hdr(&tcp_payload_tap_hdr[= i]); > >> + else=20 > >> + iov[TCP_IOV_TAP] =3D tap_hdr_iov(c, (struct tap_hdr *)&tcp_payload= _tap_hdr[i]); > > > > ..absorbing this if. >=20 > So the if statement goes into tap_hdr_iov? ok noted >=20 > > > >> iov[TCP_IOV_ETH].iov_len =3D sizeof(struct ethhdr); > >> iov[TCP_IOV_PAYLOAD].iov_base =3D &tcp_payload[i]; > >> iov[TCP_IOV_ETH_PAD].iov_base =3D eth_pad; > >> @@ -135,8 +151,7 @@ void tcp_payload_flush(const struct ctx *c, const = struct timespec *now) > >> { > >> size_t m; > >> =20 > >> - m =3D tap_send_frames(c, &tcp_l2_iov[0][0], TCP_NUM_IOVS, > >> - tcp_payload_used); > >> + m =3D tap_send_frames(c, &tcp_l2_iov[0][0], TCP_NUM_IOVS, tcp_payloa= d_used); > >> if (m !=3D tcp_payload_used) { > >> tcp_revert_seq(c, &tcp_frame_conns[m], &tcp_l2_iov[m], > >> tcp_payload_used - m, now); > >> @@ -177,7 +192,7 @@ static void tcp_l2_buf_fill_headers(const struct c= tx *c, > >> { > >> struct iov_tail tail =3D IOV_TAIL(&iov[TCP_IOV_PAYLOAD], 1, 0); > >> struct tcphdr th_storage, *th =3D IOV_REMOVE_HEADER(&tail, th_storag= e); > >> - struct tap_hdr *taph =3D iov[TCP_IOV_TAP].iov_base; > >> + > >> const struct flowside *tapside =3D TAPFLOW(conn); > >> const struct in_addr *a4 =3D inany_v4(&tapside->oaddr); > >> struct ethhdr *eh =3D iov[TCP_IOV_ETH].iov_base; > >> @@ -192,7 +207,19 @@ static void tcp_l2_buf_fill_headers(const struct = ctx *c, > >> =20 > >> l2len =3D tcp_fill_headers(c, conn, eh, ip4h, ip6h, th, &tail, > >> iov_tail_size(&tail), csum_flags, seq); > >> - tap_hdr_update(taph, l2len); > >> + > >> + /* when in pasta mode, the length of the tap header iov is zero, so = this > >> + * l2len write doesn't do anything. But this function gets called fr= om both > >> + * pasta and passt. Make the l2len write in case we are in passt mod= e, denoted by > >> + * fd_vhost being -1 since this field won't get initialized at all i= n passt. > >> + */ > >> + if (c->mode =3D=3D MODE_PASST) { > >> + iov[TCP_IOV_TAP].iov_len =3D sizeof(struct tap_hdr); > >> + struct tap_hdr *taph =3D iov[TCP_IOV_TAP].iov_base; > >> + tap_hdr_update(taph, l2len); > >> + } else if (c->fd_vhost =3D=3D -1) { /* pasta mode but without vhost = */ > >> + iov[TCP_IOV_TAP].iov_len =3D 0; > >> + } > > > > Similarly this if should go inside tap_hdr_update(). > > > >> } > >> =20 > >> /** > >> diff --git a/tcp_buf.h b/tcp_buf.h > >> index c749038..da48cd0 100644 > >> --- a/tcp_buf.h > >> +++ b/tcp_buf.h > >> @@ -9,7 +9,7 @@ > >> #include "tcp_conn.h" > >> #include "tcp_internal.h" > >> =20 > >> -void tcp_sock_iov_init(); > >> +void tcp_sock_iov_init(const struct ctx *c); > >> void tcp_payload_flush(const struct ctx *c, const struct timespec *no= w); > >> int tcp_buf_data_from_sock(const struct ctx *c, struct tcp_tap_conn *= conn, > >> uint32_t already_sent, const struct timespec *now); > >> @@ -20,7 +20,7 @@ int tcp_buf_send_flag(const struct ctx *c, struct tc= p_tap_conn *conn, int flags, > >> #define TCP_FRAMES \ > >> (c->mode =3D=3D MODE_PASTA ? 1 : TCP_FRAMES_MEM) > >> =20 > >> -extern struct tap_hdr tcp_payload_tap_hdr[TCP_FRAMES_MEM]; > >> +extern struct virtio_net_hdr_mrg_rxbuf tcp_payload_tap_hdr[TCP_FRAMES= _MEM]; > >> extern struct ethhdr tcp_eth_hdr[TCP_FRAMES_MEM]; > >> extern struct tcp_payload_t tcp_payload[TCP_FRAMES_MEM]; > >> =20 > >> diff --git a/udp.c b/udp.c > >> index d05ee66..5f90dce 100644 > >> --- a/udp.c > >> +++ b/udp.c > >> @@ -103,6 +103,7 @@ > >> #include > >> #include > >> #include > >> +#include > >> =20 > >> #include "checksum.h" > >> #include "util.h" > >> @@ -119,6 +120,18 @@ > >> #include "udp_vu.h" > >> #include "epoll_ctl.h" > >> =20 > >> +/* UDP header and data for inbound messages */ > >> +struct udp_payload_t udp_payload[UDP_MAX_FRAMES]; > >> + > >> +/* Ethernet headers for IPv4 and IPv6 frames */ > >> +struct ethhdr udp_eth_hdr[UDP_MAX_FRAMES]; > >> + > >> +/* IOVs and msghdr arrays for receiving datagrams from sockets */ > >> +struct iovec udp_iov_recv [UDP_MAX_FRAMES]; > >> +struct mmsghdr udp_mh_recv [UDP_MAX_FRAMES]; > >> + > >> +/* Pre-cooked headers for UDP packets */ > >> +struct udp_meta_t udp_meta[UDP_MAX_FRAMES]; > > > > I'm slightly confused. I see these added, but I don't see the > > existing static declarations removed. It's also not clear to me why > > these need to become non-static. >=20 > The static declarations were removed in the second patch. This addition > is misplaced, will fix. Ok. > But yeah they do need to be non static because of the need to share them > with the kernel in virtio.c Does that mean the *only* reference in virtio.c is just to get their address/size to put into the shared memory table? If that's the case delegating to a helper within udp.c to register them into the table seems like a better approach. > >> #define UDP_TIMEOUT "/proc/sys/net/netfilter/nf_conntrack_udp_timeout" > >> #define UDP_TIMEOUT_STREAM \ > >> @@ -216,7 +229,20 @@ static void udp_iov_init_one(const struct ctx *c,= size_t i) > >> *siov =3D IOV_OF_LVALUE(payload->data); > >> =20 > >> tiov[UDP_IOV_ETH] =3D IOV_OF_LVALUE(udp_eth_hdr[i]); > >> - tiov[UDP_IOV_TAP] =3D tap_hdr_iov(c, &meta->taph); > >> + /* if the vhost tap fd is initialized, this is a sign for us that > >> + * we will be using virtio transport. make the iov that is supposed > >> + * to point to a tap header point instead to a virtio_net_mrg_rxbuf > >> + */ > >> + if (c->fd_vhost !=3D -1) { > >> + struct iovec vnet_iov =3D { > >> + .iov_base =3D (void *)(&meta->vnet_hdr), > >> + .iov_len =3D sizeof(meta->vnet_hdr) > >> + }; > >> + tiov[UDP_IOV_TAP] =3D vnet_iov; > >> + } else { > >> + tiov[UDP_IOV_TAP] =3D tap_hdr_iov(c, &meta->taph); > >> + } > > > > Again, this logic should go inside tap_hdr_iov(). >=20 > Got it. >=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 --G87F1jl0m5ypDhL0 Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmp9JYkACgkQzQJF27ox 2GeQiw/9Hsi+6hdqsRX7ihy9KYXLBn0gHXZG+3O5mm3AcpxloTJ2QzPcnJkMSVCH /bHfi8OIQp0NL3thwB5qtb/Azmn56kDnvAWA29mnlrlxrF/MMhkerCJ2fZrGpaf+ xPYel1hP2bKIFM+h8GVDlnTtZl6ZAQG8YVp0JIx+/siwJgeNo4F6DImIJfqno+P7 owCW2r2P594unUZejT+CPS9qNONQfHdxPR3fvJUJJjzTuJwcW0jsClsZ2VuZe3XE IqcvTfY9nIZWHxPirPtENsYERJPKmhwJbtaefLU9Fh4p+Hv8VtcpWf/gSMd9Qu1Q KL2J4qrds7xiyo2jAbT5CXFX1H6jmW8e77dz50SwEvOP++o9FHvOJ2rp9+j+g2Oi Ka156heYA4ChZKMmcYwQ+VQcCZDduTt9sp1oSGcuuBWHvQ7LbgE1Lcg50T7azpQY oEffKXGgkh90oTDBRBKNYDMsUfJAmEqHAOHOfXgZo51D2xr7KF6A4+6iHU9JSjKD InlrczZd74jk99JDzUhauk3/hpeAOTkZVlkD21lKA5PBibhQJR5bRc/7eA6e3IOH kdE2nojJosA/FenxI8Ywc3DotB0/hsy9OuT9kcJ6AuphHCIfrNMsK9fQ+T6fK0Hw Jr1FdApjJ/SREEbQiejY/dLais4nXQrH6wWj5rdBXk1hPPl5tGo= =No4C -----END PGP SIGNATURE----- --G87F1jl0m5ypDhL0--