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=b9T5ox42; dkim-atps=neutral Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id CDC375A0265 for ; Thu, 14 May 2026 03:25:00 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202602; t=1778721896; bh=EOFkA6bt+DcYf+RgdIx5nmmt+oRaspm48FwBhFFwO94=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=b9T5ox42rLvMYssde5jj5RnAi2us/kMzZM3AuDcL0khoam2ycyaCwfOxBYuFRijYS jqR+FviT2ikS7/r/e8BclZNTEo3CDMyzYS3/+gSwABuQo/JKI5XUgmoWZyp5lNz+IV AF0ky0Wi14FSn+OzbD5DteUYt/5uMjbyPXdWA4CBk/SH2tDL4ovxedcDXGm64X8+iV jc8fC1dFssWuc29/ihotNwG8y1N7UXJH5MbIEPPM8MBsNcmiLv9Yb2WSWDjeWuAScE suFomMCS5SRyY0j7liiK14/nfbqtrU/GsS4PGEBAjIUBkKEQJpAOZME141aroy5txV akGdpCPJcGbEQ== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4gGCLJ57Xyz4wG9; Thu, 14 May 2026 11:24:56 +1000 (AEST) Date: Thu, 14 May 2026 11:24:51 +1000 From: David Gibson To: Laurent Vivier Subject: Re: [PATCH v4 10/10] vhost-user: Centralise Ethernet frame padding in vu_collect() and vu_pad() Message-ID: References: <20260513115218.1662850-1-lvivier@redhat.com> <20260513115218.1662850-11-lvivier@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="yXEWxYIzX+jq6cFZ" Content-Disposition: inline In-Reply-To: <20260513115218.1662850-11-lvivier@redhat.com> Message-ID-Hash: 2WES5RPSKWMS2MYUV6MESH3G3HAJRESI X-Message-ID-Hash: 2WES5RPSKWMS2MYUV6MESH3G3HAJRESI 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, Jon Maloy 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: --yXEWxYIzX+jq6cFZ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, May 13, 2026 at 01:52:18PM +0200, Laurent Vivier wrote: > The previous per-protocol padding done by vu_pad() in tcp_vu.c and > udp_vu.c was only correct for single-buffer frames: it assumed the > padding area always fell within the first iov, writing past its end > with a plain memset(). >=20 > It also required each caller to compute MAX(..., ETH_ZLEN + VNET_HLEN) > for vu_collect() and to call vu_pad() at the right point, duplicating > the minimum-size logic across protocols. >=20 > Move the Ethernet minimum size enforcement into vu_collect() itself, so > that enough buffer space is always reserved for padding regardless of > the requested frame size. >=20 > Rewrite vu_pad() to take a full iovec array and use iov_memset(), > making it safe for multi-buffer (mergeable rx buffer) frames. >=20 > In tcp_vu_sock_recv(), replace iov_truncate() with iov_skip_bytes(): > now that all consumers receive explicit data lengths, truncating the > iovecs is no longer needed. In tcp_vu_data_from_sock(), cap each > frame's data length against the remaining bytes actually received from > the socket, so that the last partial frame gets correct headers and > sequence number advancement. >=20 > Signed-off-by: Laurent Vivier > Reviewed-by: Jon Maloy Reviewed-by: David Gibson But following on from my comments on v3, a couple of clarity nits for possible follow up: [snip] > diff --git a/vu_common.c b/vu_common.c > index 704e908aa02c..d07f584f228a 100644 > --- a/vu_common.c > +++ b/vu_common.c > @@ -74,6 +74,7 @@ int vu_collect(const struct vu_dev *vdev, struct vu_vir= tq *vq, > size_t current_iov =3D 0; > int elem_cnt =3D 0; > =20 > + size =3D MAX(size, ETH_ZLEN /* Ethernet minimum size */ + VNET_HLEN); Here I think "size" is a reasonable name, since it's the size of the buffer we're obtaining, i.e. a bound, but not otherwise related to the length of the frame. > while (current_size < size && elem_cnt < max_elem && > current_iov < max_in_sg) { > int ret; > @@ -261,29 +262,27 @@ int vu_send_single(const struct ctx *c, const void = *buf, size_t size) > return -1; > } > =20 > - size +=3D VNET_HLEN; > elem_cnt =3D vu_collect(vdev, vq, elem, ARRAY_SIZE(elem), in_sg, > - ARRAY_SIZE(in_sg), &in_total, size, &total); > - if (elem_cnt =3D=3D 0 || total < size) { > + ARRAY_SIZE(in_sg), &in_total, VNET_HLEN + size, &total); > + if (elem_cnt =3D=3D 0 || total < VNET_HLEN + size) { Here, "l2len" would be a much better name than "size". > debug("vu_send_single: no space to send the data " > "elem_cnt %d size %zu", elem_cnt, total); > goto err; > } > =20 > - total -=3D VNET_HLEN; > - > /* copy data from the buffer to the iovec */ > - iov_from_buf(in_sg, in_total, VNET_HLEN, buf, total); > + iov_from_buf(in_sg, in_total, VNET_HLEN, buf, size); > =20 > if (*c->pcap) > pcap_iov(in_sg, in_total, VNET_HLEN, size); > =20 > + vu_pad(in_sg, in_total, VNET_HLEN + size); > vu_flush(vdev, vq, elem, elem_cnt, VNET_HLEN + size); > vu_queue_notify(vdev, vq); > =20 > - trace("vhost-user sent %zu", total); > + trace("vhost-user sent %zu", size); > =20 > - return total; > + return size; > err: > for (i =3D 0; i < elem_cnt; i++) > vu_queue_detach_element(vq); > @@ -292,15 +291,15 @@ err: > } > =20 > /** > - * vu_pad() - Pad 802.3 frame to minimum length (60 bytes) if needed > - * @iov: Buffer in iovec array where end of 802.3 frame is stored > - * @l2len: Layer-2 length already filled in frame > + * vu_pad() - Pad short frames to minimum Ethernet length and truncate i= ovec > + * @iov: Pointer to iovec array > + * @cnt: Number of entries in @iov > + * @frame_len: Data length in @iov (including virtio-net header) > */ > -void vu_pad(struct iovec *iov, size_t l2len) > +void vu_pad(const struct iovec *iov, size_t cnt, size_t frame_len) Here we have the actual frame length, including device header, but not padding. "frame_len" is different from the other standard names we use, so it's not terrible, but "frame" often refers to the L2 object so it's not great either. Not sure if 'l1len' or 'l0len' would be getting too cutesy with what "physical" layer means in a virtual network. Something like "device_len" maybe? But that should probably include padding as well. Or alternatively, vu_pad() could be updated to take l2len, and add VNET_HLEN inside. > { > - if (l2len >=3D ETH_ZLEN) > - return; > + size_t min_frame_len =3D ETH_ZLEN + VNET_HLEN; > =20 > - memset((char *)iov->iov_base + iov->iov_len, 0, ETH_ZLEN - l2len); > - iov->iov_len +=3D ETH_ZLEN - l2len; > + if (frame_len < min_frame_len) > + iov_memset(iov, cnt, frame_len, 0, min_frame_len - frame_len); > } > diff --git a/vu_common.h b/vu_common.h > index 77d1849e6115..51f70084a7cb 100644 > --- a/vu_common.h > +++ b/vu_common.h > @@ -44,6 +44,6 @@ void vu_flush(const struct vu_dev *vdev, struct vu_virt= q *vq, > void vu_kick_cb(struct vu_dev *vdev, union epoll_ref ref, > const struct timespec *now); > int vu_send_single(const struct ctx *c, const void *buf, size_t size); > -void vu_pad(struct iovec *iov, size_t l2len); > +void vu_pad(const struct iovec *iov, size_t cnt, size_t frame_len); > =20 > #endif /* VU_COMMON_H */ > --=20 > 2.54.0 >=20 --=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 --yXEWxYIzX+jq6cFZ Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmoFJGIACgkQzQJF27ox 2Gcm0w//WtNScQJ+NKr5UOjh0URObJ6ZsOkqnDxgJ2Bt4DMxAxLZdsDrORrMTCFI eM/wrjwnC4unPqPgdPSlXQsJK94fQnFcbXxXIFb7XM6KDRHTHqV31rvzNOt4zwP+ Y5GytR0frWzAc9lKReg///toL4sOY+0SRzsMV3gfutXik5yjSRtqTy8dhjkg11D4 F1DUbANAhFh/KBm1caQNaXaso18nM+kaVYDJjjZIQNvW0p1pmXFIsgrBMPiclAdh 44o1qk1BciTRUl+YqJLl+m3K694HzoTNm+bPau96hDIbZjpZkiRbL9LW4L3JLOM3 Wa0GcZ5z5Yz7j+RM2o2wbCcQnI3TpWY0OYfJ6FpvAGnblvLVWHzJaD6xBD6KpN2k Rey68jOnaGYCs2NnKBWgBZkmLtpBLg+xMvkO5Pw+gy5Hmw90CsbtvuZFuamMauBD Mye9pOBDuk+AVyqtcScKLYOqWVTOTAbwUtqdH7rxRbKoHvdx5b6175aZqpirvsAz yv5+aEp/YytB0u+EOvgt2p5cWGvmtCCRRh/cNSip+cNYuFvunfGEPY7VXsFfYXrz di5HqWRGMb+5eZpIUEkdHNEJ7D6FyM2RBPNa2/UJkbBdMWdOg+M8eCabRYisIbe2 5u+oMribtwgOz7ZEhssq7D8bx4SIQ3lq35AZBQdq/E2kqI9hrB8= =VaDD -----END PGP SIGNATURE----- --yXEWxYIzX+jq6cFZ--