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=202510 header.b=Irbvr8x9; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 2C9175A026F for ; Wed, 05 Nov 2025 04:50:48 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202510; t=1762314644; bh=EZuRS6/juMIVTnNWn6GCAHEkDXZAEBoeSg1M2vCtluc=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=Irbvr8x9YYjUtiLGsJ4Q41/h243BfhuFOrxA3V1cc6vAuTIBvVgnTcjkwfDPCRv9Q 3yyU2aHljqk4GJI+PN+6+flkyIDDHe7MpxWMIumljhFbmBg4+ZacWHm5gbcPFYHFu8 p9ySD6sx0fYPkjP2bOlgwgt4jpQMR5SjF7EA7j7/asJi65RjTUNPg6P3fxdPqkH/Pn a9mj7kz6SPP8JkwIr/Dz+xKN/Nvo+YslihWNzd4Y//rZ1xVNYT4XKRCl4y4SmKWseL n75+6iQTMvphtEoY2QV8YRzdRatS+p973R6wrD5TL/os6/k3KhO4vz93y4tav5kEnf cwbPez79gsoEw== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4d1WZD5gVHz4wBD; Wed, 05 Nov 2025 14:50:44 +1100 (AEDT) Date: Wed, 5 Nov 2025 14:49:59 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [RFC PATCH 5/5] tcp, udp: Pad batched frames for vhost-user modes to 60 bytes (802.3 minimum) Message-ID: References: <20251103101612.1412079-1-sbrivio@redhat.com> <20251103101612.1412079-6-sbrivio@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="0oBb2THfOH/cnYWA" Content-Disposition: inline In-Reply-To: <20251103101612.1412079-6-sbrivio@redhat.com> Message-ID-Hash: 5EIU6L4777MM25BHC3PPIDB67JBB6LLY X-Message-ID-Hash: 5EIU6L4777MM25BHC3PPIDB67JBB6LLY 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, Laurent Vivier 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: --0oBb2THfOH/cnYWA Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Nov 03, 2025 at 11:16:12AM +0100, Stefano Brivio wrote: > For both TCP and UDP, we request vhost-user buffers that are large > enough to reach ETH_ZLEN (60 bytes), so padding is just a matter of > increasing the appropriate iov_len and clearing bytes in the buffer > as needed. >=20 > Link: https://bugs.passt.top/show_bug.cgi?id=3D166 > Signed-off-by: Stefano Brivio I think this is correct, apart from the nasty bug Laurent spotted. I'm less certain if this is the most natural way to do it. > --- > tcp.c | 2 -- > tcp_internal.h | 1 + > tcp_vu.c | 27 +++++++++++++++++++++++++++ > udp_vu.c | 11 ++++++++++- > 4 files changed, 38 insertions(+), 3 deletions(-) >=20 > diff --git a/tcp.c b/tcp.c > index e91c0cf..039688d 100644 > --- a/tcp.c > +++ b/tcp.c > @@ -335,8 +335,6 @@ enum { > }; > #endif > =20 > -/* MSS rounding: see SET_MSS() */ > -#define MSS_DEFAULT 536 > #define WINDOW_DEFAULT 14600 /* RFC 6928 */ > =20 > #define ACK_INTERVAL 10 /* ms */ > diff --git a/tcp_internal.h b/tcp_internal.h > index 5f8fb35..d2295c9 100644 > --- a/tcp_internal.h > +++ b/tcp_internal.h > @@ -12,6 +12,7 @@ > #define BUF_DISCARD_SIZE (1 << 20) > #define DISCARD_IOV_NUM DIV_ROUND_UP(MAX_WINDOW, BUF_DISCARD_SIZE) > =20 > +#define MSS_DEFAULT /* and minimum */ 536 /* as it comes from minimum MT= U */ > #define MSS4 ROUND_DOWN(IP_MAX_MTU - \ > sizeof(struct tcphdr) - \ > sizeof(struct iphdr), \ > diff --git a/tcp_vu.c b/tcp_vu.c > index 1c81ce3..7239401 100644 > --- a/tcp_vu.c > +++ b/tcp_vu.c > @@ -60,6 +60,29 @@ static size_t tcp_vu_hdrlen(bool v6) > return hdrlen; > } > =20 > +/** > + * tcp_vu_pad() - Pad 802.3 frame to minimum length (60 bytes) if needed > + * @iov: iovec array storing 802.3 frame with TCP segment inside > + * @cnt: Number of entries in @iov > + */ > +static void tcp_vu_pad(struct iovec *iov, size_t cnt) > +{ > + size_t l2len, pad; > + > + ASSERT(iov_size(iov, cnt) >=3D sizeof(struct virtio_net_hdr_mrg_rxbuf)); > + l2len =3D iov_size(iov, cnt) - sizeof(struct virtio_net_hdr_mrg_rxbuf); Re-obtaining l2len from iov_size() seems kind of awkward, since the callers should already know the length - they've just used it to populate iov_len. > + if (l2len >=3D ETH_ZLEN) > + return; > + > + pad =3D ETH_ZLEN - l2len; > + > + /* tcp_vu_sock_recv() requests at least MSS-sized vhost-user buffers */ > + static_assert(ETH_ZLEN <=3D MSS_DEFAULT); So, this is true for the data path, but not AFAICT for the flags path. There _is_ still enough space in this case, because we request space for (tcp_vu_hdrlen() + sizeof(struct tcp_syn_opts)) which works out to: ETH_HLEN 14 + IP header 20 + TCP header 20 + tcp_syn_opts 8 ---- 62 > ETH_ZLEN But the comment and assert are misleading. It seems like it would make more sense to clamp ETH_ZLEN as a lower length bound before we vu_collect() the buffers. Or indeed, like we should be calculating l2len already including the clamping. > + > + memset(&iov[cnt - 1].iov_base + iov[cnt - 1].iov_len, 0, pad); > + iov[cnt - 1].iov_len +=3D pad; > +} > + > /** > * tcp_vu_send_flag() - Send segment with flags to vhost-user (no payloa= d) > * @c: Execution context > @@ -138,6 +161,8 @@ int tcp_vu_send_flag(const struct ctx *c, struct tcp_= tap_conn *conn, int flags) > tcp_fill_headers(c, conn, NULL, eh, ip4h, ip6h, th, &payload, > NULL, seq, !*c->pcap); > =20 > + tcp_vu_pad(&flags_elem[0].in_sg[0], 1); > + > if (*c->pcap) { > pcap_iov(&flags_elem[0].in_sg[0], 1, > sizeof(struct virtio_net_hdr_mrg_rxbuf)); > @@ -456,6 +481,8 @@ int tcp_vu_data_from_sock(const struct ctx *c, struct= tcp_tap_conn *conn) > =20 > tcp_vu_prepare(c, conn, iov, buf_cnt, &check, !*c->pcap, push); > =20 > + tcp_vu_pad(iov, buf_cnt); > + > if (*c->pcap) { > pcap_iov(iov, buf_cnt, > sizeof(struct virtio_net_hdr_mrg_rxbuf)); > diff --git a/udp_vu.c b/udp_vu.c > index 099677f..1b60860 100644 > --- a/udp_vu.c > +++ b/udp_vu.c > @@ -72,8 +72,8 @@ static int udp_vu_sock_recv(const struct ctx *c, struct= vu_virtq *vq, int s, > { > const struct vu_dev *vdev =3D c->vdev; > int iov_cnt, idx, iov_used; > + size_t off, hdrlen, l2len; > struct msghdr msg =3D { 0 }; > - size_t off, hdrlen; > =20 > ASSERT(!c->no_udp); > =20 > @@ -116,6 +116,15 @@ static int udp_vu_sock_recv(const struct ctx *c, str= uct vu_virtq *vq, int s, > iov_vu[idx].iov_len =3D off; > iov_used =3D idx + !!off; > =20 > + /* pad 802.3 frame to 60 bytes if needed */ > + l2len =3D *dlen + hdrlen - sizeof(struct virtio_net_hdr_mrg_rxbuf); > + if (l2len < ETH_ZLEN) { > + size_t pad =3D ETH_ZLEN - l2len; > + > + iov_vu[idx].iov_len +=3D pad; > + memset(&iov_vu[idx].iov_base + off, 0, pad); > + } > + > vu_set_vnethdr(vdev, iov_vu[0].iov_base, iov_used); > =20 > /* release unused buffers */ > --=20 > 2.43.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 --0oBb2THfOH/cnYWA Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmkKyVgACgkQzQJF27ox 2Geahg/+LMqoSppK5J0tRXxOLUvvVE5XxJ0ZC6HtCy3ERHqzyAR/9aw17P/ug2Cy UuIH4Z1uuhlDwweeBNRR4KAk7LIK/nVcu8mt8V0dlS/XcwkacnwDZM43luW07LVe bxmNCYufjUNvsVXnz066KwRjN9Ac5Qz2CHbLZBCynVUVGb6almiEXkNawd0romfj f87QD/0/HqQNtaJ1NaJ1LHBzjHqY6D1eTFZAz5dTKA/3NMfAp1+6PGsqC3VhuJfM skvuw43nO933EUWC5ytoz1/U4u9GJNrPEaaHBOjwSeNS54bYrgSAmH73S3Dh7XhI DNgMMH+S1oefwj01ZJ3OU2wD1hepdszAmoJy55cTRBF/SDJyhwoAUuG9r4qC1n2N bYlAfTjMqipwZydSwjNjGtPh3Zmd5o9j+qbkVtgbC4pLELrTutxm9qosp1yusb28 s78PV0CAmr5nob0At19MfIV7e5JgAGMnj3tVFze2zuEdtdCmJuRxcFHJyuPGCJvO QhGYf5f4hYpwBwt++DvKrxOZcpgxwRfmN7ZODkkHnTyKnf7ACQWmexCHXFiTzFGy imJAPBWBfyzFLGrAwalmrkWWlz6xLxEsginm3I3JKWuizCOMqPv49+hxaZs/6HY9 ZphxNKsFUenkHze2vBkk5rHzFsST3I/gMzXmrvA8QYgY5pyCE+U= =9Cng -----END PGP SIGNATURE----- --0oBb2THfOH/cnYWA--