Re: [PATCH v4 1/3] tcp: move seq_to_tap update to when frame is queued

[Jon Maloy <jmaloy@redhat.com>]

On 2024-05-15 22:24, David Gibson wrote:
> On Wed, May 15, 2024 at 11:34:27AM -0400, Jon Maloy wrote:
>> commit a469fc393fa1 ("tcp, tap: Don't increase tap-side sequence counter for dropped frames")
>> delayed update of conn->seq_to_tap until the moment the corresponding
>> frame has been successfully pushed out. This has the advantage that we
>> immediately can make a new attempt to transmit a frame after a failed
>> trasnmit, rather than waiting for the peer to later discover a gap and
>> trigger the fast retransmit mechanism to solve the problem.
>>
>> This approach has turned out to cause a problem with spurious sequence
>> number updates during peer-initiated retransmits, and we have realized
>> it may not be the best way to solve the above issue.
>>
>> We now restore the previous method, by updating the said field at the
>> moment a frame is added to the outqueue. To retain the advantage of
>> having a quick re-attempt based on local failure detection, we now scan
>> through the part of the outqueue that had do be dropped, and restore the
>> sequence counter for each affected connection to the most appropriate
>> value.
>>
>> Signed-off-by: Jon Maloy <jmaloy@redhat.com>
>>
>> ---
>> v2: - Re-spun loop in tcp_revert_seq() and some other changes based on
>>        feedback from Stefano Brivio.
>>      - Added paranoid test to avoid that seq_to_tap becomes lower than
>>        seq_ack_from_tap.
>>
>> v3: - Identical to v2. Called v3 because it was embedded in a series
>>        with that version.
>>
>> v4: - In tcp_revert_seq(), we read the sequence number from the TCP
>>        header instead of keeping a copy in struct tcp_buf_seq_update.
>>      - Since the only remaining field in struct tcp_buf_seq_update is
>>        a pointer to struct tcp_tap_conn, we eliminate the struct
>>        altogether, and make the tcp6/tcp3_buf_seq_update arrays into
>>        arrays of said pointer.
>>      - Removed 'paranoid' test in tcp_revert_seq. If it happens, it
>>        is not fatal, and will be caught by other code anyway.
>>      - Separated from the series again.
>> ---
>>   tcp.c | 59 +++++++++++++++++++++++++++++++++++++----------------------
>>   1 file changed, 37 insertions(+), 22 deletions(-)
>>
>> diff --git a/tcp.c b/tcp.c
>> index 21d0af0..976dba8 100644
>> --- a/tcp.c
>> +++ b/tcp.c
>> @@ -410,16 +410,6 @@ static int tcp_sock_ns		[NUM_PORTS][IP_VERSIONS];
>>    */
>>   static union inany_addr low_rtt_dst[LOW_RTT_TABLE_SIZE];
>>   
>> -/**
>> - * tcp_buf_seq_update - Sequences to update with length of frames once sent
>> - * @seq:	Pointer to sequence number sent to tap-side, to be updated
>> - * @len:	TCP payload length
>> - */
>> -struct tcp_buf_seq_update {
>> -	uint32_t *seq;
>> -	uint16_t len;
>> -};
>> -
>>   /* Static buffers */
>>   /**
>>    * struct tcp_payload_t - TCP header and data to send segments with payload
>> @@ -461,7 +451,8 @@ static struct tcp_payload_t	tcp4_payload[TCP_FRAMES_MEM];
>>   
>>   static_assert(MSS4 <= sizeof(tcp4_payload[0].data), "MSS4 is greater than 65516");
>>   
>> -static struct tcp_buf_seq_update tcp4_seq_update[TCP_FRAMES_MEM];
>> +/* References tracking the owner connection of frames in the tap outqueue */
>> +static struct tcp_tap_conn *tcp4_frame_conns[TCP_FRAMES_MEM];
>>   static unsigned int tcp4_payload_used;
>>   
>>   static struct tap_hdr		tcp4_flags_tap_hdr[TCP_FRAMES_MEM];
>> @@ -483,7 +474,8 @@ static struct tcp_payload_t	tcp6_payload[TCP_FRAMES_MEM];
>>   
>>   static_assert(MSS6 <= sizeof(tcp6_payload[0].data), "MSS6 is greater than 65516");
>>   
>> -static struct tcp_buf_seq_update tcp6_seq_update[TCP_FRAMES_MEM];
>> +/* References tracking the owner connection of frames in the tap outqueue */
>> +static struct tcp_tap_conn *tcp6_frame_conns[TCP_FRAMES_MEM];
>>   static unsigned int tcp6_payload_used;
>>   
>>   static struct tap_hdr		tcp6_flags_tap_hdr[TCP_FRAMES_MEM];
>> @@ -1261,25 +1253,49 @@ static void tcp_flags_flush(const struct ctx *c)
>>   	tcp4_flags_used = 0;
>>   }
>>   
>> +/**
>> + * tcp_revert_seq() - Revert affected conn->seq_to_tap after failed transmission
>> + * @conns:       Array of connection pointers corresponding to queued frames
>> + * @frames:      Two-dimensional array containing queued frames with sub-iovs
> You can make the 2d array explicit in the type as:
> 	struct iovec (*frames)[TCP_NUM_IOVS];
> See, for example the 'tap_iov' local in udp_tap_send().   (I recommend
> the command line tool 'cdecl', also available online at cdecl.org for
> working out confusing pointer-to-array types).
Nice. I  wasn't quite happy with this.
>
>> + * @num_frames:  Number of entries in the two arrays to be compared
>> + */
>> +static void tcp_revert_seq(struct tcp_tap_conn **conns, struct iovec *frames,
>> +			   int num_frames)
>> +{
>> +	int c, f;
>> +
>> +	for (c = 0, f = 0; c < num_frames; c++, f += TCP_NUM_IOVS) {
> Nit: I find having the two parallel counters kind of confusing.  It
> naturally goes away with the type change suggested above, but even
> without that I'd prefer an explicit multiply in the body.  I strongly
> suspect the compiler will be better at working out if the strength
> reduction is worth it.
>
>> +		struct tcp_tap_conn *conn = conns[c];
>> +		struct tcphdr *th = frames[f + TCP_IOV_PAYLOAD].iov_base;
>> +		uint32_t seq = ntohl(th->seq);
>> +
>> +		if (SEQ_LE(conn->seq_to_tap, seq))
> Isn't this test inverted?  We want to rewind seq_to_tap if seq is less
> than it, rather than the other way aruond.
No. We do 'continue', i.e., nothing, if this condition is fulfilled.
This may look a little non-intuitive here, but makes sense when I add 
the next patch.
>
>> +			continue;
>> +
>> +		conn->seq_to_tap = seq;
>> +	}
>> +}
>> +
>>   /**
>>    * tcp_payload_flush() - Send out buffers for segments with data
>>    * @c:		Execution context
>>    */
>>   static void tcp_payload_flush(const struct ctx *c)
>>   {
>> -	unsigned i;
>>   	size_t m;
>>   
>>   	m = tap_send_frames(c, &tcp6_l2_iov[0][0], TCP_NUM_IOVS,
>>   			    tcp6_payload_used);
>> -	for (i = 0; i < m; i++)
>> -		*tcp6_seq_update[i].seq += tcp6_seq_update[i].len;
>> +	if (m != tcp6_payload_used)
>> +		tcp_revert_seq(tcp6_frame_conns, &tcp6_l2_iov[m][0],
> With the type change above this would become just &tcp_l2_iov[m].
ok

///jon
>
>> +			       tcp6_payload_used - m);
>>   	tcp6_payload_used = 0;
>>   
>>   	m = tap_send_frames(c, &tcp4_l2_iov[0][0], TCP_NUM_IOVS,
>>   			    tcp4_payload_used);
>> -	for (i = 0; i < m; i++)
>> -		*tcp4_seq_update[i].seq += tcp4_seq_update[i].len;
>> +	if (m != tcp4_payload_used)
>> +		tcp_revert_seq(tcp4_frame_conns, &tcp4_l2_iov[m][0],
>> +			       tcp4_payload_used - m);
>>   	tcp4_payload_used = 0;
>>   }
>>   
>> @@ -2129,10 +2145,11 @@ static int tcp_sock_consume(const struct tcp_tap_conn *conn, uint32_t ack_seq)
>>   static void tcp_data_to_tap(const struct ctx *c, struct tcp_tap_conn *conn,
>>   			    ssize_t dlen, int no_csum, uint32_t seq)
>>   {
>> -	uint32_t *seq_update = &conn->seq_to_tap;
>>   	struct iovec *iov;
>>   	size_t l4len;
>>   
>> +	conn->seq_to_tap = seq + dlen;
>> +
>>   	if (CONN_V4(conn)) {
>>   		struct iovec *iov_prev = tcp4_l2_iov[tcp4_payload_used - 1];
>>   		const uint16_t *check = NULL;
>> @@ -2142,8 +2159,7 @@ static void tcp_data_to_tap(const struct ctx *c, struct tcp_tap_conn *conn,
>>   			check = &iph->check;
>>   		}
>>   
>> -		tcp4_seq_update[tcp4_payload_used].seq = seq_update;
>> -		tcp4_seq_update[tcp4_payload_used].len = dlen;
>> +		tcp4_frame_conns[tcp4_payload_used] = conn;
>>   
>>   		iov = tcp4_l2_iov[tcp4_payload_used++];
>>   		l4len = tcp_l2_buf_fill_headers(c, conn, iov, dlen, check, seq);
>> @@ -2151,8 +2167,7 @@ static void tcp_data_to_tap(const struct ctx *c, struct tcp_tap_conn *conn,
>>   		if (tcp4_payload_used > TCP_FRAMES_MEM - 1)
>>   			tcp_payload_flush(c);
>>   	} else if (CONN_V6(conn)) {
>> -		tcp6_seq_update[tcp6_payload_used].seq = seq_update;
>> -		tcp6_seq_update[tcp6_payload_used].len = dlen;
>> +		tcp6_frame_conns[tcp6_payload_used] = conn;
>>   
>>   		iov = tcp6_l2_iov[tcp6_payload_used++];
>>   		l4len = tcp_l2_buf_fill_headers(c, conn, iov, dlen, NULL, seq);