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

["Ammar Yasser" <aerosound161@gmail.com>]

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.
  
>> @@ -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 ?

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

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