Re: [RFC v3 7/8] pasta: Implement pasta vhost TX (pasta->guest) prerequisites

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 13966 bytes --]

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 lower
> >>   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().
> 
> 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.
> 
> Let me know your opinion on this!
> 
> >
> >> - 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.
> 
> 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 l2len)
> >> +{
> >> +	uint8_t padded[ETH_ZLEN] = { 0 };
> >> +	struct iovec iov[2];
> >> +	size_t iovcnt = 0;
> >> +	int m = 0;
> >> +	uint32_t vnet_len;
> >> +
> >> +	if (l2len < ETH_ZLEN) {
> >> +		memcpy(padded, data, l2len);
> >> +		data = padded;
> >> +		l2len = ETH_ZLEN;
> >> +	}
> >> +
> >> +	vnet_len = htonl(l2len);
> >> +	switch (c->mode) {
> >> +	case MODE_PASST:
> >> +		/* create an iov for the length */
> >> +		iov[iovcnt] = IOV_OF_LVALUE(vnet_len);
> >> +		iovcnt++;
> >> +		/* create the data iov */
> >> +		iov[iovcnt].iov_base = (void *)data;
> >> +		iov[iovcnt].iov_len = l2len;
> >> +		iovcnt++;
> >> +		m = 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 = (void *)data;
> >> +		iov[iovcnt].iov_len = l2len;
> >> +		iovcnt++;
> >> +
> >> +		m = 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.
> 
> 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.

> 
> >
> >> +		break;
> >> +	case MODE_VU:
> >> +		m = 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 == MODE_PASST ? sizeof(uint32_t) :
> >> +	    (c->fd_vhost != -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,
> >>  
> >>  	switch (c->mode) {
> >>  	case MODE_PASTA:
> >> -		m = 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 = tap_send_frames_pasta(c, iov, bufs_per_frame, nframes, ((c->fd_vhost != -1)));
> >>  		break;
> >>  	case MODE_PASST:
> >>  		m = 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);
> >>  
> >>  	pcap_multiple(iov, bufs_per_frame, m,
> >> -		      c->mode == MODE_PASST ? sizeof(uint32_t) : 0);
> >> +	    c->mode == MODE_PASST ? sizeof(uint32_t) :
> >> +	    (c->fd_vhost != -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.
> 
> Noted. will do
> 
>  
> >> +static inline struct iovec iov_from_virtio_net_hdr(struct virtio_net_hdr_mrg_rxbuf *hdr)
> >> +{
> >> +    return (struct iovec){
> >> +            .iov_base = hdr,
> >> +            .iov_len = sizeof(*hdr),
> >> +    };
> >> +}
> >
> > As noted, this should become a new branch of tap_hdr_iov()...
> >
> >>  /**
> >>   * tcp_sock_iov_init() - Initialise scatter-gather L2 buffers for IPv4 sockets
> >>   * @c:		Execution context
> >> @@ -89,7 +98,14 @@ void tcp_sock_iov_init(const struct ctx *c)
> >>  	for (i = 0; i < TCP_FRAMES_MEM; i++) {
> >>  		struct iovec *iov = tcp_l2_iov[i];
> >>  
> >> -		iov[TCP_IOV_TAP] = tap_hdr_iov(c, &tcp_payload_tap_hdr[i]);
> >> +		/* If we are using pasta with vhost acceleration, the first entry in the tcp buffers
> >> +		 * should point to a virtio_net header, otherwise a tap header 
> >> +		 */		
> >> +		if ((c->fd_vhost != -1))
> >> +			iov[TCP_IOV_TAP] = iov_from_virtio_net_hdr(&tcp_payload_tap_hdr[i]);
> >> +		else 
> >> +			iov[TCP_IOV_TAP] = tap_hdr_iov(c, (struct tap_hdr *)&tcp_payload_tap_hdr[i]);
> >
> > ..absorbing this if.
> 
> So the if statement goes into tap_hdr_iov? ok noted
> 
> >
> >>  		iov[TCP_IOV_ETH].iov_len = sizeof(struct ethhdr);
> >>  		iov[TCP_IOV_PAYLOAD].iov_base = &tcp_payload[i];
> >>  		iov[TCP_IOV_ETH_PAD].iov_base = eth_pad;
> >> @@ -135,8 +151,7 @@ void tcp_payload_flush(const struct ctx *c, const struct timespec *now)
> >>  {
> >>  	size_t m;
> >>  
> >> -	m = tap_send_frames(c, &tcp_l2_iov[0][0], TCP_NUM_IOVS,
> >> -			    tcp_payload_used);
> >> +	m = tap_send_frames(c, &tcp_l2_iov[0][0], TCP_NUM_IOVS, tcp_payload_used);
> >>  	if (m != 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 ctx *c,
> >>  {
> >>  	struct iov_tail tail = IOV_TAIL(&iov[TCP_IOV_PAYLOAD], 1, 0);
> >>  	struct tcphdr th_storage, *th = IOV_REMOVE_HEADER(&tail, th_storage);
> >> -	struct tap_hdr *taph = iov[TCP_IOV_TAP].iov_base;
> >> +
> >>  	const struct flowside *tapside = TAPFLOW(conn);
> >>  	const struct in_addr *a4 = inany_v4(&tapside->oaddr);
> >>  	struct ethhdr *eh = iov[TCP_IOV_ETH].iov_base;
> >> @@ -192,7 +207,19 @@ static void tcp_l2_buf_fill_headers(const struct ctx *c,
> >>  
> >>  	l2len = 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 from both
> >> +	 * pasta and passt. Make the l2len write in case we are in passt mode, denoted by
> >> +	 * fd_vhost being -1 since this field won't get initialized at all in passt.
> >> +	 */
> >> +	if (c->mode == MODE_PASST) {
> >> +		iov[TCP_IOV_TAP].iov_len = sizeof(struct tap_hdr);
> >> +		struct tap_hdr *taph = iov[TCP_IOV_TAP].iov_base;
> >> +		tap_hdr_update(taph, l2len);
> >> +	} else if (c->fd_vhost == -1) { /* pasta mode but without vhost */
> >> +		iov[TCP_IOV_TAP].iov_len = 0;
> >> +	}
> >
> > Similarly this if should go inside tap_hdr_update().
> >
> >>  }
> >>  
> >>  /**
> >> 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"
> >>  
> >> -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 *now);
> >>  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 tcp_tap_conn *conn, int flags,
> >>  #define TCP_FRAMES							   \
> >>  (c->mode == MODE_PASTA ? 1 : TCP_FRAMES_MEM)
> >>  
> >> -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];
> >>  
> >> diff --git a/udp.c b/udp.c
> >> index d05ee66..5f90dce 100644
> >> --- a/udp.c
> >> +++ b/udp.c
> >> @@ -103,6 +103,7 @@
> >>  #include <time.h>
> >>  #include <arpa/inet.h>
> >>  #include <linux/errqueue.h>
> >> +#include <linux/virtio_net.h>
> >>  
> >>  #include "checksum.h"
> >>  #include "util.h"
> >> @@ -119,6 +120,18 @@
> >>  #include "udp_vu.h"
> >>  #include "epoll_ctl.h"
> >>  
> >> +/* 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.
> 
> 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 = IOV_OF_LVALUE(payload->data);
> >>  
> >>  	tiov[UDP_IOV_ETH] = IOV_OF_LVALUE(udp_eth_hdr[i]);
> >> -	tiov[UDP_IOV_TAP] = 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 != -1) {
> >> +		struct iovec vnet_iov = {
> >> +			.iov_base = (void *)(&meta->vnet_hdr),
> >> +			.iov_len = sizeof(meta->vnet_hdr)
> >> +		};
> >> +		tiov[UDP_IOV_TAP] = vnet_iov;
> >> +	} else {
> >> +		tiov[UDP_IOV_TAP] = tap_hdr_iov(c, &meta->taph);
> >> +	}
> >
> > Again, this logic should go inside tap_hdr_iov().
> 
> Got it.
> 

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]