Re: [PATCH v2 07/10] tcp: Protect init socket pools with mutex for thread safety

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

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

On Fri, Jul 31, 2026 at 06:23:26PM +0200, Laurent Vivier wrote:
> The pre-opened socket pools init_sock_pool4/6 are consumed by
> tcp_conn_pool_sock() when creating new connections from any worker
> thread, and refilled by tcp_sock_refill_pool() from tcp_timer() in
> post_handler(). These can run concurrently on different threads.
> 
> Add a mutex protecting both operations in tcp_conn_sock() and
> tcp_sock_refill_init(), where init namespace pools are accessed.

I'm guessing you're going with a mutex rather than a per-thread socket
pool for simplicity?  That might be the right choice, but I do wonder
a bit about it.  The pool exists to avoid the latency of creating a
new socket for a new connection.  If the latency of taking the lock
exceeds that of creating a socket, there's no longer any point to the
pool.  In the unconstest case, that's almost certainly not the case -
a happy path futex() lock should be be much faster than a syscall.  If
the lock _is_ contested, I suspect opening a socket directly might win
- at least for host ns sockets.  For guest ns sockets, the latency is
higher because we need vfork()/setns()/etc.

Then again... taking a socket from the pool also involves writing the
pool, which potentially means a cacheline pingpong.  That might incur
a pretty substantial latency.

In theory we could trylock() and open a socket directly if we don't
get the lock immediately, but at that point it would probably be
simpler to have per-thread socket pools anyway.

I guess, since there is no meaningful shared state in the socket pool,
a per-thread pool seems like the more natural approach to me.

> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  tcp.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/tcp.c b/tcp.c
> index ab7cbfa5de83..aef689faf031 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -293,6 +293,7 @@
>  #include <sys/uio.h>
>  #include <time.h>
>  #include <arpa/inet.h>
> +#include <pthread.h>
>  
>  #include <linux/sockios.h>
>  #include <linux/sock_diag.h>
> @@ -439,6 +440,7 @@ static socklen_t tcp_info_size;
>  /* Pools for pre-opened sockets (in init) */
>  int init_sock_pool4		[TCP_SOCK_POOL_SIZE];
>  int init_sock_pool6		[TCP_SOCK_POOL_SIZE];
> +static pthread_mutex_t sock_pool_lock = PTHREAD_MUTEX_INITIALIZER;
>  
>  /**
>   * conn_at_sidx() - Get TCP connection specific flow at given sidx
> @@ -1581,7 +1583,11 @@ int tcp_conn_sock(sa_family_t af)
>  	int *pool = af == AF_INET6 ? init_sock_pool6 : init_sock_pool4;
>  	int s;
>  
> -	if ((s = tcp_conn_pool_sock(pool)) >= 0)
> +	pthread_mutex_lock(&sock_pool_lock);
> +	s = tcp_conn_pool_sock(pool);
> +	pthread_mutex_unlock(&sock_pool_lock);
> +
> +	if (s >= 0)
>  		return s;
>  
>  	/* If the pool is empty we just open a new one without refilling the
> @@ -2858,6 +2864,7 @@ int tcp_sock_refill_pool(int pool[], sa_family_t af)
>   */
>  static void tcp_sock_refill_init(const struct ctx *c)
>  {
> +	pthread_mutex_lock(&sock_pool_lock);
>  	if (c->ifi4) {
>  		int rc = tcp_sock_refill_pool(init_sock_pool4, AF_INET);
>  		if (rc < 0)
> @@ -2870,6 +2877,7 @@ static void tcp_sock_refill_init(const struct ctx *c)
>  			warn("TCP: Error refilling IPv6 host socket pool: %s",
>  			     strerror_(-rc));
>  	}
> +	pthread_mutex_unlock(&sock_pool_lock);
>  }
>  
>  /**
> -- 
> 2.54.0
> 

-- 
David Gibson (he or they)	| 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 --]