[PATCH 0/6] Of course there are more flow table preliminaries

[PATCH 0/6] Of course there are more flow table preliminaries

[David Gibson]

This series has some further preliminaries for the flow table.
Specifically this is focused on some clean ups to handling of indicies
for flows and sides.  We also add some additional test programs for
platform requirements.

This is based on the series adding UDP error handling.

David Gibson (6):
  flow, icmp, tcp: Clean up helpers for getting flow from index
  flow, tcp_splice: Prefer 'sidei' for variables referring to side index
  flow: Introduce flow_foreach_sidei() macro
  tcp_splice: Use parameterised macros for per-side event/flag bits
  doc: Test behaviour of closing duplicate UDP sockets
  doc: Extend zero-recv test with methods using msghdr

 doc/platform-requirements/.gitignore      |   1 +
 doc/platform-requirements/Makefile        |   4 +-
 doc/platform-requirements/recv-zero.c     |  60 +++++++--
 doc/platform-requirements/udp-close-dup.c | 105 +++++++++++++++
 flow.h                                    |  16 +--
 flow_table.h                              |  48 +++++--
 icmp.c                                    |  22 +++-
 tcp.c                                     |  28 +++-
 tcp_conn.h                                |  15 +--
 tcp_splice.c                              | 152 ++++++++++++----------
 10 files changed, 332 insertions(+), 119 deletions(-)
 create mode 100644 doc/platform-requirements/udp-close-dup.c

-- 
2.45.2

[PATCH 1/6] flow, icmp, tcp: Clean up helpers for getting flow from index

[David Gibson]

TCP (both regular and spliced) and ICMP both have macros to retrieve the
relevant protcol specific flow structure from a flow index.  In most cases
what we actually want is to get the specific flow from a sidx.  Replace
those simple macros with a more precise inline, which also asserts that
the flow is of the type we expect.

While we're they're also add a pif_at_sidx() helper to get the interface of
a specific flow & side, which is useful in some places.

Finally, fix some minor style issues in the comments on some of the
existing sidx related helpers.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 flow_table.h | 26 ++++++++++++++++++++------
 icmp.c       | 22 +++++++++++++++++++---
 tcp.c        | 28 ++++++++++++++++++++++------
 tcp_splice.c | 21 +++++++++++++++++++--
 4 files changed, 80 insertions(+), 17 deletions(-)

diff --git a/flow_table.h b/flow_table.h
index 226ddbdd..ab73e44a 100644
--- a/flow_table.h
+++ b/flow_table.h
@@ -42,7 +42,7 @@ extern unsigned flow_first_free;
 extern union flow flowtab[];
 
 
-/** flow_idx - Index of flow from common structure
+/** flow_idx() - Index of flow from common structure
  * @f:	Common flow fields pointer
  *
  * Return: index of @f in the flow table
@@ -52,21 +52,21 @@ static inline unsigned flow_idx(const struct flow_common *f)
 	return (union flow *)f - flowtab;
 }
 
-/** FLOW_IDX - Find the index of a flow
+/** FLOW_IDX() - Find the index of a flow
  * @f_:	Flow pointer, either union flow * or protocol specific
  *
  * Return: index of @f in the flow table
  */
 #define FLOW_IDX(f_)		(flow_idx(&(f_)->f))
 
-/** FLOW - Flow entry at a given index
+/** FLOW() - Flow entry at a given index
  * @idx:	Flow index
  *
  * Return: pointer to entry @idx in the flow table
  */
 #define FLOW(idx)		(&flowtab[(idx)])
 
-/** flow_at_sidx - Flow entry for a given sidx
+/** flow_at_sidx() - Flow entry for a given sidx
  * @sidx:	Flow & side index
  *
  * Return: pointer to the corresponding flow entry, or NULL
@@ -78,7 +78,21 @@ static inline union flow *flow_at_sidx(flow_sidx_t sidx)
 	return FLOW(sidx.flow);
 }
 
-/** flow_sidx_t - Index of one side of a flow from common structure
+/** pif_at_sidx() - Interface for a given flow and side
+ * @sidx:    Flow & side index
+ *
+ * Return: pif for the flow & side given by @sidx
+ */
+static inline uint8_t pif_at_sidx(flow_sidx_t sidx)
+{
+	const union flow *flow = flow_at_sidx(sidx);
+
+	if (!flow)
+		return PIF_NONE;
+	return flow->f.pif[sidx.side];
+}
+
+/** flow_sidx() - Index of one side of a flow from common structure
  * @f:		Common flow fields pointer
  * @side:	Which side to refer to (0 or 1)
  *
@@ -96,7 +110,7 @@ static inline flow_sidx_t flow_sidx(const struct flow_common *f,
 	};
 }
 
-/** FLOW_SIDX - Find the index of one side of a flow
+/** FLOW_SIDX() - Find the index of one side of a flow
  * @f_:		Flow pointer, either union flow * or protocol specific
  * @side:	Which side to index (0 or 1)
  *
diff --git a/icmp.c b/icmp.c
index d4ccc722..7cf31e60 100644
--- a/icmp.c
+++ b/icmp.c
@@ -45,11 +45,27 @@
 #define ICMP_ECHO_TIMEOUT	60 /* s, timeout for ICMP socket activity */
 #define ICMP_NUM_IDS		(1U << 16)
 
-#define PINGF(idx)		(&(FLOW(idx)->ping))
-
 /* Indexed by ICMP echo identifier */
 static struct icmp_ping_flow *icmp_id_map[IP_VERSIONS][ICMP_NUM_IDS];
 
+/**
+ * ping_at_sidx() - Get ping specific flow at given sidx
+ * @sidx:	Flow and side to retrieve
+ *
+ * Return: ping specific flow at @sidx, or NULL of @sidx is invalid.  Asserts if
+ *         the flow at @sidx is not FLOW_PING4 or FLOW_PING6
+ */
+static struct icmp_ping_flow *ping_at_sidx(flow_sidx_t sidx)
+{
+	union flow *flow = flow_at_sidx(sidx);
+
+	if (!flow)
+		return NULL;
+
+	ASSERT(flow->f.type == FLOW_PING4 || flow->f.type == FLOW_PING6);
+	return &flow->ping;
+}
+
 /**
  * icmp_sock_handler() - Handle new data from ICMP or ICMPv6 socket
  * @c:		Execution context
@@ -57,7 +73,7 @@ static struct icmp_ping_flow *icmp_id_map[IP_VERSIONS][ICMP_NUM_IDS];
  */
 void icmp_sock_handler(const struct ctx *c, union epoll_ref ref)
 {
-	struct icmp_ping_flow *pingf = PINGF(ref.flowside.flow);
+	struct icmp_ping_flow *pingf = ping_at_sidx(ref.flowside);
 	union sockaddr_inany sr;
 	socklen_t sl = sizeof(sr);
 	char buf[USHRT_MAX];
diff --git a/tcp.c b/tcp.c
index de3f9f64..04ea1f91 100644
--- a/tcp.c
+++ b/tcp.c
@@ -376,8 +376,6 @@ char		tcp_buf_discard		[MAX_WINDOW];
 /* sendmsg() to socket */
 static struct iovec	tcp_iov			[UIO_MAXIOV];
 
-#define CONN(idx)		(&(FLOW(idx)->tcp))
-
 /* Table for lookup from remote address, local port, remote port */
 static flow_sidx_t tc_hash[TCP_HASH_TABLE_SIZE];
 
@@ -388,6 +386,24 @@ static_assert(ARRAY_SIZE(tc_hash) >= FLOW_MAX,
 int init_sock_pool4		[TCP_SOCK_POOL_SIZE];
 int init_sock_pool6		[TCP_SOCK_POOL_SIZE];
 
+/**
+ * conn_at_sidx() - Get TCP connection specific flow at given sidx
+ * @sidx:	Flow and side to retrieve
+ *
+ * Return: TCP connection at @sidx, or NULL of @sidx is invalid.  Asserts if the
+ *         flow at @sidx is not FLOW_TCP.
+ */
+static struct tcp_tap_conn *conn_at_sidx(flow_sidx_t sidx)
+{
+	union flow *flow = flow_at_sidx(sidx);
+
+	if (!flow)
+		return NULL;
+
+	ASSERT(flow->f.type == FLOW_TCP);
+	return &flow->tcp;
+}
+
 /**
  * tcp_conn_epoll_events() - epoll events mask for given connection state
  * @events:	Current connection events
@@ -2339,9 +2355,10 @@ cancel:
 void tcp_timer_handler(struct ctx *c, union epoll_ref ref)
 {
 	struct itimerspec check_armed = { { 0 }, { 0 } };
-	struct tcp_tap_conn *conn = CONN(ref.flow);
+	struct tcp_tap_conn *conn = &FLOW(ref.flow)->tcp;
 
 	ASSERT(!c->no_tcp);
+	ASSERT(conn->f.type == FLOW_TCP);
 
 	/* We don't reset timers on ~ACK_FROM_TAP_DUE, ~ACK_TO_TAP_DUE. If the
 	 * timer is currently armed, this event came from a previous setting,
@@ -2397,11 +2414,10 @@ void tcp_timer_handler(struct ctx *c, union epoll_ref ref)
  */
 void tcp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events)
 {
-	struct tcp_tap_conn *conn = CONN(ref.flowside.flow);
+	struct tcp_tap_conn *conn = conn_at_sidx(ref.flowside);
 
 	ASSERT(!c->no_tcp);
-	ASSERT(conn->f.type == FLOW_TCP);
-	ASSERT(conn->f.pif[ref.flowside.side] != PIF_TAP);
+	ASSERT(pif_at_sidx(ref.flowside) != PIF_TAP);
 
 	if (conn->events == CLOSED)
 		return;
diff --git a/tcp_splice.c b/tcp_splice.c
index f2d4fc60..9fc56565 100644
--- a/tcp_splice.c
+++ b/tcp_splice.c
@@ -76,7 +76,6 @@ static int splice_pipe_pool		[TCP_SPLICE_PIPE_POOL_SIZE][2];
 #define CONN_V6(x)			((x)->flags & SPLICE_V6)
 #define CONN_V4(x)			(!CONN_V6(x))
 #define CONN_HAS(conn, set)		(((conn)->events & (set)) == (set))
-#define CONN(idx)			(&FLOW(idx)->tcp_splice)
 
 /* Display strings for connection events */
 static const char *tcp_splice_event_str[] __attribute((__unused__)) = {
@@ -94,6 +93,24 @@ static const char *tcp_splice_flag_str[] __attribute((__unused__)) = {
 static int tcp_sock_refill_ns(void *arg);
 static int tcp_conn_sock_ns(const struct ctx *c, sa_family_t af);
 
+/**
+ * conn_at_sidx() - Get spliced TCP connection specific flow at given sidx
+ * @sidx:	Flow and side to retrieve
+ *
+ * Return: Spliced TCP connection at @sidx, or NULL of @sidx is invalid.
+ *         Asserts if the flow at @sidx is not FLOW_TCP_SPLICE.
+ */
+static struct tcp_splice_conn *conn_at_sidx(flow_sidx_t sidx)
+{
+	union flow *flow = flow_at_sidx(sidx);
+
+	if (!flow)
+		return NULL;
+
+	ASSERT(flow->f.type == FLOW_TCP_SPLICE);
+	return &flow->tcp_splice;
+}
+
 /**
  * tcp_splice_conn_epoll_events() - epoll events masks for given state
  * @events:	Connection event flags
@@ -502,7 +519,7 @@ bool tcp_splice_conn_from_sock(const struct ctx *c,
 void tcp_splice_sock_handler(struct ctx *c, union epoll_ref ref,
 			     uint32_t events)
 {
-	struct tcp_splice_conn *conn = CONN(ref.flowside.flow);
+	struct tcp_splice_conn *conn = conn_at_sidx(ref.flowside);
 	unsigned side = ref.flowside.side, fromside;
 	uint8_t lowat_set_flag, lowat_act_flag;
 	int eof, never_read;
-- 
@@ -76,7 +76,6 @@ static int splice_pipe_pool		[TCP_SPLICE_PIPE_POOL_SIZE][2];
 #define CONN_V6(x)			((x)->flags & SPLICE_V6)
 #define CONN_V4(x)			(!CONN_V6(x))
 #define CONN_HAS(conn, set)		(((conn)->events & (set)) == (set))
-#define CONN(idx)			(&FLOW(idx)->tcp_splice)
 
 /* Display strings for connection events */
 static const char *tcp_splice_event_str[] __attribute((__unused__)) = {
@@ -94,6 +93,24 @@ static const char *tcp_splice_flag_str[] __attribute((__unused__)) = {
 static int tcp_sock_refill_ns(void *arg);
 static int tcp_conn_sock_ns(const struct ctx *c, sa_family_t af);
 
+/**
+ * conn_at_sidx() - Get spliced TCP connection specific flow at given sidx
+ * @sidx:	Flow and side to retrieve
+ *
+ * Return: Spliced TCP connection at @sidx, or NULL of @sidx is invalid.
+ *         Asserts if the flow at @sidx is not FLOW_TCP_SPLICE.
+ */
+static struct tcp_splice_conn *conn_at_sidx(flow_sidx_t sidx)
+{
+	union flow *flow = flow_at_sidx(sidx);
+
+	if (!flow)
+		return NULL;
+
+	ASSERT(flow->f.type == FLOW_TCP_SPLICE);
+	return &flow->tcp_splice;
+}
+
 /**
  * tcp_splice_conn_epoll_events() - epoll events masks for given state
  * @events:	Connection event flags
@@ -502,7 +519,7 @@ bool tcp_splice_conn_from_sock(const struct ctx *c,
 void tcp_splice_sock_handler(struct ctx *c, union epoll_ref ref,
 			     uint32_t events)
 {
-	struct tcp_splice_conn *conn = CONN(ref.flowside.flow);
+	struct tcp_splice_conn *conn = conn_at_sidx(ref.flowside);
 	unsigned side = ref.flowside.side, fromside;
 	uint8_t lowat_set_flag, lowat_act_flag;
 	int eof, never_read;
-- 
2.45.2

[PATCH 2/6] flow, tcp_splice: Prefer 'sidei' for variables referring to side index

[David Gibson]

In various places we have variables named 'side' or similar which always
have the value 0 or 1 (INISIDE or TGTSIDE).  Given a flow, this refers to
a specific side of it.  Upcoming flow table work will make it more useful
for "side" to refer to a specific side of a specific flow.  To make things
less confusing then, prefer the name term "side index" and name 'sidei' for
variables with just the 0 or 1 value.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 flow.h       | 16 +++++-----
 flow_table.h | 18 +++++------
 tcp_splice.c | 89 ++++++++++++++++++++++++++--------------------------
 3 files changed, 62 insertions(+), 61 deletions(-)

diff --git a/flow.h b/flow.h
index d1f49c65..5d2c34af 100644
--- a/flow.h
+++ b/flow.h
@@ -132,8 +132,8 @@ extern const uint8_t flow_proto[];
 
 #define SIDES			2
 
-#define INISIDE			0	/* Initiating side */
-#define TGTSIDE			1	/* Target side */
+#define INISIDE			0	/* Initiating side index */
+#define TGTSIDE			1	/* Target side index */
 
 /**
  * struct flow_common - Common fields for packet flows
@@ -164,17 +164,17 @@ struct flow_common {
 
 /**
  * struct flow_sidx - ID for one side of a specific flow
- * @side:	Side referenced (0 or 1)
+ * @side:	Index of side referenced (0 or 1)
  * @flow:	Index of flow referenced
  */
 typedef struct flow_sidx {
-	unsigned	side :1;
-	unsigned	flow :FLOW_INDEX_BITS;
+	unsigned	sidei :1;
+	unsigned	flowi :FLOW_INDEX_BITS;
 } flow_sidx_t;
 static_assert(sizeof(flow_sidx_t) <= sizeof(uint32_t),
 	      "flow_sidx_t must fit within 32 bits");
 
-#define FLOW_SIDX_NONE ((flow_sidx_t){ .flow = FLOW_MAX })
+#define FLOW_SIDX_NONE ((flow_sidx_t){ .flowi = FLOW_MAX })
 
 /**
  * flow_sidx_valid() - Test if a sidx is valid
@@ -184,7 +184,7 @@ static_assert(sizeof(flow_sidx_t) <= sizeof(uint32_t),
  */
 static inline bool flow_sidx_valid(flow_sidx_t sidx)
 {
-	return sidx.flow < FLOW_MAX;
+	return sidx.flowi < FLOW_MAX;
 }
 
 /**
@@ -195,7 +195,7 @@ static inline bool flow_sidx_valid(flow_sidx_t sidx)
  */
 static inline bool flow_sidx_eq(flow_sidx_t a, flow_sidx_t b)
 {
-	return (a.flow == b.flow) && (a.side == b.side);
+	return (a.flowi == b.flowi) && (a.sidei == b.sidei);
 }
 
 union flow;
diff --git a/flow_table.h b/flow_table.h
index ab73e44a..8a5f8f25 100644
--- a/flow_table.h
+++ b/flow_table.h
@@ -75,7 +75,7 @@ static inline union flow *flow_at_sidx(flow_sidx_t sidx)
 {
 	if (!flow_sidx_valid(sidx))
 		return NULL;
-	return FLOW(sidx.flow);
+	return FLOW(sidx.flowi);
 }
 
 /** pif_at_sidx() - Interface for a given flow and side
@@ -89,34 +89,34 @@ static inline uint8_t pif_at_sidx(flow_sidx_t sidx)
 
 	if (!flow)
 		return PIF_NONE;
-	return flow->f.pif[sidx.side];
+	return flow->f.pif[sidx.sidei];
 }
 
 /** flow_sidx() - Index of one side of a flow from common structure
  * @f:		Common flow fields pointer
- * @side:	Which side to refer to (0 or 1)
+ * @sidei:	Which side to refer to (0 or 1)
  *
  * Return: index of @f and @side in the flow table
  */
 static inline flow_sidx_t flow_sidx(const struct flow_common *f,
-				    int side)
+				    unsigned sidei)
 {
 	/* cppcheck-suppress [knownConditionTrueFalse, unmatchedSuppression] */
-	ASSERT(side == !!side);
+	ASSERT(sidei == !!sidei);
 
 	return (flow_sidx_t){
-		.side = side,
-		.flow = flow_idx(f),
+		.sidei = sidei,
+		.flowi = flow_idx(f),
 	};
 }
 
 /** FLOW_SIDX() - Find the index of one side of a flow
  * @f_:		Flow pointer, either union flow * or protocol specific
- * @side:	Which side to index (0 or 1)
+ * @sidei:	Which side to index (0 or 1)
  *
  * Return: index of @f and @side in the flow table
  */
-#define FLOW_SIDX(f_, side)	(flow_sidx(&(f_)->f, (side)))
+#define FLOW_SIDX(f_, sidei)	(flow_sidx(&(f_)->f, (sidei)))
 
 union flow *flow_alloc(void);
 void flow_alloc_cancel(union flow *flow);
diff --git a/tcp_splice.c b/tcp_splice.c
index 9fc56565..8bc68a1a 100644
--- a/tcp_splice.c
+++ b/tcp_splice.c
@@ -258,25 +258,25 @@ static void conn_event_do(const struct ctx *c, struct tcp_splice_conn *conn,
  */
 bool tcp_splice_flow_defer(struct tcp_splice_conn *conn)
 {
-	unsigned side;
+	unsigned sidei;
 
 	if (!(conn->flags & CLOSING))
 		return false;
 
-	for (side = 0; side < SIDES; side++) {
+	for (sidei = 0; sidei < SIDES; sidei++) {
 		/* 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 (conn->pipe[sidei][0] >= 0) {
+			close(conn->pipe[sidei][0]);
+			close(conn->pipe[sidei][1]);
+			conn->pipe[sidei][0] = conn->pipe[sidei][1] = -1;
 		}
 
-		if (conn->s[side] >= 0) {
-			close(conn->s[side]);
-			conn->s[side] = -1;
+		if (conn->s[sidei] >= 0) {
+			close(conn->s[sidei]);
+			conn->s[sidei] = -1;
 		}
 
-		conn->read[side] = conn->written[side] = 0;
+		conn->read[sidei] = conn->written[sidei] = 0;
 	}
 
 	conn->events = SPLICE_CLOSED;
@@ -296,33 +296,33 @@ bool tcp_splice_flow_defer(struct tcp_splice_conn *conn)
 static int tcp_splice_connect_finish(const struct ctx *c,
 				     struct tcp_splice_conn *conn)
 {
-	unsigned side;
+	unsigned sidei;
 	int i = 0;
 
-	for (side = 0; side < SIDES; side++) {
+	for (sidei = 0; sidei < SIDES; sidei++) {
 		for (; i < TCP_SPLICE_PIPE_POOL_SIZE; i++) {
 			if (splice_pipe_pool[i][0] >= 0) {
-				SWAP(conn->pipe[side][0],
+				SWAP(conn->pipe[sidei][0],
 				     splice_pipe_pool[i][0]);
-				SWAP(conn->pipe[side][1],
+				SWAP(conn->pipe[sidei][1],
 				     splice_pipe_pool[i][1]);
 				break;
 			}
 		}
 
-		if (conn->pipe[side][0] < 0) {
-			if (pipe2(conn->pipe[side], O_NONBLOCK | O_CLOEXEC)) {
+		if (conn->pipe[sidei][0] < 0) {
+			if (pipe2(conn->pipe[sidei], O_NONBLOCK | O_CLOEXEC)) {
 				flow_err(conn, "cannot create %d->%d pipe: %s",
-					 side, !side, strerror(errno));
+					 sidei, !sidei, strerror(errno));
 				conn_flag(c, conn, CLOSING);
 				return -EIO;
 			}
 
-			if (fcntl(conn->pipe[side][0], F_SETPIPE_SZ,
+			if (fcntl(conn->pipe[sidei][0], F_SETPIPE_SZ,
 				  c->tcp.pipe_size)) {
 				flow_trace(conn,
 					   "cannot set %d->%d pipe size to %zu",
-					   side, !side, c->tcp.pipe_size);
+					   sidei, !sidei, c->tcp.pipe_size);
 			}
 		}
 	}
@@ -520,7 +520,7 @@ void tcp_splice_sock_handler(struct ctx *c, union epoll_ref ref,
 			     uint32_t events)
 {
 	struct tcp_splice_conn *conn = conn_at_sidx(ref.flowside);
-	unsigned side = ref.flowside.side, fromside;
+	unsigned evsidei = ref.flowside.sidei, fromsidei;
 	uint8_t lowat_set_flag, lowat_act_flag;
 	int eof, never_read;
 
@@ -552,30 +552,31 @@ void tcp_splice_sock_handler(struct ctx *c, union epoll_ref ref,
 	}
 
 	if (events & EPOLLOUT) {
-		fromside = !side;
-		conn_event(c, conn, side == 0 ? ~OUT_WAIT_0 : ~OUT_WAIT_1);
+		fromsidei = !evsidei;
+		conn_event(c, conn, evsidei == 0 ? ~OUT_WAIT_0 : ~OUT_WAIT_1);
 	} else {
-		fromside = side;
+		fromsidei = evsidei;
 	}
 
 	if (events & EPOLLRDHUP)
 		/* For side 0 this is fake, but implied */
-		conn_event(c, conn, side == 0 ? FIN_RCVD_0 : FIN_RCVD_1);
+		conn_event(c, conn, evsidei == 0 ? FIN_RCVD_0 : FIN_RCVD_1);
 
 swap:
 	eof = 0;
 	never_read = 1;
 
-	lowat_set_flag = fromside == 0 ? RCVLOWAT_SET_0 : RCVLOWAT_SET_1;
-	lowat_act_flag = fromside == 0 ? RCVLOWAT_ACT_0 : RCVLOWAT_ACT_1;
+	lowat_set_flag = fromsidei == 0 ? RCVLOWAT_SET_0 : RCVLOWAT_SET_1;
+	lowat_act_flag = fromsidei == 0 ? RCVLOWAT_ACT_0 : RCVLOWAT_ACT_1;
 
 	while (1) {
 		ssize_t readlen, to_write = 0, written;
 		int more = 0;
 
 retry:
-		readlen = splice(conn->s[fromside], NULL,
-				 conn->pipe[fromside][1], NULL, c->tcp.pipe_size,
+		readlen = splice(conn->s[fromsidei], NULL,
+				 conn->pipe[fromsidei][1], NULL,
+				 c->tcp.pipe_size,
 				 SPLICE_F_MOVE | SPLICE_F_NONBLOCK);
 		flow_trace(conn, "%zi from read-side call", readlen);
 		if (readlen < 0) {
@@ -600,8 +601,8 @@ retry:
 		}
 
 eintr:
-		written = splice(conn->pipe[fromside][0], NULL,
-				 conn->s[!fromside], NULL, to_write,
+		written = splice(conn->pipe[fromsidei][0], NULL,
+				 conn->s[!fromsidei], NULL, to_write,
 				 SPLICE_F_MOVE | more | SPLICE_F_NONBLOCK);
 		flow_trace(conn, "%zi from write-side call (passed %zi)",
 			   written, to_write);
@@ -615,7 +616,7 @@ eintr:
 			    readlen > (long)c->tcp.pipe_size / 10) {
 				int lowat = c->tcp.pipe_size / 4;
 
-				if (setsockopt(conn->s[fromside], SOL_SOCKET,
+				if (setsockopt(conn->s[fromsidei], SOL_SOCKET,
 					       SO_RCVLOWAT,
 					       &lowat, sizeof(lowat))) {
 					flow_trace(conn,
@@ -630,8 +631,8 @@ eintr:
 			break;
 		}
 
-		conn->read[fromside]    += readlen > 0 ? readlen : 0;
-		conn->written[fromside] += written > 0 ? written : 0;
+		conn->read[fromsidei]    += readlen > 0 ? readlen : 0;
+		conn->written[fromsidei] += written > 0 ? written : 0;
 
 		if (written < 0) {
 			if (errno == EINTR)
@@ -640,11 +641,11 @@ eintr:
 			if (errno != EAGAIN)
 				goto close;
 
-			if (conn->read[fromside] == conn->written[fromside])
+			if (conn->read[fromsidei] == conn->written[fromsidei])
 				break;
 
 			conn_event(c, conn,
-				   fromside == 0 ? OUT_WAIT_1 : OUT_WAIT_0);
+				   fromsidei == 0 ? OUT_WAIT_1 : OUT_WAIT_0);
 			break;
 		}
 
@@ -661,14 +662,14 @@ eintr:
 	}
 
 	if ((conn->events & FIN_RCVD_0) && !(conn->events & FIN_SENT_1)) {
-		if (conn->read[fromside] == conn->written[fromside] && eof) {
+		if (conn->read[fromsidei] == conn->written[fromsidei] && eof) {
 			shutdown(conn->s[1], SHUT_WR);
 			conn_event(c, conn, FIN_SENT_1);
 		}
 	}
 
 	if ((conn->events & FIN_RCVD_1) && !(conn->events & FIN_SENT_0)) {
-		if (conn->read[fromside] == conn->written[fromside] && eof) {
+		if (conn->read[fromsidei] == conn->written[fromsidei] && eof) {
 			shutdown(conn->s[0], SHUT_WR);
 			conn_event(c, conn, FIN_SENT_0);
 		}
@@ -680,7 +681,7 @@ eintr:
 	if ((events & (EPOLLIN | EPOLLOUT)) == (EPOLLIN | EPOLLOUT)) {
 		events = EPOLLIN;
 
-		fromside = !fromside;
+		fromsidei = !fromsidei;
 		goto swap;
 	}
 
@@ -815,19 +816,19 @@ void tcp_splice_init(struct ctx *c)
  */
 void tcp_splice_timer(const struct ctx *c, struct tcp_splice_conn *conn)
 {
-	int side;
+	unsigned sidei;
 
 	ASSERT(!(conn->flags & CLOSING));
 
-	for (side = 0; side < SIDES; side++) {
-		uint8_t set = side == 0 ? RCVLOWAT_SET_0 : RCVLOWAT_SET_1;
-		uint8_t act = side == 0 ? RCVLOWAT_ACT_0 : RCVLOWAT_ACT_1;
+	for (sidei = 0; sidei < SIDES; sidei++) {
+		uint8_t set = sidei == 0 ? RCVLOWAT_SET_0 : RCVLOWAT_SET_1;
+		uint8_t act = sidei == 0 ? RCVLOWAT_ACT_0 : RCVLOWAT_ACT_1;
 
 		if ((conn->flags & set) && !(conn->flags & act)) {
-			if (setsockopt(conn->s[side], SOL_SOCKET, SO_RCVLOWAT,
+			if (setsockopt(conn->s[sidei], SOL_SOCKET, SO_RCVLOWAT,
 				       &((int){ 1 }), sizeof(int))) {
 				flow_trace(conn, "can't set SO_RCVLOWAT on %d",
-					   conn->s[side]);
+					   conn->s[sidei]);
 			}
 			conn_flag(c, conn, ~set);
 		}
-- 
@@ -258,25 +258,25 @@ static void conn_event_do(const struct ctx *c, struct tcp_splice_conn *conn,
  */
 bool tcp_splice_flow_defer(struct tcp_splice_conn *conn)
 {
-	unsigned side;
+	unsigned sidei;
 
 	if (!(conn->flags & CLOSING))
 		return false;
 
-	for (side = 0; side < SIDES; side++) {
+	for (sidei = 0; sidei < SIDES; sidei++) {
 		/* 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 (conn->pipe[sidei][0] >= 0) {
+			close(conn->pipe[sidei][0]);
+			close(conn->pipe[sidei][1]);
+			conn->pipe[sidei][0] = conn->pipe[sidei][1] = -1;
 		}
 
-		if (conn->s[side] >= 0) {
-			close(conn->s[side]);
-			conn->s[side] = -1;
+		if (conn->s[sidei] >= 0) {
+			close(conn->s[sidei]);
+			conn->s[sidei] = -1;
 		}
 
-		conn->read[side] = conn->written[side] = 0;
+		conn->read[sidei] = conn->written[sidei] = 0;
 	}
 
 	conn->events = SPLICE_CLOSED;
@@ -296,33 +296,33 @@ bool tcp_splice_flow_defer(struct tcp_splice_conn *conn)
 static int tcp_splice_connect_finish(const struct ctx *c,
 				     struct tcp_splice_conn *conn)
 {
-	unsigned side;
+	unsigned sidei;
 	int i = 0;
 
-	for (side = 0; side < SIDES; side++) {
+	for (sidei = 0; sidei < SIDES; sidei++) {
 		for (; i < TCP_SPLICE_PIPE_POOL_SIZE; i++) {
 			if (splice_pipe_pool[i][0] >= 0) {
-				SWAP(conn->pipe[side][0],
+				SWAP(conn->pipe[sidei][0],
 				     splice_pipe_pool[i][0]);
-				SWAP(conn->pipe[side][1],
+				SWAP(conn->pipe[sidei][1],
 				     splice_pipe_pool[i][1]);
 				break;
 			}
 		}
 
-		if (conn->pipe[side][0] < 0) {
-			if (pipe2(conn->pipe[side], O_NONBLOCK | O_CLOEXEC)) {
+		if (conn->pipe[sidei][0] < 0) {
+			if (pipe2(conn->pipe[sidei], O_NONBLOCK | O_CLOEXEC)) {
 				flow_err(conn, "cannot create %d->%d pipe: %s",
-					 side, !side, strerror(errno));
+					 sidei, !sidei, strerror(errno));
 				conn_flag(c, conn, CLOSING);
 				return -EIO;
 			}
 
-			if (fcntl(conn->pipe[side][0], F_SETPIPE_SZ,
+			if (fcntl(conn->pipe[sidei][0], F_SETPIPE_SZ,
 				  c->tcp.pipe_size)) {
 				flow_trace(conn,
 					   "cannot set %d->%d pipe size to %zu",
-					   side, !side, c->tcp.pipe_size);
+					   sidei, !sidei, c->tcp.pipe_size);
 			}
 		}
 	}
@@ -520,7 +520,7 @@ void tcp_splice_sock_handler(struct ctx *c, union epoll_ref ref,
 			     uint32_t events)
 {
 	struct tcp_splice_conn *conn = conn_at_sidx(ref.flowside);
-	unsigned side = ref.flowside.side, fromside;
+	unsigned evsidei = ref.flowside.sidei, fromsidei;
 	uint8_t lowat_set_flag, lowat_act_flag;
 	int eof, never_read;
 
@@ -552,30 +552,31 @@ void tcp_splice_sock_handler(struct ctx *c, union epoll_ref ref,
 	}
 
 	if (events & EPOLLOUT) {
-		fromside = !side;
-		conn_event(c, conn, side == 0 ? ~OUT_WAIT_0 : ~OUT_WAIT_1);
+		fromsidei = !evsidei;
+		conn_event(c, conn, evsidei == 0 ? ~OUT_WAIT_0 : ~OUT_WAIT_1);
 	} else {
-		fromside = side;
+		fromsidei = evsidei;
 	}
 
 	if (events & EPOLLRDHUP)
 		/* For side 0 this is fake, but implied */
-		conn_event(c, conn, side == 0 ? FIN_RCVD_0 : FIN_RCVD_1);
+		conn_event(c, conn, evsidei == 0 ? FIN_RCVD_0 : FIN_RCVD_1);
 
 swap:
 	eof = 0;
 	never_read = 1;
 
-	lowat_set_flag = fromside == 0 ? RCVLOWAT_SET_0 : RCVLOWAT_SET_1;
-	lowat_act_flag = fromside == 0 ? RCVLOWAT_ACT_0 : RCVLOWAT_ACT_1;
+	lowat_set_flag = fromsidei == 0 ? RCVLOWAT_SET_0 : RCVLOWAT_SET_1;
+	lowat_act_flag = fromsidei == 0 ? RCVLOWAT_ACT_0 : RCVLOWAT_ACT_1;
 
 	while (1) {
 		ssize_t readlen, to_write = 0, written;
 		int more = 0;
 
 retry:
-		readlen = splice(conn->s[fromside], NULL,
-				 conn->pipe[fromside][1], NULL, c->tcp.pipe_size,
+		readlen = splice(conn->s[fromsidei], NULL,
+				 conn->pipe[fromsidei][1], NULL,
+				 c->tcp.pipe_size,
 				 SPLICE_F_MOVE | SPLICE_F_NONBLOCK);
 		flow_trace(conn, "%zi from read-side call", readlen);
 		if (readlen < 0) {
@@ -600,8 +601,8 @@ retry:
 		}
 
 eintr:
-		written = splice(conn->pipe[fromside][0], NULL,
-				 conn->s[!fromside], NULL, to_write,
+		written = splice(conn->pipe[fromsidei][0], NULL,
+				 conn->s[!fromsidei], NULL, to_write,
 				 SPLICE_F_MOVE | more | SPLICE_F_NONBLOCK);
 		flow_trace(conn, "%zi from write-side call (passed %zi)",
 			   written, to_write);
@@ -615,7 +616,7 @@ eintr:
 			    readlen > (long)c->tcp.pipe_size / 10) {
 				int lowat = c->tcp.pipe_size / 4;
 
-				if (setsockopt(conn->s[fromside], SOL_SOCKET,
+				if (setsockopt(conn->s[fromsidei], SOL_SOCKET,
 					       SO_RCVLOWAT,
 					       &lowat, sizeof(lowat))) {
 					flow_trace(conn,
@@ -630,8 +631,8 @@ eintr:
 			break;
 		}
 
-		conn->read[fromside]    += readlen > 0 ? readlen : 0;
-		conn->written[fromside] += written > 0 ? written : 0;
+		conn->read[fromsidei]    += readlen > 0 ? readlen : 0;
+		conn->written[fromsidei] += written > 0 ? written : 0;
 
 		if (written < 0) {
 			if (errno == EINTR)
@@ -640,11 +641,11 @@ eintr:
 			if (errno != EAGAIN)
 				goto close;
 
-			if (conn->read[fromside] == conn->written[fromside])
+			if (conn->read[fromsidei] == conn->written[fromsidei])
 				break;
 
 			conn_event(c, conn,
-				   fromside == 0 ? OUT_WAIT_1 : OUT_WAIT_0);
+				   fromsidei == 0 ? OUT_WAIT_1 : OUT_WAIT_0);
 			break;
 		}
 
@@ -661,14 +662,14 @@ eintr:
 	}
 
 	if ((conn->events & FIN_RCVD_0) && !(conn->events & FIN_SENT_1)) {
-		if (conn->read[fromside] == conn->written[fromside] && eof) {
+		if (conn->read[fromsidei] == conn->written[fromsidei] && eof) {
 			shutdown(conn->s[1], SHUT_WR);
 			conn_event(c, conn, FIN_SENT_1);
 		}
 	}
 
 	if ((conn->events & FIN_RCVD_1) && !(conn->events & FIN_SENT_0)) {
-		if (conn->read[fromside] == conn->written[fromside] && eof) {
+		if (conn->read[fromsidei] == conn->written[fromsidei] && eof) {
 			shutdown(conn->s[0], SHUT_WR);
 			conn_event(c, conn, FIN_SENT_0);
 		}
@@ -680,7 +681,7 @@ eintr:
 	if ((events & (EPOLLIN | EPOLLOUT)) == (EPOLLIN | EPOLLOUT)) {
 		events = EPOLLIN;
 
-		fromside = !fromside;
+		fromsidei = !fromsidei;
 		goto swap;
 	}
 
@@ -815,19 +816,19 @@ void tcp_splice_init(struct ctx *c)
  */
 void tcp_splice_timer(const struct ctx *c, struct tcp_splice_conn *conn)
 {
-	int side;
+	unsigned sidei;
 
 	ASSERT(!(conn->flags & CLOSING));
 
-	for (side = 0; side < SIDES; side++) {
-		uint8_t set = side == 0 ? RCVLOWAT_SET_0 : RCVLOWAT_SET_1;
-		uint8_t act = side == 0 ? RCVLOWAT_ACT_0 : RCVLOWAT_ACT_1;
+	for (sidei = 0; sidei < SIDES; sidei++) {
+		uint8_t set = sidei == 0 ? RCVLOWAT_SET_0 : RCVLOWAT_SET_1;
+		uint8_t act = sidei == 0 ? RCVLOWAT_ACT_0 : RCVLOWAT_ACT_1;
 
 		if ((conn->flags & set) && !(conn->flags & act)) {
-			if (setsockopt(conn->s[side], SOL_SOCKET, SO_RCVLOWAT,
+			if (setsockopt(conn->s[sidei], SOL_SOCKET, SO_RCVLOWAT,
 				       &((int){ 1 }), sizeof(int))) {
 				flow_trace(conn, "can't set SO_RCVLOWAT on %d",
-					   conn->s[side]);
+					   conn->s[sidei]);
 			}
 			conn_flag(c, conn, ~set);
 		}
-- 
2.45.2

[PATCH 3/6] flow: Introduce flow_foreach_sidei() macro

[David Gibson]

We have a handful of places where we use a loop to step through each side
of a flow or flows, and we're probably going to have mroe in future.
Introduce a macro to implement this loop for convenience.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 flow_table.h | 6 ++++++
 tcp_splice.c | 6 +++---
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/flow_table.h b/flow_table.h
index 8a5f8f25..4f5d041c 100644
--- a/flow_table.h
+++ b/flow_table.h
@@ -41,6 +41,12 @@ union flow {
 extern unsigned flow_first_free;
 extern union flow flowtab[];
 
+/**
+ * flow_foreach_sidei() - 'for' type macro to step through each side of flow
+ * @sidei_:	Takes value INISIDE, then TGTSIDE
+ */
+#define flow_foreach_sidei(sidei_) \
+	for ((sidei_) = INISIDE; (sidei_) < SIDES; (sidei_)++)
 
 /** flow_idx() - Index of flow from common structure
  * @f:	Common flow fields pointer
diff --git a/tcp_splice.c b/tcp_splice.c
index 8bc68a1a..5a9325b1 100644
--- a/tcp_splice.c
+++ b/tcp_splice.c
@@ -263,7 +263,7 @@ bool tcp_splice_flow_defer(struct tcp_splice_conn *conn)
 	if (!(conn->flags & CLOSING))
 		return false;
 
-	for (sidei = 0; sidei < SIDES; sidei++) {
+	flow_foreach_sidei(sidei) {
 		/* Flushing might need to block: don't recycle them. */
 		if (conn->pipe[sidei][0] >= 0) {
 			close(conn->pipe[sidei][0]);
@@ -299,7 +299,7 @@ static int tcp_splice_connect_finish(const struct ctx *c,
 	unsigned sidei;
 	int i = 0;
 
-	for (sidei = 0; sidei < SIDES; sidei++) {
+	flow_foreach_sidei(sidei) {
 		for (; i < TCP_SPLICE_PIPE_POOL_SIZE; i++) {
 			if (splice_pipe_pool[i][0] >= 0) {
 				SWAP(conn->pipe[sidei][0],
@@ -820,7 +820,7 @@ void tcp_splice_timer(const struct ctx *c, struct tcp_splice_conn *conn)
 
 	ASSERT(!(conn->flags & CLOSING));
 
-	for (sidei = 0; sidei < SIDES; sidei++) {
+	flow_foreach_sidei(sidei) {
 		uint8_t set = sidei == 0 ? RCVLOWAT_SET_0 : RCVLOWAT_SET_1;
 		uint8_t act = sidei == 0 ? RCVLOWAT_ACT_0 : RCVLOWAT_ACT_1;
 
-- 
@@ -263,7 +263,7 @@ bool tcp_splice_flow_defer(struct tcp_splice_conn *conn)
 	if (!(conn->flags & CLOSING))
 		return false;
 
-	for (sidei = 0; sidei < SIDES; sidei++) {
+	flow_foreach_sidei(sidei) {
 		/* Flushing might need to block: don't recycle them. */
 		if (conn->pipe[sidei][0] >= 0) {
 			close(conn->pipe[sidei][0]);
@@ -299,7 +299,7 @@ static int tcp_splice_connect_finish(const struct ctx *c,
 	unsigned sidei;
 	int i = 0;
 
-	for (sidei = 0; sidei < SIDES; sidei++) {
+	flow_foreach_sidei(sidei) {
 		for (; i < TCP_SPLICE_PIPE_POOL_SIZE; i++) {
 			if (splice_pipe_pool[i][0] >= 0) {
 				SWAP(conn->pipe[sidei][0],
@@ -820,7 +820,7 @@ void tcp_splice_timer(const struct ctx *c, struct tcp_splice_conn *conn)
 
 	ASSERT(!(conn->flags & CLOSING));
 
-	for (sidei = 0; sidei < SIDES; sidei++) {
+	flow_foreach_sidei(sidei) {
 		uint8_t set = sidei == 0 ? RCVLOWAT_SET_0 : RCVLOWAT_SET_1;
 		uint8_t act = sidei == 0 ? RCVLOWAT_ACT_0 : RCVLOWAT_ACT_1;
 
-- 
2.45.2

[PATCH 4/6] tcp_splice: Use parameterised macros for per-side event/flag bits

[David Gibson]

Both the events and flags fields in tcp_splice_conn have several bits
which are per-side, e.g. OUT_WAIT_0 for side 0 and OUT_WAIT_1 for side 1.
This necessitates some rather awkward ternary expressions when we need
to get the relevant bit for a particular side.

Simplify this by using a parameterised macro for the bit values.  This
needs a ternary expression inside the macros, but makes the places we use
it substantially clearer.

That simplification in turn allows us to use a loop across each side to
implement several things which are currently open coded to do equivalent
things for each side in turn.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 tcp_conn.h   | 15 +++++--------
 tcp_splice.c | 60 +++++++++++++++++++++++++---------------------------
 2 files changed, 34 insertions(+), 41 deletions(-)

diff --git a/tcp_conn.h b/tcp_conn.h
index 5f8c8fb6..f80ef67b 100644
--- a/tcp_conn.h
+++ b/tcp_conn.h
@@ -129,19 +129,14 @@ struct tcp_splice_conn {
 #define SPLICE_CLOSED			0
 #define SPLICE_CONNECT			BIT(0)
 #define SPLICE_ESTABLISHED		BIT(1)
-#define OUT_WAIT_0			BIT(2)
-#define OUT_WAIT_1			BIT(3)
-#define FIN_RCVD_0			BIT(4)
-#define FIN_RCVD_1			BIT(5)
-#define FIN_SENT_0			BIT(6)
-#define FIN_SENT_1			BIT(7)
+#define OUT_WAIT(sidei_)		((sidei_) ? BIT(3) : BIT(2))
+#define FIN_RCVD(sidei_)		((sidei_) ? BIT(5) : BIT(4))
+#define FIN_SENT(sidei_)		((sidei_) ? BIT(7) : BIT(6))
 
 	uint8_t flags;
 #define SPLICE_V6			BIT(0)
-#define RCVLOWAT_SET_0			BIT(1)
-#define RCVLOWAT_SET_1			BIT(2)
-#define RCVLOWAT_ACT_0			BIT(3)
-#define RCVLOWAT_ACT_1			BIT(4)
+#define RCVLOWAT_SET(sidei_)		((sidei_) ? BIT(2) : BIT(1))
+#define RCVLOWAT_ACT(sidei_)		((sidei_) ? BIT(4) : BIT(3))
 #define CLOSING				BIT(5)
 
 	uint32_t read[SIDES];
diff --git a/tcp_splice.c b/tcp_splice.c
index 5a9325b1..ffddc853 100644
--- a/tcp_splice.c
+++ b/tcp_splice.c
@@ -119,19 +119,22 @@ static struct tcp_splice_conn *conn_at_sidx(flow_sidx_t sidx)
 static void tcp_splice_conn_epoll_events(uint16_t events,
 					 struct epoll_event ev[])
 {
-	ev[0].events = ev[1].events = 0;
+	unsigned sidei;
+
+	flow_foreach_sidei(sidei)
+		ev[sidei].events = 0;
 
 	if (events & SPLICE_ESTABLISHED) {
-		if (!(events & FIN_SENT_1))
-			ev[0].events = EPOLLIN | EPOLLRDHUP;
-		if (!(events & FIN_SENT_0))
-			ev[1].events = EPOLLIN | EPOLLRDHUP;
+		flow_foreach_sidei(sidei) {
+			if (!(events & FIN_SENT(!sidei)))
+				ev[sidei].events = EPOLLIN | EPOLLRDHUP;
+		}
 	} else if (events & SPLICE_CONNECT) {
 		ev[1].events = EPOLLOUT;
 	}
 
-	ev[0].events |= (events & OUT_WAIT_0) ? EPOLLOUT : 0;
-	ev[1].events |= (events & OUT_WAIT_1) ? EPOLLOUT : 0;
+	flow_foreach_sidei(sidei)
+		ev[sidei].events |= (events & OUT_WAIT(sidei)) ? EPOLLOUT : 0;
 }
 
 /**
@@ -553,21 +556,21 @@ void tcp_splice_sock_handler(struct ctx *c, union epoll_ref ref,
 
 	if (events & EPOLLOUT) {
 		fromsidei = !evsidei;
-		conn_event(c, conn, evsidei == 0 ? ~OUT_WAIT_0 : ~OUT_WAIT_1);
+		conn_event(c, conn, ~OUT_WAIT(evsidei));
 	} else {
 		fromsidei = evsidei;
 	}
 
 	if (events & EPOLLRDHUP)
 		/* For side 0 this is fake, but implied */
-		conn_event(c, conn, evsidei == 0 ? FIN_RCVD_0 : FIN_RCVD_1);
+		conn_event(c, conn, FIN_RCVD(evsidei));
 
 swap:
 	eof = 0;
 	never_read = 1;
 
-	lowat_set_flag = fromsidei == 0 ? RCVLOWAT_SET_0 : RCVLOWAT_SET_1;
-	lowat_act_flag = fromsidei == 0 ? RCVLOWAT_ACT_0 : RCVLOWAT_ACT_1;
+	lowat_set_flag = RCVLOWAT_SET(fromsidei);
+	lowat_act_flag = RCVLOWAT_ACT(fromsidei);
 
 	while (1) {
 		ssize_t readlen, to_write = 0, written;
@@ -644,8 +647,7 @@ eintr:
 			if (conn->read[fromsidei] == conn->written[fromsidei])
 				break;
 
-			conn_event(c, conn,
-				   fromsidei == 0 ? OUT_WAIT_1 : OUT_WAIT_0);
+			conn_event(c, conn, OUT_WAIT(fromsidei));
 			break;
 		}
 
@@ -661,21 +663,19 @@ eintr:
 			break;
 	}
 
-	if ((conn->events & FIN_RCVD_0) && !(conn->events & FIN_SENT_1)) {
-		if (conn->read[fromsidei] == conn->written[fromsidei] && eof) {
-			shutdown(conn->s[1], SHUT_WR);
-			conn_event(c, conn, FIN_SENT_1);
-		}
-	}
+	if (conn->read[fromsidei] == conn->written[fromsidei] && eof) {
+		unsigned sidei;
 
-	if ((conn->events & FIN_RCVD_1) && !(conn->events & FIN_SENT_0)) {
-		if (conn->read[fromsidei] == conn->written[fromsidei] && eof) {
-			shutdown(conn->s[0], SHUT_WR);
-			conn_event(c, conn, FIN_SENT_0);
+		flow_foreach_sidei(sidei) {
+			if ((conn->events & FIN_RCVD(sidei)) &&
+			    !(conn->events & FIN_SENT(!sidei))) {
+				shutdown(conn->s[!sidei], SHUT_WR);
+				conn_event(c, conn, FIN_SENT(!sidei));
+			}
 		}
 	}
 
-	if (CONN_HAS(conn, FIN_SENT_0 | FIN_SENT_1))
+	if (CONN_HAS(conn, FIN_SENT(0) | FIN_SENT(1)))
 		goto close;
 
 	if ((events & (EPOLLIN | EPOLLOUT)) == (EPOLLIN | EPOLLOUT)) {
@@ -821,19 +821,17 @@ void tcp_splice_timer(const struct ctx *c, struct tcp_splice_conn *conn)
 	ASSERT(!(conn->flags & CLOSING));
 
 	flow_foreach_sidei(sidei) {
-		uint8_t set = sidei == 0 ? RCVLOWAT_SET_0 : RCVLOWAT_SET_1;
-		uint8_t act = sidei == 0 ? RCVLOWAT_ACT_0 : RCVLOWAT_ACT_1;
-
-		if ((conn->flags & set) && !(conn->flags & act)) {
+		if ((conn->flags & RCVLOWAT_SET(sidei)) &&
+		    !(conn->flags & RCVLOWAT_ACT(sidei))) {
 			if (setsockopt(conn->s[sidei], SOL_SOCKET, SO_RCVLOWAT,
 				       &((int){ 1 }), sizeof(int))) {
 				flow_trace(conn, "can't set SO_RCVLOWAT on %d",
 					   conn->s[sidei]);
 			}
-			conn_flag(c, conn, ~set);
+			conn_flag(c, conn, ~RCVLOWAT_SET(sidei));
 		}
 	}
 
-	conn_flag(c, conn, ~RCVLOWAT_ACT_0);
-	conn_flag(c, conn, ~RCVLOWAT_ACT_1);
+	flow_foreach_sidei(sidei)
+		conn_flag(c, conn, ~RCVLOWAT_ACT(sidei));
 }
-- 
@@ -119,19 +119,22 @@ static struct tcp_splice_conn *conn_at_sidx(flow_sidx_t sidx)
 static void tcp_splice_conn_epoll_events(uint16_t events,
 					 struct epoll_event ev[])
 {
-	ev[0].events = ev[1].events = 0;
+	unsigned sidei;
+
+	flow_foreach_sidei(sidei)
+		ev[sidei].events = 0;
 
 	if (events & SPLICE_ESTABLISHED) {
-		if (!(events & FIN_SENT_1))
-			ev[0].events = EPOLLIN | EPOLLRDHUP;
-		if (!(events & FIN_SENT_0))
-			ev[1].events = EPOLLIN | EPOLLRDHUP;
+		flow_foreach_sidei(sidei) {
+			if (!(events & FIN_SENT(!sidei)))
+				ev[sidei].events = EPOLLIN | EPOLLRDHUP;
+		}
 	} else if (events & SPLICE_CONNECT) {
 		ev[1].events = EPOLLOUT;
 	}
 
-	ev[0].events |= (events & OUT_WAIT_0) ? EPOLLOUT : 0;
-	ev[1].events |= (events & OUT_WAIT_1) ? EPOLLOUT : 0;
+	flow_foreach_sidei(sidei)
+		ev[sidei].events |= (events & OUT_WAIT(sidei)) ? EPOLLOUT : 0;
 }
 
 /**
@@ -553,21 +556,21 @@ void tcp_splice_sock_handler(struct ctx *c, union epoll_ref ref,
 
 	if (events & EPOLLOUT) {
 		fromsidei = !evsidei;
-		conn_event(c, conn, evsidei == 0 ? ~OUT_WAIT_0 : ~OUT_WAIT_1);
+		conn_event(c, conn, ~OUT_WAIT(evsidei));
 	} else {
 		fromsidei = evsidei;
 	}
 
 	if (events & EPOLLRDHUP)
 		/* For side 0 this is fake, but implied */
-		conn_event(c, conn, evsidei == 0 ? FIN_RCVD_0 : FIN_RCVD_1);
+		conn_event(c, conn, FIN_RCVD(evsidei));
 
 swap:
 	eof = 0;
 	never_read = 1;
 
-	lowat_set_flag = fromsidei == 0 ? RCVLOWAT_SET_0 : RCVLOWAT_SET_1;
-	lowat_act_flag = fromsidei == 0 ? RCVLOWAT_ACT_0 : RCVLOWAT_ACT_1;
+	lowat_set_flag = RCVLOWAT_SET(fromsidei);
+	lowat_act_flag = RCVLOWAT_ACT(fromsidei);
 
 	while (1) {
 		ssize_t readlen, to_write = 0, written;
@@ -644,8 +647,7 @@ eintr:
 			if (conn->read[fromsidei] == conn->written[fromsidei])
 				break;
 
-			conn_event(c, conn,
-				   fromsidei == 0 ? OUT_WAIT_1 : OUT_WAIT_0);
+			conn_event(c, conn, OUT_WAIT(fromsidei));
 			break;
 		}
 
@@ -661,21 +663,19 @@ eintr:
 			break;
 	}
 
-	if ((conn->events & FIN_RCVD_0) && !(conn->events & FIN_SENT_1)) {
-		if (conn->read[fromsidei] == conn->written[fromsidei] && eof) {
-			shutdown(conn->s[1], SHUT_WR);
-			conn_event(c, conn, FIN_SENT_1);
-		}
-	}
+	if (conn->read[fromsidei] == conn->written[fromsidei] && eof) {
+		unsigned sidei;
 
-	if ((conn->events & FIN_RCVD_1) && !(conn->events & FIN_SENT_0)) {
-		if (conn->read[fromsidei] == conn->written[fromsidei] && eof) {
-			shutdown(conn->s[0], SHUT_WR);
-			conn_event(c, conn, FIN_SENT_0);
+		flow_foreach_sidei(sidei) {
+			if ((conn->events & FIN_RCVD(sidei)) &&
+			    !(conn->events & FIN_SENT(!sidei))) {
+				shutdown(conn->s[!sidei], SHUT_WR);
+				conn_event(c, conn, FIN_SENT(!sidei));
+			}
 		}
 	}
 
-	if (CONN_HAS(conn, FIN_SENT_0 | FIN_SENT_1))
+	if (CONN_HAS(conn, FIN_SENT(0) | FIN_SENT(1)))
 		goto close;
 
 	if ((events & (EPOLLIN | EPOLLOUT)) == (EPOLLIN | EPOLLOUT)) {
@@ -821,19 +821,17 @@ void tcp_splice_timer(const struct ctx *c, struct tcp_splice_conn *conn)
 	ASSERT(!(conn->flags & CLOSING));
 
 	flow_foreach_sidei(sidei) {
-		uint8_t set = sidei == 0 ? RCVLOWAT_SET_0 : RCVLOWAT_SET_1;
-		uint8_t act = sidei == 0 ? RCVLOWAT_ACT_0 : RCVLOWAT_ACT_1;
-
-		if ((conn->flags & set) && !(conn->flags & act)) {
+		if ((conn->flags & RCVLOWAT_SET(sidei)) &&
+		    !(conn->flags & RCVLOWAT_ACT(sidei))) {
 			if (setsockopt(conn->s[sidei], SOL_SOCKET, SO_RCVLOWAT,
 				       &((int){ 1 }), sizeof(int))) {
 				flow_trace(conn, "can't set SO_RCVLOWAT on %d",
 					   conn->s[sidei]);
 			}
-			conn_flag(c, conn, ~set);
+			conn_flag(c, conn, ~RCVLOWAT_SET(sidei));
 		}
 	}
 
-	conn_flag(c, conn, ~RCVLOWAT_ACT_0);
-	conn_flag(c, conn, ~RCVLOWAT_ACT_1);
+	flow_foreach_sidei(sidei)
+		conn_flag(c, conn, ~RCVLOWAT_ACT(sidei));
 }
-- 
2.45.2

[PATCH 5/6] doc: Test behaviour of closing duplicate UDP sockets

[David Gibson]

To simplify lifetime management of "listening" UDP sockets, UDP flow
support needs to duplicate existing bound sockets.  Those duplicates will
be close()d when their corresponding flow expires, but we expect the
original to still receive datagrams as always.  That is, we expect the
close() on the duplicate to remove the duplicated fd, but not to close the
underlying UDP socket.

Add a test program to doc/platform-requirements to verify this requirement.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 doc/platform-requirements/.gitignore      |   1 +
 doc/platform-requirements/Makefile        |   4 +-
 doc/platform-requirements/udp-close-dup.c | 105 ++++++++++++++++++++++
 3 files changed, 108 insertions(+), 2 deletions(-)
 create mode 100644 doc/platform-requirements/udp-close-dup.c

diff --git a/doc/platform-requirements/.gitignore b/doc/platform-requirements/.gitignore
index 555031d8..3b5a10ac 100644
--- a/doc/platform-requirements/.gitignore
+++ b/doc/platform-requirements/.gitignore
@@ -1,2 +1,3 @@
 /reuseaddr-priority
 /recv-zero
+/udp-close-dup
diff --git a/doc/platform-requirements/Makefile b/doc/platform-requirements/Makefile
index 82aaac29..6a7d3748 100644
--- a/doc/platform-requirements/Makefile
+++ b/doc/platform-requirements/Makefile
@@ -3,8 +3,8 @@
 # Copyright Red Hat
 # Author: David Gibson <david@gibson.dropbear.id.au>
 
-TARGETS = reuseaddr-priority recv-zero
-SRCS = reuseaddr-priority.c recv-zero.c
+TARGETS = reuseaddr-priority recv-zero udp-close-dup
+SRCS = reuseaddr-priority.c recv-zero.c udp-close-dup.c
 CFLAGS = -Wall
 
 all: cppcheck clang-tidy $(TARGETS:%=check-%)
diff --git a/doc/platform-requirements/udp-close-dup.c b/doc/platform-requirements/udp-close-dup.c
new file mode 100644
index 00000000..99060fcb
--- /dev/null
+++ b/doc/platform-requirements/udp-close-dup.c
@@ -0,0 +1,105 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+/* udp-close-dup.c
+ *
+ * Verify that closing one dup() of a UDP socket won't stop other dups from
+ * receiving packets.
+ *
+ * Copyright Red Hat
+ * Author: David Gibson <david@gibson.dropbear.id.au>
+ */
+
+#include <arpa/inet.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <net/if.h>
+#include <netinet/in.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+
+#include "common.h"
+
+#define DSTPORT	13257U
+
+/* 127.0.0.1:DSTPORT */
+static const struct sockaddr_in lo_dst = SOCKADDR_INIT(INADDR_LOOPBACK, DSTPORT);
+
+enum dup_method {
+	DUP_DUP,
+	DUP_FCNTL,
+	NUM_METHODS,
+};
+
+static void test_close_dup(enum dup_method method)
+{
+	long token;
+	int s1, s2, send_s;
+	ssize_t rc;
+
+	s1 = sock_reuseaddr();
+	if (bind(s1, (struct sockaddr *)&lo_dst, sizeof(lo_dst)) < 0)
+		die("bind(): %s\n", strerror(errno));
+
+	send_s = sock_reuseaddr();
+	if (connect(send_s, (struct sockaddr *)&lo_dst, sizeof(lo_dst)) < 0)
+		die("connect(): %s\n", strerror(errno));
+
+	/* Receive before duplicating */
+	token = random();
+	send_token(send_s, token);
+	recv_token(s1, token);
+
+	switch (method) {
+	case DUP_DUP:
+		/* NOLINTNEXTLINE(android-cloexec-dup) */
+		s2 = dup(s1);
+		if (s2 < 0)
+			die("dup(): %s\n", strerror(errno));
+		break;
+	case DUP_FCNTL:
+		s2 = fcntl(s1, F_DUPFD_CLOEXEC, 0);
+		if (s2 < 0)
+			die("F_DUPFD_CLOEXEC: %s\n", strerror(errno));
+		break;
+	default:
+		die("Bad method\n");
+	}
+
+	/* Receive via original handle */
+	token = random();
+	send_token(send_s, token);
+	recv_token(s1, token);
+
+	/* Receive via duplicated handle */
+	token = random();
+	send_token(send_s, token);
+	recv_token(s2, token);
+
+	/* Close duplicate */
+	rc = close(s2);
+	if (rc < 0)
+		die("close() dup: %s\n", strerror(errno));
+
+	/* Receive after closing duplicate */
+	token = random();
+	send_token(send_s, token);
+	recv_token(s1, token);
+}
+
+int main(int argc, char *argv[])
+{
+	enum dup_method method;
+
+	(void)argc;
+	(void)argv;
+
+	for (method = 0; method < NUM_METHODS; method++)
+		test_close_dup(method);
+
+	printf("Closing dup()ed UDP sockets seems to work as expected\n");
+
+	exit(0);
+}
-- 
@@ -0,0 +1,105 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+/* udp-close-dup.c
+ *
+ * Verify that closing one dup() of a UDP socket won't stop other dups from
+ * receiving packets.
+ *
+ * Copyright Red Hat
+ * Author: David Gibson <david@gibson.dropbear.id.au>
+ */
+
+#include <arpa/inet.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <net/if.h>
+#include <netinet/in.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+
+#include "common.h"
+
+#define DSTPORT	13257U
+
+/* 127.0.0.1:DSTPORT */
+static const struct sockaddr_in lo_dst = SOCKADDR_INIT(INADDR_LOOPBACK, DSTPORT);
+
+enum dup_method {
+	DUP_DUP,
+	DUP_FCNTL,
+	NUM_METHODS,
+};
+
+static void test_close_dup(enum dup_method method)
+{
+	long token;
+	int s1, s2, send_s;
+	ssize_t rc;
+
+	s1 = sock_reuseaddr();
+	if (bind(s1, (struct sockaddr *)&lo_dst, sizeof(lo_dst)) < 0)
+		die("bind(): %s\n", strerror(errno));
+
+	send_s = sock_reuseaddr();
+	if (connect(send_s, (struct sockaddr *)&lo_dst, sizeof(lo_dst)) < 0)
+		die("connect(): %s\n", strerror(errno));
+
+	/* Receive before duplicating */
+	token = random();
+	send_token(send_s, token);
+	recv_token(s1, token);
+
+	switch (method) {
+	case DUP_DUP:
+		/* NOLINTNEXTLINE(android-cloexec-dup) */
+		s2 = dup(s1);
+		if (s2 < 0)
+			die("dup(): %s\n", strerror(errno));
+		break;
+	case DUP_FCNTL:
+		s2 = fcntl(s1, F_DUPFD_CLOEXEC, 0);
+		if (s2 < 0)
+			die("F_DUPFD_CLOEXEC: %s\n", strerror(errno));
+		break;
+	default:
+		die("Bad method\n");
+	}
+
+	/* Receive via original handle */
+	token = random();
+	send_token(send_s, token);
+	recv_token(s1, token);
+
+	/* Receive via duplicated handle */
+	token = random();
+	send_token(send_s, token);
+	recv_token(s2, token);
+
+	/* Close duplicate */
+	rc = close(s2);
+	if (rc < 0)
+		die("close() dup: %s\n", strerror(errno));
+
+	/* Receive after closing duplicate */
+	token = random();
+	send_token(send_s, token);
+	recv_token(s1, token);
+}
+
+int main(int argc, char *argv[])
+{
+	enum dup_method method;
+
+	(void)argc;
+	(void)argv;
+
+	for (method = 0; method < NUM_METHODS; method++)
+		test_close_dup(method);
+
+	printf("Closing dup()ed UDP sockets seems to work as expected\n");
+
+	exit(0);
+}
-- 
2.45.2

[PATCH 6/6] doc: Extend zero-recv test with methods using msghdr

[David Gibson]

This test program verifies that we can receive and discard datagrams by
using recv() with a NULL buffer and zero-length.  Extend it to verify it
also works using recvmsg() and either an iov with a zero-length NULL
buffer or an iov that itself is NULL and zero-length.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 doc/platform-requirements/recv-zero.c | 60 +++++++++++++++++++++++----
 1 file changed, 52 insertions(+), 8 deletions(-)

diff --git a/doc/platform-requirements/recv-zero.c b/doc/platform-requirements/recv-zero.c
index f161e5c2..ab27704d 100644
--- a/doc/platform-requirements/recv-zero.c
+++ b/doc/platform-requirements/recv-zero.c
@@ -23,11 +23,27 @@
 
 #define DSTPORT	13257U
 
+enum discard_method {
+	DISCARD_NULL_BUF,
+	DISCARD_ZERO_IOV,
+	DISCARD_NULL_IOV,
+	NUM_METHODS,
+};
+
 /* 127.0.0.1:DSTPORT */
 static const struct sockaddr_in lo_dst = SOCKADDR_INIT(INADDR_LOOPBACK, DSTPORT);
 
-static void test_discard(void)
+static void test_discard(enum discard_method method)
 {
+	struct iovec zero_iov = { .iov_base = NULL, .iov_len = 0, };
+	struct msghdr mh_zero = {
+		.msg_iov = &zero_iov,
+		.msg_iovlen = 1,
+	};
+	struct msghdr mh_null = {
+		.msg_iov = NULL,
+		.msg_iovlen = 0,
+	};
 	long token1, token2;
 	int recv_s, send_s;
 	ssize_t rc;
@@ -46,11 +62,36 @@ static void test_discard(void)
 	send_token(send_s, token1);
 	send_token(send_s, token2);
 
-	/* cppcheck-suppress nullPointer */
-	rc = recv(recv_s, NULL, 0, MSG_DONTWAIT);
-	if (rc < 0)
-		die("discarding recv(): %s\n", strerror(errno));
-	
+	switch (method) {
+	case DISCARD_NULL_BUF:
+		/* cppcheck-suppress nullPointer */
+		rc = recv(recv_s, NULL, 0, MSG_DONTWAIT);
+		if (rc < 0)
+			die("discarding recv(): %s\n", strerror(errno));
+		break;
+
+	case DISCARD_ZERO_IOV:
+		rc = recvmsg(recv_s, &mh_zero, MSG_DONTWAIT);
+		if (rc < 0)
+			die("recvmsg() with zero-length buffer: %s\n",
+			    strerror(errno));
+		if (!((unsigned)mh_zero.msg_flags & MSG_TRUNC))
+			die("Missing MSG_TRUNC flag\n");
+		break;
+
+	case DISCARD_NULL_IOV:
+		rc = recvmsg(recv_s, &mh_null, MSG_DONTWAIT);
+		if (rc < 0)
+			die("recvmsg() with zero-length iov: %s\n",
+			    strerror(errno));
+		if (!((unsigned)mh_null.msg_flags & MSG_TRUNC))
+			die("Missing MSG_TRUNC flag\n");
+		break;
+
+	default:
+		die("Bad method\n");
+	}
+
 	recv_token(recv_s, token2);
 
 	/* cppcheck-suppress nullPointer */
@@ -63,12 +104,15 @@ static void test_discard(void)
 
 int main(int argc, char *argv[])
 {
+	enum discard_method method;
+
 	(void)argc;
 	(void)argv;
 
-	test_discard();
+	for (method = 0; method < NUM_METHODS; method++)
+		test_discard(method);
 
-	printf("Discarding datagrams with a 0-length recv() seems to work\n");
+	printf("Discarding datagrams with a 0-length receives seems to work\n");
 
 	exit(0);
 }
-- 
@@ -23,11 +23,27 @@
 
 #define DSTPORT	13257U
 
+enum discard_method {
+	DISCARD_NULL_BUF,
+	DISCARD_ZERO_IOV,
+	DISCARD_NULL_IOV,
+	NUM_METHODS,
+};
+
 /* 127.0.0.1:DSTPORT */
 static const struct sockaddr_in lo_dst = SOCKADDR_INIT(INADDR_LOOPBACK, DSTPORT);
 
-static void test_discard(void)
+static void test_discard(enum discard_method method)
 {
+	struct iovec zero_iov = { .iov_base = NULL, .iov_len = 0, };
+	struct msghdr mh_zero = {
+		.msg_iov = &zero_iov,
+		.msg_iovlen = 1,
+	};
+	struct msghdr mh_null = {
+		.msg_iov = NULL,
+		.msg_iovlen = 0,
+	};
 	long token1, token2;
 	int recv_s, send_s;
 	ssize_t rc;
@@ -46,11 +62,36 @@ static void test_discard(void)
 	send_token(send_s, token1);
 	send_token(send_s, token2);
 
-	/* cppcheck-suppress nullPointer */
-	rc = recv(recv_s, NULL, 0, MSG_DONTWAIT);
-	if (rc < 0)
-		die("discarding recv(): %s\n", strerror(errno));
-	
+	switch (method) {
+	case DISCARD_NULL_BUF:
+		/* cppcheck-suppress nullPointer */
+		rc = recv(recv_s, NULL, 0, MSG_DONTWAIT);
+		if (rc < 0)
+			die("discarding recv(): %s\n", strerror(errno));
+		break;
+
+	case DISCARD_ZERO_IOV:
+		rc = recvmsg(recv_s, &mh_zero, MSG_DONTWAIT);
+		if (rc < 0)
+			die("recvmsg() with zero-length buffer: %s\n",
+			    strerror(errno));
+		if (!((unsigned)mh_zero.msg_flags & MSG_TRUNC))
+			die("Missing MSG_TRUNC flag\n");
+		break;
+
+	case DISCARD_NULL_IOV:
+		rc = recvmsg(recv_s, &mh_null, MSG_DONTWAIT);
+		if (rc < 0)
+			die("recvmsg() with zero-length iov: %s\n",
+			    strerror(errno));
+		if (!((unsigned)mh_null.msg_flags & MSG_TRUNC))
+			die("Missing MSG_TRUNC flag\n");
+		break;
+
+	default:
+		die("Bad method\n");
+	}
+
 	recv_token(recv_s, token2);
 
 	/* cppcheck-suppress nullPointer */
@@ -63,12 +104,15 @@ static void test_discard(void)
 
 int main(int argc, char *argv[])
 {
+	enum discard_method method;
+
 	(void)argc;
 	(void)argv;
 
-	test_discard();
+	for (method = 0; method < NUM_METHODS; method++)
+		test_discard(method);
 
-	printf("Discarding datagrams with a 0-length recv() seems to work\n");
+	printf("Discarding datagrams with a 0-length receives seems to work\n");
 
 	exit(0);
 }
-- 
2.45.2

Re: [PATCH 2/6] flow, tcp_splice: Prefer 'sidei' for variables referring to side index

[Stefano Brivio]

On Wed, 17 Jul 2024 14:52:19 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> @@ -164,17 +164,17 @@ struct flow_common {
>  
>  /**
>   * struct flow_sidx - ID for one side of a specific flow
> - * @side:	Side referenced (0 or 1)
> + * @side:	Index of side referenced (0 or 1)
>   * @flow:	Index of flow referenced

Nit (I can fix this up on merge, and I didn't finish reviewing yet):
the comment to the struct should also use sidei/flowi.

>   */
>  typedef struct flow_sidx {
> -	unsigned	side :1;
> -	unsigned	flow :FLOW_INDEX_BITS;
> +	unsigned	sidei :1;
> +	unsigned	flowi :FLOW_INDEX_BITS;
>  } flow_sidx_t;

-- 
Stefano

Re: [PATCH 2/6] flow, tcp_splice: Prefer 'sidei' for variables referring to side index

[David Gibson]

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

On Wed, Jul 17, 2024 at 07:20:17AM +0200, Stefano Brivio wrote:
> On Wed, 17 Jul 2024 14:52:19 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > @@ -164,17 +164,17 @@ struct flow_common {
> >  
> >  /**
> >   * struct flow_sidx - ID for one side of a specific flow
> > - * @side:	Side referenced (0 or 1)
> > + * @side:	Index of side referenced (0 or 1)
> >   * @flow:	Index of flow referenced
> 
> Nit (I can fix this up on merge, and I didn't finish reviewing yet):
> the comment to the struct should also use sidei/flowi.

If this is the only issue, go ahead and fix on merge.  If there are
any more nits I'll make a new spin.

> 
> >   */
> >  typedef struct flow_sidx {
> > -	unsigned	side :1;
> > -	unsigned	flow :FLOW_INDEX_BITS;
> > +	unsigned	sidei :1;
> > +	unsigned	flowi :FLOW_INDEX_BITS;
> >  } flow_sidx_t;
> 

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

Re: [PATCH 2/6] flow, tcp_splice: Prefer 'sidei' for variables referring to side index

[David Gibson]

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

On Wed, Jul 17, 2024 at 03:28:05PM +1000, David Gibson wrote:
> On Wed, Jul 17, 2024 at 07:20:17AM +0200, Stefano Brivio wrote:
> > On Wed, 17 Jul 2024 14:52:19 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> > 
> > > @@ -164,17 +164,17 @@ struct flow_common {
> > >  
> > >  /**
> > >   * struct flow_sidx - ID for one side of a specific flow
> > > - * @side:	Side referenced (0 or 1)
> > > + * @side:	Index of side referenced (0 or 1)
> > >   * @flow:	Index of flow referenced
> > 
> > Nit (I can fix this up on merge, and I didn't finish reviewing yet):
> > the comment to the struct should also use sidei/flowi.
> 
> If this is the only issue, go ahead and fix on merge.  If there are
> any more nits I'll make a new spin.

Actually, never mind.  I discovered I hadn't rebased onto the latest
and there's a conflict, so I'll respin anyway.

> > >   */
> > >  typedef struct flow_sidx {
> > > -	unsigned	side :1;
> > > -	unsigned	flow :FLOW_INDEX_BITS;
> > > +	unsigned	sidei :1;
> > > +	unsigned	flowi :FLOW_INDEX_BITS;
> > >  } flow_sidx_t;
> > 
> 



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

Re: [PATCH 2/6] flow, tcp_splice: Prefer 'sidei' for variables referring to side index

[Stefano Brivio]

On Wed, 17 Jul 2024 15:50:32 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Jul 17, 2024 at 03:28:05PM +1000, David Gibson wrote:
> > On Wed, Jul 17, 2024 at 07:20:17AM +0200, Stefano Brivio wrote:  
> > > On Wed, 17 Jul 2024 14:52:19 +1000
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >   
> > > > @@ -164,17 +164,17 @@ struct flow_common {
> > > >  
> > > >  /**
> > > >   * struct flow_sidx - ID for one side of a specific flow
> > > > - * @side:	Side referenced (0 or 1)
> > > > + * @side:	Index of side referenced (0 or 1)
> > > >   * @flow:	Index of flow referenced  
> > > 
> > > Nit (I can fix this up on merge, and I didn't finish reviewing yet):
> > > the comment to the struct should also use sidei/flowi.  
> > 
> > If this is the only issue, go ahead and fix on merge.  If there are
> > any more nits I'll make a new spin.  
> 
> Actually, never mind.  I discovered I hadn't rebased onto the latest
> and there's a conflict, so I'll respin anyway.

It's trivial, just a bit of fuzz on 1/6, no need as far as I'm
concerned.

-- 
Stefano

Re: [PATCH 2/6] flow, tcp_splice: Prefer 'sidei' for variables referring to side index

[David Gibson]

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

On Wed, Jul 17, 2024 at 09:20:42AM +0200, Stefano Brivio wrote:
> On Wed, 17 Jul 2024 15:50:32 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Wed, Jul 17, 2024 at 03:28:05PM +1000, David Gibson wrote:
> > > On Wed, Jul 17, 2024 at 07:20:17AM +0200, Stefano Brivio wrote:  
> > > > On Wed, 17 Jul 2024 14:52:19 +1000
> > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > >   
> > > > > @@ -164,17 +164,17 @@ struct flow_common {
> > > > >  
> > > > >  /**
> > > > >   * struct flow_sidx - ID for one side of a specific flow
> > > > > - * @side:	Side referenced (0 or 1)
> > > > > + * @side:	Index of side referenced (0 or 1)
> > > > >   * @flow:	Index of flow referenced  
> > > > 
> > > > Nit (I can fix this up on merge, and I didn't finish reviewing yet):
> > > > the comment to the struct should also use sidei/flowi.  
> > > 
> > > If this is the only issue, go ahead and fix on merge.  If there are
> > > any more nits I'll make a new spin.  
> > 
> > Actually, never mind.  I discovered I hadn't rebased onto the latest
> > and there's a conflict, so I'll respin anyway.
> 
> It's trivial, just a bit of fuzz on 1/6, no need as far as I'm
> concerned.

Ok.  Well go ahead and apply then, if you have no additional revisions
to suggest.

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

Re: [PATCH 6/6] doc: Extend zero-recv test with methods using msghdr

[Stefano Brivio]

On Wed, 17 Jul 2024 14:52:23 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> This test program verifies that we can receive and discard datagrams by
> using recv() with a NULL buffer and zero-length.  Extend it to verify it
> also works using recvmsg() and either an iov with a zero-length NULL
> buffer or an iov that itself is NULL and zero-length.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  doc/platform-requirements/recv-zero.c | 60 +++++++++++++++++++++++----
>  1 file changed, 52 insertions(+), 8 deletions(-)
> 
> diff --git a/doc/platform-requirements/recv-zero.c b/doc/platform-requirements/recv-zero.c
> index f161e5c2..ab27704d 100644
> --- a/doc/platform-requirements/recv-zero.c
> +++ b/doc/platform-requirements/recv-zero.c
> @@ -23,11 +23,27 @@
>  
>  #define DSTPORT	13257U
>  
> +enum discard_method {
> +	DISCARD_NULL_BUF,
> +	DISCARD_ZERO_IOV,
> +	DISCARD_NULL_IOV,
> +	NUM_METHODS,
> +};
> +
>  /* 127.0.0.1:DSTPORT */
>  static const struct sockaddr_in lo_dst = SOCKADDR_INIT(INADDR_LOOPBACK, DSTPORT);
>  
> -static void test_discard(void)
> +static void test_discard(enum discard_method method)
>  {
> +	struct iovec zero_iov = { .iov_base = NULL, .iov_len = 0, };
> +	struct msghdr mh_zero = {
> +		.msg_iov = &zero_iov,
> +		.msg_iovlen = 1,
> +	};
> +	struct msghdr mh_null = {
> +		.msg_iov = NULL,
> +		.msg_iovlen = 0,
> +	};
>  	long token1, token2;
>  	int recv_s, send_s;
>  	ssize_t rc;
> @@ -46,11 +62,36 @@ static void test_discard(void)
>  	send_token(send_s, token1);
>  	send_token(send_s, token2);
>  
> -	/* cppcheck-suppress nullPointer */
> -	rc = recv(recv_s, NULL, 0, MSG_DONTWAIT);
> -	if (rc < 0)
> -		die("discarding recv(): %s\n", strerror(errno));
> -	
> +	switch (method) {
> +	case DISCARD_NULL_BUF:
> +		/* cppcheck-suppress nullPointer */
> +		rc = recv(recv_s, NULL, 0, MSG_DONTWAIT);
> +		if (rc < 0)
> +			die("discarding recv(): %s\n", strerror(errno));
> +		break;
> +
> +	case DISCARD_ZERO_IOV:
> +		rc = recvmsg(recv_s, &mh_zero, MSG_DONTWAIT);
> +		if (rc < 0)
> +			die("recvmsg() with zero-length buffer: %s\n",
> +			    strerror(errno));
> +		if (!((unsigned)mh_zero.msg_flags & MSG_TRUNC))
> +			die("Missing MSG_TRUNC flag\n");
> +		break;
> +
> +	case DISCARD_NULL_IOV:
> +		rc = recvmsg(recv_s, &mh_null, MSG_DONTWAIT);
> +		if (rc < 0)
> +			die("recvmsg() with zero-length iov: %s\n",
> +			    strerror(errno));
> +		if (!((unsigned)mh_null.msg_flags & MSG_TRUNC))
> +			die("Missing MSG_TRUNC flag\n");
> +		break;
> +
> +	default:
> +		die("Bad method\n");
> +	}
> +
>  	recv_token(recv_s, token2);
>  
>  	/* cppcheck-suppress nullPointer */
> @@ -63,12 +104,15 @@ static void test_discard(void)
>  
>  int main(int argc, char *argv[])
>  {
> +	enum discard_method method;
> +
>  	(void)argc;
>  	(void)argv;
>  
> -	test_discard();
> +	for (method = 0; method < NUM_METHODS; method++)
> +		test_discard(method);
>  
> -	printf("Discarding datagrams with a 0-length recv() seems to work\n");
> +	printf("Discarding datagrams with a 0-length receives seems to work\n");

Nit: also fixed up on merge: "with 0-length receives".

>  
>  	exit(0);
>  }

-- 
Stefano

Re: [PATCH 0/6] Of course there are more flow table preliminaries

[Stefano Brivio]

On Wed, 17 Jul 2024 14:52:17 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> This series has some further preliminaries for the flow table.
> Specifically this is focused on some clean ups to handling of indicies
> for flows and sides.  We also add some additional test programs for
> platform requirements.
> 
> This is based on the series adding UDP error handling.
> 
> David Gibson (6):
>   flow, icmp, tcp: Clean up helpers for getting flow from index
>   flow, tcp_splice: Prefer 'sidei' for variables referring to side index
>   flow: Introduce flow_foreach_sidei() macro
>   tcp_splice: Use parameterised macros for per-side event/flag bits
>   doc: Test behaviour of closing duplicate UDP sockets
>   doc: Extend zero-recv test with methods using msghdr

Applied.

-- 
Stefano