Currently tcp_sock_refill_pool() stops as soon as it finds an entry in the
pool with a valid fd.  This appears to makes sense: we always use fds from
the front of the pool, so if we find a filled one, the rest of the pool
should be filled as well.

However, that's not quite correct.  If a previous refill hit errors trying
to open new sockets, it could leave gaps between blocks of valid fds. We're
going to add some changes that could make that more likely.

So, for robustness, instead skip over the filled entry but still try to
refill the rest of the array.  We expect simply iterating over the pool to
be of small cost compared to even a single system call, so this shouldn't
have much impact.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 tcp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tcp.c b/tcp.c
index 74a3e310..9eec9f3a 100644
--- a/tcp.c
+++ b/tcp.c
@@ -3014,7 +3014,7 @@ void tcp_sock_refill_pool(const struct ctx *c, int pool[],
sa_family_t af)
 
 	for (i = 0; i < TCP_SOCK_POOL_SIZE; i++) {
 		if (pool[i] >= 0)
-			break;
+			continue;
 
 		pool[i] = tcp_conn_new_sock(c, af);
 	}
-- 
2.43.2

Currently if tcp_sock_refill_pool() is unable to fill all the slots in the
pool, it will silently exit.  This might lead to a later attempt to get
fds from the pool to fail at which point it will be harder to tell what
originally went wrong.

Instead add warnings if we're unable to refill any of the socket pools when
requested.  We have tcp_sock_refill_pool() return an error and report it
in the callers, because those callers have more context allowing for a
more useful message.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 tcp.c        | 24 ++++++++++++++++++------
 tcp_conn.h   |  2 +-
 tcp_splice.c | 16 ++++++++++++----
 3 files changed, 31 insertions(+), 11 deletions(-)

diff --git a/tcp.c b/tcp.c
index d49210bc..ad56ffc3 100644
--- a/tcp.c
+++ b/tcp.c
@@ -3007,8 +3007,10 @@ static int tcp_ns_socks_init(void *arg)
  * @c:		Execution context
  * @pool:	Pool of sockets to refill
  * @af:		Address family to use
+ *
+ * Return: 0 on success, -ve error code if there was at least one error
  */
-void tcp_sock_refill_pool(const struct ctx *c, int pool[], sa_family_t af)
+int tcp_sock_refill_pool(const struct ctx *c, int pool[], sa_family_t af)
 {
 	int i;
 
@@ -3017,8 +3019,10 @@ void tcp_sock_refill_pool(const struct ctx *c, int pool[],
sa_family_t af)
 			continue;
 
 		if ((pool[i] = tcp_conn_new_sock(c, af)) < 0)
-			break;
+			return pool[i];
 	}
+
+	return 0;
 }
 
 /**
@@ -3027,10 +3031,18 @@ void tcp_sock_refill_pool(const struct ctx *c, int pool[],
sa_family_t af)
  */
 static void tcp_sock_refill_init(const struct ctx *c)
 {
-	if (c->ifi4)
-		tcp_sock_refill_pool(c, init_sock_pool4, AF_INET);
-	if (c->ifi6)
-		tcp_sock_refill_pool(c, init_sock_pool6, AF_INET6);
+	if (c->ifi4) {
+		int rc = tcp_sock_refill_pool(c, init_sock_pool4, AF_INET);
+		if (rc < 0)
+			warn("TCP: Error refilling IPv4 host socket pool: %s",
+			     strerror(-rc));
+	}
+	if (c->ifi6) {
+		int rc = tcp_sock_refill_pool(c, init_sock_pool6, AF_INET6);
+		if (rc < 0)
+			warn("TCP: Error refilling IPv6 host socket pool: %s",
+			     strerror(-rc));
+	}
 }
 
 /**
diff --git a/tcp_conn.h b/tcp_conn.h
index 20c7cb8b..92d4807a 100644
--- a/tcp_conn.h
+++ b/tcp_conn.h
@@ -160,7 +160,7 @@ bool tcp_splice_flow_defer(union flow *flow);
 void tcp_splice_timer(const struct ctx *c, union flow *flow);
 int tcp_conn_pool_sock(int pool[]);
 int tcp_conn_new_sock(const struct ctx *c, sa_family_t af);
-void tcp_sock_refill_pool(const struct ctx *c, int pool[], sa_family_t af);
+int tcp_sock_refill_pool(const struct ctx *c, int pool[], sa_family_t af);
 void tcp_splice_refill(const struct ctx *c);
 
 #endif /* TCP_CONN_H */
diff --git a/tcp_splice.c b/tcp_splice.c
index cc9745e8..ee68029b 100644
--- a/tcp_splice.c
+++ b/tcp_splice.c
@@ -710,10 +710,18 @@ static int tcp_sock_refill_ns(void *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);
+	if (c->ifi4) {
+		int rc = tcp_sock_refill_pool(c, ns_sock_pool4, AF_INET);
+		if (rc < 0)
+			warn("TCP: Error refilling IPv4 ns socket pool: %s",
+			     strerror(-rc));
+	}
+	if (c->ifi6) {
+		int rc = tcp_sock_refill_pool(c, ns_sock_pool6, AF_INET6);
+		if (rc < 0)
+			warn("TCP: Error refilling IPv6 ns socket pool: %s",
+			     strerror(-rc));
+	}
 
 	return 0;
 }
-- 
2.43.2

We maintain pools of ready-to-connect sockets in both the original and
(for pasta) guest namespace to reduce latency when starting new TCP
connections.  If we exhaust those pools we have to take a higher
latency path to get a new socket.

Currently we open-code that fallback in the places we need it.  To improve
clarity encapsulate that into helper functions.  While we're at it, give
those helpers clearer error reporting.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 tcp.c        | 34 +++++++++++++++++++++++++++-----
 tcp_conn.h   |  2 +-
 tcp_splice.c | 55 ++++++++++++++++++++++++++++++----------------------
 3 files changed, 62 insertions(+), 29 deletions(-)

diff --git a/tcp.c b/tcp.c
index ad56ffc3..34e32641 100644
--- a/tcp.c
+++ b/tcp.c
@@ -1792,7 +1792,7 @@ int tcp_conn_pool_sock(int pool[])
  *
  * Return: socket number on success, negative code if socket creation failed
  */
-int tcp_conn_new_sock(const struct ctx *c, sa_family_t af)
+static int tcp_conn_new_sock(const struct ctx *c, sa_family_t af)
 {
 	int s;
 
@@ -1811,6 +1811,32 @@ int tcp_conn_new_sock(const struct ctx *c, sa_family_t af)
 	return s;
 }
 
+/**
+ * tcp_conn_sock() - Obtain a connectable socket in the host/init namespace
+ * @c:		Execution context
+ * @af:		Address family (AF_INET or AF_INET6)
+ *
+ * Return: Socket fd on success, -errno on failure
+ */
+int tcp_conn_sock(const struct ctx *c, 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)
+		return s;
+
+	/* If the pool is empty we just open a new one without refilling the
+	 * pool to keep latency down.
+	 */
+	if ((s = tcp_conn_new_sock(c, af)) >= 0)
+		return s;
+
+	err("TCP: Unable to open socket for new connection: %s",
+	    strerror(-s));
+	return -1;
+}
+
 /**
  * tcp_conn_tap_mss() - Get MSS value advertised by tap/guest
  * @conn:	Connection pointer
@@ -1909,7 +1935,6 @@ static void tcp_conn_from_tap(struct ctx *c, sa_family_t af,
 			      const struct tcphdr *th, const char *opts,
 			      size_t optlen, const struct timespec *now)
 {
-	int *pool = af == AF_INET6 ? init_sock_pool6 : init_sock_pool4;
 	struct sockaddr_in addr4 = {
 		.sin_family = AF_INET,
 		.sin_port = th->dest,
@@ -1931,9 +1956,8 @@ static void tcp_conn_from_tap(struct ctx *c, sa_family_t af,
 	if (!(flow = flow_alloc()))
 		return;
 
-	if ((s = tcp_conn_pool_sock(pool)) < 0)
-		if ((s = tcp_conn_new_sock(c, af)) < 0)
-			goto cancel;
+	if ((s = tcp_conn_sock(c, af)) < 0)
+		goto cancel;
 
 	if (!c->no_map_gw) {
 		if (af == AF_INET && IN4_ARE_ADDR_EQUAL(daddr, &c->ip4.gw))
diff --git a/tcp_conn.h b/tcp_conn.h
index 92d4807a..d280b222 100644
--- a/tcp_conn.h
+++ b/tcp_conn.h
@@ -159,7 +159,7 @@ bool tcp_flow_defer(union flow *flow);
 bool tcp_splice_flow_defer(union flow *flow);
 void tcp_splice_timer(const struct ctx *c, union flow *flow);
 int tcp_conn_pool_sock(int pool[]);
-int tcp_conn_new_sock(const struct ctx *c, sa_family_t af);
+int tcp_conn_sock(const struct ctx *c, sa_family_t af);
 int tcp_sock_refill_pool(const struct ctx *c, int pool[], sa_family_t af);
 void tcp_splice_refill(const struct ctx *c);
 
diff --git a/tcp_splice.c b/tcp_splice.c
index ee68029b..5b38a82d 100644
--- a/tcp_splice.c
+++ b/tcp_splice.c
@@ -376,6 +376,34 @@ static int tcp_splice_connect(const struct ctx *c, struct
tcp_splice_conn *conn,
 	return 0;
 }
 
+/**
+ * tcp_conn_sock_ns() - Obtain a connectable socket in the namespace
+ * @c:		Execution context
+ * @af:		Address family (AF_INET or AF_INET6)
+ *
+ * Return: Socket fd in the namespace on success, -errno on failure
+ */
+static int tcp_conn_sock_ns(const struct ctx *c, sa_family_t af)
+{
+	int *p = af == AF_INET6 ? ns_sock_pool6 : ns_sock_pool4;
+	int s;
+
+	if ((s = tcp_conn_pool_sock(p)) >= 0)
+		return s;
+
+	/* If the pool is empty we have to incur the latency of entering the ns.
+	 * Therefore, we might as well refill the whole pool while we're at it.
+	 * This differs from tcp_conn_sock().
+	 */
+	NS_CALL(tcp_sock_refill_ns, c);
+
+	if ((s = tcp_conn_pool_sock(p)) >= 0)
+		return s;
+
+	err("TCP: No available ns sockets for new connection");
+	return -1;
+}
+
 /**
  * tcp_splice_new() - Handle new spliced connection
  * @c:		Execution context
@@ -388,38 +416,19 @@ static int tcp_splice_connect(const struct ctx *c, struct
tcp_splice_conn *conn,
 static int tcp_splice_new(const struct ctx *c, struct tcp_splice_conn *conn,
 			  in_port_t port, uint8_t pif)
 {
+	sa_family_t af = CONN_V6(conn) ? AF_INET6 : AF_INET;
 	int s = -1;
 
-	/* If the pool is empty we take slightly different approaches
-	 * for init or ns sockets.  For init sockets we just open a
-	 * new one without refilling the pool to keep latency down.
-	 * For ns sockets, we're going to incur the latency of
-	 * entering the ns anyway, so we might as well refill the
-	 * pool.
-	 */
 	if (pif == PIF_SPLICE) {
-		int *p = CONN_V6(conn) ? init_sock_pool6 : init_sock_pool4;
-		sa_family_t af = CONN_V6(conn) ? AF_INET6 : AF_INET;
-
-		s = tcp_conn_pool_sock(p);
-		if (s < 0)
-			s = tcp_conn_new_sock(c, af);
+		s = tcp_conn_sock(c, af);
 	} else {
-		int *p = CONN_V6(conn) ? ns_sock_pool6 : ns_sock_pool4;
-
 		ASSERT(pif == PIF_HOST);
 
-		/* If pool is empty, refill it first */
-		if (p[TCP_SOCK_POOL_SIZE-1] < 0)
-			NS_CALL(tcp_sock_refill_ns, c);
-
-		s = tcp_conn_pool_sock(p);
+		s = tcp_conn_sock_ns(c, af);
 	}
 
-	if (s < 0) {
-		warn("Couldn't open connectable socket for splice (%d)", s);
+	if (s < 0)
 		return s;
-	}
 
 	return tcp_splice_connect(c, conn, s, port);
 }
-- 
2.43.2

On Mon, 19 Feb 2024 18:56:45 +1100
David Gibson <david(a)gibson.dropbear.id.au> wrote:

...
  To reduce latency, the TCP code maintains several
pools of ready to
 connect TCP sockets.  This patch series contains a number of
 improvements to improve error handling and reporting when we're unable
 to refill these pools, or unable to obtain a socket from these pools.
 
 David Gibson (6):
   treewide: Use sa_family_t for address family variables
   tcp: Don't stop refilling socket pool if we find a filled entry
   tcp: Stop on first error when refilling socket pools
   tcp, tcp_splice: Issue warnings if unable to refill socket pool
   tcp, tcp_splice: Helpers for getting sockets from the pools
   tcp: Don't store errnos in socket pool 
Applied (with minor changes as mentioned for 4/6 and 6/6).

-- 
Stefano