Re: [PATCH v6 2/4] tcp_vu: Build headers on the stack and write them into the iovec

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

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

On Thu, Apr 16, 2026 at 06:16:16PM +0200, Laurent Vivier wrote:
> tcp_vu_prepare() currently assumes the first iovec element provided by
> the guest is large enough to hold all L2-L4 headers, and builds them
> in place via pointer casts into iov[0].iov_base.  This assumption is
> enforced by an assert().
> 
> Since the headers in the buffer are uninitialized anyway, we can just
> as well build the Ethernet, IP, and TCP headers on the stack instead,
> and write them into the iovec with IOV_PUSH_HEADER().  This mirrors the
> approach already used in udp_vu_prepare(), and prepares for support of
> elements with multiple iovecs.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

Although one nit below.

> ---
>  tcp_vu.c | 60 ++++++++++++++++++++++++--------------------------------
>  1 file changed, 26 insertions(+), 34 deletions(-)
> 
> diff --git a/tcp_vu.c b/tcp_vu.c
> index 3e399c20f0d7..2017aec90342 100644
> --- a/tcp_vu.c
> +++ b/tcp_vu.c
> @@ -296,49 +296,41 @@ static void tcp_vu_prepare(const struct ctx *c, struct tcp_tap_conn *conn,
>  	bool v6 = !(inany_v4(&toside->eaddr) && inany_v4(&toside->oaddr));
>  	size_t hdrlen = tcp_vu_hdrlen(v6);
>  	struct iov_tail payload = IOV_TAIL(iov, iov_cnt, hdrlen);
> -	char *base = iov[0].iov_base;
> -	struct ipv6hdr *ip6h = NULL;
> -	struct iphdr *ip4h = NULL;
> -	struct tcphdr *th;
> -	struct ethhdr *eh;
> -
> -	/* we guess the first iovec provided by the guest can embed
> -	 * all the headers needed by L2 frame, including any padding
> -	 */
> -	assert(iov[0].iov_len >= hdrlen);
> +	struct ipv6hdr ip6h;
> +	struct iphdr ip4h;
> +	struct tcphdr th;
> +	struct ethhdr eh;
>  
> -	eh = vu_eth(base);
> -
> -	memcpy(eh->h_dest, c->guest_mac, sizeof(eh->h_dest));
> +	memcpy(eh.h_dest, c->guest_mac, sizeof(eh.h_dest));
>  
>  	/* initialize header */
>  
> -	if (!v6) {
> -		eh->h_proto = htons(ETH_P_IP);
> -
> -		ip4h = vu_ip(base);
> -		*ip4h = (struct iphdr)L2_BUF_IP4_INIT(IPPROTO_TCP);
> -		th = vu_payloadv4(base);
> -	} else {
> -		eh->h_proto = htons(ETH_P_IPV6);
> +	if (!v6)
> +		ip4h = (struct iphdr)L2_BUF_IP4_INIT(IPPROTO_TCP);
> +	else
> +		ip6h = (struct ipv6hdr)L2_BUF_IP6_INIT(IPPROTO_TCP);
>  
> -		ip6h = vu_ip(base);
> -		*ip6h = (struct ipv6hdr)L2_BUF_IP6_INIT(IPPROTO_TCP);
> +	memset(&th, 0, sizeof(th));
> +	th.doff = sizeof(th) / 4;
> +	th.ack = 1;
> +	th.psh = push;

These 4 lines could now become an initialiser for th.

>  
> -		th = vu_payloadv6(base);
> -	}
> +	tcp_fill_headers(c, conn, &eh, v6 ? NULL : &ip4h, v6 ? &ip6h : NULL, &th,
> +			 &payload, dlen, *csum_flags, conn->seq_to_tap);
>  
> -	memset(th, 0, sizeof(*th));
> -	th->doff = sizeof(*th) / 4;
> -	th->ack = 1;
> -	th->psh = push;
> +	/* Preserve TCP_CSUM, overwrite IP4_CSUM as we set the checksum */
> +	if (!v6)
> +		*csum_flags = (*csum_flags & TCP_CSUM) | ip4h.check;
>  
> -	tcp_fill_headers(c, conn, eh, ip4h, ip6h, th, &payload, dlen,
> -			 *csum_flags, conn->seq_to_tap);
> +	/* write headers */
> +	payload = IOV_TAIL(iov, iov_cnt, VNET_HLEN);
>  
> -	/* Preserve TCP_CSUM, overwrite IP4_CSUM as we set the checksum */
> -	if (ip4h)
> -		*csum_flags = (*csum_flags & TCP_CSUM) | ip4h->check;
> +	IOV_PUSH_HEADER(&payload, eh);
> +	if (!v6)
> +		IOV_PUSH_HEADER(&payload, ip4h);
> +	else
> +		IOV_PUSH_HEADER(&payload, ip6h);
> +	IOV_PUSH_HEADER(&payload, th);
>  }
>  
>  /**
> -- 
> 2.53.0
> 

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