[PATCH v2 0/2] tcp: unify IPv4 and IPv6 tap queues

[PATCH v2 0/2] tcp: unify IPv4 and IPv6 tap queues

[Jon Maloy]

This should save us some memory and code.

---
v2: - Setting pointers to pre-set IP and MAC headers on the fly
      instead of copying them.
    - Merged patch #2 and #3 from v1

Jon Maloy (2):
  tcp: set ip and eth headers in l2 tap queues on the fly
  tcp: unify l2 TCPv4 and TCPv6 queues and structures

 tcp.c     |   6 +-
 tcp_buf.c | 257 ++++++++++++++++++------------------------------------
 tcp_buf.h |   3 +-
 3 files changed, 88 insertions(+), 178 deletions(-)

-- 
2.45.2

[PATCH v2 1/2] tcp: set ip and eth headers in l2 tap queues on the fly

[Jon Maloy]

l2 tap queue entries are currently initialized at system start, and
reused with preset headers through its whole life time. The only
fields we need to update per message are things like payload size
and checksums.

If we want to reuse these entries between ipv4 and ipv6 messages we
will need to set the pointer to the right header on the fly per
message, since the header type may differ between entries in the same
queue.

The same needs to be done for the ethernet header.

We do these changes here.

Signed-off-by: Jon Maloy <jmaloy@redhat.com>

---
v2: Setting pointers to pre-initialized IP and MAC headers instead of
    copying them in on the fly.
---
 tcp_buf.c | 42 ++++++++++++++++++++++++++----------------
 1 file changed, 26 insertions(+), 16 deletions(-)

diff --git a/tcp_buf.c b/tcp_buf.c
index c31e9f3..af80cc5 100644
--- a/tcp_buf.c
+++ b/tcp_buf.c
@@ -159,8 +159,7 @@ void tcp_sock4_iov_init(const struct ctx *c)
 		iov = tcp4_l2_iov[i];
 
 		iov[TCP_IOV_TAP] = tap_hdr_iov(c, &tcp4_payload_tap_hdr[i]);
-		iov[TCP_IOV_ETH] = IOV_OF_LVALUE(tcp4_eth_src);
-		iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp4_payload_ip[i]);
+		iov[TCP_IOV_ETH].iov_len = sizeof(struct ethhdr);
 		iov[TCP_IOV_PAYLOAD].iov_base = &tcp4_payload[i];
 	}
 
@@ -203,8 +202,7 @@ void tcp_sock6_iov_init(const struct ctx *c)
 		iov = tcp6_l2_iov[i];
 
 		iov[TCP_IOV_TAP] = tap_hdr_iov(c, &tcp6_payload_tap_hdr[i]);
-		iov[TCP_IOV_ETH] = IOV_OF_LVALUE(tcp6_eth_src);
-		iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp6_payload_ip[i]);
+		iov[TCP_IOV_ETH].iov_len = sizeof(struct ethhdr);
 		iov[TCP_IOV_PAYLOAD].iov_base = &tcp6_payload[i];
 	}
 
@@ -303,11 +301,15 @@ int tcp_buf_send_flag(struct ctx *c, struct tcp_tap_conn *conn, int flags)
 	uint32_t seq;
 	int ret;
 
-	if (CONN_V4(conn))
-		iov = tcp4_l2_flags_iov[tcp4_flags_used++];
-	else
-		iov = tcp6_l2_flags_iov[tcp6_flags_used++];
-
+	if (CONN_V4(conn)) {
+		iov = tcp4_l2_flags_iov[tcp4_flags_used];
+		iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp4_flags_ip[tcp4_flags_used++]);
+		iov[TCP_IOV_ETH].iov_base = &tcp4_eth_src;
+	} else {
+		iov = tcp6_l2_flags_iov[tcp6_flags_used];
+		iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp6_flags_ip[tcp6_flags_used++]);
+		iov[TCP_IOV_ETH].iov_base = &tcp6_eth_src;
+	}
 	payload = iov[TCP_IOV_PAYLOAD].iov_base;
 
 	seq = conn->seq_to_tap;
@@ -328,11 +330,15 @@ int tcp_buf_send_flag(struct ctx *c, struct tcp_tap_conn *conn, int flags)
 		struct iovec *dup_iov;
 		int i;
 
-		if (CONN_V4(conn))
-			dup_iov = tcp4_l2_flags_iov[tcp4_flags_used++];
-		else
-			dup_iov = tcp6_l2_flags_iov[tcp6_flags_used++];
-
+		if (CONN_V4(conn)) {
+			dup_iov = tcp4_l2_flags_iov[tcp4_flags_used];
+			dup_iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp4_flags_ip[tcp4_flags_used++]);
+			dup_iov[TCP_IOV_ETH].iov_base = &tcp4_eth_src;
+		} else {
+			dup_iov = tcp4_l2_flags_iov[tcp6_flags_used];
+			dup_iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp6_flags_ip[tcp6_flags_used++]);
+			dup_iov[TCP_IOV_ETH].iov_base = &tcp6_eth_src;
+		}
 		for (i = 0; i < TCP_NUM_IOVS; i++)
 			memcpy(dup_iov[i].iov_base, iov[i].iov_base,
 			       iov[i].iov_len);
@@ -377,7 +383,9 @@ static void tcp_data_to_tap(struct ctx *c, struct tcp_tap_conn *conn,
 
 		tcp4_frame_conns[tcp4_payload_used] = conn;
 
-		iov = tcp4_l2_iov[tcp4_payload_used++];
+		iov = tcp4_l2_iov[tcp4_payload_used];
+		iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp4_payload_ip[tcp4_payload_used++]);
+		iov[TCP_IOV_ETH].iov_base = &tcp4_eth_src;
 		l4len = tcp_l2_buf_fill_headers(conn, iov, dlen, check, seq);
 		iov[TCP_IOV_PAYLOAD].iov_len = l4len;
 		if (tcp4_payload_used > TCP_FRAMES_MEM - 1)
@@ -385,7 +393,9 @@ static void tcp_data_to_tap(struct ctx *c, struct tcp_tap_conn *conn,
 	} else if (CONN_V6(conn)) {
 		tcp6_frame_conns[tcp6_payload_used] = conn;
 
-		iov = tcp6_l2_iov[tcp6_payload_used++];
+		iov = tcp6_l2_iov[tcp6_payload_used];
+		iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp6_payload_ip[tcp6_payload_used++]);
+		iov[TCP_IOV_ETH].iov_base = &tcp6_eth_src;
 		l4len = tcp_l2_buf_fill_headers(conn, iov, dlen, NULL, seq);
 		iov[TCP_IOV_PAYLOAD].iov_len = l4len;
 		if (tcp6_payload_used > TCP_FRAMES_MEM - 1)
-- 
@@ -159,8 +159,7 @@ void tcp_sock4_iov_init(const struct ctx *c)
 		iov = tcp4_l2_iov[i];
 
 		iov[TCP_IOV_TAP] = tap_hdr_iov(c, &tcp4_payload_tap_hdr[i]);
-		iov[TCP_IOV_ETH] = IOV_OF_LVALUE(tcp4_eth_src);
-		iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp4_payload_ip[i]);
+		iov[TCP_IOV_ETH].iov_len = sizeof(struct ethhdr);
 		iov[TCP_IOV_PAYLOAD].iov_base = &tcp4_payload[i];
 	}
 
@@ -203,8 +202,7 @@ void tcp_sock6_iov_init(const struct ctx *c)
 		iov = tcp6_l2_iov[i];
 
 		iov[TCP_IOV_TAP] = tap_hdr_iov(c, &tcp6_payload_tap_hdr[i]);
-		iov[TCP_IOV_ETH] = IOV_OF_LVALUE(tcp6_eth_src);
-		iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp6_payload_ip[i]);
+		iov[TCP_IOV_ETH].iov_len = sizeof(struct ethhdr);
 		iov[TCP_IOV_PAYLOAD].iov_base = &tcp6_payload[i];
 	}
 
@@ -303,11 +301,15 @@ int tcp_buf_send_flag(struct ctx *c, struct tcp_tap_conn *conn, int flags)
 	uint32_t seq;
 	int ret;
 
-	if (CONN_V4(conn))
-		iov = tcp4_l2_flags_iov[tcp4_flags_used++];
-	else
-		iov = tcp6_l2_flags_iov[tcp6_flags_used++];
-
+	if (CONN_V4(conn)) {
+		iov = tcp4_l2_flags_iov[tcp4_flags_used];
+		iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp4_flags_ip[tcp4_flags_used++]);
+		iov[TCP_IOV_ETH].iov_base = &tcp4_eth_src;
+	} else {
+		iov = tcp6_l2_flags_iov[tcp6_flags_used];
+		iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp6_flags_ip[tcp6_flags_used++]);
+		iov[TCP_IOV_ETH].iov_base = &tcp6_eth_src;
+	}
 	payload = iov[TCP_IOV_PAYLOAD].iov_base;
 
 	seq = conn->seq_to_tap;
@@ -328,11 +330,15 @@ int tcp_buf_send_flag(struct ctx *c, struct tcp_tap_conn *conn, int flags)
 		struct iovec *dup_iov;
 		int i;
 
-		if (CONN_V4(conn))
-			dup_iov = tcp4_l2_flags_iov[tcp4_flags_used++];
-		else
-			dup_iov = tcp6_l2_flags_iov[tcp6_flags_used++];
-
+		if (CONN_V4(conn)) {
+			dup_iov = tcp4_l2_flags_iov[tcp4_flags_used];
+			dup_iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp4_flags_ip[tcp4_flags_used++]);
+			dup_iov[TCP_IOV_ETH].iov_base = &tcp4_eth_src;
+		} else {
+			dup_iov = tcp4_l2_flags_iov[tcp6_flags_used];
+			dup_iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp6_flags_ip[tcp6_flags_used++]);
+			dup_iov[TCP_IOV_ETH].iov_base = &tcp6_eth_src;
+		}
 		for (i = 0; i < TCP_NUM_IOVS; i++)
 			memcpy(dup_iov[i].iov_base, iov[i].iov_base,
 			       iov[i].iov_len);
@@ -377,7 +383,9 @@ static void tcp_data_to_tap(struct ctx *c, struct tcp_tap_conn *conn,
 
 		tcp4_frame_conns[tcp4_payload_used] = conn;
 
-		iov = tcp4_l2_iov[tcp4_payload_used++];
+		iov = tcp4_l2_iov[tcp4_payload_used];
+		iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp4_payload_ip[tcp4_payload_used++]);
+		iov[TCP_IOV_ETH].iov_base = &tcp4_eth_src;
 		l4len = tcp_l2_buf_fill_headers(conn, iov, dlen, check, seq);
 		iov[TCP_IOV_PAYLOAD].iov_len = l4len;
 		if (tcp4_payload_used > TCP_FRAMES_MEM - 1)
@@ -385,7 +393,9 @@ static void tcp_data_to_tap(struct ctx *c, struct tcp_tap_conn *conn,
 	} else if (CONN_V6(conn)) {
 		tcp6_frame_conns[tcp6_payload_used] = conn;
 
-		iov = tcp6_l2_iov[tcp6_payload_used++];
+		iov = tcp6_l2_iov[tcp6_payload_used];
+		iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp6_payload_ip[tcp6_payload_used++]);
+		iov[TCP_IOV_ETH].iov_base = &tcp6_eth_src;
 		l4len = tcp_l2_buf_fill_headers(conn, iov, dlen, NULL, seq);
 		iov[TCP_IOV_PAYLOAD].iov_len = l4len;
 		if (tcp6_payload_used > TCP_FRAMES_MEM - 1)
-- 
2.45.2

[PATCH v2 2/2] tcp: unify l2 TCPv4 and TCPv6 queues and structures

[Jon Maloy]

Following the preparations in the previous commit, we can now remove
the payload and flag queues dedicated for TCPv6 and TCPv4 and move all
traffic into common queues handling both protocol types.

Apart from reducing code and memory footprint, this change reduces
a potential risk for TCPv4 traffic starving out TCPv6 traffic.
Since we always flush out the TCPv4 frame queue before the TCPv6 queue,
the latter will never be handled if the former fails to send all its
frames.

Tests with iperf3 shows no measurable change in performance after this
change.

Signed-off-by: Jon Maloy <jmaloy@redhat.com>

---
v2: Merged previous patch #3, where the tcp4_ prefix was changed to
    tcp_ into this patch.
---
 tcp.c     |   6 +-
 tcp_buf.c | 241 +++++++++++++++++-------------------------------------
 tcp_buf.h |   3 +-
 3 files changed, 75 insertions(+), 175 deletions(-)

diff --git a/tcp.c b/tcp.c
index f9fe1b9..294eeee 100644
--- a/tcp.c
+++ b/tcp.c
@@ -2504,11 +2504,7 @@ int tcp_init(struct ctx *c)
 {
 	ASSERT(!c->no_tcp);
 
-	if (c->ifi4)
-		tcp_sock4_iov_init(c);
-
-	if (c->ifi6)
-		tcp_sock6_iov_init(c);
+	tcp_sock_iov_init(c);
 
 	memset(init_sock_pool4,		0xff,	sizeof(init_sock_pool4));
 	memset(init_sock_pool6,		0xff,	sizeof(init_sock_pool6));
diff --git a/tcp_buf.c b/tcp_buf.c
index af80cc5..ab55476 100644
--- a/tcp_buf.c
+++ b/tcp_buf.c
@@ -67,59 +67,42 @@ struct tcp_flags_t {
 } __attribute__ ((packed, aligned(__alignof__(unsigned int))));
 #endif
 
-/* Ethernet header for IPv4 frames */
+/* Ethernet header for IPv4 and IPv6 frames */
 static struct ethhdr		tcp4_eth_src;
+static struct ethhdr		tcp6_eth_src;
+
+static struct tap_hdr		tcp_payload_tap_hdr[TCP_FRAMES_MEM];
 
-static struct tap_hdr		tcp4_payload_tap_hdr[TCP_FRAMES_MEM];
-/* IPv4 headers */
-static struct iphdr		tcp4_payload_ip[TCP_FRAMES_MEM];
-/* TCP segments with payload for IPv4 frames */
-static struct tcp_payload_t	tcp4_payload[TCP_FRAMES_MEM];
+/* IP headers for IPv4 and IPv6 */
+struct iphdr		tcp4_payload_ip[TCP_FRAMES_MEM];
+struct ipv6hdr		tcp6_payload_ip[TCP_FRAMES_MEM];
 
-static_assert(MSS4 <= sizeof(tcp4_payload[0].data), "MSS4 is greater than 65516");
+/* TCP segments with payload for IPv4 and IPv6 frames */
+static struct tcp_payload_t	tcp_payload[TCP_FRAMES_MEM];
+
+static_assert(sizeof(tcp_payload[0].data) > MSS4, "MSS4 is greater than 65516");
 
 /* References tracking the owner connection of frames in the tap outqueue */
-static struct tcp_tap_conn *tcp4_frame_conns[TCP_FRAMES_MEM];
-static unsigned int tcp4_payload_used;
+static struct tcp_tap_conn *tcp_frame_conns[TCP_FRAMES_MEM];
+static unsigned int tcp_payload_used;
 
-static struct tap_hdr		tcp4_flags_tap_hdr[TCP_FRAMES_MEM];
+static struct tap_hdr		tcp_flags_tap_hdr[TCP_FRAMES_MEM];
 /* IPv4 headers for TCP segment without payload */
 static struct iphdr		tcp4_flags_ip[TCP_FRAMES_MEM];
 /* TCP segments without payload for IPv4 frames */
-static struct tcp_flags_t	tcp4_flags[TCP_FRAMES_MEM];
-
-static unsigned int tcp4_flags_used;
+static struct tcp_flags_t	tcp_flags[TCP_FRAMES_MEM];
 
-/* Ethernet header for IPv6 frames */
-static struct ethhdr		tcp6_eth_src;
-
-static struct tap_hdr		tcp6_payload_tap_hdr[TCP_FRAMES_MEM];
-/* IPv6 headers */
-static struct ipv6hdr		tcp6_payload_ip[TCP_FRAMES_MEM];
-/* TCP headers and data for IPv6 frames */
-static struct tcp_payload_t	tcp6_payload[TCP_FRAMES_MEM];
+static unsigned int tcp_flags_used;
 
-static_assert(MSS6 <= sizeof(tcp6_payload[0].data), "MSS6 is greater than 65516");
-
-/* References tracking the owner connection of frames in the tap outqueue */
-static struct tcp_tap_conn *tcp6_frame_conns[TCP_FRAMES_MEM];
-static unsigned int tcp6_payload_used;
-
-static struct tap_hdr		tcp6_flags_tap_hdr[TCP_FRAMES_MEM];
 /* IPv6 headers for TCP segment without payload */
 static struct ipv6hdr		tcp6_flags_ip[TCP_FRAMES_MEM];
-/* TCP segment without payload for IPv6 frames */
-static struct tcp_flags_t	tcp6_flags[TCP_FRAMES_MEM];
-
-static unsigned int tcp6_flags_used;
 
 /* recvmsg()/sendmsg() data for tap */
 static struct iovec	iov_sock		[TCP_FRAMES_MEM + 1];
 
-static struct iovec	tcp4_l2_iov		[TCP_FRAMES_MEM][TCP_NUM_IOVS];
-static struct iovec	tcp6_l2_iov		[TCP_FRAMES_MEM][TCP_NUM_IOVS];
-static struct iovec	tcp4_l2_flags_iov	[TCP_FRAMES_MEM][TCP_NUM_IOVS];
-static struct iovec	tcp6_l2_flags_iov	[TCP_FRAMES_MEM][TCP_NUM_IOVS];
+static struct iovec	tcp_l2_iov[TCP_FRAMES_MEM][TCP_NUM_IOVS];
+static struct iovec	tcp_l2_flags_iov[TCP_FRAMES_MEM][TCP_NUM_IOVS];
+
 /**
  * tcp_update_l2_buf() - Update Ethernet header buffers with addresses
  * @eth_d:	Ethernet destination address, NULL if unchanged
@@ -132,87 +115,47 @@ void tcp_update_l2_buf(const unsigned char *eth_d, const unsigned char *eth_s)
 }
 
 /**
- * tcp_sock4_iov_init() - Initialise scatter-gather L2 buffers for IPv4 sockets
+ * tcp_sock_iov_init() - Initialise scatter-gather L2 buffers for IPv4 sockets
  * @c:		Execution context
  */
-void tcp_sock4_iov_init(const struct ctx *c)
+void tcp_sock_iov_init(const struct ctx *c)
 {
+	struct ipv6hdr ip6 = L2_BUF_IP6_INIT(IPPROTO_TCP);
 	struct iphdr iph = L2_BUF_IP4_INIT(IPPROTO_TCP);
 	struct iovec *iov;
 	int i;
 
+	tcp6_eth_src.h_proto = htons_constant(ETH_P_IPV6);
 	tcp4_eth_src.h_proto = htons_constant(ETH_P_IP);
 
-	for (i = 0; i < ARRAY_SIZE(tcp4_payload); i++) {
+	for (i = 0; i < ARRAY_SIZE(tcp_payload); i++) {
+		tcp6_payload_ip[i] = ip6;
 		tcp4_payload_ip[i] = iph;
-		tcp4_payload[i].th.doff = sizeof(struct tcphdr) / 4;
-		tcp4_payload[i].th.ack = 1;
+		tcp_payload[i].th.doff = sizeof(struct tcphdr) / 4;
+		tcp_payload[i].th.ack = 1;
 	}
 
-	for (i = 0; i < ARRAY_SIZE(tcp4_flags); i++) {
+	for (i = 0; i < ARRAY_SIZE(tcp_flags); i++) {
+		tcp6_flags_ip[i] = ip6;
 		tcp4_flags_ip[i] = iph;
-		tcp4_flags[i].th.doff = sizeof(struct tcphdr) / 4;
-		tcp4_flags[i].th.ack = 1;
+		tcp_flags[i].th.doff = sizeof(struct tcphdr) / 4;
+		tcp_flags[i].th.ack = 1;
 	}
 
 	for (i = 0; i < TCP_FRAMES_MEM; i++) {
-		iov = tcp4_l2_iov[i];
+		iov = tcp_l2_iov[i];
 
-		iov[TCP_IOV_TAP] = tap_hdr_iov(c, &tcp4_payload_tap_hdr[i]);
+		iov[TCP_IOV_TAP] = tap_hdr_iov(c, &tcp_payload_tap_hdr[i]);
 		iov[TCP_IOV_ETH].iov_len = sizeof(struct ethhdr);
-		iov[TCP_IOV_PAYLOAD].iov_base = &tcp4_payload[i];
+		iov[TCP_IOV_PAYLOAD].iov_base = &tcp_payload[i];
 	}
 
 	for (i = 0; i < TCP_FRAMES_MEM; i++) {
-		iov = tcp4_l2_flags_iov[i];
-
-		iov[TCP_IOV_TAP] = tap_hdr_iov(c, &tcp4_flags_tap_hdr[i]);
-		iov[TCP_IOV_ETH].iov_base = &tcp4_eth_src;
-		iov[TCP_IOV_ETH] = IOV_OF_LVALUE(tcp4_eth_src);
-		iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp4_flags_ip[i]);
-		iov[TCP_IOV_PAYLOAD].iov_base = &tcp4_flags[i];
-	}
-}
-
-/**
- * tcp_sock6_iov_init() - Initialise scatter-gather L2 buffers for IPv6 sockets
- * @c:		Execution context
- */
-void tcp_sock6_iov_init(const struct ctx *c)
-{
-	struct ipv6hdr ip6 = L2_BUF_IP6_INIT(IPPROTO_TCP);
-	struct iovec *iov;
-	int i;
-
-	tcp6_eth_src.h_proto = htons_constant(ETH_P_IPV6);
+		iov = tcp_l2_flags_iov[i];
 
-	for (i = 0; i < ARRAY_SIZE(tcp6_payload); i++) {
-		tcp6_payload_ip[i] = ip6;
-		tcp6_payload[i].th.doff = sizeof(struct tcphdr) / 4;
-		tcp6_payload[i].th.ack = 1;
-	}
-
-	for (i = 0; i < ARRAY_SIZE(tcp6_flags); i++) {
-		tcp6_flags_ip[i] = ip6;
-		tcp6_flags[i].th.doff = sizeof(struct tcphdr) / 4;
-		tcp6_flags[i].th .ack = 1;
-	}
-
-	for (i = 0; i < TCP_FRAMES_MEM; i++) {
-		iov = tcp6_l2_iov[i];
-
-		iov[TCP_IOV_TAP] = tap_hdr_iov(c, &tcp6_payload_tap_hdr[i]);
+		iov[TCP_IOV_TAP] = tap_hdr_iov(c, &tcp_flags_tap_hdr[i]);
 		iov[TCP_IOV_ETH].iov_len = sizeof(struct ethhdr);
-		iov[TCP_IOV_PAYLOAD].iov_base = &tcp6_payload[i];
-	}
-
-	for (i = 0; i < TCP_FRAMES_MEM; i++) {
-		iov = tcp6_l2_flags_iov[i];
-
-		iov[TCP_IOV_TAP] = tap_hdr_iov(c, &tcp6_flags_tap_hdr[i]);
-		iov[TCP_IOV_ETH] = IOV_OF_LVALUE(tcp6_eth_src);
-		iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp6_flags_ip[i]);
-		iov[TCP_IOV_PAYLOAD].iov_base = &tcp6_flags[i];
+		iov[TCP_IOV_PAYLOAD].iov_base = &tcp_flags[i];
 	}
 }
 
@@ -222,13 +165,9 @@ void tcp_sock6_iov_init(const struct ctx *c)
  */
 void tcp_flags_flush(const struct ctx *c)
 {
-	tap_send_frames(c, &tcp6_l2_flags_iov[0][0], TCP_NUM_IOVS,
-			tcp6_flags_used);
-	tcp6_flags_used = 0;
-
-	tap_send_frames(c, &tcp4_l2_flags_iov[0][0], TCP_NUM_IOVS,
-			tcp4_flags_used);
-	tcp4_flags_used = 0;
+	tap_send_frames(c, &tcp_l2_flags_iov[0][0], TCP_NUM_IOVS,
+			tcp_flags_used);
+	tcp_flags_used = 0;
 }
 
 /**
@@ -267,21 +206,13 @@ void tcp_payload_flush(struct ctx *c)
 {
 	size_t m;
 
-	m = tap_send_frames(c, &tcp6_l2_iov[0][0], TCP_NUM_IOVS,
-			    tcp6_payload_used);
-	if (m != tcp6_payload_used) {
-		tcp_revert_seq(c, &tcp6_frame_conns[m], &tcp6_l2_iov[m],
-			       tcp6_payload_used - m);
-	}
-	tcp6_payload_used = 0;
-
-	m = tap_send_frames(c, &tcp4_l2_iov[0][0], TCP_NUM_IOVS,
-			    tcp4_payload_used);
-	if (m != tcp4_payload_used) {
-		tcp_revert_seq(c, &tcp4_frame_conns[m], &tcp4_l2_iov[m],
-			       tcp4_payload_used - m);
+	m = tap_send_frames(c, &tcp_l2_iov[0][0], TCP_NUM_IOVS,
+			    tcp_payload_used);
+	if (m != tcp_payload_used) {
+		tcp_revert_seq(c, &tcp_frame_conns[m], &tcp_l2_iov[m],
+			       tcp_payload_used - m);
 	}
-	tcp4_payload_used = 0;
+	tcp_payload_used = 0;
 }
 
 /**
@@ -301,28 +232,23 @@ int tcp_buf_send_flag(struct ctx *c, struct tcp_tap_conn *conn, int flags)
 	uint32_t seq;
 	int ret;
 
+	iov = tcp_l2_flags_iov[tcp_flags_used];
 	if (CONN_V4(conn)) {
-		iov = tcp4_l2_flags_iov[tcp4_flags_used];
-		iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp4_flags_ip[tcp4_flags_used++]);
+		iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp4_flags_ip[tcp_flags_used]);
 		iov[TCP_IOV_ETH].iov_base = &tcp4_eth_src;
 	} else {
-		iov = tcp6_l2_flags_iov[tcp6_flags_used];
-		iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp6_flags_ip[tcp6_flags_used++]);
+		iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp6_flags_ip[tcp_flags_used]);
 		iov[TCP_IOV_ETH].iov_base = &tcp6_eth_src;
 	}
-	payload = iov[TCP_IOV_PAYLOAD].iov_base;
 
+	payload = iov[TCP_IOV_PAYLOAD].iov_base;
 	seq = conn->seq_to_tap;
 	ret = tcp_prepare_flags(c, conn, flags, &payload->th,
 				payload->opts, &optlen);
-	if (ret <= 0) {
-		if (CONN_V4(conn))
-			tcp4_flags_used--;
-		else
-			tcp6_flags_used--;
+	if (ret <= 0)
 		return ret;
-	}
 
+	tcp_flags_used++;
 	l4len = tcp_l2_buf_fill_headers(conn, iov, optlen, NULL, seq);
 	iov[TCP_IOV_PAYLOAD].iov_len = l4len;
 
@@ -330,28 +256,22 @@ int tcp_buf_send_flag(struct ctx *c, struct tcp_tap_conn *conn, int flags)
 		struct iovec *dup_iov;
 		int i;
 
+		dup_iov = tcp_l2_flags_iov[tcp_flags_used];
 		if (CONN_V4(conn)) {
-			dup_iov = tcp4_l2_flags_iov[tcp4_flags_used];
-			dup_iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp4_flags_ip[tcp4_flags_used++]);
+			dup_iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp4_flags_ip[tcp_flags_used]);
 			dup_iov[TCP_IOV_ETH].iov_base = &tcp4_eth_src;
 		} else {
-			dup_iov = tcp4_l2_flags_iov[tcp6_flags_used];
-			dup_iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp6_flags_ip[tcp6_flags_used++]);
+			dup_iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp6_flags_ip[tcp_flags_used]);
 			dup_iov[TCP_IOV_ETH].iov_base = &tcp6_eth_src;
 		}
 		for (i = 0; i < TCP_NUM_IOVS; i++)
-			memcpy(dup_iov[i].iov_base, iov[i].iov_base,
-			       iov[i].iov_len);
+			memcpy(dup_iov[i].iov_base, iov[i].iov_base, iov[i].iov_len);
 		dup_iov[TCP_IOV_PAYLOAD].iov_len = iov[TCP_IOV_PAYLOAD].iov_len;
+		tcp_flags_used++;
 	}
 
-	if (CONN_V4(conn)) {
-		if (tcp4_flags_used > TCP_FRAMES_MEM - 2)
-			tcp_flags_flush(c);
-	} else {
-		if (tcp6_flags_used > TCP_FRAMES_MEM - 2)
-			tcp_flags_flush(c);
-	}
+	if (tcp_flags_used > TCP_FRAMES_MEM - 2)
+		tcp_flags_flush(c);
 
 	return 0;
 }
@@ -367,40 +287,29 @@ int tcp_buf_send_flag(struct ctx *c, struct tcp_tap_conn *conn, int flags)
 static void tcp_data_to_tap(struct ctx *c, struct tcp_tap_conn *conn,
 			    ssize_t dlen, int no_csum, uint32_t seq)
 {
+	const uint16_t *check = NULL;
 	struct iovec *iov;
 	size_t l4len;
 
 	conn->seq_to_tap = seq + dlen;
-
+	tcp_frame_conns[tcp_payload_used] = conn;
+	iov = tcp_l2_iov[tcp_payload_used];
 	if (CONN_V4(conn)) {
-		struct iovec *iov_prev = tcp4_l2_iov[tcp4_payload_used - 1];
-		const uint16_t *check = NULL;
-
 		if (no_csum) {
+			struct iovec *iov_prev = tcp_l2_iov[tcp_payload_used - 1];
 			struct iphdr *iph = iov_prev[TCP_IOV_IP].iov_base;
 			check = &iph->check;
 		}
-
-		tcp4_frame_conns[tcp4_payload_used] = conn;
-
-		iov = tcp4_l2_iov[tcp4_payload_used];
-		iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp4_payload_ip[tcp4_payload_used++]);
+		iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp4_payload_ip[tcp_payload_used]);
 		iov[TCP_IOV_ETH].iov_base = &tcp4_eth_src;
-		l4len = tcp_l2_buf_fill_headers(conn, iov, dlen, check, seq);
-		iov[TCP_IOV_PAYLOAD].iov_len = l4len;
-		if (tcp4_payload_used > TCP_FRAMES_MEM - 1)
-			tcp_payload_flush(c);
 	} else if (CONN_V6(conn)) {
-		tcp6_frame_conns[tcp6_payload_used] = conn;
-
-		iov = tcp6_l2_iov[tcp6_payload_used];
-		iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp6_payload_ip[tcp6_payload_used++]);
+		iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp6_payload_ip[tcp_payload_used]);
 		iov[TCP_IOV_ETH].iov_base = &tcp6_eth_src;
-		l4len = tcp_l2_buf_fill_headers(conn, iov, dlen, NULL, seq);
-		iov[TCP_IOV_PAYLOAD].iov_len = l4len;
-		if (tcp6_payload_used > TCP_FRAMES_MEM - 1)
-			tcp_payload_flush(c);
 	}
+	l4len = tcp_l2_buf_fill_headers(conn, iov, dlen, check, seq);
+	iov[TCP_IOV_PAYLOAD].iov_len = l4len;
+	if (++tcp_payload_used > TCP_FRAMES_MEM - 1)
+		tcp_payload_flush(c);
 }
 
 /**
@@ -464,19 +373,15 @@ int tcp_buf_data_from_sock(struct ctx *c, struct tcp_tap_conn *conn)
 		mh_sock.msg_iovlen = fill_bufs;
 	}
 
-	if (( v4 && tcp4_payload_used + fill_bufs > TCP_FRAMES_MEM) ||
-	    (!v4 && tcp6_payload_used + fill_bufs > TCP_FRAMES_MEM)) {
+	if ((v4 && tcp_payload_used + fill_bufs > TCP_FRAMES_MEM)) {
 		tcp_payload_flush(c);
 
 		/* Silence Coverity CWE-125 false positive */
-		tcp4_payload_used = tcp6_payload_used = 0;
+		tcp_payload_used = 0;
 	}
 
 	for (i = 0, iov = iov_sock + 1; i < fill_bufs; i++, iov++) {
-		if (v4)
-			iov->iov_base = &tcp4_payload[tcp4_payload_used + i].data;
-		else
-			iov->iov_base = &tcp6_payload[tcp6_payload_used + i].data;
+		iov->iov_base = &tcp_payload[tcp_payload_used + i].data;
 		iov->iov_len = mss;
 	}
 	if (iov_rem)
@@ -524,7 +429,7 @@ int tcp_buf_data_from_sock(struct ctx *c, struct tcp_tap_conn *conn)
 	dlen = mss;
 	seq = conn->seq_to_tap;
 	for (i = 0; i < send_bufs; i++) {
-		int no_csum = i && i != send_bufs - 1 && tcp4_payload_used;
+		int no_csum = i && i != send_bufs - 1 && tcp_payload_used;
 
 		if (i == send_bufs - 1)
 			dlen = last_len;
diff --git a/tcp_buf.h b/tcp_buf.h
index 3db4c56..03a2273 100644
--- a/tcp_buf.h
+++ b/tcp_buf.h
@@ -6,8 +6,7 @@
 #ifndef TCP_BUF_H
 #define TCP_BUF_H
 
-void tcp_sock4_iov_init(const struct ctx *c);
-void tcp_sock6_iov_init(const struct ctx *c);
+void tcp_sock_iov_init(const struct ctx *c);
 void tcp_flags_flush(const struct ctx *c);
 void tcp_payload_flush(struct ctx *c);
 int tcp_buf_data_from_sock(struct ctx *c, struct tcp_tap_conn *conn);
-- 
@@ -6,8 +6,7 @@
 #ifndef TCP_BUF_H
 #define TCP_BUF_H
 
-void tcp_sock4_iov_init(const struct ctx *c);
-void tcp_sock6_iov_init(const struct ctx *c);
+void tcp_sock_iov_init(const struct ctx *c);
 void tcp_flags_flush(const struct ctx *c);
 void tcp_payload_flush(struct ctx *c);
 int tcp_buf_data_from_sock(struct ctx *c, struct tcp_tap_conn *conn);
-- 
2.45.2

Re: [PATCH v2 0/2] tcp: unify IPv4 and IPv6 tap queues

[Stefano Brivio]

On Fri, 13 Sep 2024 20:37:16 -0400
Jon Maloy <jmaloy@redhat.com> wrote:

> This should save us some memory and code.
> 
> ---
> v2: - Setting pointers to pre-set IP and MAC headers on the fly
>       instead of copying them.

Thanks, at a glance this looks much safer than v1.

I didn't review in detail and didn't test, yet.

-- 
Stefano

Re: [PATCH v2 1/2] tcp: set ip and eth headers in l2 tap queues on the fly

[David Gibson]

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

On Fri, Sep 13, 2024 at 08:37:17PM -0400, Jon Maloy wrote:
> l2 tap queue entries are currently initialized at system start, and
> reused with preset headers through its whole life time. The only
> fields we need to update per message are things like payload size
> and checksums.
> 
> If we want to reuse these entries between ipv4 and ipv6 messages we
> will need to set the pointer to the right header on the fly per
> message, since the header type may differ between entries in the same
> queue.
> 
> The same needs to be done for the ethernet header.
> 
> We do these changes here.
> 
> Signed-off-by: Jon Maloy <jmaloy@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

.. in the context of the next path, it wouldn't make much sense to
apply this without the following one.

> 
> ---
> v2: Setting pointers to pre-initialized IP and MAC headers instead of
>     copying them in on the fly.
> ---
>  tcp_buf.c | 42 ++++++++++++++++++++++++++----------------
>  1 file changed, 26 insertions(+), 16 deletions(-)
> 
> diff --git a/tcp_buf.c b/tcp_buf.c
> index c31e9f3..af80cc5 100644
> --- a/tcp_buf.c
> +++ b/tcp_buf.c
> @@ -159,8 +159,7 @@ void tcp_sock4_iov_init(const struct ctx *c)
>  		iov = tcp4_l2_iov[i];
>  
>  		iov[TCP_IOV_TAP] = tap_hdr_iov(c, &tcp4_payload_tap_hdr[i]);
> -		iov[TCP_IOV_ETH] = IOV_OF_LVALUE(tcp4_eth_src);
> -		iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp4_payload_ip[i]);
> +		iov[TCP_IOV_ETH].iov_len = sizeof(struct ethhdr);
>  		iov[TCP_IOV_PAYLOAD].iov_base = &tcp4_payload[i];
>  	}
>  
> @@ -203,8 +202,7 @@ void tcp_sock6_iov_init(const struct ctx *c)
>  		iov = tcp6_l2_iov[i];
>  
>  		iov[TCP_IOV_TAP] = tap_hdr_iov(c, &tcp6_payload_tap_hdr[i]);
> -		iov[TCP_IOV_ETH] = IOV_OF_LVALUE(tcp6_eth_src);
> -		iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp6_payload_ip[i]);
> +		iov[TCP_IOV_ETH].iov_len = sizeof(struct ethhdr);
>  		iov[TCP_IOV_PAYLOAD].iov_base = &tcp6_payload[i];
>  	}
>  
> @@ -303,11 +301,15 @@ int tcp_buf_send_flag(struct ctx *c, struct tcp_tap_conn *conn, int flags)
>  	uint32_t seq;
>  	int ret;
>  
> -	if (CONN_V4(conn))
> -		iov = tcp4_l2_flags_iov[tcp4_flags_used++];
> -	else
> -		iov = tcp6_l2_flags_iov[tcp6_flags_used++];
> -
> +	if (CONN_V4(conn)) {
> +		iov = tcp4_l2_flags_iov[tcp4_flags_used];
> +		iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp4_flags_ip[tcp4_flags_used++]);
> +		iov[TCP_IOV_ETH].iov_base = &tcp4_eth_src;
> +	} else {
> +		iov = tcp6_l2_flags_iov[tcp6_flags_used];
> +		iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp6_flags_ip[tcp6_flags_used++]);
> +		iov[TCP_IOV_ETH].iov_base = &tcp6_eth_src;
> +	}
>  	payload = iov[TCP_IOV_PAYLOAD].iov_base;
>  
>  	seq = conn->seq_to_tap;
> @@ -328,11 +330,15 @@ int tcp_buf_send_flag(struct ctx *c, struct tcp_tap_conn *conn, int flags)
>  		struct iovec *dup_iov;
>  		int i;
>  
> -		if (CONN_V4(conn))
> -			dup_iov = tcp4_l2_flags_iov[tcp4_flags_used++];
> -		else
> -			dup_iov = tcp6_l2_flags_iov[tcp6_flags_used++];
> -
> +		if (CONN_V4(conn)) {
> +			dup_iov = tcp4_l2_flags_iov[tcp4_flags_used];
> +			dup_iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp4_flags_ip[tcp4_flags_used++]);
> +			dup_iov[TCP_IOV_ETH].iov_base = &tcp4_eth_src;
> +		} else {
> +			dup_iov = tcp4_l2_flags_iov[tcp6_flags_used];
> +			dup_iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp6_flags_ip[tcp6_flags_used++]);
> +			dup_iov[TCP_IOV_ETH].iov_base = &tcp6_eth_src;
> +		}
>  		for (i = 0; i < TCP_NUM_IOVS; i++)
>  			memcpy(dup_iov[i].iov_base, iov[i].iov_base,
>  			       iov[i].iov_len);
> @@ -377,7 +383,9 @@ static void tcp_data_to_tap(struct ctx *c, struct tcp_tap_conn *conn,
>  
>  		tcp4_frame_conns[tcp4_payload_used] = conn;
>  
> -		iov = tcp4_l2_iov[tcp4_payload_used++];
> +		iov = tcp4_l2_iov[tcp4_payload_used];
> +		iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp4_payload_ip[tcp4_payload_used++]);
> +		iov[TCP_IOV_ETH].iov_base = &tcp4_eth_src;
>  		l4len = tcp_l2_buf_fill_headers(conn, iov, dlen, check, seq);
>  		iov[TCP_IOV_PAYLOAD].iov_len = l4len;
>  		if (tcp4_payload_used > TCP_FRAMES_MEM - 1)
> @@ -385,7 +393,9 @@ static void tcp_data_to_tap(struct ctx *c, struct tcp_tap_conn *conn,
>  	} else if (CONN_V6(conn)) {
>  		tcp6_frame_conns[tcp6_payload_used] = conn;
>  
> -		iov = tcp6_l2_iov[tcp6_payload_used++];
> +		iov = tcp6_l2_iov[tcp6_payload_used];
> +		iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp6_payload_ip[tcp6_payload_used++]);
> +		iov[TCP_IOV_ETH].iov_base = &tcp6_eth_src;
>  		l4len = tcp_l2_buf_fill_headers(conn, iov, dlen, NULL, seq);
>  		iov[TCP_IOV_PAYLOAD].iov_len = l4len;
>  		if (tcp6_payload_used > TCP_FRAMES_MEM - 1)

-- 
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 v2 2/2] tcp: unify l2 TCPv4 and TCPv6 queues and structures

[David Gibson]

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

On Fri, Sep 13, 2024 at 08:37:18PM -0400, Jon Maloy wrote:
> Following the preparations in the previous commit, we can now remove
> the payload and flag queues dedicated for TCPv6 and TCPv4 and move all
> traffic into common queues handling both protocol types.
> 
> Apart from reducing code and memory footprint, this change reduces
> a potential risk for TCPv4 traffic starving out TCPv6 traffic.
> Since we always flush out the TCPv4 frame queue before the TCPv6 queue,
> the latter will never be handled if the former fails to send all its
> frames.
> 
> Tests with iperf3 shows no measurable change in performance after this
> change.
> 
> Signed-off-by: Jon Maloy <jmaloy@redhat.com>

Mostly LGTM.  I've noted a handful of things, some of which are
important, but they should be easy to fix.
 
> ---
> v2: Merged previous patch #3, where the tcp4_ prefix was changed to
>     tcp_ into this patch.
> ---
>  tcp.c     |   6 +-
>  tcp_buf.c | 241 +++++++++++++++++-------------------------------------
>  tcp_buf.h |   3 +-
>  3 files changed, 75 insertions(+), 175 deletions(-)
> 
> diff --git a/tcp.c b/tcp.c
> index f9fe1b9..294eeee 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -2504,11 +2504,7 @@ int tcp_init(struct ctx *c)
>  {
>  	ASSERT(!c->no_tcp);
>  
> -	if (c->ifi4)
> -		tcp_sock4_iov_init(c);
> -
> -	if (c->ifi6)
> -		tcp_sock6_iov_init(c);
> +	tcp_sock_iov_init(c);
>  
>  	memset(init_sock_pool4,		0xff,	sizeof(init_sock_pool4));
>  	memset(init_sock_pool6,		0xff,	sizeof(init_sock_pool6));
> diff --git a/tcp_buf.c b/tcp_buf.c
> index af80cc5..ab55476 100644
> --- a/tcp_buf.c
> +++ b/tcp_buf.c
> @@ -67,59 +67,42 @@ struct tcp_flags_t {
>  } __attribute__ ((packed, aligned(__alignof__(unsigned int))));
>  #endif
>  
> -/* Ethernet header for IPv4 frames */
> +/* Ethernet header for IPv4 and IPv6 frames */
>  static struct ethhdr		tcp4_eth_src;
> +static struct ethhdr		tcp6_eth_src;
> +
> +static struct tap_hdr		tcp_payload_tap_hdr[TCP_FRAMES_MEM];
>  
> -static struct tap_hdr		tcp4_payload_tap_hdr[TCP_FRAMES_MEM];
> -/* IPv4 headers */
> -static struct iphdr		tcp4_payload_ip[TCP_FRAMES_MEM];
> -/* TCP segments with payload for IPv4 frames */
> -static struct tcp_payload_t	tcp4_payload[TCP_FRAMES_MEM];
> +/* IP headers for IPv4 and IPv6 */
> +struct iphdr		tcp4_payload_ip[TCP_FRAMES_MEM];
> +struct ipv6hdr		tcp6_payload_ip[TCP_FRAMES_MEM];
>  
> -static_assert(MSS4 <= sizeof(tcp4_payload[0].data), "MSS4 is greater than 65516");
> +/* TCP segments with payload for IPv4 and IPv6 frames */
> +static struct tcp_payload_t	tcp_payload[TCP_FRAMES_MEM];
> +
> +static_assert(sizeof(tcp_payload[0].data) > MSS4, "MSS4 is greater than 65516");

I'm not sure why you're reversing the order of the inequality here,
seems like an unrelated change.  If you do reverse it it should be >=
not >.

>  /* References tracking the owner connection of frames in the tap outqueue */
> -static struct tcp_tap_conn *tcp4_frame_conns[TCP_FRAMES_MEM];
> -static unsigned int tcp4_payload_used;
> +static struct tcp_tap_conn *tcp_frame_conns[TCP_FRAMES_MEM];
> +static unsigned int tcp_payload_used;
>  
> -static struct tap_hdr		tcp4_flags_tap_hdr[TCP_FRAMES_MEM];
> +static struct tap_hdr		tcp_flags_tap_hdr[TCP_FRAMES_MEM];
>  /* IPv4 headers for TCP segment without payload */
>  static struct iphdr		tcp4_flags_ip[TCP_FRAMES_MEM];
>  /* TCP segments without payload for IPv4 frames */
> -static struct tcp_flags_t	tcp4_flags[TCP_FRAMES_MEM];
> -
> -static unsigned int tcp4_flags_used;
> +static struct tcp_flags_t	tcp_flags[TCP_FRAMES_MEM];
>  
> -/* Ethernet header for IPv6 frames */
> -static struct ethhdr		tcp6_eth_src;
> -
> -static struct tap_hdr		tcp6_payload_tap_hdr[TCP_FRAMES_MEM];
> -/* IPv6 headers */
> -static struct ipv6hdr		tcp6_payload_ip[TCP_FRAMES_MEM];
> -/* TCP headers and data for IPv6 frames */
> -static struct tcp_payload_t	tcp6_payload[TCP_FRAMES_MEM];
> +static unsigned int tcp_flags_used;
>  
> -static_assert(MSS6 <= sizeof(tcp6_payload[0].data), "MSS6 is greater than 65516");

AFAICT you've dropped this assert entirely, but it's still relevant.
With the payload buffers unified, we still want to make sure each
buffer is large enough for both the largest IPv4 and the largest IPv6
frame.  This is, in a sense, redundant with the IPv4 check, since MSS4
will be bigger than MSS6, but I'd prefer to explcitly verify it rather
than rely on that here.

> -/* References tracking the owner connection of frames in the tap outqueue */
> -static struct tcp_tap_conn *tcp6_frame_conns[TCP_FRAMES_MEM];
> -static unsigned int tcp6_payload_used;
> -
> -static struct tap_hdr		tcp6_flags_tap_hdr[TCP_FRAMES_MEM];
>  /* IPv6 headers for TCP segment without payload */
>  static struct ipv6hdr		tcp6_flags_ip[TCP_FRAMES_MEM];
> -/* TCP segment without payload for IPv6 frames */
> -static struct tcp_flags_t	tcp6_flags[TCP_FRAMES_MEM];
> -
> -static unsigned int tcp6_flags_used;
>  
>  /* recvmsg()/sendmsg() data for tap */
>  static struct iovec	iov_sock		[TCP_FRAMES_MEM + 1];
>  
> -static struct iovec	tcp4_l2_iov		[TCP_FRAMES_MEM][TCP_NUM_IOVS];
> -static struct iovec	tcp6_l2_iov		[TCP_FRAMES_MEM][TCP_NUM_IOVS];
> -static struct iovec	tcp4_l2_flags_iov	[TCP_FRAMES_MEM][TCP_NUM_IOVS];
> -static struct iovec	tcp6_l2_flags_iov	[TCP_FRAMES_MEM][TCP_NUM_IOVS];
> +static struct iovec	tcp_l2_iov[TCP_FRAMES_MEM][TCP_NUM_IOVS];
> +static struct iovec	tcp_l2_flags_iov[TCP_FRAMES_MEM][TCP_NUM_IOVS];
> +
>  /**
>   * tcp_update_l2_buf() - Update Ethernet header buffers with addresses
>   * @eth_d:	Ethernet destination address, NULL if unchanged
> @@ -132,87 +115,47 @@ void tcp_update_l2_buf(const unsigned char *eth_d, const unsigned char *eth_s)
>  }
>  
>  /**
> - * tcp_sock4_iov_init() - Initialise scatter-gather L2 buffers for IPv4 sockets
> + * tcp_sock_iov_init() - Initialise scatter-gather L2 buffers for IPv4 sockets
>   * @c:		Execution context
>   */
> -void tcp_sock4_iov_init(const struct ctx *c)
> +void tcp_sock_iov_init(const struct ctx *c)
>  {
> +	struct ipv6hdr ip6 = L2_BUF_IP6_INIT(IPPROTO_TCP);
>  	struct iphdr iph = L2_BUF_IP4_INIT(IPPROTO_TCP);
>  	struct iovec *iov;
>  	int i;
>  
> +	tcp6_eth_src.h_proto = htons_constant(ETH_P_IPV6);
>  	tcp4_eth_src.h_proto = htons_constant(ETH_P_IP);

Aside: I guess we could make this a static initialiser, but that's not
the job of this patch.

>  
> -	for (i = 0; i < ARRAY_SIZE(tcp4_payload); i++) {
> +	for (i = 0; i < ARRAY_SIZE(tcp_payload); i++) {
> +		tcp6_payload_ip[i] = ip6;
>  		tcp4_payload_ip[i] = iph;
> -		tcp4_payload[i].th.doff = sizeof(struct tcphdr) / 4;
> -		tcp4_payload[i].th.ack = 1;
> +		tcp_payload[i].th.doff = sizeof(struct tcphdr) / 4;
> +		tcp_payload[i].th.ack = 1;
>  	}
>  
> -	for (i = 0; i < ARRAY_SIZE(tcp4_flags); i++) {
> +	for (i = 0; i < ARRAY_SIZE(tcp_flags); i++) {
> +		tcp6_flags_ip[i] = ip6;
>  		tcp4_flags_ip[i] = iph;
> -		tcp4_flags[i].th.doff = sizeof(struct tcphdr) / 4;
> -		tcp4_flags[i].th.ack = 1;
> +		tcp_flags[i].th.doff = sizeof(struct tcphdr) / 4;
> +		tcp_flags[i].th.ack = 1;
>  	}
>  
>  	for (i = 0; i < TCP_FRAMES_MEM; i++) {
> -		iov = tcp4_l2_iov[i];
> +		iov = tcp_l2_iov[i];

Aside: and it would probably make sense to have iov local to these blocks.

>  
> -		iov[TCP_IOV_TAP] = tap_hdr_iov(c, &tcp4_payload_tap_hdr[i]);
> +		iov[TCP_IOV_TAP] = tap_hdr_iov(c, &tcp_payload_tap_hdr[i]);
>  		iov[TCP_IOV_ETH].iov_len = sizeof(struct ethhdr);
> -		iov[TCP_IOV_PAYLOAD].iov_base = &tcp4_payload[i];
> +		iov[TCP_IOV_PAYLOAD].iov_base = &tcp_payload[i];
>  	}
>  
>  	for (i = 0; i < TCP_FRAMES_MEM; i++) {
> -		iov = tcp4_l2_flags_iov[i];
> -
> -		iov[TCP_IOV_TAP] = tap_hdr_iov(c, &tcp4_flags_tap_hdr[i]);
> -		iov[TCP_IOV_ETH].iov_base = &tcp4_eth_src;
> -		iov[TCP_IOV_ETH] = IOV_OF_LVALUE(tcp4_eth_src);
> -		iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp4_flags_ip[i]);
> -		iov[TCP_IOV_PAYLOAD].iov_base = &tcp4_flags[i];
> -	}
> -}
> -
> -/**
> - * tcp_sock6_iov_init() - Initialise scatter-gather L2 buffers for IPv6 sockets
> - * @c:		Execution context
> - */
> -void tcp_sock6_iov_init(const struct ctx *c)
> -{
> -	struct ipv6hdr ip6 = L2_BUF_IP6_INIT(IPPROTO_TCP);
> -	struct iovec *iov;
> -	int i;
> -
> -	tcp6_eth_src.h_proto = htons_constant(ETH_P_IPV6);
> +		iov = tcp_l2_flags_iov[i];
>  
> -	for (i = 0; i < ARRAY_SIZE(tcp6_payload); i++) {
> -		tcp6_payload_ip[i] = ip6;
> -		tcp6_payload[i].th.doff = sizeof(struct tcphdr) / 4;
> -		tcp6_payload[i].th.ack = 1;
> -	}
> -
> -	for (i = 0; i < ARRAY_SIZE(tcp6_flags); i++) {
> -		tcp6_flags_ip[i] = ip6;
> -		tcp6_flags[i].th.doff = sizeof(struct tcphdr) / 4;
> -		tcp6_flags[i].th .ack = 1;
> -	}
> -
> -	for (i = 0; i < TCP_FRAMES_MEM; i++) {
> -		iov = tcp6_l2_iov[i];
> -
> -		iov[TCP_IOV_TAP] = tap_hdr_iov(c, &tcp6_payload_tap_hdr[i]);
> +		iov[TCP_IOV_TAP] = tap_hdr_iov(c, &tcp_flags_tap_hdr[i]);
>  		iov[TCP_IOV_ETH].iov_len = sizeof(struct ethhdr);
> -		iov[TCP_IOV_PAYLOAD].iov_base = &tcp6_payload[i];
> -	}
> -
> -	for (i = 0; i < TCP_FRAMES_MEM; i++) {
> -		iov = tcp6_l2_flags_iov[i];
> -
> -		iov[TCP_IOV_TAP] = tap_hdr_iov(c, &tcp6_flags_tap_hdr[i]);
> -		iov[TCP_IOV_ETH] = IOV_OF_LVALUE(tcp6_eth_src);
> -		iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp6_flags_ip[i]);
> -		iov[TCP_IOV_PAYLOAD].iov_base = &tcp6_flags[i];
> +		iov[TCP_IOV_PAYLOAD].iov_base = &tcp_flags[i];
>  	}
>  }
>  
> @@ -222,13 +165,9 @@ void tcp_sock6_iov_init(const struct ctx *c)
>   */
>  void tcp_flags_flush(const struct ctx *c)
>  {
> -	tap_send_frames(c, &tcp6_l2_flags_iov[0][0], TCP_NUM_IOVS,
> -			tcp6_flags_used);
> -	tcp6_flags_used = 0;
> -
> -	tap_send_frames(c, &tcp4_l2_flags_iov[0][0], TCP_NUM_IOVS,
> -			tcp4_flags_used);
> -	tcp4_flags_used = 0;
> +	tap_send_frames(c, &tcp_l2_flags_iov[0][0], TCP_NUM_IOVS,
> +			tcp_flags_used);
> +	tcp_flags_used = 0;
>  }
>  
>  /**
> @@ -267,21 +206,13 @@ void tcp_payload_flush(struct ctx *c)
>  {
>  	size_t m;
>  
> -	m = tap_send_frames(c, &tcp6_l2_iov[0][0], TCP_NUM_IOVS,
> -			    tcp6_payload_used);
> -	if (m != tcp6_payload_used) {
> -		tcp_revert_seq(c, &tcp6_frame_conns[m], &tcp6_l2_iov[m],
> -			       tcp6_payload_used - m);
> -	}
> -	tcp6_payload_used = 0;
> -
> -	m = tap_send_frames(c, &tcp4_l2_iov[0][0], TCP_NUM_IOVS,
> -			    tcp4_payload_used);
> -	if (m != tcp4_payload_used) {
> -		tcp_revert_seq(c, &tcp4_frame_conns[m], &tcp4_l2_iov[m],
> -			       tcp4_payload_used - m);
> +	m = tap_send_frames(c, &tcp_l2_iov[0][0], TCP_NUM_IOVS,
> +			    tcp_payload_used);
> +	if (m != tcp_payload_used) {
> +		tcp_revert_seq(c, &tcp_frame_conns[m], &tcp_l2_iov[m],
> +			       tcp_payload_used - m);
>  	}
> -	tcp4_payload_used = 0;
> +	tcp_payload_used = 0;
>  }
>  
>  /**
> @@ -301,28 +232,23 @@ int tcp_buf_send_flag(struct ctx *c, struct tcp_tap_conn *conn, int flags)
>  	uint32_t seq;
>  	int ret;
>  
> +	iov = tcp_l2_flags_iov[tcp_flags_used];
>  	if (CONN_V4(conn)) {
> -		iov = tcp4_l2_flags_iov[tcp4_flags_used];
> -		iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp4_flags_ip[tcp4_flags_used++]);
> +		iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp4_flags_ip[tcp_flags_used]);
>  		iov[TCP_IOV_ETH].iov_base = &tcp4_eth_src;
>  	} else {
> -		iov = tcp6_l2_flags_iov[tcp6_flags_used];
> -		iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp6_flags_ip[tcp6_flags_used++]);
> +		iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp6_flags_ip[tcp_flags_used]);
>  		iov[TCP_IOV_ETH].iov_base = &tcp6_eth_src;
>  	}
> -	payload = iov[TCP_IOV_PAYLOAD].iov_base;
>  
> +	payload = iov[TCP_IOV_PAYLOAD].iov_base;
>  	seq = conn->seq_to_tap;
>  	ret = tcp_prepare_flags(c, conn, flags, &payload->th,
>  				payload->opts, &optlen);
> -	if (ret <= 0) {
> -		if (CONN_V4(conn))
> -			tcp4_flags_used--;
> -		else
> -			tcp6_flags_used--;
> +	if (ret <= 0)
>  		return ret;
> -	}
>  
> +	tcp_flags_used++;
>  	l4len = tcp_l2_buf_fill_headers(conn, iov, optlen, NULL, seq);
>  	iov[TCP_IOV_PAYLOAD].iov_len = l4len;
>  
> @@ -330,28 +256,22 @@ int tcp_buf_send_flag(struct ctx *c, struct tcp_tap_conn *conn, int flags)
>  		struct iovec *dup_iov;
>  		int i;
>  
> +		dup_iov = tcp_l2_flags_iov[tcp_flags_used];
>  		if (CONN_V4(conn)) {
> -			dup_iov = tcp4_l2_flags_iov[tcp4_flags_used];
> -			dup_iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp4_flags_ip[tcp4_flags_used++]);
> +			dup_iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp4_flags_ip[tcp_flags_used]);
>  			dup_iov[TCP_IOV_ETH].iov_base = &tcp4_eth_src;
>  		} else {
> -			dup_iov = tcp4_l2_flags_iov[tcp6_flags_used];
> -			dup_iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp6_flags_ip[tcp6_flags_used++]);
> +			dup_iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp6_flags_ip[tcp_flags_used]);
>  			dup_iov[TCP_IOV_ETH].iov_base = &tcp6_eth_src;
>  		}
>  		for (i = 0; i < TCP_NUM_IOVS; i++)
> -			memcpy(dup_iov[i].iov_base, iov[i].iov_base,
> -			       iov[i].iov_len);
> +			memcpy(dup_iov[i].iov_base, iov[i].iov_base, iov[i].iov_len);

This will conflict with a fix I have in the pipeline (we should
exclude TCP_IOV_ETH here, or we'll do an overlapping memcpy()).

That said.. since we're updating TCP_IOV_IP on the fly each time now,
we could just point it at the previous frame's IP buffer, rather than
memcpy()ing the whole header.  That would mean we only need to copy
the TCP_IOV_TAP and TCP_IOV_PAYLOAD buffers.

>  		dup_iov[TCP_IOV_PAYLOAD].iov_len = iov[TCP_IOV_PAYLOAD].iov_len;
> +		tcp_flags_used++;
>  	}
>  
> -	if (CONN_V4(conn)) {
> -		if (tcp4_flags_used > TCP_FRAMES_MEM - 2)
> -			tcp_flags_flush(c);
> -	} else {
> -		if (tcp6_flags_used > TCP_FRAMES_MEM - 2)
> -			tcp_flags_flush(c);
> -	}
> +	if (tcp_flags_used > TCP_FRAMES_MEM - 2)
> +		tcp_flags_flush(c);
>  
>  	return 0;
>  }
> @@ -367,40 +287,29 @@ int tcp_buf_send_flag(struct ctx *c, struct tcp_tap_conn *conn, int flags)
>  static void tcp_data_to_tap(struct ctx *c, struct tcp_tap_conn *conn,
>  			    ssize_t dlen, int no_csum, uint32_t seq)
>  {
> +	const uint16_t *check = NULL;
>  	struct iovec *iov;
>  	size_t l4len;
>  
>  	conn->seq_to_tap = seq + dlen;
> -
> +	tcp_frame_conns[tcp_payload_used] = conn;
> +	iov = tcp_l2_iov[tcp_payload_used];
>  	if (CONN_V4(conn)) {
> -		struct iovec *iov_prev = tcp4_l2_iov[tcp4_payload_used - 1];
> -		const uint16_t *check = NULL;
> -
>  		if (no_csum) {
> +			struct iovec *iov_prev = tcp_l2_iov[tcp_payload_used - 1];
>  			struct iphdr *iph = iov_prev[TCP_IOV_IP].iov_base;
>  			check = &iph->check;
>  		}
> -
> -		tcp4_frame_conns[tcp4_payload_used] = conn;
> -
> -		iov = tcp4_l2_iov[tcp4_payload_used];
> -		iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp4_payload_ip[tcp4_payload_used++]);
> +		iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp4_payload_ip[tcp_payload_used]);
>  		iov[TCP_IOV_ETH].iov_base = &tcp4_eth_src;
> -		l4len = tcp_l2_buf_fill_headers(conn, iov, dlen, check, seq);
> -		iov[TCP_IOV_PAYLOAD].iov_len = l4len;
> -		if (tcp4_payload_used > TCP_FRAMES_MEM - 1)
> -			tcp_payload_flush(c);
>  	} else if (CONN_V6(conn)) {
> -		tcp6_frame_conns[tcp6_payload_used] = conn;
> -
> -		iov = tcp6_l2_iov[tcp6_payload_used];
> -		iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp6_payload_ip[tcp6_payload_used++]);
> +		iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp6_payload_ip[tcp_payload_used]);
>  		iov[TCP_IOV_ETH].iov_base = &tcp6_eth_src;
> -		l4len = tcp_l2_buf_fill_headers(conn, iov, dlen, NULL, seq);
> -		iov[TCP_IOV_PAYLOAD].iov_len = l4len;
> -		if (tcp6_payload_used > TCP_FRAMES_MEM - 1)
> -			tcp_payload_flush(c);
>  	}
> +	l4len = tcp_l2_buf_fill_headers(conn, iov, dlen, check, seq);
> +	iov[TCP_IOV_PAYLOAD].iov_len = l4len;
> +	if (++tcp_payload_used > TCP_FRAMES_MEM - 1)
> +		tcp_payload_flush(c);
>  }
>  
>  /**
> @@ -464,19 +373,15 @@ int tcp_buf_data_from_sock(struct ctx *c, struct tcp_tap_conn *conn)
>  		mh_sock.msg_iovlen = fill_bufs;
>  	}
>  
> -	if (( v4 && tcp4_payload_used + fill_bufs > TCP_FRAMES_MEM) ||
> -	    (!v4 && tcp6_payload_used + fill_bufs > TCP_FRAMES_MEM)) {
> +	if ((v4 && tcp_payload_used + fill_bufs > TCP_FRAMES_MEM)) {

I don't think you want the 'v4' here any more.

>  		tcp_payload_flush(c);
>  
>  		/* Silence Coverity CWE-125 false positive */
> -		tcp4_payload_used = tcp6_payload_used = 0;
> +		tcp_payload_used = 0;
>  	}
>  
>  	for (i = 0, iov = iov_sock + 1; i < fill_bufs; i++, iov++) {
> -		if (v4)
> -			iov->iov_base = &tcp4_payload[tcp4_payload_used + i].data;
> -		else
> -			iov->iov_base = &tcp6_payload[tcp6_payload_used + i].data;
> +		iov->iov_base = &tcp_payload[tcp_payload_used + i].data;
>  		iov->iov_len = mss;
>  	}
>  	if (iov_rem)
> @@ -524,7 +429,7 @@ int tcp_buf_data_from_sock(struct ctx *c, struct tcp_tap_conn *conn)
>  	dlen = mss;
>  	seq = conn->seq_to_tap;
>  	for (i = 0; i < send_bufs; i++) {
> -		int no_csum = i && i != send_bufs - 1 && tcp4_payload_used;
> +		int no_csum = i && i != send_bufs - 1 && tcp_payload_used;
>  
>  		if (i == send_bufs - 1)
>  			dlen = last_len;
> diff --git a/tcp_buf.h b/tcp_buf.h
> index 3db4c56..03a2273 100644
> --- a/tcp_buf.h
> +++ b/tcp_buf.h
> @@ -6,8 +6,7 @@
>  #ifndef TCP_BUF_H
>  #define TCP_BUF_H
>  
> -void tcp_sock4_iov_init(const struct ctx *c);
> -void tcp_sock6_iov_init(const struct ctx *c);
> +void tcp_sock_iov_init(const struct ctx *c);
>  void tcp_flags_flush(const struct ctx *c);
>  void tcp_payload_flush(struct ctx *c);
>  int tcp_buf_data_from_sock(struct ctx *c, struct tcp_tap_conn *conn);

-- 
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 v2 2/2] tcp: unify l2 TCPv4 and TCPv6 queues and structures

[Stefano Brivio]

On top of what David already noted, this needs a rebase after commit
5ff5d55291d2 ("tcp: Avoid overlapping memcpy() in DUP_ACK handling").

Similar to v1, I'm still getting a iperf3 hang in the "TCP throughput
over IPv6: host to guest" case of the perf/passt_tcp test. I had a
quick look: the server doesn't seem to be getting any frames, while
both handshakes (control and data) succeed and the client is actually
sending some frames at the beginning.

I guess it's because of this condition:

  ((v4 && tcp_payload_used + fill_bufs > TCP_FRAMES_MEM))

in tcp_buf_data_from_sock(), also reported by David. If you have IPv6
frames only, you'll never flush the queue, I suppose.

One nit about the commit message:

On Fri, 13 Sep 2024 20:37:18 -0400
Jon Maloy <jmaloy@redhat.com> wrote:

> Following the preparations in the previous commit, we can now remove
> the payload and flag queues dedicated for TCPv6 and TCPv4 and move all
> traffic into common queues handling both protocol types.
> 
> Apart from reducing code and memory footprint, this change reduces

...I insist that in a general case this patch doesn't reduce the memory
footprint. If both IPv4 and IPv6 are enabled, then yes, but that's
because it also cuts in half the number of available buffers.

The series looks otherwise good to me.

-- 
Stefano