Re: [PATCH 07/12] udp_vu: Use iov_tail in udp_vu_prepare()

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

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

On Fri, Feb 27, 2026 at 03:03:25PM +0100, Laurent Vivier wrote:
> Rework udp_vu_prepare() to use IOV_REMOVE_HEADER() and IOV_PUT_HEADER()
> to walk through Ethernet, IP and UDP headers instead of the layout-specific
> helpers (vu_eth(), vu_ip(), vu_payloadv4(), vu_payloadv6()) that assume a
> contiguous buffer.  The payload length is now implicit in the iov_tail, so
> drop the dlen parameter.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

LGTM, a few nits below.

> ---
>  iov.c    |  1 -
>  udp_vu.c | 63 ++++++++++++++++++++++++++++++--------------------------
>  2 files changed, 34 insertions(+), 30 deletions(-)
> 
> diff --git a/iov.c b/iov.c
> index 2cf23d284e4a..7c9641968271 100644
> --- a/iov.c
> +++ b/iov.c
> @@ -304,7 +304,6 @@ void *iov_peek_header_(struct iov_tail *tail, void *v, size_t len, size_t align)
>   *
>   * Return: number of bytes written
>   */
> -/* cppcheck-suppress unusedFunction */
>  size_t iov_put_header_(struct iov_tail *tail, const void *v, size_t len)
>  {
>  	size_t l = len;
> diff --git a/udp_vu.c b/udp_vu.c
> index dd8904d65a38..6d87f4872268 100644
> --- a/udp_vu.c
> +++ b/udp_vu.c
> @@ -101,52 +101,54 @@ static ssize_t udp_vu_sock_recv(struct iov_tail *data, int s, bool v6)
>   * @c:		Execution context
>   * @data:	IO vector tail for the frame
>   * @toside:	Address information for one side of the flow
> - * @dlen:	Packet data length
>   *
>   * Return: Layer-4 length
>   */
>  static size_t udp_vu_prepare(const struct ctx *c, const struct iov_tail *data,
> -			     const struct flowside *toside, ssize_t dlen)
> +			     const struct flowside *toside)
>  {
> -	const struct iovec *iov = data->iov;
> -	struct ethhdr *eh;
> +	struct iov_tail current = *data;
> +	struct ethhdr *eh, eh_storage;
> +	struct udphdr *uh, uh_storage;
>  	size_t l4len;
>  
>  	/* ethernet header */
> -	eh = vu_eth(iov[0].iov_base);
> +	eh = IOV_REMOVE_HEADER(&current, eh_storage);
>  
>  	memcpy(eh->h_dest, c->guest_mac, sizeof(eh->h_dest));
>  	memcpy(eh->h_source, c->our_tap_mac, sizeof(eh->h_source));
>  
>  	/* initialize header */
>  	if (inany_v4(&toside->eaddr) && inany_v4(&toside->oaddr)) {
> -		struct iphdr *iph = vu_ip(iov[0].iov_base);
> -		struct udp_payload_t *bp = vu_payloadv4(iov[0].iov_base);
> -		const struct iovec payload_iov = {
> -			.iov_base = bp->data,
> -			.iov_len = dlen,
> -		};
> -		struct iov_tail payload = IOV_TAIL(&payload_iov, 1, 0);
> +		struct iphdr *iph, iph_storage;
>  
>  		eh->h_proto = htons(ETH_P_IP);
>  
> +		iph = IOV_REMOVE_HEADER(&current, iph_storage);
>  		*iph = (struct iphdr)L2_BUF_IP4_INIT(IPPROTO_UDP);
>  
> -		l4len = udp_update_hdr4(iph, &bp->uh, &payload, toside, true);
> +		uh = IOV_REMOVE_HEADER(&current, uh_storage);
> +		l4len = udp_update_hdr4(iph, uh, &current, toside, true);
> +
> +		current = *data;
> +		IOV_PUT_HEADER(&current, eh);
> +		IOV_PUT_HEADER(&current, iph);
> +		IOV_PUT_HEADER(&current, uh);

The puts for eh and uh can be factored out of the if/else.

>  	} else {
> -		struct ipv6hdr *ip6h = vu_ip(iov[0].iov_base);
> -		struct udp_payload_t *bp = vu_payloadv6(iov[0].iov_base);
> -		const struct iovec payload_iov = {
> -			.iov_base = bp->data,
> -			.iov_len = dlen,
> -		};
> -		struct iov_tail payload = IOV_TAIL(&payload_iov, 1, 0);
> +		struct ipv6hdr *ip6h, ip6h_storage;
>  
>  		eh->h_proto = htons(ETH_P_IPV6);
>  
> +		ip6h = IOV_REMOVE_HEADER(&current, ip6h_storage);
>  		*ip6h = (struct ipv6hdr)L2_BUF_IP6_INIT(IPPROTO_UDP);
>  
> -		l4len = udp_update_hdr6(ip6h, &bp->uh, &payload, toside, true);
> +		uh = IOV_REMOVE_HEADER(&current, uh_storage);
> +		l4len = udp_update_hdr6(ip6h, uh, &current, toside, true);
> +
> +		current = *data;
> +		IOV_PUT_HEADER(&current, eh);
> +		IOV_PUT_HEADER(&current, ip6h);
> +		IOV_PUT_HEADER(&current, uh);
>  	}
>  
>  	return l4len;
> @@ -165,9 +167,10 @@ static void udp_vu_csum(const struct flowside *toside,
>  	struct iov_tail payload = *data;
>  	struct udphdr *uh, uh_storage;
>  	bool ipv4 = src4 && dst4;
> +	int hdrlen = sizeof(struct ethhdr) +
> +		     (ipv4 ? sizeof(struct iphdr) : sizeof(struct ipv6hdr));
>  
> -	iov_drop_header(&payload,
> -			udp_vu_hdrlen(!ipv4) - sizeof(struct udphdr));
> +	iov_drop_header(&payload, hdrlen);

udp_vu_prepare() and udp_vu_csum() independently locate the UDP
header, but they're called in fairly close proximity.  Would it make
more sense to pass the UDP header and payload tail separately to each
of them?

>  	uh = IOV_REMOVE_HEADER(&payload, uh_storage);
>  
>  	if (ipv4)
> @@ -208,8 +211,8 @@ void udp_vu_sock_to_tap(const struct ctx *c, int s, int n, flow_sidx_t tosidx)
>  	}
>  
>  	for (i = 0; i < n; i++) {
> +		int elem_cnt, elem_used;
>  		ssize_t dlen;
> -		int elem_cnt;
>  
>  		vu_init_elem(elem, iov_vu, ARRAY_SIZE(elem));
>  
> @@ -225,18 +228,20 @@ void udp_vu_sock_to_tap(const struct ctx *c, int s, int n, flow_sidx_t tosidx)
>  			vu_queue_rewind(vq, elem_cnt);
>  			continue;
>  		}
> +		elem_used = data.cnt;
>  
>  		/* release unused buffers */
> -		vu_queue_rewind(vq, elem_cnt - data.cnt);
> +		vu_queue_rewind(vq, elem_cnt - elem_used);
>  
>  		if (data.cnt > 0) {
> -			vu_set_vnethdr(vdev, data.iov[0].iov_base, data.cnt);
> -			udp_vu_prepare(c, &data, toside, dlen);
> +			vu_set_vnethdr(vdev, data.iov[0].iov_base, elem_used);
> +			iov_drop_header(&data, VNET_HLEN);
> +			udp_vu_prepare(c, &data, toside);
>  			if (*c->pcap) {
>  				udp_vu_csum(toside, &data);
> -				pcap_iov(data.iov, data.cnt, VNET_HLEN);
> +				pcap_iov(data.iov, data.cnt, 0);
>  			}
> -			vu_flush(vdev, vq, elem, data.cnt);
> +			vu_flush(vdev, vq, elem, elem_used);
>  		}
>  	}
>  }
> -- 
> 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 --]