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

[Stefano Brivio <sbrivio@redhat.com>]

On Tue, 4 Nov 2025 17:28:52 +0100
Laurent Vivier <lvivier@redhat.com> wrote:

> On 11/4/25 17:09, Stefano Brivio wrote:
> > On Tue, 4 Nov 2025 16:50:43 +0100
> > Laurent Vivier <lvivier@redhat.com> wrote:
> >   
> >> On 11/3/25 11:16, 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>
> >>> ---
> >>>    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);
> >>> +	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);
> >>> +
> >>> +	memset(&iov[cnt - 1].iov_base + iov[cnt - 1].iov_len, 0, pad);  
> >>
> >> I think it should be
> >>
> >> 	memset((char *)iov[cnt - 1].iov_base + iov[cnt - 1].iov_len, 0, pad);  
> > 
> > Right, thanks, I always forget that sizeof(void) being 1 is a gcc
> > extension:
> > 
> >    https://gcc.gnu.org/onlinedocs/gcc/Pointer-Arith.html
> > 
> > What's rather confusing, actually, is that even if I explicitly enable
> > -Wpointer-arith, I don't get a warning for that. Any clue?  
> 
> in fact it's sizeof(void **) as iov[cnt - 1].iov_base is (void *) and you take &(void *).
> 
> Normally gcc spits out warnings when we do .iov_base + .iov_len, but as you take address 
> of iov_base all is fine :P

Ouch, wow, how does it even work? I checked captures of most functional
tests with vhost-user, connections worked and the right ACK segments
had the right amount of padding.

I guess it's just that there was nothing fundamental at the resulting
address and we already happened to have zeroes in the buffers.

Thanks for spotting that, I'll fix in v2.

-- 
Stefano