Re: [PATCH v2 6/7] tap: Implement the pasta vhost-net to-guest path

[Laurent Vivier <lvivier@redhat.com>]

On 10/16/26 22:49, aerosouund wrote:
> From: Ammar Yasser <aerosound161@gmail.com>
> 
> Add a vhost argument to tap_send_frames_pasta(). Callers pass it to say
> whether this send should go through vhost-net, and when it does sending
> goes through tap_send_frames_vhost(), which takes a descriptor from the
> queue shared with the kernel for each iov and points it at that iov's
> base address, chaining the bufs_per_frame descriptors of a frame
> together so the guest receives it in one go.
> 
> tx_reap() reclaims the descriptors the kernel has finished with, walking
> each used chain to its end so that every descriptor in it is counted as
> free again.
> 
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> Signed-off-by: Ammar Yasser <aerosound161@gmail.com>
> ---
>   tap.c | 150 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 147 insertions(+), 3 deletions(-)
> 
> diff --git a/tap.c b/tap.c
> index fd500f1..bc2700f 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -359,12 +359,151 @@ void tap_icmp6_send(const struct ctx *c,
>   	tap_send_single(c, buf, l4len + ((char *)icmp6h - buf));
>   }
> 
> +/**
> + * tx_reap() - Reclaim the descriptors the kernel has already processed
> + */
> +static void tx_reap(void)
> +{
> +	struct vring_used_pasta *used = &vring_used_all[VHOST_USED_Q_IDX];
> +	uint16_t used_idx = le16toh(used->idx);
> +
> +	smp_rmb();
> +
> +	/* increment last_used_idx until it reaches the kernel's used index */
> +	while (vhost_vq_state[VHOST_USED_Q_IDX].last_used_idx != used_idx) {
> +		uint16_t last_used;
> +		uint32_t desc_id;
> +
> +		last_used = vhost_vq_state[VHOST_USED_Q_IDX].last_used_idx % VHOST_NDESCS;
> +		desc_id = le32toh(used->ring[last_used].id);
> +
> +		for (;;) {

We should check we don't have a circular chain rather than allowing an infinite loop

> +			/* keep going until we find a descriptor without the
> +			 * next flag
> +			 */
> +			vhost_vq_state[VHOST_USED_Q_IDX].num_free++;

Check desc_id < VHOST_NDESCS

> +			if (!(le16toh(vring_desc[VHOST_USED_Q_IDX][desc_id].flags) &
> +			      VRING_DESC_F_NEXT))
> +				break;
> +
> +			/* this descriptor wasn't the last, set desc_id to the
> +			 * next one and keep going
> +			 */
> +			desc_id = le32toh(vring_desc[VHOST_USED_Q_IDX][desc_id].next);

.next is __virtio16(), le16toh().

> +		}
> +
> +		vhost_vq_state[VHOST_USED_Q_IDX].last_used_idx++;
> +	}
> +}
> +
> +/**
> + * tap_send_frames_vhost() - Send multiple frames to the pasta tap
> + * @c:			Execution context
> + * @iov:		Array of buffers
> + * @bufs_per_frame:	Number of buffers (iovec entries) per frame
> + * @nframes:		Number of frames to send
> + *
> + * @iov must have total length @bufs_per_frame * @nframes, with each set of
> + * @bufs_per_frame contiguous buffers representing a single frame.
> + *
> + * Return: number of frames successfully sent
> + */
> +static size_t tap_send_frames_vhost(const struct ctx *c,
> +				    const struct iovec *iov,
> +				    size_t bufs_per_frame, size_t nframes)
> +{
> +	struct vring_avail_pasta *avail =
> +		&vring_avail_all[VHOST_USED_Q_IDX];
> +	size_t processed_frames = 0;
> +	size_t i;
> +
> +	/* reclaim descriptors if we don't have enough available buffers to
> +	 * perform this send
> +	 */
> +	if (vhost_vq_state[VHOST_USED_Q_IDX].num_free < bufs_per_frame * nframes)
> +		tx_reap();
> +
> +	for (i = 0; i < nframes; i++) {
> +		uint16_t head;
> +		size_t j;
> +
> +		/* it's likely that tx_reap returned to us less than
> +		 * bufs_per_frame descs
> +		 */
> +		if (vhost_vq_state[VHOST_USED_Q_IDX].num_free < bufs_per_frame)

Perhaps you can add a log message here.

> +			break;
> +
> +		/* set the index of the avail ring in the tx queue to be our
> +		 * last_used_idx
> +		 */
> +		head = vhost_vq_state[VHOST_USED_Q_IDX].next_free % VHOST_NDESCS;
> +		avail->ring[(avail->idx + i) % VHOST_NDESCS] = htole16(head);
> +
> +		/* we will be consuming bufs_per_frame descriptors for every
> +		 * frame, decrement the local num_free
> +		 */
> +		vhost_vq_state[VHOST_USED_Q_IDX].num_free -= bufs_per_frame;
> +
> +		for (j = 0; j < bufs_per_frame; ++j) {
> +			uint16_t next =
> +				vhost_vq_state[VHOST_USED_Q_IDX].next_free %
> +				VHOST_NDESCS;
> +			/* get the last_used_idx descriptor */
> +			struct vring_desc_pasta *desc =
> +				&vring_desc[VHOST_USED_Q_IDX][next];
> +			const struct iovec *iov_i;
> +
> +			/* the iov variable contains the iovecs for all frames
> +			 * we will send. access a single fragment of a frame
> +			 * (each fragment is one of tcp_iov_parts) denoted by
> +			 * iov at index i (index of the frame being processed)
> +			 * * bufs_per_frame plus j (the index of the fragment
> +			 * being processed)
> +			 */
> +			iov_i = &iov[i * bufs_per_frame + j];
> +
> +			/* set that descriptor's address to the base of the iov
> +			 * and set the VRING_DESC_F_NEXT flag on the descriptor
> +			 * if its not the last frame fragment, so that the
> +			 * guest would recieve the entire frame in one go.
> +			 */

I think you should skip the iov when iov_len is 0. I can happen with UDP_IOV_ETH_PAD.
> +			desc->addr = (uint64_t)iov_i->iov_base;

We need htole64()

> +			desc->len = iov_i->iov_len;

htole32()

> +			desc->flags = (j == bufs_per_frame - 1) ?
> +				      0 : htole16(VRING_DESC_F_NEXT);
> +
> +			vhost_vq_state[VHOST_USED_Q_IDX].next_free++;
> +		}
> +
> +		processed_frames++;
> +	}
> +
> +	/* we didn't process any frames, no need to notify the kernel */
> +	if (!processed_frames)
> +		return 0;
> +
> +	smp_wmb();
> +
> +	/* we will have used nframes descriptor chains */
> +	avail->idx = htole16(le16toh(avail->idx) + processed_frames);

The value to increment is not processed_frames but the number of desc that have been consumed.
> +
> +	vhost_kick(&vring_used_all[VHOST_USED_Q_IDX],
> +		   c->vhost.vq[VHOST_USED_Q_IDX].kick_fd);
> +
> +	/* wait until the kernel finishes processing this send */
> +	while (avail->idx != vring_used_all[1].used.idx)
> +		;
> +
> +	return processed_frames;
> +}
> +
>   /**
>    * tap_send_frames_pasta() - Send multiple frames to the pasta tap
>    * @c:			Execution context
>    * @iov:		Array of buffers
>    * @bufs_per_frame:	Number of buffers (iovec entries) per frame
>    * @nframes:		Number of frames to send
> + * @vhost:		Send through vhost-net rather than writing to the tap
>    *
>    * @iov must have total length @bufs_per_frame * @nframes, with each set of
>    * @bufs_per_frame contiguous buffers representing a single frame.
> @@ -375,11 +514,15 @@ void tap_icmp6_send(const struct ctx *c,
>    */
>   static size_t tap_send_frames_pasta(const struct ctx *c,
>   				    const struct iovec *iov,
> -				    size_t bufs_per_frame, size_t nframes)
> +				    size_t bufs_per_frame, size_t nframes,
> +				    bool vhost)
>   {
>   	size_t nbufs = bufs_per_frame * nframes;
>   	size_t i;
> 
> +	if (vhost)
> +		return tap_send_frames_vhost(c, iov, bufs_per_frame, nframes);
> +
>   	for (i = 0; i < nbufs; i += bufs_per_frame) {
>   		ssize_t rc = writev(c->fd_tap, iov + i, bufs_per_frame);
>   		size_t framelen = iov_size(iov + i, bufs_per_frame);
> @@ -515,7 +658,7 @@ void tap_send_single(const struct ctx *c, const void *data, size_t l2len)
>   		iov[iovcnt].iov_len = l2len;
>   		iovcnt++;
> 
> -		m = tap_send_frames_pasta(c, iov, iovcnt, 1);
> +		m = tap_send_frames_pasta(c, iov, iovcnt, 1, false);
>   		break;
>   	case MODE_VU:
>   		vu_send_single(c, data, l2len);
> @@ -553,7 +696,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);
> +		m = tap_send_frames_pasta(c, iov, bufs_per_frame, nframes,
> +					  c->vhost.fd != -1);
>   		break;
>   	case MODE_PASST:
>   		m = tap_send_frames_passt(c, iov, bufs_per_frame, nframes);
> --
> 2.39.5 (Apple Git-154)