Re: [PATCH 2/4] udp: Fold checking of splice flag into udp_mmh_splice_port()

[Stefano Brivio <sbrivio@redhat.com>]

On Wed,  5 Jun 2024 11:39:01 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> udp_mmh_splice_port() is used to determine if a UDP datagram can be
> "spliced" (forwarded via a socket instead of tap).  We only invoke it if
> the origin socket has the 'splice' flag set.
> 
> Fold the checking of the flag into the helper itself, which makes the
> caller simpler.  It does mean we have a loop looking for a batch of
> spliceable or non-spliceable packets even in the case where the flag is
> clear.  This shouldn't be that expensive though, since each call to
> udp_mmh_splice_port() will return without accessing memory in that case.
> In any case we're going to need a similar loop in more cases with upcoming
> flow table work.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  udp.c | 31 ++++++++++++++++---------------
>  1 file changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/udp.c b/udp.c
> index 3abafc99..7487d2b2 100644
> --- a/udp.c
> +++ b/udp.c
> @@ -467,21 +467,25 @@ static int udp_splice_new_ns(void *arg)
>  
>  /**
>   * udp_mmh_splice_port() - Is source address of message suitable for splicing?
> - * @v6:		Is @sa a sockaddr_in6 (otherwise sockaddr_in)?
> + * @uref:	UDP epoll reference for incoming message's origin socket
>   * @mmh:	mmsghdr of incoming message
>   *
> - * Return: if @sa refers to localhost (127.0.0.1 or ::1) the port from
> - *         @sa in host order, otherwise -1.
> + * Return: if source address of message in @mmh refers to localhost (127.0.0.1

Pre-existing, and I guess this might change again with the complete
flow table implementation, so it probably doesn't make sense to fix
this now: it's 127.0.0.0/8, not necessarily 127.0.0.1.

As to whether we actually need to preserve a source address that's not
127.0.0.1, but in 127.0.0.0/8, as we "splice", I'm not quite sure. I
think we could bind() the socket in the target namespace, but I haven't
tried, and I don't know if it makes sense at all (I can't think of any
use case).

> + *         or ::1) its source port (host order), otherwise -1.
>   */
> -static int udp_mmh_splice_port(bool v6, const struct mmsghdr *mmh)
> +static int udp_mmh_splice_port(union udp_epoll_ref uref,
> +			       const struct mmsghdr *mmh)
>  {
>  	const struct sockaddr_in6 *sa6 = mmh->msg_hdr.msg_name;
>  	const struct sockaddr_in *sa4 = mmh->msg_hdr.msg_name;
>  
> -	if (v6 && IN6_IS_ADDR_LOOPBACK(&sa6->sin6_addr))
> +	if (!uref.splice)
> +		return -1;
> +
> +	if (uref.v6 && IN6_IS_ADDR_LOOPBACK(&sa6->sin6_addr))
>  		return ntohs(sa6->sin6_port);
>  
> -	if (!v6 && IN4_IS_ADDR_LOOPBACK(&sa4->sin_addr))
> +	if (!uref.v6 && IN4_IS_ADDR_LOOPBACK(&sa4->sin_addr))
>  		return ntohs(sa4->sin_port);
>  
>  	return -1;
> @@ -768,18 +772,15 @@ void udp_sock_handler(const struct ctx *c, union epoll_ref ref, uint32_t events,

(now renamed to udp_buf_sock_handler() if you're wondering)

>  
>  	for (i = 0; i < n; i += m) {
>  		int splicefrom = -1;
> -		m = n;
>  
> -		if (ref.udp.splice) {
> -			splicefrom = udp_mmh_splice_port(v6, mmh_recv + i);
> +		splicefrom = udp_mmh_splice_port(ref.udp, mmh_recv + i);
>  
> -			for (m = 1; i + m < n; m++) {
> -				int p;
> +		for (m = 1; i + m < n; m++) {
> +			int p;
>  
> -				p = udp_mmh_splice_port(v6, mmh_recv + i + m);
> -				if (p != splicefrom)
> -					break;
> -			}
> +			p = udp_mmh_splice_port(ref.udp, mmh_recv + i + m);
> +			if (p != splicefrom)
> +				break;
>  		}
>  
>  		if (splicefrom >= 0)

-- 
Stefano