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

[Laurent Vivier <lvivier@redhat.com>]

On 4/3/26 08:20, Stefano Brivio wrote:
> 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).

I rework this part in the TCP series.

This is one of the reasons the multibuffer series are needed.

Here we have a simplification as we have one iovec per element, so the number of iovec is 
equal to the number of element, and head point to an element that is also one iovec.

iov_cnt is the total number of iovec for a frame (spread across several elements) but as 
we have one iovec per element it's also the number of element.

buf_cnt is the number of elements for a frame but also the number of iovec for a frame (as 
we have one iovec / element).

So:
- vu_flush() and vu_set_vnethdr() need the number of element / frame (buf_cnt as element 
count)
- pcap_iov() needs the number of iovec in a frame (buf_cnt as iovec count)

In this patch I only want to move vu_set_vnethdr(), no more.

Thanks,
Laurent