Re: [PATCH v4 8/8] tcp: Introduce ipv4_fill_headers()/ipv6_fill_headers()

[Laurent Vivier <lvivier@redhat.com>]

On 3/1/24 00:54, David Gibson wrote:
> On Thu, Feb 29, 2024 at 05:59:55PM +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:
>>      v4:
>>        - group all the ip6g initialisations together and
>>          remove flow_lbl preset to 0
>>        - add ASSERT(a4)
>>      
>>      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 5b2fdf662a6c..ced22534a103 100644
>> --- a/tcp.c
>> +++ b/tcp.c
>> @@ -1326,6 +1326,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
> 
> I don't love the name, since it does also fill the TCP header.  Maybe
> 'tcp_fill_headers4()'?
> 
>> + * @c:		Execution context
>> + * @conn:	Connection pointer
>> + * @iph:	Pointer to IPv4 header, immediately followed by a TCP header
> 
> Again, really don't love accessing beyond a given pointer's type.
> 
>> + * @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
> 
> AFAICT the return value does *not* include the L2 headers..
> 
>> + */
>> +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);
>> +
>> +	ASSERT(a4);
>> +
>> +	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);
> 
> It's a bit ugly that tcp_fill_header() fills the TCP header, but *not*
> the checksum.  Could we handle this by passing the pseudo-header psum
> into tcp_fill_header()?  Then the logic for that in
> tcp_update_check_tcp4() would become part of this function.

The problem with that is we must also pass the payload (that is after the TCP header) and 
the payload length. So we need to add two parameters to tcp_fill_header(); psum and 
payload_length (guessing also the payload is following "th").

Moreover in the vhost-user part tcp_update_check_tcp4() is now called conditionally, 
because the checksum is computed in the vhost-code as the payload is stripped along 
several iovecs.

I'm going to send my v5 without updating this part. If you really think it should be done 
differently please give me more details (considering also the vhost-user part).

Thanks,
Laurent