Re: [PATCH v2 4/7] udp: Share more logic between vu and non-vu reply socket paths

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

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

On Wed, Mar 26, 2025 at 11:14:33PM +0100, Stefano Brivio wrote:
> On Wed, 26 Mar 2025 14:44:04 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > Share some additional miscellaneous logic between the vhost-user and "buf"
> > paths for data on udp reply sockets.  The biggest piece is error handling
> > of cases where we can't forward between the two pifs of the flow.  We also
> > make common some more simple logic locating the correct flow and its
> > parameters.
> > 
> > This adds some lines of code due to extra comment lines, but nonetheless
> > reduces logic duplication.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  udp.c    | 41 ++++++++++++++++++++++++++---------------
> >  udp_vu.c | 26 +++++++++++---------------
> >  udp_vu.h |  3 ++-
> >  3 files changed, 39 insertions(+), 31 deletions(-)
> > 
> > diff --git a/udp.c b/udp.c
> > index 55021ac4..4258812e 100644
> > --- a/udp.c
> > +++ b/udp.c
> > @@ -754,24 +754,25 @@ void udp_listen_sock_handler(const struct ctx *c,
> >  /**
> >   * udp_buf_reply_sock_data() - Handle new data from flow specific socket
> >   * @c:		Execution context
> > - * @ref:	epoll reference
> > + * @s:		Socket to read data from
> > + * @tosidx:	Flow & side to forward data from @s to
> >   * @now:	Current timestamp
> >   *
> > + * Return: true on success, false if can't forward from socket to flow's pif
> 
> "on success"...
> 
> > + *
> >   * #syscalls recvmmsg
> >   */
> > -static void udp_buf_reply_sock_data(const struct ctx *c, union epoll_ref ref,
> > +static bool udp_buf_reply_sock_data(const struct ctx *c,
> > +				    int s, flow_sidx_t tosidx,
> >  				    const struct timespec *now)
> >  {
> > -	flow_sidx_t tosidx = flow_sidx_opposite(ref.flowside);
> >  	const struct flowside *toside = flowside_at_sidx(tosidx);
> > -	struct udp_flow *uflow = udp_at_sidx(ref.flowside);
> > +	struct udp_flow *uflow = udp_at_sidx(tosidx);
> >  	uint8_t topif = pif_at_sidx(tosidx);
> > -	int n, i, from_s;
> > -
> > -	from_s = uflow->s[ref.flowside.sidei];
> > +	int n, i;
> >  
> > -	if ((n = udp_sock_recv(c, from_s, udp_mh_recv)) <= 0)
> > -		return;
> > +	if ((n = udp_sock_recv(c, s, udp_mh_recv)) <= 0)
> > +		return true;
> 
> is not exactly accurate: 0 means either no datagrams (should never
> happen) or error on recvmmsg(). But I think it's clear enough, and
> you'll have follow-ups anyway, so I went ahead and applied this.
> 
> Maybe:
> 
>  * Return: false if we can't forward from socket to flow's pif, true otherwise
> 
> ? Actually, looking at the caller:

Yeah.  While writing subsequent patches, I think I spotted a better
way to structure this.  I'll put some fixups in the next series.

> >  	flow_trace(uflow, "Received %d datagrams on reply socket", n);
> >  	uflow->ts = now->tv_sec;
> > @@ -790,11 +791,10 @@ static void udp_buf_reply_sock_data(const struct ctx *c, union epoll_ref ref,
> >  	} else if (topif == PIF_TAP) {
> >  		tap_send_frames(c, &udp_l2_iov[0][0], UDP_NUM_IOVS, n);
> >  	} else {
> > -		uint8_t frompif = pif_at_sidx(ref.flowside);
> > -
> > -		flow_err(uflow, "No support for forwarding UDP from %s to %s",
> > -			 pif_name(frompif), pif_name(topif));
> > +		return false;
> >  	}
> > +
> > +	return true;
> >  }
> >  
> >  /**
> > @@ -821,10 +821,21 @@ void udp_reply_sock_handler(const struct ctx *c, union epoll_ref ref,
> >  	}
> >  
> >  	if (events & EPOLLIN) {
> > +		flow_sidx_t tosidx = flow_sidx_opposite(ref.flowside);
> > +		int s = ref.fd;
> > +		bool ret;
> > +
> >  		if (c->mode == MODE_VU)
> > -			udp_vu_reply_sock_data(c, ref, now);
> > +			ret = udp_vu_reply_sock_data(c, s, tosidx, now);
> >  		else
> > -			udp_buf_reply_sock_data(c, ref, now);
> > +			ret = udp_buf_reply_sock_data(c, s, tosidx, now);
> > +
> > +		if (!ret) {
> 
> the opposite would probably be less surprising, if (ret) flow_err(...).
> 
> > +			flow_err(uflow,
> > +				 "No support for forwarding UDP from %s to %s",
> > +				 pif_name(pif_at_sidx(ref.flowside)),
> > +				 pif_name(pif_at_sidx(tosidx)));
> > +		}
> >  	}
> >  }
> >  
> > diff --git a/udp_vu.c b/udp_vu.c
> > index 6e1823a9..06bdeae6 100644
> > --- a/udp_vu.c
> > +++ b/udp_vu.c
> > @@ -273,38 +273,32 @@ void udp_vu_listen_sock_data(const struct ctx *c, union epoll_ref ref,
> >  /**
> >   * udp_vu_reply_sock_data() - Handle new data from flow specific socket
> >   * @c:		Execution context
> > - * @ref:	epoll reference
> > + * @s:		Socket to read data from
> > + * @tosidx:	Flow & side to forward data from @s to
> >   * @now:	Current timestamp
> > + *
> > + * Return: true on success, false if can't forward from socket to flow's pif
> >   */
> > -void udp_vu_reply_sock_data(const struct ctx *c, union epoll_ref ref,
> > +bool udp_vu_reply_sock_data(const struct ctx *c, int s, flow_sidx_t tosidx,
> >  			    const struct timespec *now)
> >  {
> > -	flow_sidx_t tosidx = flow_sidx_opposite(ref.flowside);
> >  	const struct flowside *toside = flowside_at_sidx(tosidx);
> >  	bool v6 = !(inany_v4(&toside->eaddr) && inany_v4(&toside->oaddr));
> > -	struct udp_flow *uflow = udp_at_sidx(ref.flowside);
> > -	int from_s = uflow->s[ref.flowside.sidei];
> > +	struct udp_flow *uflow = udp_at_sidx(tosidx);
> >  	struct vu_dev *vdev = c->vdev;
> >  	struct vu_virtq *vq = &vdev->vq[VHOST_USER_RX_QUEUE];
> > -	uint8_t topif = pif_at_sidx(tosidx);
> >  	int i;
> >  
> >  	ASSERT(uflow);
> >  
> > -	if (topif != PIF_TAP) {
> > -		uint8_t frompif = pif_at_sidx(ref.flowside);
> > -
> > -		flow_err(uflow,
> > -			 "No support for forwarding UDP from %s to %s",
> > -			 pif_name(frompif), pif_name(topif));
> > -		return;
> > -	}
> > +	if (pif_at_sidx(tosidx) != PIF_TAP)
> > +		return false;
> >  
> >  	for (i = 0; i < UDP_MAX_FRAMES; i++) {
> >  		ssize_t dlen;
> >  		int iov_used;
> >  
> > -		iov_used = udp_vu_sock_recv(c, from_s, v6, &dlen);
> > +		iov_used = udp_vu_sock_recv(c, s, v6, &dlen);
> >  		if (iov_used <= 0)
> >  			break;
> >  		flow_trace(uflow, "Received 1 datagram on reply socket");
> > @@ -318,4 +312,6 @@ void udp_vu_reply_sock_data(const struct ctx *c, union epoll_ref ref,
> >  		}
> >  		vu_flush(vdev, vq, elem, iov_used);
> >  	}
> > +
> > +	return true;
> >  }
> > diff --git a/udp_vu.h b/udp_vu.h
> > index 4f2262d0..2299b51f 100644
> > --- a/udp_vu.h
> > +++ b/udp_vu.h
> > @@ -8,6 +8,7 @@
> >  
> >  void udp_vu_listen_sock_data(const struct ctx *c, union epoll_ref ref,
> >  			     const struct timespec *now);
> > -void udp_vu_reply_sock_data(const struct ctx *c, union epoll_ref ref,
> > +bool udp_vu_reply_sock_data(const struct ctx *c, int s, flow_sidx_t tosidx,
> >  			    const struct timespec *now);
> > +
> >  #endif /* UDP_VU_H */
> 

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