Re: [PATCH v2 08/13] udp_vu: Use iov_tail in udp_vu_prepare()

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

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

On Mon, Mar 09, 2026 at 10:47:39AM +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>
> ---
>  iov.c    |  1 -
>  udp_vu.c | 64 ++++++++++++++++++++++++++++++--------------------------
>  2 files changed, 34 insertions(+), 31 deletions(-)
> 
> diff --git a/iov.c b/iov.c
> index 296f24b61067..1f554f5ac297 100644
> --- a/iov.c
> +++ b/iov.c
> @@ -313,7 +313,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 2a5d3f822bf6..a21a03dbf23e 100644
> --- a/udp_vu.c
> +++ b/udp_vu.c
> @@ -101,52 +101,54 @@ static ssize_t udp_vu_sock_recv(struct iovec *iov, size_t *cnt, 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;

Oof, having to reset the tail to the original position in order to
replay the PUTs in the right order seems kind of fragile.  I'm
beginning to wonder if for writing (not reading) headers, trying to
use an in-place pointer is more trouble than it's worth.  It would
certainly be easier to reason about this, if you always construct the
header in an external buffer, then there's just a sequence of
IOV_PUT_HEADER()s to stack them into the iov tail.

> +		IOV_PUT_HEADER(&current, eh);
> +		IOV_PUT_HEADER(&current, iph);
> +		IOV_PUT_HEADER(&current, uh);
>  	} 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);
>  	uh = IOV_REMOVE_HEADER(&payload, uh_storage);
>  
>  	if (ipv4)
> @@ -207,9 +210,9 @@ 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;
>  		size_t iov_cnt;
>  		ssize_t dlen;
> -		int elem_cnt;
>  
>  		vu_init_elem(elem, iov_vu, ARRAY_SIZE(elem));
>  
> @@ -224,19 +227,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);
>  			break;
>  		}
> +		elem_used = iov_cnt;

I don't see under circumstances elem_used would become different from
iov_cnt (and therefore I'm not sure why elem_used is needed).

>  
>  		/* release unused buffers */
> -		vu_queue_rewind(vq, elem_cnt - iov_cnt);
> -
> +		vu_queue_rewind(vq, elem_cnt - elem_used);
>  		if (iov_cnt > 0) {
>  			struct iov_tail data = IOV_TAIL(iov_vu, iov_cnt, 0);
> -			vu_set_vnethdr(iov_vu[0].iov_base, iov_cnt);
> -			udp_vu_prepare(c, &data, toside, dlen);
> +			vu_set_vnethdr(iov_vu[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, data.off);

A pcap_iov_tail() might be a useful at some point.

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