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

[Laurent Vivier <lvivier@redhat.com>]

On 3/12/26 05:30, David Gibson wrote:
> 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.

I can use one PEEK and PUT with something like:
         struct {
                 struct ethhdr eh;
                 struct struct iphdr
		struct udphdr *uh
         } __attribute__((__packed__)) *hdr4, hdr4_storage;
	hdr4 = IOV_PEEK_HEADER(&current, hdr4_storage);
         ...
         IOV_PUT_HEADER(&current, hdr4);

or as you proposed:

     with_header(hdr4, &current) {
         ....
     }

Thanks,
Laurent