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

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

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

On Wed, May 15, 2024 at 10:57:06PM -0400, Jon Maloy wrote:
> 
> 
> 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.

Oh, of course, my mistake.

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