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

[David Gibson <david@gibson.dropbear.id.au>]

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

On Sat, Sep 21, 2024 at 03:43:33PM -0400, Jon Maloy wrote:
> In order to reduce memory and code footprint, we merge the array
> 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 <jmaloy@redhat.com>
> ---
>  tcp.c     |  3 +--
>  tcp_buf.c | 70 ++++++++++++-------------------------------------------
>  2 files changed, 16 insertions(+), 57 deletions(-)
> 
> 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);

As Stefano also noted, I think it would make more sense to just
combine these into a single "tcp_flush()".

>  }
>  
> @@ -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,

This seems like an unrelated change to the rest.

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

Similarly changing this just to "tcp_buf_used" or the like would make
sense.

> -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);

I think tcp_flags_flush() can just be removed entirely.

>  }
>  
>  /**
> @@ -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;
>  	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]);
>  		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;
> -
>  	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 think this will run afoul of the C strict aliasing rules (they're
disabled in the kernel, but not for us).

> +	*(flags) = PAYLOAD_FLAGS;
>  	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)

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