Re: [RFC v3 6/8] virtio: Implement pasta vhost acceleration guest->pasta path

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

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

On Wed, Aug 12, 2026 at 09:17:30PM +0300, Ammar Yasser wrote:
> On Mon Aug 10, 2026 at 10:34 AM EEST, David Gibson wrote:
> > On Sun, Aug 02, 2026 at 01:21:53PM +0000, Ammar Yasser wrote:
> >> Add a new function called tap_vhost_input that will be used to indicate
> >> that the guest is trying to send data to us through the rx queue and the
> >> kernel is informing us to handle this data.
> >> 
> >> We will however still recieve epoll events on the fd normally. We should
> >
> > "the fd" is not very clear here, there are a bunch of fds.  I'm
> > guessing you mean the character device fd?
> 
> Yeah. And tying it back to your comment about the fact that we
> shouldn't call epoll add with the device fd if the user requested vhost
> means that this comment will be redundant anyways, will delete it.
> 
> For the record, while debugging performance, the fact that we received a
> double kick on the device fd and the queue fd was a major slow down
> reason. I did remove epoll registration for the device in this
> experimental branch. Will include it in my next revision.

Oh, excellent :).

>   
> >> @@ -1359,7 +1362,8 @@ void tap_handler_pasta(struct ctx *c, uint32_t events,
> >>  	if (events & (EPOLLRDHUP | EPOLLHUP | EPOLLERR))
> >>  		die("Disconnect event on /dev/net/tun device, exiting");
> >>  
> >> -	if (events & EPOLLIN)
> >> +	/* don't proceed with the normal tap processing in case vhost acceleration was required */
> >> +	if (events & EPOLLIN && !c->vhost)
> >
> > Seems like it would be more elegant to remove the character device fd
> > from the epoll set (or at least remove EPOLLIN) when vhost is
> > activated, rather than take it's events then ignore the.
> 
> Yeah, agreed as per my comment above.
> 
> >
> >>  		tap_pasta_input(c, now);
> >>  }
> >>  
> >> @@ -1514,6 +1518,90 @@ void tap_listen_handler(struct ctx *c, uint32_t events)
> >>  	tap_start_connection(c);
> >>  }
> >>  
> >> +/**
> >> + * consume_one_rx_descriptor() - Consume one used RX descriptor from the kernel
> >
> > I'd suggest avoiding "Rx" or "Tx" terminology throughout, using
> > "toguest" or "fromguest" instead.  Note that for extra confusion Rx
> > here is meaning "fromguest", but for vhost-user it means "toguest".
> 
> Noted
> 
> >
> >> + * @len:	Set to the length of data written by the kernel
> >> + *
> >> + * Pops a single entry from the used ring. Advances vqs[0].last_used_idx
> >> + * (the number of entries we have consumed) and vqs[0].num_free (the count
> >> + * of descriptors awaiting refill announcement).
> >> + * 
> >> + * NOTE: This function assumes the kernel is going to post single descriptors
> >> + * always, No chains. If that changes, we would need to increment num_free
> >> + * as we advertise back to the kernel the free descriptors by the length of the chain.
> >> + *
> >> + * Return: pointer to the packet buffer, or NULL if no data is available
> >> + */
> >> +static void *consume_one_rx_descriptor(unsigned *len)
> >
> > For a length of data, we'd typically use a size_t rather than a bare
> > unsigned.
> 
> Noted
> 
> >> +	/* read the last index we consumed */
> >> +	last_used = vqs[0].last_used_idx % VHOST_NDESCS;
> >> +	/* read the index of what the */
> >> +	i = le32toh(used->ring[last_used].id);
> >> +	*len = le32toh(used->ring[last_used].len);
> >> +
> >> +	if (i != last_used) {
> >> +		die("vhost: id %u at used position %u != %u", i, last_used, i);
> >> +	}
> >
> > Braces not necessary for single line if blocks.
> 
> Ok
> 
> >
> >> +
> >> +	/* the kernel has queued for us something we cannot receive */
> >> +	if (*len > PKT_BUF_BYTES/VHOST_NDESCS) {
> >
> > This seems oddly circular: deriving the maximum frame size from the
> > buffer and number of descriptors, when the number of descriptors was
> > derived from the buffer size and maximum frame size.  Better to use
> > (or add) an L2_MAX_LEN_XX constant.
> 
> Will do
> 
> >
> >> +		die("vhost: id %d len %u > %zu", i, *len, PKT_BUF_BYTES/VHOST_NDESCS);
> >> +	}
> >> +
> >> +	vqs[0].last_used_idx++;
> >> +	vqs[0].num_free++;
> >> +	return pkt_buf + i * (PKT_BUF_BYTES/VHOST_NDESCS);
> >> +}
> >> +
> >> +
> >> +/**
> >> + * tap_vhost_input() - Handler for new data on the tun socket to hypervisor vq
> >> + * @c:		Execution context
> >> + * @ref:	epoll reference
> >> + * @now:	Current timestamp
> >> + */
> >> +void tap_vhost_input(struct ctx *c, union epoll_ref ref, const struct timespec *now)
> >> +{
> >> +	eventfd_read(ref.fd, (eventfd_t[]){ 0 });
> >> +
> >> +	tap_flush_pools();
> >> +
> >> +	struct virtio_net_hdr_mrg_rxbuf *hdr;
> >> +	struct iov_tail data;
> >> +	unsigned len;
> >
> > My convention, we don't use inline declarations, even though we could
> > in C11 - move these up to the top.
> >
> >> +
> >> +	while ((hdr = consume_one_rx_descriptor(&len))) {
> >> +		if (len < sizeof(*hdr)) {
> >
> > I think you want to check that at least an ethernet (L2) header is
> > there as well as the virtio_net_hdr_mrg_rxbuf: both tap_pasta_input()
> > and tap_passt_input() appear to check for this.
> 
> Sure, i can add that. But just for my knowledge is there any case where
> there can be a virtio_net header without an ethernet header ?

Maybe.  It might be possible to pass in tap device in with --fd, but
have it misconfigured as a "tun" device (raw IP) instead of "tap"
(virtual ethernet).  Not sure if that's currently possible to combine
with vhost-kernel, but it's close enough to possible that it makes
sense to be cautious.  I'm also not sure what would happen if the
guest used an AF_PACKET socket to send malformed Ethernet frames.
There's a good chance it would get filtered before reaching us, but
still, better to check.

> 
> >
> >> +			warn("vhost: invalid len %u", len);
> >> +			continue;
> >> +		}
> >> +		
> >> +		/* skip over the vnet header, we wanna add the packet without it*/
> >> +		data = IOV_TAIL_FROM_BUF((void *)(hdr+1), len - sizeof(*hdr), 0);
> >> +		tap_add_packet(c, &data, now);
> >> +	}
> >> +
> >> +	tap_handler(c, now);
> >> +	rx_descriptor_handoff(c);
> >> +}
> >> +
> >>  /**
> >>   * tap_ns_tun() - Get tuntap fd in namespace
> >>   * @c:		Execution context
> >> @@ -1524,16 +1612,15 @@ void tap_listen_handler(struct ctx *c, uint32_t events)
> >>   */
> >>  static int tap_ns_tun(void *arg)
> >>  {
> >> -	struct ifreq ifr = { .ifr_flags = IFF_TAP | IFF_NO_PI };
> >> -	int flags = O_RDWR | O_NONBLOCK | O_CLOEXEC;
> >>  	struct ctx *c = (struct ctx *)arg;
> >> +	struct ifreq ifr = { .ifr_flags = IFF_TAP | IFF_NO_PI };
> >>  	int fd, rc;
> >>  
> >>  	c->fd_tap = -1;
> >>  	memcpy(ifr.ifr_name, c->pasta_ifn, IFNAMSIZ);
> >>  	ns_enter(c);
> >>  
> >> -	fd = open("/dev/net/tun", flags);
> >> +	fd = open("/dev/net/tun", O_RDWR | O_NONBLOCK | O_CLOEXEC);
> >
> > This change seems unrelated to the actual aim of this patch.
> 
> Woops, removing
> 
> >
> >>  	if (fd < 0)
> >>  		die_perror("Failed to open() /dev/net/tun");
> >>  
> >> @@ -1561,6 +1648,20 @@ static void tap_sock_tun_init(struct ctx *c)
> >>  			die("Failed to set up tap device in namespace");
> >>  	}
> >>  
> >> +	/* initialize the vhost-net dev file descriptor */
> >> +	if (c->vhost) {
> >> +		setup_vhost_net(c);
> >> +
> >> +		for (int i = 0; i < ARRAY_SIZE(c->vq); i++)
> >> +			setup_eventfds(c, i);
> >> +
> >> +		if (setup_memory_table(c) < 0)
> >> +			die_perror("VHOST_SET_MEM_TABLE ioctl on /dev/vhost-net failed");
> >> +
> >> +		for (int i = 0; i < ARRAY_SIZE(c->vq); i++)
> >> +			set_vring_for_queue(c, i, c->fd_tap);
> >> +	}
> >
> > It's not clear to me if this code block needs to execute within the
> > guest namespace.  If not, it should probably be in tap_sock_tun_init()
> > rather than tap_ns_tun().  If it _does_ have to be in tap_ns_tun(),
> > then tap_ns_tun() probably needs a rename, since it's now doing rather
> > more in the vhost case that just getting a device fd.
> 
> Not sure i follow. its already in tap_sock_tun_init not tap_ns_tun.
> But generally speaking it needs to be after tap_ns_tun. because it
> relies on the device fd that tap_ns_tun has created in the namespace

Oh, sorry, I misread the diff.

> >> +
> >>  	pasta_ns_conf(c);
> >>  
> >>  	if (!c->splice_only)
> >> diff --git a/tap.h b/tap.h
> >> index 1625975..eb02da8 100644
> >> --- a/tap.h
> >> +++ b/tap.h
> >> @@ -66,6 +66,7 @@ static inline void tap_hdr_update(struct tap_hdr *thdr, size_t l2len)
> >>  	thdr->vnet_len = htonl(l2len);
> >>  }
> >>  
> >> +void tap_vhost_input(struct ctx *c, union epoll_ref ref, const struct timespec *now);
> >>  unsigned long tap_l2_max_len(const struct ctx *c);
> >>  void *tap_push_l2h(const struct ctx *c, void *buf,
> >>  		   const void *src_mac, uint16_t proto);
> >> -- 
> >> 2.34.1
> >> 
> 

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