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: 4582 bytes --]

On Thu, Mar 12, 2026 at 09:19:52AM +0100, Laurent Vivier wrote:
> 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);

True.  It means the headers need to be jointly contiguous, not just
individually, which seems a pity - at least if we're attempting to
keep in-place updates.

> or as you proposed:
> 
>     with_header(hdr4, &current) {
>         ....
>     }
> 
> Thanks,
> Laurent
> 

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