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

[Stefano Brivio <sbrivio@redhat.com>]

On Wed, 18 Mar 2026 08:21:10 +0100
Laurent Vivier <lvivier@redhat.com> wrote:

> On 3/18/26 02:16, David Gibson wrote:
> > On Tue, Mar 17, 2026 at 05:30:32PM +0100, Laurent Vivier wrote:  
> >> On 3/17/26 16:23, Stefano Brivio wrote:  
> >>> On Tue, 17 Mar 2026 08:25:49 +0100
> >>> Laurent Vivier <lvivier@redhat.com> wrote:
> >>>  
> >>>> On 3/17/26 03:40, David Gibson wrote:  
> >>>>> On Fri, Mar 13, 2026 at 07:26:18PM +0100, Laurent Vivier 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>  
> >>>>>
> >>>>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> >>>>>
> >>>>> Couple of thoughts on possible polish below.
> >>>>>
> >>>>> [snip]  
> >>>>>>     /**
> >>>>>>      * vu_collect() - collect virtio buffers from a given virtqueue
> >>>>>>      * @vdev:		vhost-user device
> >>>>>>      * @vq:			virtqueue to collect from
> >>>>>> - * @elem:		Array of virtqueue element
> >>>>>> - * 			each element must be initialized with one iovec entry
> >>>>>> - * 			in the in_sg array.
> >>>>>> + * @elem:		Array of @max_elem virtqueue elements
> >>>>>>      * @max_elem:		Number of virtqueue elements in the array
> >>>>>> + * @in_sg:		Incoming iovec array for device-writable descriptors
> >>>>>> + * @max_in_sg:		Maximum number of entries in @in_sg
> >>>>>> + * @in_num:		Number of collected entries from @in_sg (output)
> >>>>>>      * @size:		Maximum size of the data in the frame
> >>>>>>      * @collected:		Collected buffer length, up to @size, set on return
> >>>>>>      *
> >>>>>> @@ -80,20 +67,21 @@ void vu_init_elem(struct vu_virtq_element *elem, struct iovec *iov, int elem_cnt
> >>>>>>      */
> >>>>>>     int vu_collect(const struct vu_dev *vdev, struct vu_virtq *vq,
> >>>>>>     	       struct vu_virtq_element *elem, int max_elem,
> >>>>>> +	       struct iovec *in_sg, size_t max_in_sg, size_t *in_num,
> >>>>>>     	       size_t size, size_t *collected)
> >>>>>>     {
> >>>>>>     	size_t current_size = 0;
> >>>>>> +	size_t current_iov = 0;
> >>>>>>     	int elem_cnt = 0;
> >>>>>> -	while (current_size < size && elem_cnt < max_elem) {
> >>>>>> -		struct iovec *iov;
> >>>>>> +	while (current_size < size && elem_cnt < max_elem &&
> >>>>>> +	       current_iov < max_in_sg) {
> >>>>>>     		int ret;
> >>>>>>     		ret = vu_queue_pop(vdev, vq, &elem[elem_cnt],
> >>>>>> -				   elem[elem_cnt].in_sg,
> >>>>>> -				   elem[elem_cnt].in_num,
> >>>>>> -				   elem[elem_cnt].out_sg,
> >>>>>> -				   elem[elem_cnt].out_num);
> >>>>>> +				   &in_sg[current_iov],
> >>>>>> +				   max_in_sg - current_iov,
> >>>>>> +				   NULL, 0);
> >>>>>>     		if (ret < 0)
> >>>>>>     			break;
> >>>>>> @@ -103,18 +91,22 @@ int vu_collect(const struct vu_dev *vdev, struct vu_virtq *vq,
> >>>>>>     			break;
> >>>>>>     		}
> >>>>>> -		iov = &elem[elem_cnt].in_sg[0];
> >>>>>> -
> >>>>>> -		if (iov->iov_len > size - current_size)
> >>>>>> -			iov->iov_len = size - current_size;
> >>>>>> +		elem[elem_cnt].in_num = iov_truncate(elem[elem_cnt].in_sg,
> >>>>>> +						     elem[elem_cnt].in_num,
> >>>>>> +						     size - current_size);  
> >>>>>
> >>>>> Will elem[].in_num always end up with the same value as the @in_num
> >>>>> parameter?  If so, do we need the explicit parameter?  
> >>>>
> >>>> @in_num parameter of vu_collect()?
> >>>>
> >>>> @in_num is the sum of all elem[].in_num, it can be computed by the caller function from
> >>>> elem, but it is simpler to return it as we need to compute it in the loop.  
> >>>
> >>> I'm not sure I understood the point of David's comment here, and this
> >>> explanation makes sense to me now, but it took me a bit to figure that
> >>> out.
> >>>
> >>> Could it be that @in_num is a bit confusing as it has "in" and "num" in
> >>> it, but it's actually an output representing how many "in" entries we
> >>> used/need?  
> >>
> >> For an element, *ìn_*num is the number of *in_*sg we have read from the ring for an element.
> >>
> >> It's virtio semantic, so  *in_* means sg going *in* the guest.
> >>
> >> For *out_*sg we have *out_*num.
> >>  
> >>>
> >>> What if we rename it to @in_used or @in_collected?
> >>>  
> >>
> >> The idea was to keep the same name as in the element. But we can change this to @in_used.  
> > 
> > Would "in_total" work better to suggest that it's the sum of all the
> > elements' in_num?
> 
> Yes, I think it gives the information it's the sum of the in_num

Okay, should I change this on merge, or do you plan to repost
(which would be slightly more convenient for me but not really needed)?

-- 
Stefano