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

[Stefano Brivio <sbrivio@redhat.com>]

On Fri, 5 Dec 2025 06:48:42 +0100
Stefano Brivio <sbrivio@redhat.com> wrote:

> On Fri,  5 Dec 2025 01:51:56 +0100
> Stefano Brivio <sbrivio@redhat.com> wrote:
> 
> > Add a further iovec frame part, TCP_IOV_ETH_PAD for TCP and
> > UDP_IOV_ETH_PAD for UDP, after the payload, make that point to a
> > zero-filled buffer, and send out a part of it if needed to reach
> > the minimum frame length given by 802.3, that is, 60 bytes altogether.
> > 
> > The frames we might need to pad are IPv4 only (the IPv6 header is
> > larger), and are typically TCP ACK segments but can also be small
> > data segments or datagrams.
> > 
> > Link: https://bugs.passt.top/show_bug.cgi?id=166
> > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > Reviewed-by: Laurent Vivier <lvivier@redhat.com>  
> 
> ...for some reason, in combination with the previous series with TCP
> throughput fixes, this patch now seems to break basic transfers ("large
> transfer", IPv4, guest to host), with passt only.

Whoa, it turns out I didn't test this one at all, sorry for that, I
don't know how it happened.

This "clearly" needs:

---
diff --git a/tcp.c b/tcp.c
index c5486bc..7140b22 100644
--- a/tcp.c
+++ b/tcp.c
@@ -1037,7 +1037,7 @@ void tcp_fill_headers(const struct ctx *c, struct tcp_tap_conn *conn,
 	else
 		tcp_update_csum(psum, th, payload);
 
-	tap_hdr_update(taph, l3len + sizeof(struct ethhdr));
+	tap_hdr_update(taph, MAX(l3len + sizeof(struct ethhdr), ETH_ZLEN));
 }
 
 /**
diff --git a/udp.c b/udp.c
index f32f553..08bec50 100644
--- a/udp.c
+++ b/udp.c
@@ -381,19 +381,25 @@ static void udp_tap_prepare(const struct mmsghdr *mmh,
 	struct ethhdr *eh = (*tap_iov)[UDP_IOV_ETH].iov_base;
 	struct udp_payload_t *bp = &udp_payload[idx];
 	struct udp_meta_t *bm = &udp_meta[idx];
-	size_t l4len;
+	size_t l4len, l2len;
 
 	eth_update_mac(eh, NULL, tap_omac);
 	if (!inany_v4(&toside->eaddr) || !inany_v4(&toside->oaddr)) {
 		l4len = udp_update_hdr6(&bm->ip6h, bp, toside,
 					mmh[idx].msg_len, no_udp_csum);
-		tap_hdr_update(&bm->taph, l4len + sizeof(bm->ip6h) + ETH_HLEN);
+
+		l2len = MAX(l4len + sizeof(bm->ip6h) + ETH_HLEN, ETH_ZLEN);
+		tap_hdr_update(&bm->taph, l2len);
+
 		eh->h_proto = htons_constant(ETH_P_IPV6);
 		(*tap_iov)[UDP_IOV_IP] = IOV_OF_LVALUE(bm->ip6h);
 	} else {
 		l4len = udp_update_hdr4(&bm->ip4h, bp, toside,
 					mmh[idx].msg_len, no_udp_csum);
-		tap_hdr_update(&bm->taph, l4len + sizeof(bm->ip4h) + ETH_HLEN);
+
+		l2len = MAX(l4len + sizeof(bm->ip4h) + ETH_HLEN, ETH_ZLEN);
+		tap_hdr_update(&bm->taph, l2len);
+
 		eh->h_proto = htons_constant(ETH_P_IP);
 		(*tap_iov)[UDP_IOV_IP] = IOV_OF_LVALUE(bm->ip4h);
 	}
---

on top. I'll respin the whole series in a bit (once I addressed David's
comments to 5/5 as well).

-- 
Stefano