Re: [PATCH 3/5] tcp: Move socket pool declarations around

[Stefano Brivio <sbrivio@redhat.com>]

On Mon,  9 Jan 2023 11:50:24 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> tcp_splice.c has some explicit extern declarations to access the socket
> pools.  This is pretty dangerous - if we changed the type of these
> variables in tcp.c, we'd have tcp.c and tcp_splice.c using the memory
> differently with no compiler error.  So, move the extern declarations to
> tcp_conn.h so they're visible to both tcp.c and tcp_splice.c, but not the
> rest of pasta.
> 
> In fact the pools for the guest namespace are necessarily only used by
> tcp_splice.c - we have no sockets on the guest side if we're not splicing.
> So move those declarations - and the functions that deal exclusively with
> them - to tcp_splice.c
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  tcp.c        | 41 ++++-------------------------------------
>  tcp.h        |  2 --
>  tcp_conn.h   | 10 ++++++++--
>  tcp_splice.c | 50 +++++++++++++++++++++++++++++++++++++++++++-------
>  4 files changed, 55 insertions(+), 48 deletions(-)
> 
> diff --git a/tcp.c b/tcp.c
> index 4da823c..19fd17b 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -370,8 +370,6 @@ struct tcp6_l2_head {	/* For MSS6 macro: keep in sync with tcp6_l2_buf_t */
>  #define FIN_TIMEOUT			60
>  #define ACT_TIMEOUT			7200
>  
> -#define TCP_SOCK_POOL_TSH		16 /* Refill in ns if > x used */
> -
>  #define LOW_RTT_TABLE_SIZE		8
>  #define LOW_RTT_THRESHOLD		10 /* us */
>  
> @@ -595,11 +593,9 @@ static inline struct tcp_tap_conn *conn_at_idx(int index)
>  /* Table for lookup from remote address, local port, remote port */
>  static struct tcp_tap_conn *tc_hash[TCP_HASH_TABLE_SIZE];
>  
> -/* Pools for pre-opened sockets */
> +/* Pools for pre-opened sockets (init namespace) */
>  int init_sock_pool4		[TCP_SOCK_POOL_SIZE];
>  int init_sock_pool6		[TCP_SOCK_POOL_SIZE];
> -int ns_sock_pool4		[TCP_SOCK_POOL_SIZE];
> -int ns_sock_pool6		[TCP_SOCK_POOL_SIZE];
>  
>  /**
>   * tcp_conn_epoll_events() - epoll events mask for given connection state
> @@ -2988,7 +2984,7 @@ static int tcp_ns_socks_init(void *arg)
>   * @pool:	Pool of sockets to refill
>   * @af:		Address family to use
>   */
> -static void tcp_sock_refill_pool(const struct ctx *c, int pool[], int af)
> +void tcp_sock_refill_pool(const struct ctx *c, int pool[], int af)
>  {
>  	int i;
>  
> @@ -3022,26 +3018,6 @@ static void tcp_sock_refill_init(const struct ctx *c)
>  		tcp_sock_refill_pool(c, init_sock_pool6, AF_INET6);
>  }
>  
> -/**
> - * tcp_sock_refill_ns() - Refill pools of pre-opened sockets in namespace
> - * @arg:	Execution context cast to void *
> - *
> - * Return: 0
> - */
> -static int tcp_sock_refill_ns(void *arg)
> -{
> -	const struct ctx *c = (const struct ctx *)arg;
> -
> -	ns_enter(c);
> -
> -	if (c->ifi4)
> -		tcp_sock_refill_pool(c, ns_sock_pool4, AF_INET);
> -	if (c->ifi6)
> -		tcp_sock_refill_pool(c, ns_sock_pool6, AF_INET6);
> -
> -	return 0;
> -}
> -
>  /**
>   * tcp_init() - Get initial sequence, hash secret, initialise per-socket data
>   * @c:		Execution context
> @@ -3090,8 +3066,6 @@ int tcp_init(struct ctx *c)
>  
>  	memset(init_sock_pool4,		0xff,	sizeof(init_sock_pool4));
>  	memset(init_sock_pool6,		0xff,	sizeof(init_sock_pool6));
> -	memset(ns_sock_pool4,		0xff,	sizeof(ns_sock_pool4));
> -	memset(ns_sock_pool6,		0xff,	sizeof(ns_sock_pool6));
>  	memset(tcp_sock_init_ext,	0xff,	sizeof(tcp_sock_init_ext));
>  	memset(tcp_sock_ns,		0xff,	sizeof(tcp_sock_ns));
>  
> @@ -3101,8 +3075,6 @@ int tcp_init(struct ctx *c)
>  		tcp_splice_init(c);
>  
>  		NS_CALL(tcp_ns_socks_init, c);
> -
> -		NS_CALL(tcp_sock_refill_ns, c);
>  	}
>  
>  	return 0;
> @@ -3255,11 +3227,6 @@ void tcp_timer(struct ctx *c, const struct timespec *ts)
>  	}
>  
>  	tcp_sock_refill_init(c);
> -	if (c->mode == MODE_PASTA) {
> -		if ((c->ifi4 && ns_sock_pool4[TCP_SOCK_POOL_TSH] < 0) ||
> -		    (c->ifi6 && ns_sock_pool6[TCP_SOCK_POOL_TSH] < 0))
> -			NS_CALL(tcp_sock_refill_ns, c);
> -
> -		tcp_splice_pipe_refill(c);
> -	}
> +	if (c->mode == MODE_PASTA)
> +		tcp_splice_refill(c);
>  }
> diff --git a/tcp.h b/tcp.h
> index 739b451..236038f 100644
> --- a/tcp.h
> +++ b/tcp.h
> @@ -11,8 +11,6 @@
>  #define TCP_CONN_INDEX_BITS		17	/* 128k */
>  #define TCP_MAX_CONNS			(1 << TCP_CONN_INDEX_BITS)
>  
> -#define TCP_SOCK_POOL_SIZE		32
> -
>  struct ctx;
>  
>  void tcp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events,
> diff --git a/tcp_conn.h b/tcp_conn.h
> index 70f4a7c..9951b0a 100644
> --- a/tcp_conn.h
> +++ b/tcp_conn.h
> @@ -182,11 +182,17 @@ union tcp_conn {
>  /* TCP connections */
>  extern union tcp_conn tc[];
>  
> +/* Socket pools */
> +#define TCP_SOCK_POOL_SIZE		32
> +
> +extern int init_sock_pool4	[TCP_SOCK_POOL_SIZE];
> +extern int init_sock_pool6	[TCP_SOCK_POOL_SIZE];
> +
>  void tcp_splice_conn_update(struct ctx *c, struct tcp_splice_conn *new);
>  void tcp_table_compact(struct ctx *c, union tcp_conn *hole);
>  void tcp_splice_destroy(struct ctx *c, union tcp_conn *conn_union);
>  void tcp_splice_timer(struct ctx *c, union tcp_conn *conn_union);
> -void tcp_splice_pipe_refill(const struct ctx *c);
> -
> +void tcp_sock_refill_pool(const struct ctx *c, int pool[], int af);
> +void tcp_splice_refill(const struct ctx *c);
>  
>  #endif /* TCP_CONN_H */
> diff --git a/tcp_splice.c b/tcp_splice.c
> index 72b1672..688d776 100644
> --- a/tcp_splice.c
> +++ b/tcp_splice.c
> @@ -61,11 +61,11 @@
>  #define TCP_SPLICE_CONN_PRESSURE	30	/* % of conn_count */
>  #define TCP_SPLICE_FILE_PRESSURE	30	/* % of c->nofile */
>  
> -/* From tcp.c */
> -extern int init_sock_pool4		[TCP_SOCK_POOL_SIZE];
> -extern int init_sock_pool6		[TCP_SOCK_POOL_SIZE];
> -extern int ns_sock_pool4		[TCP_SOCK_POOL_SIZE];
> -extern int ns_sock_pool6		[TCP_SOCK_POOL_SIZE];
> +/* Pools for pre-opened sockets (guest namespace) */

What you mean by "guest namespace" is perfectly clear, but strictly
speaking, that's not a thing, and I'm a bit concerned that it might
cause confusion (especially with passt).

What about using "init" (instead of "init namespace") above, and
"namespace" here?

-- 
Stefano