Re: [PATCH v2 3/3] vu_common: Move iovec management into vu_collect()

[Stefano Brivio <sbrivio@redhat.com>]

On Fri, 13 Mar 2026 19:26:18 +0100
Laurent Vivier <lvivier@redhat.com> wrote:

> Previously, callers had to pre-initialize virtqueue elements with iovec
> entries using vu_set_element() or vu_init_elem() before calling
> vu_collect().  This meant each element owned a fixed, pre-assigned iovec
> slot.
> 
> Move the iovec array into vu_collect() as explicit parameters (in_sg,
> max_in_sg, and in_num), letting it pass the remaining iovec capacity
> directly to vu_queue_pop().  A running current_iov counter tracks
> consumed entries across elements, so multiple elements share a single
> iovec pool.  The optional in_num output parameter reports how many iovec
> entries were consumed, allowing callers to track usage across multiple
> vu_collect() calls.
> 
> This removes vu_set_element() and vu_init_elem() which are no longer
> needed, and is a prerequisite for multi-buffer support where a single
> virtqueue element can use more than one iovec entry.  For now, callers
> assert the current single-iovec-per-element invariant until they are
> updated to handle multiple iovecs.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

This looks fine to me other than the comment about @in_num comment
(not sure if it makes sense) and pending clarification with David
regarding that argument to vu_collect(), so once that's sorted, if you
don't want further changes, I would apply it like it is. But I have a
question, just to be sure:

> ---
>  tcp_vu.c    | 25 +++++++++++---------
>  udp_vu.c    | 21 ++++++++++-------
>  vu_common.c | 68 ++++++++++++++++++++++++-----------------------------
>  vu_common.h | 22 +++--------------
>  4 files changed, 60 insertions(+), 76 deletions(-)
> 
> diff --git a/tcp_vu.c b/tcp_vu.c
> index fd734e857b3b..d470ab54bcea 100644
> --- a/tcp_vu.c
> +++ b/tcp_vu.c
> @@ -87,13 +87,13 @@ int tcp_vu_send_flag(const struct ctx *c, struct tcp_tap_conn *conn, int flags)
>  
>  	hdrlen = tcp_vu_hdrlen(CONN_V6(conn));
>  
> -	vu_set_element(&flags_elem[0], NULL, &flags_iov[0]);
> -
>  	elem_cnt = vu_collect(vdev, vq, &flags_elem[0], 1,
> +			      &flags_iov[0], 1, NULL,
>  			      MAX(hdrlen + sizeof(*opts), ETH_ZLEN + VNET_HLEN), NULL);
>  	if (elem_cnt != 1)
>  		return -1;
>  
> +	ASSERT(flags_elem[0].in_num == 1);
>  	ASSERT(flags_elem[0].in_sg[0].iov_len >=
>  	       MAX(hdrlen + sizeof(*opts), ETH_ZLEN + VNET_HLEN));
>  
> @@ -148,9 +148,8 @@ int tcp_vu_send_flag(const struct ctx *c, struct tcp_tap_conn *conn, int flags)
>  	nb_ack = 1;
>  
>  	if (flags & DUP_ACK) {
> -		vu_set_element(&flags_elem[1], NULL, &flags_iov[1]);
> -
>  		elem_cnt = vu_collect(vdev, vq, &flags_elem[1], 1,
> +				      &flags_iov[1], 1, NULL,
>  				      flags_elem[0].in_sg[0].iov_len, NULL);
>  		if (elem_cnt == 1 &&
>  		    flags_elem[1].in_sg[0].iov_len >=
> @@ -191,8 +190,8 @@ static ssize_t tcp_vu_sock_recv(const struct ctx *c, struct vu_virtq *vq,
>  	const struct vu_dev *vdev = c->vdev;
>  	struct msghdr mh_sock = { 0 };
>  	uint16_t mss = MSS_GET(conn);
> +	size_t hdrlen, iov_used;
>  	int s = conn->sock;
> -	size_t hdrlen;
>  	int elem_cnt;
>  	ssize_t ret;
>  	int i;
> @@ -201,22 +200,26 @@ static ssize_t tcp_vu_sock_recv(const struct ctx *c, struct vu_virtq *vq,
>  
>  	hdrlen = tcp_vu_hdrlen(v6);
>  
> -	vu_init_elem(elem, &iov_vu[DISCARD_IOV_NUM], VIRTQUEUE_MAX_SIZE);
> -
> +	iov_used = 0;
>  	elem_cnt = 0;
>  	*head_cnt = 0;
> -	while (fillsize > 0 && elem_cnt < VIRTQUEUE_MAX_SIZE) {
> +	while (fillsize > 0 && elem_cnt < ARRAY_SIZE(elem) &&
> +	       iov_used < VIRTQUEUE_MAX_SIZE) {
> +		size_t frame_size, dlen, in_num;
>  		struct iovec *iov;
> -		size_t frame_size, dlen;
>  		int cnt;
>  
>  		cnt = vu_collect(vdev, vq, &elem[elem_cnt],
> -				 VIRTQUEUE_MAX_SIZE - elem_cnt,
> +				 ARRAY_SIZE(elem) - elem_cnt,
> +				 &iov_vu[DISCARD_IOV_NUM + iov_used],
> +				 VIRTQUEUE_MAX_SIZE - iov_used, &in_num,
>  				 MAX(MIN(mss, fillsize) + hdrlen, ETH_ZLEN + VNET_HLEN),
>  				 &frame_size);
>  		if (cnt == 0)
>  			break;
> +		ASSERT((size_t)cnt == in_num);	/* one iovec per element */

...this (and the UDP equivalent) will trigger if there are multiple
iovecs per element, until the point the next pending series deals with
it.

My assumption is that, if there multiple iovecs per element, we crash
in the way you reported in https://bugs.passt.top/show_bug.cgi?id=196
anyway, so triggering this assertion here doesn't make it any worse for
the moment.

Is my assumption correct, or do we risk adding additional cases where
we crash meanwhile? If we do, maybe it would be better to only merge
this patch with the next series.

-- 
Stefano