Re: [PATCH 03/10] vu_common: Move vnethdr setup into vu_flush()

[Stefano Brivio <sbrivio@redhat.com>]

On Wed,  1 Apr 2026 21:18:19 +0200
Laurent Vivier <lvivier@redhat.com> wrote:

> Every caller of vu_flush() was calling vu_set_vnethdr() beforehand with
> the same pattern.  Move it into vu_flush().
> 
> Remove vu_queue_notify() from vu_flush() and let callers invoke it
> explicitly.  This allows paths that perform multiple flushes, such as
> tcp_vu_send_flag() and tcp_vu_data_from_sock(), to issue a single guest
> notification at the end.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  tcp_vu.c    | 19 ++++++++-----------
>  udp_vu.c    |  3 +--
>  vu_common.c |  9 +++++----
>  vu_common.h |  1 -
>  4 files changed, 14 insertions(+), 18 deletions(-)
> 
> diff --git a/tcp_vu.c b/tcp_vu.c
> index dc0e17c0f03f..0cd01190d612 100644
> --- a/tcp_vu.c
> +++ b/tcp_vu.c
> @@ -82,7 +82,6 @@ int tcp_vu_send_flag(const struct ctx *c, struct tcp_tap_conn *conn, int flags)
>  	struct ethhdr *eh;
>  	uint32_t seq;
>  	int elem_cnt;
> -	int nb_ack;
>  	int ret;
>  
>  	hdrlen = tcp_vu_hdrlen(CONN_V6(conn));
> @@ -97,8 +96,6 @@ int tcp_vu_send_flag(const struct ctx *c, struct tcp_tap_conn *conn, int flags)
>  	assert(flags_elem[0].in_sg[0].iov_len >=
>  	       MAX(hdrlen + sizeof(*opts), ETH_ZLEN + VNET_HLEN));
>  
> -	vu_set_vnethdr(flags_elem[0].in_sg[0].iov_base, 1);
> -
>  	eh = vu_eth(flags_elem[0].in_sg[0].iov_base);
>  
>  	memcpy(eh->h_dest, c->guest_mac, sizeof(eh->h_dest));
> @@ -143,9 +140,10 @@ int tcp_vu_send_flag(const struct ctx *c, struct tcp_tap_conn *conn, int flags)
>  	l2len = optlen + hdrlen - VNET_HLEN;
>  	vu_pad(&flags_elem[0].in_sg[0], l2len);
>  
> +	vu_flush(vdev, vq, flags_elem, 1);
> +
>  	if (*c->pcap)
>  		pcap_iov(&flags_elem[0].in_sg[0], 1, VNET_HLEN);
> -	nb_ack = 1;
>  
>  	if (flags & DUP_ACK) {
>  		elem_cnt = vu_collect(vdev, vq, &flags_elem[1], 1,
> @@ -157,14 +155,14 @@ int tcp_vu_send_flag(const struct ctx *c, struct tcp_tap_conn *conn, int flags)
>  			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);
> -			nb_ack++;
> +
> +			vu_flush(vdev, vq, &flags_elem[1], 1);
>  
>  			if (*c->pcap)
>  				pcap_iov(&flags_elem[1].in_sg[0], 1, VNET_HLEN);
>  		}
>  	}
> -
> -	vu_flush(vdev, vq, flags_elem, nb_ack);
> +	vu_queue_notify(vdev, vq);
>  
>  	return 0;
>  }
> @@ -451,7 +449,6 @@ int tcp_vu_data_from_sock(const struct ctx *c, struct tcp_tap_conn *conn)
>  		assert(frame_size >= hdrlen);
>  
>  		dlen = frame_size - hdrlen;
> -		vu_set_vnethdr(iov->iov_base, buf_cnt);
>  
>  		/* The IPv4 header checksum varies only with dlen */
>  		if (previous_dlen != dlen)
> @@ -464,14 +461,14 @@ int tcp_vu_data_from_sock(const struct ctx *c, struct tcp_tap_conn *conn)
>  		l2len = dlen + hdrlen - VNET_HLEN;
>  		vu_pad(iov, l2len);
>  
> +		vu_flush(vdev, vq, &elem[head[i]], buf_cnt);
> +
>  		if (*c->pcap)
>  			pcap_iov(iov, buf_cnt, VNET_HLEN);
>  
>  		conn->seq_to_tap += dlen;
>  	}
> -
> -	/* send packets */
> -	vu_flush(vdev, vq, elem, iov_cnt);
> +	vu_queue_notify(vdev, vq);

If I understand correctly, this makes iov_cnt set by tcp_vu_sock_recv()
redundant because you're now calling vu_flush() from inside the loop
that iterates up to head_cnt (which is the only count we need to know).

Maybe it would be clearer to have a patch in this series dropping
iov_cnt as argument altogether (especially the commit message of that
kind of change might make things clearer).

>  
>  	conn_flag(c, conn, ACK_FROM_TAP_DUE);
>  
> diff --git a/udp_vu.c b/udp_vu.c
> index cc69654398f0..f8629af58ab5 100644
> --- a/udp_vu.c
> +++ b/udp_vu.c
> @@ -124,8 +124,6 @@ static int udp_vu_sock_recv(const struct ctx *c, struct vu_virtq *vq, int s,
>  	l2len = *dlen + hdrlen - VNET_HLEN;
>  	vu_pad(&iov_vu[0], l2len);
>  
> -	vu_set_vnethdr(iov_vu[0].iov_base, elem_used);
> -
>  	/* release unused buffers */
>  	vu_queue_rewind(vq, elem_cnt - elem_used);
>  
> @@ -230,6 +228,7 @@ void udp_vu_sock_to_tap(const struct ctx *c, int s, int n, flow_sidx_t tosidx)
>  				pcap_iov(iov_vu, iov_used, VNET_HLEN);
>  			}
>  			vu_flush(vdev, vq, elem, iov_used);
> +			vu_queue_notify(vdev, vq);
>  		}
>  	}
>  }
> diff --git a/vu_common.c b/vu_common.c
> index 13b1e51001d4..57949ca32309 100644
> --- a/vu_common.c
> +++ b/vu_common.c
> @@ -118,7 +118,8 @@ int vu_collect(const struct vu_dev *vdev, struct vu_virtq *vq,
>   * @vnethdr:		Address of the header to set
>   * @num_buffers:	Number of guest buffers of the frame
>   */
> -void vu_set_vnethdr(struct virtio_net_hdr_mrg_rxbuf *vnethdr, int num_buffers)
> +static void vu_set_vnethdr(struct virtio_net_hdr_mrg_rxbuf *vnethdr,
> +			   int num_buffers)
>  {
>  	vnethdr->hdr = VU_HEADER;
>  	/* Note: if VIRTIO_NET_F_MRG_RXBUF is not negotiated,
> @@ -139,6 +140,8 @@ void vu_flush(const struct vu_dev *vdev, struct vu_virtq *vq,
>  {
>  	int i;
>  
> +	vu_set_vnethdr(elem[0].in_sg[0].iov_base, elem_cnt);
> +
>  	for (i = 0; i < elem_cnt; i++) {
>  		size_t elem_size = iov_size(elem[i].in_sg, elem[i].in_num);
>  
> @@ -146,7 +149,6 @@ void vu_flush(const struct vu_dev *vdev, struct vu_virtq *vq,
>  	}
>  
>  	vu_queue_flush(vdev, vq, elem_cnt);
> -	vu_queue_notify(vdev, vq);
>  }
>  
>  /**
> @@ -260,8 +262,6 @@ int vu_send_single(const struct ctx *c, const void *buf, size_t size)
>  		goto err;
>  	}
>  
> -	vu_set_vnethdr(in_sg[0].iov_base, elem_cnt);
> -
>  	total -= VNET_HLEN;
>  
>  	/* copy data from the buffer to the iovec */
> @@ -271,6 +271,7 @@ int vu_send_single(const struct ctx *c, const void *buf, size_t size)
>  		pcap_iov(in_sg, in_total, VNET_HLEN);
>  
>  	vu_flush(vdev, vq, elem, elem_cnt);
> +	vu_queue_notify(vdev, vq);
>  
>  	trace("vhost-user sent %zu", total);
>  
> diff --git a/vu_common.h b/vu_common.h
> index 7b060eb6184f..4037ab765b7d 100644
> --- a/vu_common.h
> +++ b/vu_common.h
> @@ -39,7 +39,6 @@ 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,
>  	       size_t size, size_t *collected);
> -void vu_set_vnethdr(struct virtio_net_hdr_mrg_rxbuf *vnethdr, int num_buffers);
>  void vu_flush(const struct vu_dev *vdev, struct vu_virtq *vq,
>  	      struct vu_virtq_element *elem, int elem_cnt);
>  void vu_kick_cb(struct vu_dev *vdev, union epoll_ref ref,

-- 
Stefano