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

[Stefano Brivio <sbrivio@redhat.com>]

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:

>  
>  	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 */

-- 
Stefano