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

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

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

On Thu, Jan 12, 2023 at 07:09:25PM +0100, Stefano Brivio wrote:
> 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).

Hm, right.

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

Yeah, alright.  I don't love this either, since just "namespace" and
"init" are also kind of ambiguous without context.  But, it's more
consistent with the terminology in other places, so it will do for
now.

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