[PATCH net 0/2] rxrpc: RACK-TLP fixes

[PATCH net 0/2] rxrpc: RACK-TLP fixes

[David Howells]

Here are some fixes for AF_RXRPC's RACK-TLP implementation.  It's split
into two patches: the first just fixes the RFC number in comments but has
no actual code changes; the second does the actual fixing, including:

 - Making the in/exiting fast/RTO recovery flags available to RACK-TLP code
   after the Reorder timer expires.

 - Arming the RTO timer if no other timer is running and there is unacked
   data.

 - Initialising the TLP state in the correct place.

 - Not resetting the xmit timestamp when marking a packet lost.

 - Correctly checking the flight size.

 - Correctly tracking the number of packets in the Tx window that have been
   re-sent and thus correctly estimating the flight size.

David

The patches can be found here also:

	http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=rxrpc-fixes-2

David Howells (2):
  rxrpc: Fix the comments saying RFC8958 to be RFC8985
  rxrpc: Fix RACK-TLP implementation

 net/rxrpc/ar-internal.h |  6 +++---
 net/rxrpc/call_event.c  | 13 +++++++++++++
 net/rxrpc/input.c       | 10 ++++++----
 net/rxrpc/input_rack.c  | 28 ++++++++++++++--------------
 net/rxrpc/output.c      |  4 ++--
 5 files changed, 38 insertions(+), 23 deletions(-)

[PATCH net 1/2] rxrpc: Fix the comments saying RFC8958 to be RFC8985

[David Howells]

Fix the comments saying RFC8958 to be RFC8985 - ie. The RACK-TLP Loss
Detection Algorithm for TCP.

Fixes: 7c482665931b ("rxrpc: Implement RACK/TLP to deal with transmission stalls [RFC8985]")
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Marc Dionne <marc.dionne@auristor.com>
cc: Jeffrey Altman <jaltman@auristor.com>
cc: Eric Dumazet <edumazet@google.com>
cc: "David S. Miller" <davem@davemloft.net>
cc: Jakub Kicinski <kuba@kernel.org>
cc: Paolo Abeni <pabeni@redhat.com>
cc: Simon Horman <horms@kernel.org>
cc: linux-afs@lists.infradead.org
cc: stable@vger.kernel.org
---
 net/rxrpc/ar-internal.h |  2 +-
 net/rxrpc/input_rack.c  | 20 ++++++++++----------
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h
index 865f05fe37ab..f0c8cbcc5dd5 100644
--- a/net/rxrpc/ar-internal.h
+++ b/net/rxrpc/ar-internal.h
@@ -1264,7 +1264,7 @@ void rxrpc_tlp_send_probe(struct rxrpc_call *call);
 void rxrpc_tlp_process_ack(struct rxrpc_call *call, struct rxrpc_ack_summary *summary);
 void rxrpc_rack_timer_expired(struct rxrpc_call *call, ktime_t overran_by);
 
-/* Initialise TLP state [RFC8958 7.1]. */
+/* Initialise TLP state [RFC8985 7.1]. */
 static inline void rxrpc_tlp_init(struct rxrpc_call *call)
 {
 	call->tlp_serial = 0;
diff --git a/net/rxrpc/input_rack.c b/net/rxrpc/input_rack.c
index 9eb109ffba56..072e1d9ecb2e 100644
--- a/net/rxrpc/input_rack.c
+++ b/net/rxrpc/input_rack.c
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
-/* RACK-TLP [RFC8958] Implementation
+/* RACK-TLP [RFC8985] Implementation
  *
  * Copyright (C) 2024 Red Hat, Inc. All Rights Reserved.
  * Written by David Howells (dhowells@redhat.com)
@@ -57,7 +57,7 @@ static unsigned long rxrpc_tq_nacks(const struct rxrpc_txqueue *tq)
 
 /*
  * Update the RACK state for the most recently sent packet that has been
- * delivered [RFC8958 6.2 Step 2].
+ * delivered [RFC8985 6.2 Step 2].
  */
 static void rxrpc_rack_update(struct rxrpc_call *call,
 			      struct rxrpc_ack_summary *summary,
@@ -98,7 +98,7 @@ static void rxrpc_rack_update(struct rxrpc_call *call,
 }
 
 /*
- * Detect data segment reordering [RFC8958 6.2 Step 3].
+ * Detect data segment reordering [RFC8985 6.2 Step 3].
  */
 static void rxrpc_rack_detect_reordering(struct rxrpc_call *call,
 					 struct rxrpc_ack_summary *summary,
@@ -143,7 +143,7 @@ void rxrpc_input_rack(struct rxrpc_call *call,
 }
 
 /*
- * Update the reordering window [RFC8958 6.2 Step 4].  Returns the updated
+ * Update the reordering window [RFC8985 6.2 Step 4].  Returns the updated
  * duration of the reordering window.
  *
  * Note that the Rx protocol doesn't have a 'DSACK option' per se, but ACKs can
@@ -192,7 +192,7 @@ static ktime_t rxrpc_rack_update_reo_wnd(struct rxrpc_call *call,
 }
 
 /*
- * Detect losses [RFC8958 6.2 Step 5].
+ * Detect losses [RFC8985 6.2 Step 5].
  */
 static ktime_t rxrpc_rack_detect_loss(struct rxrpc_call *call,
 				      struct rxrpc_ack_summary *summary)
@@ -239,7 +239,7 @@ static ktime_t rxrpc_rack_detect_loss(struct rxrpc_call *call,
 }
 
 /*
- * Detect losses and set a timer to retry the detection [RFC8958 6.2 Step 5].
+ * Detect losses and set a timer to retry the detection [RFC8985 6.2 Step 5].
  */
 void rxrpc_rack_detect_loss_and_arm_timer(struct rxrpc_call *call,
 					  struct rxrpc_ack_summary *summary)
@@ -255,7 +255,7 @@ void rxrpc_rack_detect_loss_and_arm_timer(struct rxrpc_call *call,
 }
 
 /*
- * Handle RACK-TLP RTO expiration [RFC8958 6.3].
+ * Handle RACK-TLP RTO expiration [RFC8985 6.3].
  */
 static void rxrpc_rack_mark_losses_on_rto(struct rxrpc_call *call)
 {
@@ -285,7 +285,7 @@ static void rxrpc_rack_mark_losses_on_rto(struct rxrpc_call *call)
 }
 
 /*
- * Calculate the TLP loss probe timeout (PTO) [RFC8958 7.2].
+ * Calculate the TLP loss probe timeout (PTO) [RFC8985 7.2].
  */
 ktime_t rxrpc_tlp_calc_pto(struct rxrpc_call *call, ktime_t now)
 {
@@ -309,7 +309,7 @@ ktime_t rxrpc_tlp_calc_pto(struct rxrpc_call *call, ktime_t now)
 }
 
 /*
- * Send a TLP loss probe on PTO expiration [RFC8958 7.3].
+ * Send a TLP loss probe on PTO expiration [RFC8985 7.3].
  */
 void rxrpc_tlp_send_probe(struct rxrpc_call *call)
 {
@@ -356,7 +356,7 @@ void rxrpc_tlp_send_probe(struct rxrpc_call *call)
 }
 
 /*
- * Detect losses using the ACK of a TLP loss probe [RFC8958 7.4].
+ * Detect losses using the ACK of a TLP loss probe [RFC8985 7.4].
  */
 void rxrpc_tlp_process_ack(struct rxrpc_call *call, struct rxrpc_ack_summary *summary)
 {

[PATCH net 2/2] rxrpc: Fix RACK-TLP implementation

[David Howells]

Fix the RACK-TLP implementation in the following ways:

 (1) Move the "in/exiting Fast or RTO recovery" flags to the rxrpc_call
     struct rather than rxrpc_ack_summary so that they persist beyond ACK
     parsing to the next time rxrpc_congestion_management() happens (when
     the next ACK is parsed).  This allows those values to be accessed by
     RACK-TLP when invoked by the Reorder timer expiring.

 (2) In rxrpc_input_call_event(), rearm the RTO timer before the function
     returns if no other timer is running and if DATA packets have been
     sent but not yet ACK'd.  This prevents us not having a timer set to
     drive retransmission.

 (3) In rxrpc_congestion_management(), initialise the TLP state when
     entering the FAST_RETRANSMIT congestion control state, not every time
     we process that state - otherwise TLP doesn't happen in that state.

 (4) Don't reset segment_xmit_ts in rxrpc_rack_mark_lost() as that's called
     from two places, one of which shouldn't do that.  Instead, do it in
     rxrpc_rack_detect_loss().

 (5) In rxrpc_tlp_calc_pto(), check flight_size is at most one jumbo
     packet's worth of subpackets in size, not just any non-zero value
     (RFC8985 7.2 has a comparison against 1, so this is approximated
     because we'll be sending jumbo packets if possible).

 (6) In rxrpc_prepare_data_packet(), only increment call->tx_nr_resent if
     the DATA packet being retransmitted was not previously transmitted
     otherwise tx_nr_resent will keep getting bigger and cause RACK-TLP to
     malfunction (the value is used in the in-flight calculation).
     tx_nr_resent is decremented when a packet that has been retransmitted
     is discarded - but only by 1 per packet.

Fixes: 7c482665931b ("rxrpc: Implement RACK/TLP to deal with transmission stalls [RFC8985]")
Signed-off-by: David Howells <dhowells@redhat.com>
Reviewed-by: Marc Dionne <marc.dionne@auristor.com>
Tested-by: Marc Dionne <marc.dionne@auristor.com>
Reviewed-by: Jeffrey Altman <jaltman@auristor.com>
cc: Eric Dumazet <edumazet@google.com>
cc: "David S. Miller" <davem@davemloft.net>
cc: Jakub Kicinski <kuba@kernel.org>
cc: Paolo Abeni <pabeni@redhat.com>
cc: Simon Horman <horms@kernel.org>
cc: linux-afs@lists.infradead.org
cc: stable@vger.kernel.org
---
 net/rxrpc/ar-internal.h |  4 ++--
 net/rxrpc/call_event.c  | 13 +++++++++++++
 net/rxrpc/input.c       | 10 ++++++----
 net/rxrpc/input_rack.c  |  8 ++++----
 net/rxrpc/output.c      |  4 ++--
 5 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h
index f0c8cbcc5dd5..9c7b2e0817e7 100644
--- a/net/rxrpc/ar-internal.h
+++ b/net/rxrpc/ar-internal.h
@@ -818,6 +818,8 @@ struct rxrpc_call {
 	u16			cong_dup_acks;	/* Count of ACKs showing missing packets */
 	u16			cong_cumul_acks; /* Cumulative ACK count */
 	ktime_t			cong_tstamp;	/* Last time cwnd was changed */
+	bool			cong_in_recovery:1;		/* If we're in Fast/RTO recovery */
+	bool			cong_exiting_recovery:1;	/* If we're leaving Fast/RTO recovery */
 
 	/* RACK-TLP [RFC8985] state. */
 	ktime_t			rack_xmit_ts;	/* Latest transmission timestamp */
@@ -896,8 +898,6 @@ struct rxrpc_ack_summary {
 	bool		retrans_timeo:1;	/* T if reTx due to timeout happened */
 	bool		need_retransmit:1;	/* T if we need transmission */
 	bool		rtt_sample_avail:1;	/* T if RTT sample available */
-	bool		in_fast_or_rto_recovery:1;
-	bool		exiting_fast_or_rto_recovery:1;
 	bool		tlp_probe_acked:1;	/* T if the TLP probe seq was acked */
 	u8 /*enum rxrpc_congest_change*/ change;
 };
diff --git a/net/rxrpc/call_event.c b/net/rxrpc/call_event.c
index 21be9c86d7a7..07e9aac42163 100644
--- a/net/rxrpc/call_event.c
+++ b/net/rxrpc/call_event.c
@@ -440,6 +440,19 @@ bool rxrpc_input_call_event(struct rxrpc_call *call)
 				       rxrpc_propose_ack_input_data);
 	}
 
+	/* Need to rearm the RTO timer if we don't start the TLP-PTO
+	 * timer [RFC8985 7.3].
+	 */
+	if (call->rack_timer_mode == RXRPC_CALL_RACKTIMER_OFF &&
+	    rxrpc_tx_in_flight(call) > 0) {
+		ktime_t rto = rxrpc_get_rto_backoff(call, true);
+
+		call->rack_timer_mode = RXRPC_CALL_RACKTIMER_RTO;
+		call->rack_timo_at = ktime_add(ktime_get_real(), rto);
+		trace_rxrpc_rack_timer(call, rto, false);
+		trace_rxrpc_timer_set(call, rto, rxrpc_timer_trace_rack_rto);
+	}
+
 	/* Make sure the timer is restarted */
 	if (!__rxrpc_call_is_complete(call)) {
 		ktime_t next = READ_ONCE(call->expect_term_by), delay;
diff --git a/net/rxrpc/input.c b/net/rxrpc/input.c
index 73cafe6bfa9f..0e9f6a1e9f00 100644
--- a/net/rxrpc/input.c
+++ b/net/rxrpc/input.c
@@ -34,6 +34,8 @@ static void rxrpc_congestion_management(struct rxrpc_call *call,
 {
 	summary->change = rxrpc_cong_no_change;
 	summary->in_flight = rxrpc_tx_in_flight(call);
+	call->cong_in_recovery = false;
+	call->cong_exiting_recovery = false;
 
 	if (test_and_clear_bit(RXRPC_CALL_RETRANS_TIMEOUT, &call->flags)) {
 		summary->retrans_timeo = true;
@@ -106,12 +108,12 @@ static void rxrpc_congestion_management(struct rxrpc_call *call,
 		call->cong_extra = 0;
 		call->cong_dup_acks = 0;
 		summary->need_retransmit = true;
-		summary->in_fast_or_rto_recovery = true;
+		call->cong_in_recovery = true;
+		rxrpc_tlp_init(call);
 		goto out;
 
 	case RXRPC_CA_FAST_RETRANSMIT:
-		rxrpc_tlp_init(call);
-		summary->in_fast_or_rto_recovery = true;
+		call->cong_in_recovery = true;
 		if (!summary->new_low_snack) {
 			if (summary->nr_new_sacks == 0)
 				call->cong_cwnd += 1;
@@ -125,7 +127,7 @@ static void rxrpc_congestion_management(struct rxrpc_call *call,
 			summary->change = rxrpc_cong_progress;
 			call->cong_cwnd = call->cong_ssthresh;
 			if (call->acks_nr_snacks == 0) {
-				summary->exiting_fast_or_rto_recovery = true;
+				call->cong_exiting_recovery = true;
 				goto resume_normality;
 			}
 		}
diff --git a/net/rxrpc/input_rack.c b/net/rxrpc/input_rack.c
index 072e1d9ecb2e..66ef2fc6d3d9 100644
--- a/net/rxrpc/input_rack.c
+++ b/net/rxrpc/input_rack.c
@@ -29,7 +29,6 @@ static void rxrpc_rack_mark_lost(struct rxrpc_call *call,
 	} else {
 		call->tx_nr_lost++;
 	}
-	tq->segment_xmit_ts[ix] = UINT_MAX;
 }
 
 /*
@@ -174,14 +173,14 @@ static ktime_t rxrpc_rack_update_reo_wnd(struct rxrpc_call *call,
 		call->rack_dsack_round = snd_nxt;
 		call->rack_reo_wnd_mult++;
 		call->rack_reo_wnd_persist = 16;
-	} else if (summary->exiting_fast_or_rto_recovery) {
+	} else if (call->cong_exiting_recovery) {
 		call->rack_reo_wnd_persist--;
 		if (call->rack_reo_wnd_persist <= 0)
 			call->rack_reo_wnd_mult = 1;
 	}
 
 	if (!call->rack_reordering_seen) {
-		if (summary->in_fast_or_rto_recovery)
+		if (call->cong_in_recovery)
 			return 0;
 		if (call->acks_nr_sacks >= dup_thresh)
 			return 0;
@@ -227,6 +226,7 @@ static ktime_t rxrpc_rack_detect_loss(struct rxrpc_call *call,
 				remaining = ktime_sub(ktime_add(xmit_ts, lost_after), now);
 				if (remaining <= 0) {
 					rxrpc_rack_mark_lost(call, tq, ix);
+					tq->segment_xmit_ts[ix] = UINT_MAX;
 					trace_rxrpc_rack_detect_loss(call, summary, seq);
 				} else {
 					timeout = max(remaining, timeout);
@@ -297,7 +297,7 @@ ktime_t rxrpc_tlp_calc_pto(struct rxrpc_call *call, ktime_t now)
 	if (call->rtt_count > 0) {
 		/* Use 2*SRTT as the timeout. */
 		pto = ns_to_ktime(call->srtt_us * NSEC_PER_USEC / 4);
-		if (flight_size)
+		if (flight_size <= call->peer->pmtud_jumbo)
 			pto = ktime_add(pto, call->tlp_max_ack_delay);
 	} else {
 		pto = NSEC_PER_SEC;
diff --git a/net/rxrpc/output.c b/net/rxrpc/output.c
index 88cad087f13b..9e082f740331 100644
--- a/net/rxrpc/output.c
+++ b/net/rxrpc/output.c
@@ -576,8 +576,8 @@ static size_t rxrpc_prepare_data_packet(struct rxrpc_call *call,
 			call->tx_nr_lost--;
 		if (req->retrans) {
 			__set_bit(ix, &tq->ever_retransmitted);
-			__set_bit(ix, &tq->segment_retransmitted);
-			call->tx_nr_resent++;
+			if (!__test_and_set_bit(ix, &tq->segment_retransmitted))
+				call->tx_nr_resent++;
 		} else {
 			call->tx_nr_sent++;
 			start_tlp = true;