Re: [RFC PATCH 5/5] tcp, udp: Pad batched frames for vhost-user modes to 60 bytes (802.3 minimum)

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 6859 bytes --]

On Fri, Dec 05, 2025 at 01:51:43AM +0100, Stefano Brivio wrote:
> On Wed, 5 Nov 2025 14:49:59 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > 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.
> > > 
> > > Link: https://bugs.passt.top/show_bug.cgi?id=166
> > > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>  
> > 
> > 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(-)
> > > 
> > > diff --git a/tcp.c b/tcp.c
> > > index e91c0cf..039688d 100644
> > > --- a/tcp.c
> > > +++ b/tcp.c
> > > @@ -335,8 +335,6 @@ enum {
> > >  };
> > >  #endif
> > >  
> > > -/* MSS rounding: see SET_MSS() */
> > > -#define MSS_DEFAULT			536
> > >  #define WINDOW_DEFAULT			14600		/* RFC 6928 */
> > >  
> > >  #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)
> > >  
> > > +#define MSS_DEFAULT /* and minimum */	536 /* as it comes from minimum MTU */
> > >  #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;
> > >  }
> > >  
> > > +/**
> > > + * 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) >= sizeof(struct virtio_net_hdr_mrg_rxbuf));
> > > +	l2len = 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.
> 
> That's only the case for tcp_vu_send_flag() though, because
> tcp_vu_data_from_sock() can use split buffers and iov_len of the first
> element is not the same as the whole frame length.

Yes, but..

> That is, you could (very much in theory) have iov_len set to 50 for the
> first iov item, set to 4 for the second iov item, and the frame needs
> padding, but you can't tell from the first iov item itself.

Before we call tcp_vu_prepare() on that path, we've already calculated
'dlen' from iov_size, here we're calling iov_size a second time.

> 
> > 
> > > +	if (l2len >= ETH_ZLEN)
> > > +		return;  
> > 
> > 
> > > +
> > > +	pad = ETH_ZLEN - l2len;
> > > +
> > > +	/* tcp_vu_sock_recv() requests at least MSS-sized vhost-user buffers */
> > > +	static_assert(ETH_ZLEN <= 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.
> 
> Dropped, in favour of:
> 
> > It seems like it would make more sense to clamp ETH_ZLEN as a lower
> > length bound before we vu_collect() the buffers.
> 
> this.

👍

> > Or indeed, like we should be calculating l2len already including the
> > clamping.
> 
> That's not trivial to do for the data path, I think (see above). I
> think it would be doable with a rework of the tcp_vu_data_from_sock()
> loop but I'd say it's beyond the scope of this series.

Yeah, fair enough.

> 
> > > +	memset(&iov[cnt - 1].iov_base + iov[cnt - 1].iov_len, 0, pad);
> > > +	iov[cnt - 1].iov_len += pad;
> > > +}
> > > +
> > >  /**
> > >   * tcp_vu_send_flag() - Send segment with flags to vhost-user (no payload)
> > >   * @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);
> > >  
> > > +	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)
> > >  
> > >  		tcp_vu_prepare(c, conn, iov, buf_cnt, &check, !*c->pcap, push);
> > >  
> > > +		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 = c->vdev;
> > >  	int iov_cnt, idx, iov_used;
> > > +	size_t off, hdrlen, l2len;
> > >  	struct msghdr msg  = { 0 };
> > > -	size_t off, hdrlen;
> > >  
> > >  	ASSERT(!c->no_udp);
> > >  
> > > @@ -116,6 +116,15 @@ static int udp_vu_sock_recv(const struct ctx *c, struct vu_virtq *vq, int s,
> > >  		iov_vu[idx].iov_len = off;
> > >  	iov_used = idx + !!off;
> > >  
> > > +	/* pad 802.3 frame to 60 bytes if needed */
> > > +	l2len = *dlen + hdrlen - sizeof(struct virtio_net_hdr_mrg_rxbuf);
> > > +	if (l2len < ETH_ZLEN) {
> > > +		size_t pad = ETH_ZLEN - l2len;
> > > +
> > > +		iov_vu[idx].iov_len += pad;
> > > +		memset(&iov_vu[idx].iov_base + off, 0, pad);
> > > +	}
> > > +
> > >  	vu_set_vnethdr(vdev, iov_vu[0].iov_base, iov_used);
> > >  
> > >  	/* release unused buffers */
> > > -- 
> > > 2.43.0
> 
> -- 
> Stefano
> 

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]