Re: [PATCH] tcp: unify payload and flags l2 frames array

22 Sep 2024

On Sat, 21 Sep 2024 15:43:33 -0400
Jon Maloy &lt;jmaloy(a)redhat.com&gt; wrote:

...
  In order to reduce memory and code footprint, we merge
the array 
As it is, with this one, I think we're actually wasting more memory,
because, if I understand this patch correctly, every ACK we send needs
its own 64k unused buffer.

...
  for l2 flag frames into the one for payload frames.

 This change also ensures that no flag message will be sent out
 over the l2 media bypassing already queued payload messages.

 Signed-off-by: Jon Maloy &lt;jmaloy(a)redhat.com&gt;
 ---
  tcp.c     |  3 +--
  tcp_buf.c | 70 ++++++++++++-------------------------------------------
  2 files changed, 16 insertions(+), 57 deletions(-) 
If I apply this on top of "[PATCH v3 0/2] tcp: unify IPv4 and IPv6 tap
queues" (it doesn't apply otherwise), it doesn't build:

tcp.c:1149:5: error: conflicting types for ‘tcp_prepare_flags’; have ‘int(const struct ctx
*, struct tcp_tap_conn *, int,  struct tcphdr *, uint8_t *, size_t *)’ {aka ‘int(const
struct ctx *, struct tcp_tap_conn *, int,  struct tcphdr *, unsigned char *, long unsigned
int *)’}
 1149 | int tcp_prepare_flags(const struct ctx *c, struct tcp_tap_conn *conn,
      |     ^~~~~~~~~~~~~~~~~
In file included from tcp.c:305:
tcp_internal.h:98:5: note: previous declaration of ‘tcp_prepare_flags’ with type
‘int(const struct ctx *, struct tcp_tap_conn *, int,  struct tcphdr *, char *, size_t *)’
{aka ‘int(const struct ctx *, struct tcp_tap_conn *, int,  struct tcphdr *, char *, long
unsigned int *)’}
   98 | int tcp_prepare_flags(const struct ctx *c, struct tcp_tap_conn *conn, int flags,
      |     ^~~~~~~~~~~~~~~~~
tcp_buf.c: In function ‘tcp_buf_send_flag’:
tcp_buf.c:204:40: warning: pointer targets in passing argument 5 of ‘tcp_prepare_flags’
differ in signedness [-Wpointer-sign]
  204 |                                 payload->data, &optlen);
      |                                 ~~~~~~~^~~~~~
      |                                        |
      |                                        uint8_t * {aka unsigned char *}
In file included from tcp_buf.c:33:
tcp_internal.h:99:48: note: expected ‘char *’ but argument is of type ‘uint8_t *’ {aka
‘unsigned char *’}
   99 |                       struct tcphdr *th, char *data, size_t *optlen);
      |                                          ~~~~~~^~~~
make: *** [Makefile:122: passt] Error 1

...
  diff --git a/tcp.c b/tcp.c
 index 12e864f..dd060d0 100644
 --- a/tcp.c
 +++ b/tcp.c
 @@ -868,7 +868,6 @@ bool tcp_flow_defer(const struct tcp_tap_conn *conn)
  /* cppcheck-suppress [constParameterPointer, unmatchedSuppression] */
  void tcp_defer_handler(struct ctx *c)
  {
 -	tcp_flags_flush(c);
  	tcp_payload_flush(c);
  }

 @@ -1148,7 +1147,7 @@ static void tcp_update_seqack_from_tap(const struct ctx *c,
   *	     1 otherwise
   */
  int tcp_prepare_flags(const struct ctx *c, struct tcp_tap_conn *conn,
 -		      int flags, struct tcphdr *th, char *data,
 +		      int flags, struct tcphdr *th, uint8_t *data,
  		      size_t *optlen)
  {
  	struct tcp_info tinfo = { 0 };
 diff --git a/tcp_buf.c b/tcp_buf.c
 index 625c29c..58636d1 100644
 --- a/tcp_buf.c
 +++ b/tcp_buf.c
 @@ -33,6 +33,7 @@
  #include "tcp_internal.h"
  #include "tcp_buf.h"

 +#define PAYLOAD_FLAGS htons(0x5010) /* doff = 5, ack = 1 */
  #define TCP_FRAMES_MEM			128
  #define TCP_FRAMES							   \
  	(c->mode == MODE_PASTA ? 1 : TCP_FRAMES_MEM)
 @@ -52,21 +53,6 @@ struct tcp_payload_t {
  } __attribute__ ((packed, aligned(__alignof__(unsigned int))));
  #endif

 -/**
 - * struct tcp_flags_t - TCP header and data to send zero-length
 - *                      segments (flags)
 - * @th:		TCP header
 - * @opts	TCP options
 - */
 -struct tcp_flags_t {
 -	struct tcphdr th;
 -	char opts[OPT_MSS_LEN + OPT_WS_LEN + 1];
 -#ifdef __AVX2__
 -} __attribute__ ((packed, aligned(32)));
 -#else
 -} __attribute__ ((packed, aligned(__alignof__(unsigned int))));
 -#endif
 -
  /* Ethernet header for IPv4 and IPv6 frames */
  static struct ethhdr		tcp4_eth_src;
  static struct ethhdr		tcp6_eth_src;
 @@ -87,22 +73,10 @@ static_assert(MSS6 <= sizeof(tcp_payload[0].data), "MSS6 is
greater than 65516")
  static struct tcp_tap_conn *tcp_frame_conns[TCP_FRAMES_MEM];
  static unsigned int tcp_payload_used;

 -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	tcp_flags[TCP_FRAMES_MEM];
 -
 -static unsigned int tcp_flags_used;
 -
 -/* IPv6 headers for TCP segment without payload */
 -static struct ipv6hdr		tcp6_flags_ip[TCP_FRAMES_MEM];
 -
  /* recvmsg()/sendmsg() data for tap */
  static struct iovec	iov_sock		[TCP_FRAMES_MEM + 1];

  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
 @@ -135,13 +109,6 @@ void tcp_sock_iov_init(const struct ctx *c)
  		tcp_payload[i].th.ack = 1;
  	}

 -	for (i = 0; i < ARRAY_SIZE(tcp_flags); i++) {
 -		tcp6_flags_ip[i] = ip6;
 -		tcp4_flags_ip[i] = iph;
 -		tcp_flags[i].th.doff = sizeof(struct tcphdr) / 4;
 -		tcp_flags[i].th.ack = 1;
 -	}
 -
  	for (i = 0; i < TCP_FRAMES_MEM; i++) {
  		struct iovec *iov = tcp_l2_iov[i];

 @@ -149,14 +116,6 @@ void tcp_sock_iov_init(const struct ctx *c)
  		iov[TCP_IOV_ETH].iov_len = sizeof(struct ethhdr);
  		iov[TCP_IOV_PAYLOAD].iov_base = &tcp_payload[i];
  	}
 -
 -	for (i = 0; i < TCP_FRAMES_MEM; i++) {
 -		struct iovec *iov = tcp_l2_flags_iov[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 = &tcp_flags[i];
 -	}
  }

  /**
 @@ -165,9 +124,7 @@ void tcp_sock_iov_init(const struct ctx *c)
   */
  void tcp_flags_flush(const struct ctx *c)
  {
 -	tap_send_frames(c, &tcp_l2_flags_iov[0][0], TCP_NUM_IOVS,
 -			tcp_flags_used);
 -	tcp_flags_used = 0;
 +	tcp_payload_flush(c); 
Should this still be called tcp_payload_flush() instead of, say,
tcp_packets_flush() or tcp_frames_flush()? Does it make sense to
keep tcp_flags_flush() at all?

...
   }

  /**
 @@ -225,37 +182,35 @@ void tcp_payload_flush(const struct ctx *c)
   */
  int tcp_buf_send_flag(const struct ctx *c, struct tcp_tap_conn *conn, int flags)
  {
 -	struct tcp_flags_t *payload;
 +	struct tcp_payload_t *payload; 
Should the struct still be defined as tcp_payload_t (instead of
tcp_packet_t or tcp_frame_t) and this still be called payload?

...
   	struct iovec *iov;
  	size_t optlen;
  	size_t l4len;
  	uint32_t seq;
  	int ret;

 -	iov = tcp_l2_flags_iov[tcp_flags_used];
 +	iov = tcp_l2_iov[tcp_payload_used];
  	if (CONN_V4(conn)) {
 -		iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp4_flags_ip[tcp_flags_used]);
 +		iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp4_payload_ip[tcp_payload_used]); 
Same with this counter.

...
   		iov[TCP_IOV_ETH].iov_base = &tcp4_eth_src;
  	} else {
 -		iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp6_flags_ip[tcp_flags_used]);
 +		iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp6_payload_ip[tcp_payload_used]);
  		iov[TCP_IOV_ETH].iov_base = &tcp6_eth_src;
  	}

  	payload = iov[TCP_IOV_PAYLOAD].iov_base;
  	seq = conn->seq_to_tap;
  	ret = tcp_prepare_flags(c, conn, flags, &payload->th,
 -				payload->opts, &optlen);
 +				payload->data, &optlen);
  	if (ret <= 0)
  		return ret;

 -	tcp_flags_used++;
 +	tcp_payload_used++;
  	l4len = tcp_l2_buf_fill_headers(conn, iov, optlen, NULL, seq, false);
  	iov[TCP_IOV_PAYLOAD].iov_len = l4len; 
...with this, we still have 64k reserved just to write (typically) 20
bytes. I think that, for these frames, we could avoid wasting all this
memory by using a separate set of iov items and setting pointers as
needed.

...
  -
  	if (flags & DUP_ACK) {
 -		struct iovec *dup_iov;
 +		struct iovec *dup_iov = tcp_l2_iov[tcp_payload_used++];

 -		dup_iov = tcp_l2_flags_iov[tcp_flags_used++];
  		memcpy(dup_iov[TCP_IOV_TAP].iov_base, iov[TCP_IOV_TAP].iov_base,
  		       iov[TCP_IOV_TAP].iov_len);
  		dup_iov[TCP_IOV_ETH].iov_base = iov[TCP_IOV_ETH].iov_base;
 @@ -265,7 +220,7 @@ int tcp_buf_send_flag(const struct ctx *c, struct tcp_tap_conn *conn,
int flags)
  		dup_iov[TCP_IOV_PAYLOAD].iov_len = l4len;
  	}

 -	if (tcp_flags_used > TCP_FRAMES_MEM - 2)
 +	if (tcp_payload_used > TCP_FRAMES_MEM - 2)
  		tcp_flags_flush(c);

  	return 0;
 @@ -282,8 +237,10 @@ int tcp_buf_send_flag(const struct ctx *c, struct tcp_tap_conn
*conn, int flags)
  static void tcp_data_to_tap(const struct ctx *c, struct tcp_tap_conn *conn,
  			    ssize_t dlen, int no_csum, uint32_t seq)
  {
 +	struct tcp_payload_t *payload;
  	const uint16_t *check = NULL;
  	struct iovec *iov;
 +	uint16_t *flags;
  	size_t l4len;

  	conn->seq_to_tap = seq + dlen;
 @@ -302,6 +259,9 @@ static void tcp_data_to_tap(const struct ctx *c, struct tcp_tap_conn
*conn,
  		iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp6_payload_ip[tcp_payload_used]);
  		iov[TCP_IOV_ETH].iov_base = &tcp6_eth_src;
  	}
 +	payload = iov[TCP_IOV_PAYLOAD].iov_base;
 +	flags = &payload->th.window - 1; 
I guess using offsetof(struct tcp_hdr, doff) is a bit less mysterious.

...
  +	*(flags) = PAYLOAD_FLAGS; 
Excess parentheses.

...
   	l4len = tcp_l2_buf_fill_headers(conn, iov, dlen,
check, seq, false);
  	iov[TCP_IOV_PAYLOAD].iov_len = l4len;
  	if (++tcp_payload_used > TCP_FRAMES_MEM - 1) 
-- 
Stefano

2024

2023

2022

2021

Re: [PATCH] tcp: unify payload and flags l2 frames array