[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;

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Commit "rxrpc: Fix the comments saying RFC8958 to be RFC8985"
  only changes comments.

--

rxrpc: Fix the comments saying RFC8958 to be RFC8985

This commit fixes comments in net/rxrpc/input_rack.c and
net/rxrpc/ar-internal.h that cited RFC8958 when they meant RFC8985, the
RACK-TLP loss detection algorithm for TCP. Only comments change. The
compiled output is the same.

> Fixes: 7c482665931b ("rxrpc: Implement RACK/TLP to deal with transmission stalls [RFC8985]")
> Signed-off-by: David Howells <dhowells@redhat.com>

[ ... ]

> cc: linux-afs@lists.infradead.org
> cc: stable@vger.kernel.org

[Severity: Low]
This isn't a bug, but should the stable Cc be here?

Each hunk swaps RFC8958 for RFC8985 in a comment, for example the file
header in input_rack.c:

-/* RACK-TLP [RFC8958] Implementation
+/* RACK-TLP [RFC8985] Implementation

Documentation/process/stable-kernel-rules.rst says:

  - No "trivial" fixes without benefit for users (spelling changes, whitespace
    cleanups, etc).

The Cc also doesn't seem needed as a prerequisite for the next patch in
the series, "rxrpc: Fix RACK-TLP implementation". That patch's hunks and
context lines don't touch any of the comments edited here. It changes
rxrpc_rack_mark_lost(), the bodies of rxrpc_rack_update_reo_wnd(),
rxrpc_rack_detect_loss() and rxrpc_tlp_calc_pto(), and the struct fields
under the "RACK-TLP [RFC8985] state" comment, which was already correct.
The functional fix should therefore apply cleanly without this patch.

Could the stable Cc be dropped, and would this patch fit better in
net-next than in net?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260925191520.2206700-1-dhowells%40redhat.com

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 (2) · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The new RTO rearm in rxrpc_input_call_event()
  (net/rxrpc/call_event.c:446-454) calls rxrpc_get_rto_backoff(call,…
- [Medium] The patch moves the recovery-exit flag into struct rxrpc_call
  (call->cong_exiting_recovery), and it is cleared only at the start of…
- [Low] rxrpc_tlp_calc_pto() (net/rxrpc/input_rack.c:300) now uses `if
  (flight_size <= call->peer->pmtud_jumbo)`.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but
  rxrpc_tx_in_flight() (ar-internal.h:1613-1616, `tx_nr_sent -…
- [Medium] This problem wasn't introduced by this patch, but
  rxrpc_rack_detect_loss() (input_rack.c:228-229) still writes the…

--

rxrpc: Fix RACK-TLP implementation

This commit fixes several problems in the rxrpc RACK-TLP implementation.
It moves the in/exiting recovery flags into struct rxrpc_call and rearms
the RTO timer at the end of rxrpc_input_call_event(). It also moves the
TLP init to FAST_RETRANSMIT entry, moves the segment_xmit_ts reset into
rxrpc_rack_detect_loss(), changes the PTO flight_size check, and stops
tx_nr_resent growing on repeated retransmission.

> diff --git a/net/rxrpc/call_event.c b/net/rxrpc/call_event.c
> index 21be9c86d7a74..07e9aac42163f 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);

[Severity: Medium]
Does passing retrans == true here make the RTO backoff grow twice per step?

rxrpc_get_rto_backoff() increments call->backoff when retrans is true:

net/rxrpc/rtt.c:rxrpc_get_rto_backoff() {
    ...
	if (retrans && timo_us * 2 <= RXRPC_RTO_MAX)
		WRITE_ONCE(call->backoff, backoff + 1);
    ...
}

When the RACK RTO fires, one pass of rxrpc_input_call_event() looks like
this:

rxrpc_input_call_event()
  rxrpc_rack_timer_expired()
    call->rack_timer_mode = RXRPC_CALL_RACKTIMER_OFF;
    rxrpc_rack_mark_losses_on_rto()
  rxrpc_resend()                                 /* tx_nr_lost > 0 */
    rxrpc_get_rto_backoff(call, req.did_send);   /* backoff + 1 */
  new block above
    rxrpc_get_rto_backoff(call, true);           /* backoff + 1 again */

rxrpc_prepare_data_packet() only sets rack_timer_mode for new data or a
TLP probe, not for retransmissions. So the mode is still OFF here, and
rxrpc_tx_in_flight() is non-zero because it counts the resent segments.

With a 1s base RTO, wouldn't the armed intervals become 1s, 2s, 8s, 32s,
64s instead of 1s, 2s, 4s, 8s, 16s, ...? That leaves fewer retransmission
attempts before expect_rx_by aborts the call.

The same double increment seems to happen after a RACK reorder timer
expiry that marks losses and returns a zero timeout, even though no
retransmission timeout has occurred.

For comparison, rxrpc_tlp_send_probe() rearms the RTO for the same
RFC 8985 7.3 reason with retrans set to false:

	if (in_flight != 0) {
		ktime_t rto = rxrpc_get_rto_backoff(call, false);

Should this call pass false as well? The commit message doesn't mention
any change to the backoff behaviour.

> +
> +		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);
> +	}
> +

[ ... ]

> diff --git a/net/rxrpc/input_rack.c b/net/rxrpc/input_rack.c
> index 072e1d9ecb2ec..66ef2fc6d3d9b 100644
> --- a/net/rxrpc/input_rack.c
> +++ b/net/rxrpc/input_rack.c

[ ... ]

> @@ -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;
>  	}

[Severity: Medium]
Can rack_reo_wnd_persist now be decremented more than once for a single
recovery?

call->cong_exiting_recovery is only cleared at the start of the next
rxrpc_congestion_management(), which runs on the next ACK:

	call->cong_in_recovery = false;
	call->cong_exiting_recovery = false;

rxrpc_rack_update_reo_wnd() also runs from the reorder timer path. That
path uses a zeroed summary, so have_dsack_option is false and this else-if
branch is taken:

rxrpc_rack_timer_expired()
  struct rxrpc_ack_summary summary = {};
  case RXRPC_CALL_RACKTIMER_RACK_REORDER:
    rxrpc_rack_detect_loss_and_arm_timer()
      rxrpc_rack_detect_loss()
        rxrpc_rack_update_reo_wnd()
          call->rack_reo_wnd_persist--;

A possible sequence:

- An earlier ACK in FAST_RETRANSMIT arms a RACK_REORDER timer.
- The exit ACK sets cong_exiting_recovery and decrements persist once.
- That ACK's rxrpc_rack_detect_loss() returns 0.
  rxrpc_rack_detect_loss_and_arm_timer() only touches the timer
  if (timeout), so the old REORDER timer stays pending.
- No new data is sent, so the mode isn't switched to TLP_PTO.
- The REORDER timer fires before the next ACK and decrements persist
  again.

Before this patch, the timer path's zeroed summary had
exiting_fast_or_rto_recovery == false, so this couldn't happen.

Wouldn't this let rack_reo_wnd_mult drop back to 1 after fewer than the
"16 DSACK-free recoveries" described in the comment above?

> @@ -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);

[Severity: Medium]
This is a pre-existing issue and wasn't introduced by this patch. What
happens if a segment marked lost here is ACKed before it is retransmitted?

rxrpc_get_xmit_ts() maps the UINT_MAX sentinel to KTIME_MAX, and
rxrpc_rack_update() doesn't check for it:

net/rxrpc/input_rack.c:rxrpc_rack_update() {
	ktime_t xmit_ts = rxrpc_get_xmit_ts(tq, ix);
	ktime_t rtt = ktime_sub(call->acks_latest_ts, xmit_ts);
    ...
	if (ktime_after(xmit_ts, call->rack_rtt_ts)) {
		call->rack_rtt	  = rtt;
		call->rack_rtt_ts = xmit_ts;
	}

	if (rxrpc_rack_sent_after(xmit_ts, seq, call->rack_xmit_ts, call->rack_end_seq)) {
		call->rack_rtt = rtt;
		call->rack_xmit_ts = xmit_ts;
		call->rack_end_seq = seq;
	}
}

A later queued ACK can soft-ACK (NACK->ACK) or hard-ACK the segment before
it is resent. rxrpc_input_call_event() processes every skb on rx_queue
before rxrpc_resend() runs, and the retransmit can also be skipped if the
header allocation fails. The segment isn't marked retransmitted, so the
segment_retransmitted check in rxrpc_rack_update() doesn't return early.

rack_rtt_ts and rack_xmit_ts would then become KTIME_MAX, and rack_rtt
would become hugely negative. No real transmit time is after KTIME_MAX, so
these values are never replaced.

>From then on, lost_after in rxrpc_rack_detect_loss() is hugely negative,
and rxrpc_rack_sent_after(KTIME_MAX, ...) is true for every segment.
Wouldn't every NACKed segment then be marked lost immediately, with no
reordering tolerance, for the rest of the call?

This patch removes the sentinel from the RTO path but keeps it here.

[ ... ]

> @@ -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);

[Severity: Low]
Does this check work as intended when pmtud_jumbo is 0?

peer->pmtud_jumbo is only written by rxrpc_input_probe_for_pmtud(). It is
0 on a newly allocated peer, and it stays 0 for peers that never do PMTUD.
rxrpc_transmit_fresh_data() clamps the same field:

		int limit = min(space, max(call->peer->pmtud_jumbo, 1));

With pmtud_jumbo == 0, flight_size is at least 1 right after sending, so
the single-packet case from RFC 8985 7.2 never matches here.

There's no visible effect today, though. call->tlp_max_ack_delay is never
assigned. It is only declared in struct rxrpc_call and read here, and the
call is zero-allocated, so ktime_add() adds 0 whichever way the check goes.

Is item (5) in the commit message expected to have any effect before
tlp_max_ack_delay is actually set? And should this use a clamp like
max(pmtud_jumbo, 1), as rxrpc_transmit_fresh_data() does?

> diff --git a/net/rxrpc/output.c b/net/rxrpc/output.c
> index 88cad087f13bf..9e082f7403312 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++;

[Severity: Medium]
This isn't a bug introduced by this patch, but does rxrpc_tx_in_flight()
count a lost-then-retransmitted segment twice?

	return call->tx_nr_sent - rxrpc_left_out(call) + call->tx_nr_resent;

On retransmit, this code clears segment_lost and decrements tx_nr_lost.
It then sets segment_retransmitted and increments tx_nr_resent. For one
such segment that gives 1 - 0 + 1 = 2.

RFC 6675 pipe and tcp_packets_in_flight() keep the lost mark after
retransmission, so there the segment is counted once. The first branch of
rxrpc_rack_mark_lost() appears to assume that model:

	if (__test_and_set_bit(ix, &tq->segment_lost)) {
		if (__test_and_clear_bit(ix, &tq->segment_retransmitted))
			call->tx_nr_resent--;

However, segment_lost has already been cleared by this point.

Wouldn't in-flight then be overestimated while retransmissions are
outstanding? That would inflate the ssthresh that
rxrpc_congestion_management() computes from in_flight / 2.

This change stops tx_nr_resent growing without bound, but the commit
message doesn't claim to fix the double count.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260925191520.2206700-1-dhowells%40redhat.com