public inbox for passt-dev@passt.top
 help / color / mirror / code / Atom feed
From: David Gibson <david@gibson.dropbear.id.au>
To: passt-dev@passt.top, Stefano Brivio <sbrivio@redhat.com>
Cc: David Gibson <david@gibson.dropbear.id.au>
Subject: [PATCH v3 08/20] tcp_splice: Simplify clean up logic
Date: Wed, 28 Feb 2024 22:25:08 +1100	[thread overview]
Message-ID: <20240228112520.2078220-9-david@gibson.dropbear.id.au> (raw)
In-Reply-To: <20240228112520.2078220-1-david@gibson.dropbear.id.au>

Currently tcp_splice_flow_defer() contains specific logic to determine
if we're far enough initialised that we need to close pipes and/or
sockets.  This is potentially fragile if we change something about the
order in which we do things.  We can simplify this by initialising the
pipe and socket fields to -1 very early, then close()ing them if and
only if they're non-negative.

This lets us remove a special case cleanup if our connect() fails.
This will already trigger a CLOSING event, and the socket fd in
question is populated in the connection structure.  Thus we can let
the new cleanup logic handle it rather than requiring an explicit
close().

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 tcp_splice.c | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/tcp_splice.c b/tcp_splice.c
index 4828b099..8187220d 100644
--- a/tcp_splice.c
+++ b/tcp_splice.c
@@ -246,16 +246,14 @@ bool tcp_splice_flow_defer(union flow *flow)
 		return false;
 
 	for (side = 0; side < SIDES; side++) {
-		if (conn->events & SPLICE_ESTABLISHED) {
-			/* Flushing might need to block: don't recycle them. */
-			if (conn->pipe[side][0] != -1) {
-				close(conn->pipe[side][0]);
-				close(conn->pipe[side][1]);
-				conn->pipe[side][0] = conn->pipe[side][1] = -1;
-			}
+		/* Flushing might need to block: don't recycle them. */
+		if (conn->pipe[side][0] >= 0) {
+			close(conn->pipe[side][0]);
+			close(conn->pipe[side][1]);
+			conn->pipe[side][0] = conn->pipe[side][1] = -1;
 		}
 
-		if (side == 0 || conn->events & SPLICE_CONNECT) {
+		if (conn->s[side] >= 0) {
 			close(conn->s[side]);
 			conn->s[side] = -1;
 		}
@@ -284,8 +282,6 @@ static int tcp_splice_connect_finish(const struct ctx *c,
 	int i = 0;
 
 	for (side = 0; side < SIDES; side++) {
-		conn->pipe[side][0] = conn->pipe[side][1] = -1;
-
 		for (; i < TCP_SPLICE_PIPE_POOL_SIZE; i++) {
 			if (splice_pipe_pool[i][0] >= 0) {
 				SWAP(conn->pipe[side][0],
@@ -361,12 +357,9 @@ static int tcp_splice_connect(const struct ctx *c, struct tcp_splice_conn *conn,
 	}
 
 	if (connect(conn->s[1], sa, sl)) {
-		if (errno != EINPROGRESS) {
-			int ret = -errno;
+		if (errno != EINPROGRESS)
+			return -errno;
 
-			close(sock_conn);
-			return ret;
-		}
 		conn_event(c, conn, SPLICE_CONNECT);
 	} else {
 		conn_event(c, conn, SPLICE_ESTABLISHED);
@@ -466,6 +459,9 @@ bool tcp_splice_conn_from_sock(const struct ctx *c,
 	conn->f.type = FLOW_TCP_SPLICE;
 	conn->flags = inany_v4(&aany) ? 0 : SPLICE_V6;
 	conn->s[0] = s;
+	conn->s[1] = -1;
+	conn->pipe[0][0] = conn->pipe[0][1] = -1;
+	conn->pipe[1][0] = conn->pipe[1][1] = -1;
 
 	if (tcp_splice_new(c, conn, ref.port, ref.pif))
 		conn_flag(c, conn, CLOSING);
-- 
@@ -246,16 +246,14 @@ bool tcp_splice_flow_defer(union flow *flow)
 		return false;
 
 	for (side = 0; side < SIDES; side++) {
-		if (conn->events & SPLICE_ESTABLISHED) {
-			/* Flushing might need to block: don't recycle them. */
-			if (conn->pipe[side][0] != -1) {
-				close(conn->pipe[side][0]);
-				close(conn->pipe[side][1]);
-				conn->pipe[side][0] = conn->pipe[side][1] = -1;
-			}
+		/* Flushing might need to block: don't recycle them. */
+		if (conn->pipe[side][0] >= 0) {
+			close(conn->pipe[side][0]);
+			close(conn->pipe[side][1]);
+			conn->pipe[side][0] = conn->pipe[side][1] = -1;
 		}
 
-		if (side == 0 || conn->events & SPLICE_CONNECT) {
+		if (conn->s[side] >= 0) {
 			close(conn->s[side]);
 			conn->s[side] = -1;
 		}
@@ -284,8 +282,6 @@ static int tcp_splice_connect_finish(const struct ctx *c,
 	int i = 0;
 
 	for (side = 0; side < SIDES; side++) {
-		conn->pipe[side][0] = conn->pipe[side][1] = -1;
-
 		for (; i < TCP_SPLICE_PIPE_POOL_SIZE; i++) {
 			if (splice_pipe_pool[i][0] >= 0) {
 				SWAP(conn->pipe[side][0],
@@ -361,12 +357,9 @@ static int tcp_splice_connect(const struct ctx *c, struct tcp_splice_conn *conn,
 	}
 
 	if (connect(conn->s[1], sa, sl)) {
-		if (errno != EINPROGRESS) {
-			int ret = -errno;
+		if (errno != EINPROGRESS)
+			return -errno;
 
-			close(sock_conn);
-			return ret;
-		}
 		conn_event(c, conn, SPLICE_CONNECT);
 	} else {
 		conn_event(c, conn, SPLICE_ESTABLISHED);
@@ -466,6 +459,9 @@ bool tcp_splice_conn_from_sock(const struct ctx *c,
 	conn->f.type = FLOW_TCP_SPLICE;
 	conn->flags = inany_v4(&aany) ? 0 : SPLICE_V6;
 	conn->s[0] = s;
+	conn->s[1] = -1;
+	conn->pipe[0][0] = conn->pipe[0][1] = -1;
+	conn->pipe[1][0] = conn->pipe[1][1] = -1;
 
 	if (tcp_splice_new(c, conn, ref.port, ref.pif))
 		conn_flag(c, conn, CLOSING);
-- 
2.43.2


  parent reply	other threads:[~2024-02-28 11:25 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-02-28 11:25 [PATCH v3 00/20] More flow table preliminaries: address handling improvements David Gibson
2024-02-28 11:25 ` [PATCH v3 01/20] inany: Helper to test for various address types David Gibson
2024-02-28 11:25 ` [PATCH v3 02/20] inany: Add inany_ntop() helper David Gibson
2024-02-28 11:25 ` [PATCH v3 03/20] inany: Provide more conveniently typed constants for special addresses David Gibson
2024-02-28 11:25 ` [PATCH v3 04/20] inany: Introduce union sockaddr_inany David Gibson
2024-02-28 11:25 ` [PATCH v3 05/20] util: Allow IN4_IS_* macros to operate on untyped addresses David Gibson
2024-02-28 11:25 ` [PATCH v3 06/20] tcp, udp: Don't precompute port remappings in epoll references David Gibson
2024-02-28 11:25 ` [PATCH v3 07/20] flow: Add helper to determine a flow's protocol David Gibson
2024-02-28 11:25 ` David Gibson [this message]
2024-02-28 11:25 ` [PATCH v3 09/20] tcp_splice: Don't use flow_trace() before setting flow type David Gibson
2024-02-28 11:25 ` [PATCH v3 10/20] flow: Clarify flow entry life cycle, introduce uniform logging David Gibson
2024-02-28 11:25 ` [PATCH v3 11/20] tcp_splice: More specific variable names in new splice path David Gibson
2024-02-28 11:25 ` [PATCH v3 12/20] tcp_splice: Merge tcp_splice_new() into its caller David Gibson
2024-02-28 11:25 ` [PATCH v3 13/20] tcp_splice: Make tcp_splice_connect() create its own sockets David Gibson
2024-02-28 11:25 ` [PATCH v3 14/20] tcp_splice: Improve error reporting on connect path David Gibson
2024-02-28 11:25 ` [PATCH v3 15/20] tcp_splice: Improve logic deciding when to splice David Gibson
2024-02-28 11:25 ` [PATCH v3 16/20] tcp, tcp_splice: Parse listening socket epoll ref in tcp_listen_handler() David Gibson
2024-02-28 11:25 ` [PATCH v3 17/20] tcp: Validate TCP endpoint addresses David Gibson
2024-02-28 11:25 ` [PATCH v3 18/20] tap: Disallow loopback addresses on tap interface David Gibson
2024-02-28 11:25 ` [PATCH v3 19/20] port_fwd: Fix copypasta error in port_fwd_scan_udp() comments David Gibson
2024-02-28 11:25 ` [PATCH v3 20/20] fwd: Rename port_fwd.[ch] and their contents David Gibson
2024-02-29 10:53 ` [PATCH v3 00/20] More flow table preliminaries: address handling improvements Stefano Brivio

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20240228112520.2078220-9-david@gibson.dropbear.id.au \
    --to=david@gibson.dropbear.id.au \
    --cc=passt-dev@passt.top \
    --cc=sbrivio@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://passt.top/passt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).