Re: [PATCH v7 22/27] udp: Remove obsolete splice tracking

[Stefano Brivio <sbrivio@redhat.com>]

On Fri,  5 Jul 2024 12:07:19 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> Now that spliced datagrams are managed via the flow table, remove
> UDP_ACT_SPLICE_NS and UDP_ACT_SPLICE_INIT which are no longer used.  With
> those removed, the 'ts' field in udp_splice_port is also no longer used.
> struct udp_splice_port now contains just a socket fd, so replace it with
> a plain int in udp_splice_ns[] and udp_splice_init[].  The latter are still
> used for tracking of automatic port forwarding.
> 
> Finally, the 'splice' field of union udp_epoll_ref is no longer used so
> remove it as well.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  udp.c | 65 +++++++++++++++++------------------------------------------
>  udp.h |  3 +--
>  2 files changed, 19 insertions(+), 49 deletions(-)
> 
> diff --git a/udp.c b/udp.c
> index f4c696db..a4eb6d0f 100644
> --- a/udp.c
> +++ b/udp.c
> @@ -136,27 +136,15 @@ struct udp_tap_port {
>  	time_t ts;
>  };
>  
> -/**
> - * struct udp_splice_port - Bound socket for spliced communication
> - * @sock:	Socket bound to index port
> - * @ts:		Activity timestamp
> - */
> -struct udp_splice_port {
> -	int sock;
> -	time_t ts;
> -};
> -
>  /* Port tracking, arrays indexed by packet source port (host order) */
>  static struct udp_tap_port	udp_tap_map	[IP_VERSIONS][NUM_PORTS];
>  
>  /* "Spliced" sockets indexed by bound port (host order) */
> -static struct udp_splice_port udp_splice_ns  [IP_VERSIONS][NUM_PORTS];
> -static struct udp_splice_port udp_splice_init[IP_VERSIONS][NUM_PORTS];
> +static int udp_splice_ns  [IP_VERSIONS][NUM_PORTS];
> +static int udp_splice_init[IP_VERSIONS][NUM_PORTS];
>  
>  enum udp_act_type {
>  	UDP_ACT_TAP,
> -	UDP_ACT_SPLICE_NS,
> -	UDP_ACT_SPLICE_INIT,
>  	UDP_ACT_TYPE_MAX,
>  };
>  
> @@ -246,8 +234,8 @@ void udp_portmap_clear(void)
>  
>  	for (i = 0; i < NUM_PORTS; i++) {
>  		udp_tap_map[V4][i].sock = udp_tap_map[V6][i].sock = -1;
> -		udp_splice_ns[V4][i].sock = udp_splice_ns[V6][i].sock = -1;
> -		udp_splice_init[V4][i].sock = udp_splice_init[V6][i].sock = -1;
> +		udp_splice_ns[V4][i] = udp_splice_ns[V6][i] = -1;
> +		udp_splice_init[V4][i] = udp_splice_init[V6][i] = -1;
>  	}
>  }
>  
> @@ -1050,8 +1038,7 @@ int udp_tap_handler(struct ctx *c, uint8_t pif,
>  int udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
>  		  const void *addr, const char *ifname, in_port_t port)
>  {
> -	union udp_epoll_ref uref = { .splice = (c->mode == MODE_PASTA),
> -				     .orig = true, .port = port };
> +	union udp_epoll_ref uref = { .orig = true, .port = port };
>  	int s, r4 = FD_REF_MAX + 1, r6 = FD_REF_MAX + 1;
>  
>  	if (ns)
> @@ -1067,12 +1054,12 @@ int udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
>  					 ifname, port, uref.u32);
>  
>  			udp_tap_map[V4][port].sock = s < 0 ? -1 : s;
> -			udp_splice_init[V4][port].sock = s < 0 ? -1 : s;
> +			udp_splice_init[V4][port] = s < 0 ? -1 : s;
>  		} else {
>  			r4 = s = sock_l4(c, AF_INET, EPOLL_TYPE_UDP,
>  					 &in4addr_loopback,
>  					 ifname, port, uref.u32);
> -			udp_splice_ns[V4][port].sock = s < 0 ? -1 : s;
> +			udp_splice_ns[V4][port] = s < 0 ? -1 : s;
>  		}
>  	}
>  
> @@ -1084,12 +1071,12 @@ int udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
>  					 ifname, port, uref.u32);
>  
>  			udp_tap_map[V6][port].sock = s < 0 ? -1 : s;
> -			udp_splice_init[V6][port].sock = s < 0 ? -1 : s;
> +			udp_splice_init[V6][port] = s < 0 ? -1 : s;
>  		} else {
>  			r6 = s = sock_l4(c, AF_INET6, EPOLL_TYPE_UDP,
>  					 &in6addr_loopback,
>  					 ifname, port, uref.u32);
> -			udp_splice_ns[V6][port].sock = s < 0 ? -1 : s;
> +			udp_splice_ns[V6][port] = s < 0 ? -1 : s;
>  		}
>  	}
>  
> @@ -1130,7 +1117,6 @@ static void udp_splice_iov_init(void)
>  static void udp_timer_one(struct ctx *c, int v6, enum udp_act_type type,
>  			  in_port_t port, const struct timespec *now)
>  {
> -	struct udp_splice_port *sp;
>  	struct udp_tap_port *tp;
>  	int *sockp = NULL;
>  
> @@ -1143,20 +1129,6 @@ static void udp_timer_one(struct ctx *c, int v6, enum udp_act_type type,
>  			tp->flags = 0;
>  		}
>  
> -		break;
> -	case UDP_ACT_SPLICE_INIT:
> -		sp = &udp_splice_init[v6 ? V6 : V4][port];
> -
> -		if (now->tv_sec - sp->ts > UDP_CONN_TIMEOUT)
> -			sockp = &sp->sock;
> -
> -		break;
> -	case UDP_ACT_SPLICE_NS:
> -		sp = &udp_splice_ns[v6 ? V6 : V4][port];
> -
> -		if (now->tv_sec - sp->ts > UDP_CONN_TIMEOUT)
> -			sockp = &sp->sock;
> -
>  		break;
>  	default:
>  		return;
> @@ -1184,20 +1156,19 @@ static void udp_port_rebind(struct ctx *c, bool outbound)
>  		= outbound ? c->udp.fwd_out.f.map : c->udp.fwd_in.f.map;
>  	const uint8_t *rmap
>  		= outbound ? c->udp.fwd_in.f.map : c->udp.fwd_out.f.map;
> -	struct udp_splice_port (*socks)[NUM_PORTS]
> -		= outbound ? udp_splice_ns : udp_splice_init;
> +	int (*socks)[NUM_PORTS] = outbound ? udp_splice_ns : udp_splice_init;

Nit: this should be moved up now, before the declaration of 'fmap'.

>  	unsigned port;
>  
>  	for (port = 0; port < NUM_PORTS; port++) {
>  		if (!bitmap_isset(fmap, port)) {
> -			if (socks[V4][port].sock >= 0) {
> -				close(socks[V4][port].sock);
> -				socks[V4][port].sock = -1;
> +			if (socks[V4][port] >= 0) {
> +				close(socks[V4][port]);
> +				socks[V4][port] = -1;
>  			}
>  
> -			if (socks[V6][port].sock >= 0) {
> -				close(socks[V6][port].sock);
> -				socks[V6][port].sock = -1;
> +			if (socks[V6][port] >= 0) {
> +				close(socks[V6][port]);
> +				socks[V6][port] = -1;
>  			}
>  
>  			continue;
> @@ -1207,8 +1178,8 @@ static void udp_port_rebind(struct ctx *c, bool outbound)
>  		if (bitmap_isset(rmap, port))
>  			continue;
>  
> -		if ((c->ifi4 && socks[V4][port].sock == -1) ||
> -		    (c->ifi6 && socks[V6][port].sock == -1))
> +		if ((c->ifi4 && socks[V4][port] == -1) ||
> +		    (c->ifi6 && socks[V6][port] == -1))
>  			udp_sock_init(c, outbound, AF_UNSPEC, NULL, NULL, port);
>  	}
>  }
> diff --git a/udp.h b/udp.h
> index db5e546e..310f42fd 100644
> --- a/udp.h
> +++ b/udp.h
> @@ -36,8 +36,7 @@ union udp_epoll_ref {
>  	struct {
>  		in_port_t	port;
>  		uint8_t		pif;
> -		bool		splice:1,
> -				orig:1,
> +		bool		orig:1,

The comment to the union should be updated, removing 'splice'. While at
it, I guess you could also drop 'bound' (removed in 851723924356 ("udp:
Remove the @bound field from union udp_epoll_ref"), but the comment wasn't
updated).

-- 
Stefano