From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202602 header.b=LxsnWUxY; dkim-atps=neutral Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id 300D75A0262 for ; Wed, 18 Mar 2026 02:17:51 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202602; t=1773796669; bh=bg+hFkZ5e90xkTZwf8B6tv9RThn2lVw426K7xsuT02U=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=LxsnWUxYvCbvBy3q4GPBFuRra16tqtG3+PgARd4QlVwOGTyDNn/XYiYOBQtOmiH5g yPJEZd4idxalOwo2JEx5zV/JKKrez+LrrEXen03eAElrk2JkkJxCIRgWr/5mX8kH0x 2B5lhujL4YUM2aBhZMvrQFxecuOwrVBxMK1gFBNrDJIQyvgQx4MlytJWQ0apCRNhut lo85Kz0I3LQs57Wqdsc6TOY3kFWCCbXn4iYtwBi3JR12oIEg8d4ToUqQ1SlXY08v4q 8TC+HitzGc8EqRDYN2lA58hQw1tiPr051dzRasuuL4ggzZxQpWdrFZVpR2ubaeZOXS 2UFCePxdRKLSA== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4fb9tP0RkHz4wB7; Wed, 18 Mar 2026 12:17:49 +1100 (AEDT) Date: Wed, 18 Mar 2026 12:15:30 +1100 From: David Gibson To: Laurent Vivier Subject: Re: [PATCH v2 3/3] vu_common: Move iovec management into vu_collect() Message-ID: References: <20260313182618.4157365-1-lvivier@redhat.com> <20260313182618.4157365-4-lvivier@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="7VbLACPEQ9JOHvss" Content-Disposition: inline In-Reply-To: Message-ID-Hash: CVZSGX5VPGMPXB76PJ36JVP4JAW7QBWY X-Message-ID-Hash: CVZSGX5VPGMPXB76PJ36JVP4JAW7QBWY X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --7VbLACPEQ9JOHvss Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Mar 17, 2026 at 08:25:49AM +0100, Laurent Vivier 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 iov= ec > > > entries using vu_set_element() or vu_init_elem() before calling > > > vu_collect(). This meant each element owned a fixed, pre-assigned io= vec > > > slot. > > >=20 > > > 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 io= vec > > > entries were consumed, allowing callers to track usage across multiple > > > vu_collect() calls. > > >=20 > > > 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. > > >=20 > > > Signed-off-by: Laurent Vivier > >=20 > > Reviewed-by: David Gibson > >=20 > > Couple of thoughts on possible polish below. > >=20 > > [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 =3D 0; > > > + size_t current_iov =3D 0; > > > int elem_cnt =3D 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 =3D 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 =3D &elem[elem_cnt].in_sg[0]; > > > - > > > - if (iov->iov_len > size - current_size) > > > - iov->iov_len =3D size - current_size; > > > + elem[elem_cnt].in_num =3D iov_truncate(elem[elem_cnt].in_sg, > > > + elem[elem_cnt].in_num, > > > + size - current_size); > >=20 > > Will elem[].in_num always end up with the same value as the @in_num > > parameter? If so, do we need the explicit parameter? >=20 > @in_num parameter of vu_collect()? >=20 > @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. Oh, right, sorry. I'm getting confused again by the two-level heirarchy - this gathers multiple elems as well as multiple iovs. > >=20 > > > - current_size +=3D iov->iov_len; > > > + current_size +=3D iov_size(elem[elem_cnt].in_sg, > > > + elem[elem_cnt].in_num); > > > + current_iov +=3D elem[elem_cnt].in_num; > > > elem_cnt++; > > > if (!vu_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF)) > > > break; > > > } > > > + if (in_num) > > > + *in_num =3D current_iov; > > > + > > > if (collected) > > > *collected =3D current_size; > > > @@ -147,8 +139,11 @@ void vu_flush(const struct vu_dev *vdev, struct = vu_virtq *vq, > > > { > > > int i; > > > - for (i =3D 0; i < elem_cnt; i++) > > > - vu_queue_fill(vdev, vq, &elem[i], elem[i].in_sg[0].iov_len, i); > > > + for (i =3D 0; i < elem_cnt; i++) { > > > + size_t elem_size =3D iov_size(elem[i].in_sg, elem[i].in_num); > >=20 > > IIUC, the elem structure itself isn't shared with vhost, so we can > > alter it. Would it make sense to cache the number of bytes allocated > > to the element there, to avoid repeated calls to iov_size()? >=20 > It's possible. But I think it could be complicated to keep in sync the > actual size of the iovec array and the value we store in elem, as we alter > the array at several points. Ok. We do expect the iovs to be pretty short in practice, so iov_size() shouldn't be too expensive. --=20 David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson --7VbLACPEQ9JOHvss Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmm5/LEACgkQzQJF27ox 2GcCCA/+Pq5Xt9UpIZJVtG8ZNSjCA39iyo7NcmDdEW6EQ4iL3HnBPWHcqD0+T/c1 1HukrvzkYb0al4RyJwTNdGXcDhKM5m7j/Hq4Mwod491knrvswZ950GRApSVx6NF/ MwqjxgDoQo0ZSnK1YGDCzP0YfNLKR01jKc+76okVCEN6RcrY1zGTI8H3kKbpHNpL SuqID+wUdLozU5QsrApwccXf56kjgIBw2g6BSRa5S/9IbQq/L0NUX+U/qdSoUsf1 veYIMFrp0ztS0Dln6LpqrjeihsFptIt8JHPcqImM43VKFrSxtfFwHn8h/BH6v/Fv 1kfWSysyonXxiotZ9waWMO9xYF2CM7Wdr/gWlB3QZDeP+lYnOHpPrgTocbaQBniS +brXg+9jLBieRhrJd1cQd6AksR7V+aCQH1p02jxbJIxSOWoATZ7m0c2TtXSUqu3Q 1HO243KUQgT50SUXi6zdBzpP0sd43MnVsJyvco4TE4Y2LtoOOuVIbUwHmIuOzYgD 2ddxnpze0PRpE3QGSbZ9Sqrl5gszqcTv8ZfJhVq8CCjqnZKGbSHGw1aQjHNXB/8x 1IhEnxp31gmfV5UdARPQzENuQZhqtBvklw1snK4VWMluOMUUiUdhwVyqK7u7UKuZ QWcdnKC2dUGOSfCdxCIXxt8QJJkr9YLSIIXvn5zYb4Gy69GpmE0= =S7cj -----END PGP SIGNATURE----- --7VbLACPEQ9JOHvss--