Re: [PATCH 1/4] tcp: create unified struct for IPv4 and IPv6 header

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

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

On Fri, Sep 06, 2024 at 05:34:24PM -0400, Jon Maloy wrote:
> As a preparation for unifying the l2 queues for TCPv4 and TCPv6 traffic
> we prepare a unified struct that has space for both IP address types.
> 
> With this change, the arrays tcp4_l2_iov and tcp4_l2_iov, as well as
> tcp4_l2_flags_iov and tcp4_l2_flags_iov, are identical in structure
> and size.
> 
> Signed-off-by: Jon Maloy <jmaloy@redhat.com>
> ---
>  tcp_buf.c | 40 ++++++++++++++++++++++++++++------------
>  1 file changed, 28 insertions(+), 12 deletions(-)
> 
> diff --git a/tcp_buf.c b/tcp_buf.c
> index c31e9f3..1af4786 100644
> --- a/tcp_buf.c
> +++ b/tcp_buf.c
> @@ -52,6 +52,18 @@ struct tcp_payload_t {
>  } __attribute__ ((packed, aligned(__alignof__(unsigned int))));
>  #endif
>  
> +/**
> + * struct iphdr_t - Area with space for both IPv4 and IPv6 header
> + * @ip4:	IPv4 header
> + * @ip6:	IPv6 header
> + */
> +struct iphdr_t {
> +	union {
> +		struct iphdr ip4;
> +		struct ipv6hdr ip6;
> +	};
> +};

So, I haven't read the rest of the series yet, so it's possible
something there would clarify this.

Having the two headers in a union does require that essentially the
whole header be re-initialized for every packet.  For UDP, what we do
instead is have both headers in paralllel, partially initialised.  For
each packet we update the appropriate header as needed, and point our
IOV to the correct header (v4 or v6).

I don't know if that's a better approach or not: it's plausible the
dcache savings outweigh the extra re-initialisation, but it's
plausible they don't too.  I wonder if it might be better sticking
with the same approach as UDP for consistency, at least until we have
specific data suggesting one or the other approach is better.

> +
>  /**
>   * struct tcp_flags_t - TCP header and data to send zero-length
>   *                      segments (flags)
> @@ -72,7 +84,7 @@ static struct ethhdr		tcp4_eth_src;
>  
>  static struct tap_hdr		tcp4_payload_tap_hdr[TCP_FRAMES_MEM];
>  /* IPv4 headers */
> -static struct iphdr		tcp4_payload_ip[TCP_FRAMES_MEM];
> +static struct iphdr_t		tcp4_payload_ip[TCP_FRAMES_MEM];
>  /* TCP segments with payload for IPv4 frames */
>  static struct tcp_payload_t	tcp4_payload[TCP_FRAMES_MEM];
>  
> @@ -84,7 +96,7 @@ static unsigned int tcp4_payload_used;
>  
>  static struct tap_hdr		tcp4_flags_tap_hdr[TCP_FRAMES_MEM];
>  /* IPv4 headers for TCP segment without payload */
> -static struct iphdr		tcp4_flags_ip[TCP_FRAMES_MEM];
> +static struct iphdr_t		tcp4_flags_ip[TCP_FRAMES_MEM];
>  /* TCP segments without payload for IPv4 frames */
>  static struct tcp_flags_t	tcp4_flags[TCP_FRAMES_MEM];
>  
> @@ -95,7 +107,7 @@ 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];
> +static struct iphdr_t		tcp6_payload_ip[TCP_FRAMES_MEM];
>  /* TCP headers and data for IPv6 frames */
>  static struct tcp_payload_t	tcp6_payload[TCP_FRAMES_MEM];
>  
> @@ -107,7 +119,7 @@ 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];
> +static struct iphdr_t		tcp6_flags_ip[TCP_FRAMES_MEM];
>  /* TCP segment without payload for IPv6 frames */
>  static struct tcp_flags_t	tcp6_flags[TCP_FRAMES_MEM];
>  
> @@ -144,13 +156,13 @@ void tcp_sock4_iov_init(const struct ctx *c)
>  	tcp4_eth_src.h_proto = htons_constant(ETH_P_IP);
>  
>  	for (i = 0; i < ARRAY_SIZE(tcp4_payload); i++) {
> -		tcp4_payload_ip[i] = iph;
> +		tcp4_payload_ip[i].ip4 = iph;
>  		tcp4_payload[i].th.doff = sizeof(struct tcphdr) / 4;
>  		tcp4_payload[i].th.ack = 1;
>  	}
>  
>  	for (i = 0; i < ARRAY_SIZE(tcp4_flags); i++) {
> -		tcp4_flags_ip[i] = iph;
> +		tcp4_flags_ip[i].ip4 = iph;
>  		tcp4_flags[i].th.doff = sizeof(struct tcphdr) / 4;
>  		tcp4_flags[i].th.ack = 1;
>  	}
> @@ -160,7 +172,8 @@ void tcp_sock4_iov_init(const struct ctx *c)
>  
>  		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_IP].iov_base = &tcp4_payload_ip[i];
> +		iov[TCP_IOV_IP].iov_len = sizeof(tcp4_payload_ip[i].ip4);
>  		iov[TCP_IOV_PAYLOAD].iov_base = &tcp4_payload[i];
>  	}
>  
> @@ -170,7 +183,8 @@ void tcp_sock4_iov_init(const struct ctx *c)
>  		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_IP].iov_base = &tcp4_flags_ip[i];
> +		iov[TCP_IOV_IP].iov_len = sizeof(tcp4_flags_ip[i].ip4);
>  		iov[TCP_IOV_PAYLOAD].iov_base = &tcp4_flags[i];
>  	}
>  }
> @@ -188,13 +202,13 @@ void tcp_sock6_iov_init(const struct ctx *c)
>  	tcp6_eth_src.h_proto = htons_constant(ETH_P_IPV6);
>  
>  	for (i = 0; i < ARRAY_SIZE(tcp6_payload); i++) {
> -		tcp6_payload_ip[i] = ip6;
> +		tcp6_payload_ip[i].ip6 = 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_ip[i].ip6 = ip6;
>  		tcp6_flags[i].th.doff = sizeof(struct tcphdr) / 4;
>  		tcp6_flags[i].th .ack = 1;
>  	}
> @@ -204,7 +218,8 @@ void tcp_sock6_iov_init(const struct ctx *c)
>  
>  		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_IP].iov_base = &tcp6_payload_ip[i];
> +		iov[TCP_IOV_IP].iov_len = sizeof(tcp6_payload_ip[i].ip6);
>  		iov[TCP_IOV_PAYLOAD].iov_base = &tcp6_payload[i];
>  	}
>  
> @@ -213,7 +228,8 @@ void tcp_sock6_iov_init(const struct ctx *c)
>  
>  		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_IP].iov_base = &tcp6_flags_ip[i];
> +		iov[TCP_IOV_IP].iov_len = sizeof(tcp6_flags_ip[i].ip6);
>  		iov[TCP_IOV_PAYLOAD].iov_base = &tcp6_flags[i];
>  	}
>  }

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