Re: [PATCH v3 9/9] tcp: Introduce ipv4_fill_headers()/ipv6_fill_headers()

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

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

On Sat, Feb 17, 2024 at 04:07:25PM +0100, Laurent Vivier wrote:
> Replace the macro SET_TCP_HEADER_COMMON_V4_V6() by a new function
> tcp_fill_header().
> 
> Move IPv4 and IPv6 code from tcp_l2_buf_fill_headers() to
> tcp_fill_ipv4_header() and tcp_fill_ipv6_header()
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
> 
> Notes:
>     v3:
>       - add to sub-series part 1
>     
>     v2:
>       - extract header filling functions from
>         "tcp: extract buffer management from tcp_send_flag()"
>       - rename them tcp_fill_flag_header()/tcp_fill_ipv4_header(),
>         tcp_fill_ipv6_header()
>       - use upside-down Christmas tree arguments order
>       - replace (void *) by (struct tcphdr *)
> 
>  tcp.c | 154 +++++++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 104 insertions(+), 50 deletions(-)
> 
> diff --git a/tcp.c b/tcp.c
> index aa03c20712f6..bc57a4f6e611 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -1324,6 +1324,108 @@ void tcp_defer_handler(struct ctx *c)
>  	tcp_l2_data_buf_flush(c);
>  }
>  
> +/**
> + * tcp_fill_header() - Fill the TCP header fields for a given TCP segment.
> + *
> + * @th:		Pointer to the TCP header structure
> + * @conn:	Pointer to the TCP connection structure
> + * @seq:	Sequence number
> + */
> +static void tcp_fill_header(struct tcphdr *th,
> +			       const struct tcp_tap_conn *conn, uint32_t seq)
> +{
> +	th->source = htons(conn->fport);
> +	th->dest = htons(conn->eport);
> +	th->seq = htonl(seq);
> +	th->ack_seq = htonl(conn->seq_ack_to_tap);
> +	if (conn->events & ESTABLISHED)	{
> +		th->window = htons(conn->wnd_to_tap);
> +	} else {
> +		unsigned wnd = conn->wnd_to_tap << conn->ws_to_tap;
> +
> +		th->window = htons(MIN(wnd, USHRT_MAX));
> +	}
> +}
> +
> +/**
> + * tcp_fill_ipv4_header() - Fill 802.3, IPv4, TCP headers in pre-cooked buffers
> + * @c:		Execution context
> + * @conn:	Connection pointer
> + * @iph:	Pointer to IPv4 header, immediately followed by a TCP header
> + * @plen:	Payload length (including TCP header options)
> + * @check:	Checksum, if already known
> + * @seq:	Sequence number for this segment
> + *
> + * Return: IP frame length including L2 headers, host order
> + */
> +static size_t tcp_fill_ipv4_header(const struct ctx *c,
> +				   const struct tcp_tap_conn *conn,
> +				   struct iphdr *iph, size_t plen,
> +				   const uint16_t *check, uint32_t seq)
> +{
> +	size_t ip_len = plen + sizeof(struct iphdr) + sizeof(struct tcphdr);
> +	const struct in_addr *a4 = inany_v4(&conn->faddr);
> +	struct tcphdr *th = (struct tcphdr *)(iph + 1);
> +
> +	iph->tot_len = htons(ip_len);
> +	iph->saddr = a4->s_addr;
> +	iph->daddr = c->ip4.addr_seen.s_addr;
> +
> +	iph->check = check ? *check :
> +		     csum_ip4_header(iph->tot_len, IPPROTO_TCP,
> +				     iph->saddr, iph->daddr);
> +
> +
> +	tcp_fill_header(th, conn, seq);
> +
> +	tcp_update_check_tcp4(iph);
> +
> +	return ip_len;
> +}
> +
> +/**
> + * tcp_fill_ipv6_header() - Fill 802.3, IPv6, TCP headers in pre-cooked buffers
> + * @c:		Execution context
> + * @conn:	Connection pointer
> + * @ip6h:	Pointer to IPv6 header, immediately followed by a TCP header
> + * @plen:	Payload length (including TCP header options)
> + * @check:	Checksum, if already known
> + * @seq:	Sequence number for this segment
> + *
> + * Return: The total length of the IPv6 packet, host order
> + */
> +static size_t tcp_fill_ipv6_header(const struct ctx *c,
> +				   const struct tcp_tap_conn *conn,
> +				   struct ipv6hdr *ip6h, size_t plen,
> +				   uint32_t seq)
> +{
> +	size_t ip_len = plen + sizeof(struct ipv6hdr) + sizeof(struct tcphdr);
> +	struct tcphdr *th = (struct tcphdr *)(ip6h + 1);
> +
> +	ip6h->payload_len = htons(plen + sizeof(struct tcphdr));
> +	ip6h->saddr = conn->faddr.a6;
> +	if (IN6_IS_ADDR_LINKLOCAL(&ip6h->saddr))
> +		ip6h->daddr = c->ip6.addr_ll_seen;
> +	else
> +		ip6h->daddr = c->ip6.addr_seen;
> +
> +	memset(ip6h->flow_lbl, 0, 3);
> +
> +	tcp_fill_header(th, conn, seq);
> +
> +	tcp_update_check_tcp6(ip6h);
> +
> +	ip6h->hop_limit = 255;
> +	ip6h->version = 6;
> +	ip6h->nexthdr = IPPROTO_TCP;
> +
> +	ip6h->flow_lbl[0] = (conn->sock >> 16) & 0xf;
> +	ip6h->flow_lbl[1] = (conn->sock >> 8) & 0xff;
> +	ip6h->flow_lbl[2] = (conn->sock >> 0) & 0xff;

IIUC, the reason part of the ip6h update is done after the TCP header
update, but part before was a consequence of how we did the
checksumming: we computed the pseudo-header checksum by doing a full
checksum operation over the partially filled header, meaning filling
the fields not in the pseudo-header had to be done afterwards.

Now that you've reworked the checksumming, that's no longer necessary,
so you could group all the ip6g initialisations together.   Oh.. and
also avoid the pre-filling of the flow_lbl with 0s before filling the
real values.

> +	return ip_len;
> +}
> +
>  /**
>   * tcp_l2_buf_fill_headers() - Fill 802.3, IP, TCP headers in pre-cooked buffers
>   * @c:		Execution context
> @@ -1343,67 +1445,19 @@ static size_t tcp_l2_buf_fill_headers(const struct ctx *c,
>  	const struct in_addr *a4 = inany_v4(&conn->faddr);
>  	size_t ip_len, tlen;
>  
> -#define SET_TCP_HEADER_COMMON_V4_V6(b, conn, seq)			\
> -do {									\
> -	b->th.source = htons(conn->fport);				\
> -	b->th.dest = htons(conn->eport);				\
> -	b->th.seq = htonl(seq);						\
> -	b->th.ack_seq = htonl(conn->seq_ack_to_tap);			\
> -	if (conn->events & ESTABLISHED)	{				\
> -		b->th.window = htons(conn->wnd_to_tap);			\
> -	} else {							\
> -		unsigned wnd = conn->wnd_to_tap << conn->ws_to_tap;	\
> -									\
> -		b->th.window = htons(MIN(wnd, USHRT_MAX));		\
> -	}								\
> -} while (0)
> -
>  	if (a4) {
>  		struct tcp4_l2_buf_t *b = (struct tcp4_l2_buf_t *)p;
>  
> -		ip_len = plen + sizeof(struct iphdr) + sizeof(struct tcphdr);
> -		b->iph.tot_len = htons(ip_len);
> -		b->iph.saddr = a4->s_addr;
> -		b->iph.daddr = c->ip4.addr_seen.s_addr;
> -
> -		b->iph.check = check ? *check :
> -			       csum_ip4_header(b->iph.tot_len, IPPROTO_TCP,
> -					       b->iph.saddr, b->iph.daddr);
> -
> -		SET_TCP_HEADER_COMMON_V4_V6(b, conn, seq);
> -
> -		tcp_update_check_tcp4(&b->iph);
> +		ip_len = tcp_fill_ipv4_header(c, conn, &b->iph, plen, check, seq);
>  
>  		tlen = tap_iov_len(c, &b->taph, ip_len);
>  	} else {
>  		struct tcp6_l2_buf_t *b = (struct tcp6_l2_buf_t *)p;
>  
> -		ip_len = plen + sizeof(struct ipv6hdr) + sizeof(struct tcphdr);
> -
> -		b->ip6h.payload_len = htons(plen + sizeof(struct tcphdr));
> -		b->ip6h.saddr = conn->faddr.a6;
> -		if (IN6_IS_ADDR_LINKLOCAL(&b->ip6h.saddr))
> -			b->ip6h.daddr = c->ip6.addr_ll_seen;
> -		else
> -			b->ip6h.daddr = c->ip6.addr_seen;
> -
> -		memset(b->ip6h.flow_lbl, 0, 3);
> -
> -		SET_TCP_HEADER_COMMON_V4_V6(b, conn, seq);
> -
> -		tcp_update_check_tcp6(&b->ip6h);
> -
> -		b->ip6h.hop_limit = 255;
> -		b->ip6h.version = 6;
> -		b->ip6h.nexthdr = IPPROTO_TCP;
> -
> -		b->ip6h.flow_lbl[0] = (conn->sock >> 16) & 0xf;
> -		b->ip6h.flow_lbl[1] = (conn->sock >> 8) & 0xff;
> -		b->ip6h.flow_lbl[2] = (conn->sock >> 0) & 0xff;
> +		ip_len = tcp_fill_ipv6_header(c, conn, &b->ip6h, plen, seq);
>  
>  		tlen = tap_iov_len(c, &b->taph, ip_len);
>  	}
> -#undef SET_TCP_HEADER_COMMON_V4_V6
>  
>  	return tlen;
>  }

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