Re: [PATCH v7 4/4] tcp_vu: Support multibuffer frames in tcp_vu_send_flag()

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

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

On Wed, May 20, 2026 at 05:18:27PM +0200, Laurent Vivier wrote:
> Build the Ethernet, IP, and TCP headers on the stack instead of
> directly in the buffer via pointer casts, then write them into the
> iovec with IOV_PUSH_HEADER().  This mirrors the approach already used
> in tcp_vu_prepare() and udp_vu_prepare().
> 
> Remove the vu_eth(), vu_ip(), vu_payloadv4() and vu_payloadv6() helpers
> from vu_common.h, as they are no longer used anywhere.
> 
> Introduce tcp_vu_send_dup() to handle DUP_ACK duplication using
> vu_collect() and iov_memcpy() instead of a plain memcpy(), so that
> the duplicated frame is also properly scattered across multiple iovecs.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> Reviewed-by: Jon Maloy <jmaloy@redhat.com>

One query below.  But at worst it can be addressed as a follow up, so

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

> ---
>  iov.c       |   1 -
>  tcp_vu.c    | 156 ++++++++++++++++++++++++++++++----------------------
>  vu_common.h |  20 -------
>  3 files changed, 91 insertions(+), 86 deletions(-)
> 
> diff --git a/iov.c b/iov.c
> index 9248ba95a9f2..6fd684ad1a6b 100644
> --- a/iov.c
> +++ b/iov.c
> @@ -208,7 +208,6 @@ void iov_memset(const struct iovec *iov, size_t iov_cnt, size_t offset, int c,
>   *
>   * Return: total number of bytes copied
>   */
> -/* cppcheck-suppress unusedFunction */
>  size_t iov_memcpy(struct iovec *dst_iov, size_t dst_iov_cnt, size_t dst_offset,
>  		  const struct iovec *src_iov, size_t src_iov_cnt,
>  		  size_t src_offset, size_t length)
> diff --git a/tcp_vu.c b/tcp_vu.c
> index 55242ec40410..30633760e4ec 100644
> --- a/tcp_vu.c
> +++ b/tcp_vu.c
> @@ -74,6 +74,43 @@ static size_t tcp_vu_hdrlen(bool v6)
>  	return hdrlen;
>  }
>  
> +/**
> + * tcp_vu_send_dup() - Duplicate a frame into a new virtqueue element
> + * @c:		Execution context
> + * @vq:		Receive virtqueue
> + * @dest_elem:	Destination virtqueue element to collect
> + * @dest_iov:	Destination iovec array for collected buffers
> + * @max_dest_iov: Maximum number of entries in @dest_iov
> + * @src_iov:	Source iovec array containing the frame to duplicate
> + * @src_cnt:	Number of entries in @src_iov
> + * @vnlen:	Total frame length including virtio-net header
> + *
> + * Return: number of virtqueue elements collected (0 if none available)
> + */
> +static int tcp_vu_send_dup(const struct ctx *c, struct vu_virtq *vq,
> +			   struct vu_virtq_element *dest_elem,
> +			   struct iovec *dest_iov, size_t max_dest_iov,
> +			   const struct iovec *src_iov, size_t src_cnt,
> +			   size_t vnlen)
> +{
> +	const struct vu_dev *vdev = c->vdev;
> +	size_t dest_cnt;
> +	int elem_cnt;
> +
> +	elem_cnt = vu_collect(vdev, vq, dest_elem, 1, dest_iov, max_dest_iov,
> +			      &dest_cnt, vnlen, NULL);

This (and the one in tcp_vu_send_flag()) still allows a maximum of 1
element, although it now allows multiple iovs. That differs from the
main data path, as of the previous patch..  Is that intentional, or
just a gap?

> +	if (elem_cnt == 0)
> +		return 0;
> +
> +	iov_memcpy(dest_iov, dest_cnt, 0, src_iov, src_cnt, 0,
> +		   MAX(VNET_HLEN + ETH_ZLEN, vnlen));
> +
> +	if (*c->pcap)
> +		pcap_iov(dest_iov, dest_cnt, VNET_HLEN, vnlen - VNET_HLEN);
> +
> +	return elem_cnt;
> +}
> +
>  /**
>   * tcp_vu_send_flag() - Send segment with flags to vhost-user (no payload)
>   * @c:		Execution context
> @@ -88,97 +125,86 @@ int tcp_vu_send_flag(const struct ctx *c, struct tcp_tap_conn *conn, int flags)
>  {
>  	struct vu_dev *vdev = c->vdev;
>  	struct vu_virtq *vq = &vdev->vq[VHOST_USER_RX_QUEUE];
> +	size_t optlen, hdrlen, iov_cnt, iov_used;
>  	struct vu_virtq_element flags_elem[2];
> -	size_t optlen, hdrlen, l2len;
> -	struct ipv6hdr *ip6h = NULL;
> -	struct iphdr *ip4h = NULL;
> -	struct iovec flags_iov[2];
> -	struct tcp_syn_opts *opts;
> -	struct iov_tail payload;
> -	struct tcphdr *th;
> -	struct ethhdr *eh;
> +	struct iov_tail payload, l2frame;
> +	int elem_cnt, dup_elem_cnt = 0;
> +	struct iovec flags_iov[64];
> +	struct tcp_syn_opts opts;
> +	struct tcphdr th = { 0 };
> +	struct ipv6hdr ip6h;
> +	struct iphdr ip4h;
> +	struct ethhdr eh;
>  	uint32_t seq;
> -	int elem_cnt;
>  	int ret;
>  
>  	hdrlen = tcp_vu_hdrlen(CONN_V6(conn));
>  
>  	elem_cnt = vu_collect(vdev, vq, &flags_elem[0], 1,
> -			      &flags_iov[0], 1, NULL,
> -			      hdrlen + sizeof(*opts), NULL);
> -	if (elem_cnt != 1)
> +			      flags_iov, ARRAY_SIZE(flags_iov), &iov_cnt,
> +			      hdrlen + sizeof(opts), NULL);
> +	if (elem_cnt == 0)
>  		return -EAGAIN;
>  
> -	assert(flags_elem[0].in_num == 1);
> -	assert(flags_elem[0].in_sg[0].iov_len >=
> -	       MAX(hdrlen + sizeof(*opts), ETH_ZLEN + VNET_HLEN));
> -
> -	eh = vu_eth(flags_elem[0].in_sg[0].iov_base);
> -
> -	memcpy(eh->h_dest, c->guest_mac, sizeof(eh->h_dest));
> -	memcpy(eh->h_source, c->our_tap_mac, sizeof(eh->h_source));
> -
> -	if (CONN_V4(conn)) {
> -		eh->h_proto = htons(ETH_P_IP);
> -
> -		ip4h = vu_ip(flags_elem[0].in_sg[0].iov_base);
> -		*ip4h = (struct iphdr)L2_BUF_IP4_INIT(IPPROTO_TCP);
> -
> -		th = vu_payloadv4(flags_elem[0].in_sg[0].iov_base);
> -	} else {
> -		eh->h_proto = htons(ETH_P_IPV6);
> -
> -		ip6h = vu_ip(flags_elem[0].in_sg[0].iov_base);
> -		*ip6h = (struct ipv6hdr)L2_BUF_IP6_INIT(IPPROTO_TCP);
> -		th = vu_payloadv6(flags_elem[0].in_sg[0].iov_base);
> -	}
> +	memcpy(eh.h_dest, c->guest_mac, sizeof(eh.h_dest));
>  
> -	memset(th, 0, sizeof(*th));
> -	th->doff = sizeof(*th) / 4;
> -	th->ack = 1;
> +	if (CONN_V4(conn))
> +		ip4h = (struct iphdr)L2_BUF_IP4_INIT(IPPROTO_TCP);
> +	else
> +		ip6h = (struct ipv6hdr)L2_BUF_IP6_INIT(IPPROTO_TCP);
>  
>  	seq = conn->seq_to_tap;
> -	opts = (struct tcp_syn_opts *)(th + 1);
> -	ret = tcp_prepare_flags(c, conn, flags, th, opts, &optlen);
> +	ret = tcp_prepare_flags(c, conn, flags, &th, &opts, &optlen);
>  	if (ret <= 0) {
> -		vu_queue_rewind(vq, 1);
> +		vu_queue_rewind(vq, elem_cnt);
>  		return ret;
>  	}
>  
> -	payload = IOV_TAIL(flags_elem[0].in_sg, 1, hdrlen);
> -
>  	if (flags & KEEPALIVE)
>  		seq--;
>  
> -	tcp_fill_headers(c, conn, eh, ip4h, ip6h, th, &payload,
> +	iov_used = iov_skip_bytes(flags_iov, iov_cnt,
> +				  MAX(optlen + hdrlen, VNET_HLEN + ETH_ZLEN),
> +				  NULL);
> +	if (iov_used < iov_cnt)
> +		iov_used++;
> +	iov_cnt = iov_used;
> +
> +	payload = IOV_TAIL(flags_elem[0].in_sg, iov_cnt, hdrlen);
> +	iov_from_buf(payload.iov, payload.cnt, payload.off, &opts, optlen);
> +	tcp_fill_headers(c, conn, &eh, CONN_V4(conn) ? &ip4h : NULL,
> +			 CONN_V6(conn) ? &ip6h : NULL, &th, &payload,
>  			 optlen, IP4_CSUM | (*c->pcap ? TCP_CSUM : 0), seq);
>  
> -	vu_pad(flags_elem[0].in_sg, 1, hdrlen + optlen);
> -	vu_flush(vdev, vq, flags_elem, 1, hdrlen + optlen);
> +	vu_pad(flags_elem[0].in_sg, iov_cnt, hdrlen + optlen);
> +
> +	/* write headers */
> +	l2frame = IOV_TAIL(flags_elem[0].in_sg, iov_cnt, VNET_HLEN);
> +
> +	IOV_PUSH_HEADER(&l2frame, eh);
> +	if (CONN_V4(conn))
> +		IOV_PUSH_HEADER(&l2frame, ip4h);
> +	else
> +		IOV_PUSH_HEADER(&l2frame, ip6h);
> +	IOV_PUSH_HEADER(&l2frame, th);
>  
> -	l2len = hdrlen + optlen - VNET_HLEN;
>  	if (*c->pcap)
> -		pcap_iov(&flags_elem[0].in_sg[0], 1, VNET_HLEN, l2len);
> +		pcap_iov(flags_elem[0].in_sg, iov_cnt, VNET_HLEN,
> +			 hdrlen + optlen - VNET_HLEN);
>  
>  	if (flags & DUP_ACK) {
> -		elem_cnt = vu_collect(vdev, vq, &flags_elem[1], 1,
> -				      &flags_iov[1], 1, NULL,
> -				      hdrlen + optlen, NULL);
> -		if (elem_cnt == 1 &&
> -		    flags_elem[1].in_sg[0].iov_len >=
> -		    flags_elem[0].in_sg[0].iov_len) {
> -			memcpy(flags_elem[1].in_sg[0].iov_base,
> -			       flags_elem[0].in_sg[0].iov_base,
> -			       flags_elem[0].in_sg[0].iov_len);
> -
> -			vu_flush(vdev, vq, &flags_elem[1], 1, hdrlen + optlen);
> -
> -			if (*c->pcap) {
> -				pcap_iov(&flags_elem[1].in_sg[0], 1, VNET_HLEN,
> -					 l2len);
> -			}
> -		}
> +		dup_elem_cnt = tcp_vu_send_dup(c, vq, &flags_elem[elem_cnt],
> +					       &flags_iov[iov_cnt],
> +					       ARRAY_SIZE(flags_iov) - iov_cnt,
> +					       flags_elem[0].in_sg, iov_cnt,
> +					       hdrlen + optlen);
>  	}
> +	vu_flush(vdev, vq, flags_elem, elem_cnt, hdrlen + optlen);
> +	if (dup_elem_cnt) {
> +		vu_flush(vdev, vq, &flags_elem[elem_cnt], dup_elem_cnt,
> +			 hdrlen + optlen);
> +	}
> +
>  	vu_queue_notify(vdev, vq);
>  
>  	return 0;
> diff --git a/vu_common.h b/vu_common.h
> index 51f70084a7cb..817384175a1d 100644
> --- a/vu_common.h
> +++ b/vu_common.h
> @@ -15,26 +15,6 @@
>  #include "ip.h"
>  #include "virtio.h"
>  
> -static inline void *vu_eth(void *base)
> -{
> -	return ((char *)base + VNET_HLEN);
> -}
> -
> -static inline void *vu_ip(void *base)
> -{
> -	return (struct ethhdr *)vu_eth(base) + 1;
> -}
> -
> -static inline void *vu_payloadv4(void *base)
> -{
> -	return (struct iphdr *)vu_ip(base) + 1;
> -}
> -
> -static inline void *vu_payloadv6(void *base)
> -{
> -	return (struct ipv6hdr *)vu_ip(base) + 1;
> -}
> -
>  int vu_collect(const struct vu_dev *vdev, struct vu_virtq *vq,
>  	       struct vu_virtq_element *elem, int max_elem,
>  	       struct iovec *in_sg, size_t max_in_sg, size_t *in_total,
> -- 
> 2.54.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 --]