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

[Stefano Brivio <sbrivio@redhat.com>]

On Tue, 17 Mar 2026 17:18:58 +0100
Laurent Vivier <lvivier@redhat.com> wrote:

> On 3/17/26 16:23, Stefano Brivio wrote:
> > 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.
> >   
> 
> You're right. The idea is to mark clearly that udp_vu.c and tcp_vu.c only manage 1 iovec 
> per element. But they can be removed from this series if you prefer.

No, no, then it's convenient! It's also more convenient for any
debugging meanwhile to have things crash right there, I suppose.

-- 
Stefano