[PATCH net v2 2/3] rxrpc: Fix counting of in-flight transmitted packets

[PATCH net v2 2/3] rxrpc: Fix counting of in-flight transmitted packets

[David Howells]

The RACK-TLP algorithm [RFC8985] makes use of the count of in-flight
packets in decisions (denoted as FlightSize).  This is currently based on
an approximation derived from the sum of other values:

	tx_nr_sent - (acks_nr_sacks + tx_nr_lost) + tx_nr_resent

This, however, turns out not to be very good here as the component counters
aren't updated together and this may lead to an incorrect estimate.

Fix counting of in-flight transmitted packets by tracking them directly
rather than deriving the value:

 (1) On each rxrpc_txqueue struct, keep a bitmap of which packets (txbuf)
     from the set managed by that txqueue are considered in-flight.

 (2) A new counter, tx_in_flight, is then added to the rxrpc_call struct to
     keep track of the number of packets considered to be in flight.

 (3) Set a packet's bit when the packet is transmitted and if the bit
     wasn't already set, increment tx_in_flight.

 (4) Clear a packet's bit when an ACK is received for the packet or if the
     packet is declared lost - and if the bit wasn't already cleared,
     decrement tx_in_flight.

The value in tx_in_flight is then used instead of the approximation.

The tx_nr_sent and tx_nr_resent counters are then removed and the
rxrpc_rack tracepoint updated.

Fixes: 7c482665931b ("rxrpc: Implement RACK/TLP to deal with transmission stalls [RFC8985]")
Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260925191520.2206700-1-dhowells%40redhat.com
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
---
 include/trace/events/rxrpc.h | 11 ++++-------
 net/rxrpc/ar-internal.h      | 18 ++----------------
 net/rxrpc/input.c            |  8 ++++----
 net/rxrpc/input_rack.c       | 19 +++++++++++--------
 net/rxrpc/output.c           |  5 +++--
 5 files changed, 24 insertions(+), 37 deletions(-)

diff --git a/include/trace/events/rxrpc.h b/include/trace/events/rxrpc.h
index 56dc9b614071..67a5612cdc55 100644
--- a/include/trace/events/rxrpc.h
+++ b/include/trace/events/rxrpc.h
@@ -2584,9 +2584,8 @@ TRACE_EVENT(rxrpc_rack,
 		    __field(rxrpc_serial_t,	ack_serial)
 		    __field(rxrpc_seq_t,	seq)
 		    __field(enum rxrpc_rack_timer_mode, mode)
-		    __field(unsigned short,	nr_sent)
+		    __field(unsigned short,	in_flight)
 		    __field(unsigned short,	nr_lost)
-		    __field(unsigned short,	nr_resent)
 		    __field(unsigned short,	nr_sacked)
 		    __field(ktime_t,		timo)
 			     ),
@@ -2596,18 +2595,16 @@ TRACE_EVENT(rxrpc_rack,
 		    __entry->ack_serial	= call->rx_serial;
 		    __entry->seq	= call->rack_end_seq;
 		    __entry->mode	= call->rack_timer_mode;
-		    __entry->nr_sent	= call->tx_nr_sent;
+		    __entry->in_flight	= call->tx_in_flight;
 		    __entry->nr_lost	= call->tx_nr_lost;
-		    __entry->nr_resent	= call->tx_nr_resent;
 		    __entry->nr_sacked	= call->acks_nr_sacks;
 		    __entry->timo	= timo;
 			   ),
 
-	    TP_printk("c=%08x r=%08x q=%08x %s slrs=%u,%u,%u,%u t=%lld",
+	    TP_printk("c=%08x r=%08x q=%08x %s fls=%u,%u,%u t=%lld",
 		      __entry->call, __entry->ack_serial, __entry->seq,
 		      __print_symbolic(__entry->mode, rxrpc_rack_timer_modes),
-		      __entry->nr_sent, __entry->nr_lost,
-		      __entry->nr_resent, __entry->nr_sacked,
+		      __entry->in_flight, __entry->nr_lost, __entry->nr_sacked,
 		      ktime_to_us(__entry->timo))
 	    );
 
diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h
index f0c8cbcc5dd5..d0e18eb6301e 100644
--- a/net/rxrpc/ar-internal.h
+++ b/net/rxrpc/ar-internal.h
@@ -782,9 +782,8 @@ struct rxrpc_call {
 	rxrpc_seq_t		tx_top;		/* Highest Tx slot allocated. */
 	rxrpc_serial_t		tx_last_serial;	/* Serial of last DATA transmitted */
 	u16			tx_backoff;	/* Delay to insert due to Tx failure (ms) */
-	u16			tx_nr_sent;	/* Number of packets sent, but unacked */
+	u16			tx_in_flight;	/* Number of packets considered "in flight" */
 	u16			tx_nr_lost;	/* Number of packets marked lost */
-	u16			tx_nr_resent;	/* Number of packets resent, but unacked */
 	u16			tx_winsize;	/* Maximum size of Tx window */
 #define RXRPC_TX_MAX_WINDOW	128
 	u8			tx_jumbo_max;	/* Maximum subpkts peer will accept */
@@ -980,6 +979,7 @@ struct rxrpc_txqueue {
 	unsigned long		segment_acked;	/* Bit-per-buf: Set if ACK'd */
 	unsigned long		segment_lost;	/* Bit-per-buf: Set if declared lost */
 	unsigned long		segment_retransmitted; /* Bit-per-buf: Set if retransmitted */
+	unsigned long		in_flight;	/* Bit-per-buf: Set if "in flight" */
 	unsigned long		rtt_samples;	/* Bit-per-buf: Set if available for RTT */
 	unsigned long		ever_retransmitted; /* Bit-per-buf: Set if ever retransmitted */
 
@@ -1601,20 +1601,6 @@ static inline unsigned int rxrpc_tx_window_space(const struct rxrpc_call *call)
 	return max(winsize - transmitted, 0);
 }
 
-static inline unsigned int rxrpc_left_out(const struct rxrpc_call *call)
-{
-	return call->acks_nr_sacks + call->tx_nr_lost;
-}
-
-/*
- * Calculate the number of transmitted DATA packets assumed to be in flight
- * [approx RFC6675].
- */
-static inline unsigned int rxrpc_tx_in_flight(const struct rxrpc_call *call)
-{
-	return call->tx_nr_sent - rxrpc_left_out(call) + call->tx_nr_resent;
-}
-
 /*
  * debug tracing
  */
diff --git a/net/rxrpc/input.c b/net/rxrpc/input.c
index 73cafe6bfa9f..f497eba59cf6 100644
--- a/net/rxrpc/input.c
+++ b/net/rxrpc/input.c
@@ -33,7 +33,7 @@ static void rxrpc_congestion_management(struct rxrpc_call *call,
 					struct rxrpc_ack_summary *summary)
 {
 	summary->change = rxrpc_cong_no_change;
-	summary->in_flight = rxrpc_tx_in_flight(call);
+	summary->in_flight = call->tx_in_flight;
 
 	if (test_and_clear_bit(RXRPC_CALL_RETRANS_TIMEOUT, &call->flags)) {
 		summary->retrans_timeo = true;
@@ -288,11 +288,11 @@ static bool rxrpc_rotate_tx_window(struct rxrpc_call *call, rxrpc_seq_t to,
 			trace_rxrpc_rotate(call, tq, summary, seq, rxrpc_rotate_trace_snak);
 		}
 
-		call->tx_nr_sent--;
+		if (__test_and_clear_bit(ix, &tq->in_flight))
+			call->tx_in_flight--;
 		if (__test_and_clear_bit(ix, &tq->segment_lost))
 			call->tx_nr_lost--;
-		if (__test_and_clear_bit(ix, &tq->segment_retransmitted))
-			call->tx_nr_resent--;
+		__clear_bit(ix, &tq->segment_retransmitted);
 		__clear_bit(ix, &tq->ever_retransmitted);
 
 		rxrpc_put_txbuf(tq->bufs[ix], rxrpc_txbuf_put_rotated);
diff --git a/net/rxrpc/input_rack.c b/net/rxrpc/input_rack.c
index 072e1d9ecb2e..e43ce032a261 100644
--- a/net/rxrpc/input_rack.c
+++ b/net/rxrpc/input_rack.c
@@ -23,12 +23,13 @@ static bool rxrpc_rack_sent_after(ktime_t t1, rxrpc_seq_t seq1,
 static void rxrpc_rack_mark_lost(struct rxrpc_call *call,
 				 struct rxrpc_txqueue *tq, unsigned int ix)
 {
-	if (__test_and_set_bit(ix, &tq->segment_lost)) {
-		if (__test_and_clear_bit(ix, &tq->segment_retransmitted))
-			call->tx_nr_resent--;
-	} else {
+	if (__test_and_set_bit(ix, &tq->segment_lost))
+		__clear_bit(ix, &tq->segment_retransmitted);
+	else
 		call->tx_nr_lost++;
-	}
+
+	if (__test_and_clear_bit(ix, &tq->in_flight))
+		call->tx_in_flight--;
 	tq->segment_xmit_ts[ix] = UINT_MAX;
 }
 
@@ -136,6 +137,8 @@ void rxrpc_input_rack(struct rxrpc_call *call,
 		unsigned int ix = __ffs(new_acks);
 
 		__clear_bit(ix, &new_acks);
+		if (__test_and_clear_bit(ix, &tq->in_flight))
+			call->tx_in_flight--;
 		rxrpc_input_rack_one(call, summary, tq, ix);
 	}
 
@@ -289,7 +292,7 @@ static void rxrpc_rack_mark_losses_on_rto(struct rxrpc_call *call)
  */
 ktime_t rxrpc_tlp_calc_pto(struct rxrpc_call *call, ktime_t now)
 {
-	unsigned int flight_size = rxrpc_tx_in_flight(call);
+	unsigned int flight_size = call->tx_in_flight;
 	ktime_t rto_at = ktime_add(call->tx_last_sent,
 				   rxrpc_get_rto_backoff(call, false));
 	ktime_t pto;
@@ -313,7 +316,7 @@ ktime_t rxrpc_tlp_calc_pto(struct rxrpc_call *call, ktime_t now)
  */
 void rxrpc_tlp_send_probe(struct rxrpc_call *call)
 {
-	unsigned int in_flight = rxrpc_tx_in_flight(call);
+	unsigned int in_flight = call->tx_in_flight;
 
 	if (after_eq(call->acks_hard_ack, call->tx_transmitted))
 		return; /* Everything we transmitted has been acked. */
@@ -333,7 +336,7 @@ void rxrpc_tlp_send_probe(struct rxrpc_call *call)
 			call->tlp_serial = call->tx_last_serial;
 			call->tlp_seq = call->tx_transmitted;
 			trace_rxrpc_tlp_probe(call, rxrpc_tlp_probe_trace_transmit_new);
-			in_flight = rxrpc_tx_in_flight(call);
+			in_flight = call->tx_in_flight;
 		} else {
 			/* Retransmit the highest-sequence DATA sent */
 			call->tx_last_serial = 0;
diff --git a/net/rxrpc/output.c b/net/rxrpc/output.c
index 88cad087f13b..3ccd9a27643a 100644
--- a/net/rxrpc/output.c
+++ b/net/rxrpc/output.c
@@ -571,15 +571,16 @@ static size_t rxrpc_prepare_data_packet(struct rxrpc_call *call,
 
 		_debug("prep[%u] tq=%x q=%x", i, tq->qbase, seq);
 
+		if (!__test_and_set_bit(ix, &tq->in_flight))
+			call->tx_in_flight++;
+
 		/* Record (re-)transmission for RACK [RFC8985 6.1]. */
 		if (__test_and_clear_bit(ix, &tq->segment_lost))
 			call->tx_nr_lost--;
 		if (req->retrans) {
 			__set_bit(ix, &tq->ever_retransmitted);
 			__set_bit(ix, &tq->segment_retransmitted);
-			call->tx_nr_resent++;
 		} else {
-			call->tx_nr_sent++;
 			start_tlp = true;
 		}
 		tq->segment_xmit_ts[ix] = xmit_ts;

Re: [PATCH net v2 2/3] rxrpc: Fix counting of in-flight transmitted packets

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 0 (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] Incomplete in-flight accounting: if a segment that is already
  soft-ACKed gets retransmitted, its tq->in_flight bit is set again and…
- [Medium] Behaviour change on soft-ACK reneging (ACK->NAK flip).

Pre-existing issues:
- [Low] In rxrpc_tlp_send_probe(), the retransmit branch
  (rxrpc_resend_tlp()) does not re-read call->tx_in_flight, so the `if…

--

rxrpc: Fix counting of in-flight transmitted packets

RACK-TLP used a FlightSize value derived from several counters. This
commit replaces it with a direct count: each txqueue gets an in_flight
bitmap, which is set on transmission and cleared on ACK or loss, and
call->tx_in_flight counts the set bits. The tx_nr_sent and tx_nr_resent
counters are removed and the rxrpc_rack tracepoint is updated.

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

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

[ ... ]

> @@ -136,6 +137,8 @@ void rxrpc_input_rack(struct rxrpc_call *call,
>  		unsigned int ix = __ffs(new_acks);
>  
>  		__clear_bit(ix, &new_acks);
> +		if (__test_and_clear_bit(ix, &tq->in_flight))
> +			call->tx_in_flight--;
>  		rxrpc_input_rack_one(call, summary, tq, ix);
>  	}
>  

[Severity: Medium]
What happens to a segment whose soft ACK is later reneged? The in_flight
bit is cleared here when the segment is first soft-ACKed. The ACK->NAK
flip path in rxrpc_input_soft_ack_tq() only adjusts the SACK counters:

net/rxrpc/input.c:rxrpc_input_soft_ack_tq() {
    ...
	flipped = (tq->segment_acked ^ extracted_acks) & old_reported;
	if (flipped) {
		n_to_a = ~tq->segment_acked & flipped; /* Old NAK -> ACK */
		a_to_n =  tq->segment_acked & flipped; /* Old ACK -> NAK */
		...
		call->acks_nr_sacks	+= a - n;
		call->acks_nr_snacks	+= n - a;
    ...
}

Under the old formula, the drop in acks_nr_sacks put the reneged packet
back into FlightSize. Now only rxrpc_prepare_data_packet() sets
tq->in_flight. So until RACK marks the reneged segment lost and it is
retransmitted, it counts as neither acked, lost nor in flight.

In the same ACK, rxrpc_congestion_management() reads call->tx_in_flight
for cong_ssthresh before RACK loss detection runs. The segment may not be
marked lost straight away, for example if it was the most recently
delivered segment or the reorder window delays it. In that case the
undercount also reaches rxrpc_tlp_calc_pto(), the in_flight != 0 RTO
decision in rxrpc_tlp_send_probe(), and the tx_in_flight > 0 RTO-rearm
check added by the later commit "rxrpc: Fix RACK-TLP implementation".

Should the a_to_n case set the in_flight bit again and increment
tx_in_flight?

[ ... ]

> @@ -313,7 +316,7 @@ ktime_t rxrpc_tlp_calc_pto(struct rxrpc_call *call, ktime_t now)
>   */
>  void rxrpc_tlp_send_probe(struct rxrpc_call *call)
>  {
> -	unsigned int in_flight = rxrpc_tx_in_flight(call);
> +	unsigned int in_flight = call->tx_in_flight;
>  
>  	if (after_eq(call->acks_hard_ack, call->tx_transmitted))
>  		return; /* Everything we transmitted has been acked. */
> @@ -333,7 +336,7 @@ void rxrpc_tlp_send_probe(struct rxrpc_call *call)
>  			call->tlp_serial = call->tx_last_serial;
>  			call->tlp_seq = call->tx_transmitted;
>  			trace_rxrpc_tlp_probe(call, rxrpc_tlp_probe_trace_transmit_new);
> -			in_flight = rxrpc_tx_in_flight(call);
> +			in_flight = call->tx_in_flight;
>  		} else {
>  			/* Retransmit the highest-sequence DATA sent */
>  			call->tx_last_serial = 0;

[Severity: Low]
This is a pre-existing issue, not one introduced by this patch. The
new-data branch re-reads in_flight, but the retransmit branch does not
re-read it after rxrpc_resend_tlp(). So the later check:

	if (in_flight != 0) {
		...
		call->rack_timer_mode = RXRPC_CALL_RACKTIMER_RTO;

uses the value from before the probe was sent. If the flight size was 0
before the probe, could a TLP retransmit leave no RTO armed? The old code
took its snapshot in the same order.

The later commit "rxrpc: Fix RACK-TLP implementation" seems to cover the
stall. At the end of rxrpc_input_call_event() it arms the RTO when
rack_timer_mode is RXRPC_CALL_RACKTIMER_OFF and tx_in_flight > 0, so
what remains may only be an inconsistency. Would it be cleaner to re-read
in_flight in both branches?

> diff --git a/net/rxrpc/output.c b/net/rxrpc/output.c
> index 88cad087f13bf..3ccd9a27643a8 100644
> --- a/net/rxrpc/output.c
> +++ b/net/rxrpc/output.c
> @@ -571,15 +571,16 @@ static size_t rxrpc_prepare_data_packet(struct rxrpc_call *call,
>  
>  		_debug("prep[%u] tq=%x q=%x", i, tq->qbase, seq);
>  
> +		if (!__test_and_set_bit(ix, &tq->in_flight))
> +			call->tx_in_flight++;
> +

[Severity: Medium]
Can this count a segment that is already soft-ACKed as in flight again?

The commit message says:

  (4) Clear a packet's bit when an ACK is received for the packet ...

However, the bit is only cleared in rxrpc_input_rack() for segments in
new_acks. rxrpc_input_soft_ack_tq() builds that mask from newly reported
ACKs and NAK->ACK flips:

	new_acks |= n_to_a;
	if (new_acks)
		rxrpc_input_rack(call, summary, tq, new_acks);

One possible sequence:

1. DATA is sent and rxrpc_prepare_data_packet() arms the TLP PTO timer.

2. The peer soft-ACKs call->tx_transmitted but leaves a hole lower down,
   so acks_hard_ack < tx_transmitted. The hole is marked lost at once, so
   no reorder timer replaces the PTO.

3. The PTO fires. rxrpc_tlp_send_probe() passes the
   after_eq(acks_hard_ack, tx_transmitted) check. With no unsent data or
   no window space it calls rxrpc_resend_tlp(), which retransmits
   seq == call->tx_transmitted without checking tq->segment_acked.

4. rxrpc_prepare_data_packet() sets the in_flight bit again and
   increments call->tx_in_flight.

5. Later ACKs keep reporting the segment as ACKed. It is in old_reported
   and does not flip, so it never reaches rxrpc_input_rack(). The bit
   stays set until rxrpc_rotate_tx_window() handles the hard ACK.

Until that hard ACK, tx_in_flight counts a SACKed segment. That value
feeds cong_ssthresh in rxrpc_congestion_management(), the flight_size
test in rxrpc_tlp_calc_pto(), and the tx_in_flight > 0 RTO-rearm check
added by the later commit "rxrpc: Fix RACK-TLP implementation".

rxrpc_resend() doesn't seem to hit this, because rxrpc_rack_update()
clears segment_lost when a segment is soft-ACKed. The old formula also
overcounted here through tx_nr_resent++.

Since the aim of this patch is exact tracking, should the in_flight set
be skipped when segment_acked is already set? Alternatively, should
in_flight be cleared for every segment reported as ACKed, not only the
newly ACKed ones?

>  		/* Record (re-)transmission for RACK [RFC8985 6.1]. */
>  		if (__test_and_clear_bit(ix, &tq->segment_lost))
>  			call->tx_nr_lost--;

[ ... ]

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

Re: [PATCH net v2 2/3] rxrpc: Fix counting of in-flight transmitted packets

[Paolo Abeni]

On 10/22/26 02:00, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 2 · Low: 0 (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] Incomplete in-flight accounting: if a segment that is already
>    soft-ACKed gets retransmitted, its tq->in_flight bit is set again and…
> - [Medium] Behaviour change on soft-ACK reneging (ACK->NAK flip).
Basically sashiko says that the accounting looks still not perfect. Is not
clear to me the new status is good enough.

Side notes: sashiko comments on patch 3/3 are not blockers, but you should
revisit the commit message in case of a repost (and do not include patch 1/3,
postponed to net-next).

/P

Re: [PATCH net v2 2/3] rxrpc: Fix counting of in-flight transmitted packets

[Jakub Kicinski]

On Tue, 27 Oct 2026 10:07:14 +0100 Paolo Abeni wrote:
> On 10/22/26 02:00, netdev-bot+sashiko@kernel.org wrote:
> > Thank you for your contribution! Sashiko AI review found 2 potential
> > issue(s) to consider.
> > 
> > Critical: 0 · High: 0 · Medium: 2 · Low: 0 (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] Incomplete in-flight accounting: if a segment that is already
> >    soft-ACKed gets retransmitted, its tq->in_flight bit is set again and…
> > - [Medium] Behaviour change on soft-ACK reneging (ACK->NAK flip).  
> Basically sashiko says that the accounting looks still not perfect. Is not
> clear to me the new status is good enough.
> 
> Side notes: sashiko comments on patch 3/3 are not blockers, but you should
> revisit the commit message in case of a repost (and do not include patch 1/3,
> postponed to net-next).

David, this posting is almost 9 days old (which is a cut off point for
some of our trackers). Could you please respond?

Re: [PATCH net v2 2/3] rxrpc: Fix counting of in-flight transmitted packets

[Jeffrey E Altman]

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

On 10/28/2026 6:35 PM, Jakub Kicinski wrote:

> On Tue, 27 Oct 2026 10:07:14 +0100 Paolo Abeni wrote:
>> On 10/22/26 02:00,netdev-bot+sashiko@kernel.org wrote:
>>> Thank you for your contribution! Sashiko AI review found 2 potential
>>> issue(s) to consider.
>>>
>>> Critical: 0 · High: 0 · Medium: 2 · Low: 0 (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] Incomplete in-flight accounting: if a segment that is already
>>>     soft-ACKed gets retransmitted, its tq->in_flight bit is set again and…
>>> - [Medium] Behaviour change on soft-ACK reneging (ACK->NAK flip).
>> Basically sashiko says that the accounting looks still not perfect. Is not
>> clear to me the new status is good enough.
>>
>> Side notes: sashiko comments on patch 3/3 are not blockers, but you should
>> revisit the commit message in case of a repost (and do not include patch 1/3,
>> postponed to net-next).
> David, this posting is almost 9 days old (which is a cut off point for
> some of our trackers). Could you please respond?

Jakub,

The sashiko comments on in-flight accounting are still under review.  
There isn't a new patch ready to submit yet.

Thanks.

Jeffrey Altman



[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4506 bytes --]

[PATCH net v2 3/3] 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 |  2 +-
 net/rxrpc/call_event.c  | 13 +++++++++++++
 net/rxrpc/input.c       |  7 ++++---
 net/rxrpc/input_rack.c  |  6 +++---
 4 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h
index d0e18eb6301e..d3b1b095192d 100644
--- a/net/rxrpc/ar-internal.h
+++ b/net/rxrpc/ar-internal.h
@@ -811,6 +811,7 @@ struct rxrpc_call {
 #define RXRPC_TX_SMSS		RXRPC_JUMBO_DATALEN
 #define RXRPC_MIN_CWND		4
 	enum rxrpc_ca_state	cong_ca_state;	/* Congestion control state */
+	bool			cong_in_recovery:1; /* If we're in Fast/RTO recovery */
 	u8			cong_extra;	/* Extra to send for congestion management */
 	u16			cong_cwnd;	/* Congestion window size */
 	u16			cong_ssthresh;	/* Slow-start threshold */
@@ -895,7 +896,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..f83db83df89d 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 &&
+	    call->tx_in_flight > 0) {
+		ktime_t rto = rxrpc_get_rto_backoff(call, false);
+
+		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 f497eba59cf6..815749c7abeb 100644
--- a/net/rxrpc/input.c
+++ b/net/rxrpc/input.c
@@ -34,6 +34,7 @@ static void rxrpc_congestion_management(struct rxrpc_call *call,
 {
 	summary->change = rxrpc_cong_no_change;
 	summary->in_flight = call->tx_in_flight;
+	call->cong_in_recovery = false;
 
 	if (test_and_clear_bit(RXRPC_CALL_RETRANS_TIMEOUT, &call->flags)) {
 		summary->retrans_timeo = true;
@@ -106,12 +107,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;
diff --git a/net/rxrpc/input_rack.c b/net/rxrpc/input_rack.c
index e43ce032a261..1f6b51fa9721 100644
--- a/net/rxrpc/input_rack.c
+++ b/net/rxrpc/input_rack.c
@@ -30,7 +30,6 @@ static void rxrpc_rack_mark_lost(struct rxrpc_call *call,
 
 	if (__test_and_clear_bit(ix, &tq->in_flight))
 		call->tx_in_flight--;
-	tq->segment_xmit_ts[ix] = UINT_MAX;
 }
 
 /*
@@ -184,7 +183,7 @@ static ktime_t rxrpc_rack_update_reo_wnd(struct rxrpc_call *call,
 	}
 
 	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;
@@ -230,6 +229,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);
@@ -300,7 +300,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 <= max(call->peer->pmtud_jumbo, 1))
 			pto = ktime_add(pto, call->tlp_max_ack_delay);
 	} else {
 		pto = NSEC_PER_SEC;

Re: [PATCH net v2 3/3] 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: 1 (2) · Low: 2

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] Item (6) of the commit message describes a change that is not
  in the patch.
- [Low] Item (1) of the commit message says the "in/exiting Fast or RTO
  recovery" flags, plural, move from rxrpc_ack_summary to rxrpc_call so…
- [Low] In rxrpc_congestion_management() (net/rxrpc/input.c), the
  RXRPC_CA_FAST_RETRANSMIT case sets call->cong_in_recovery = true (line…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but
  call->cong_in_recovery is documented as "If we're in Fast/RTO…
- [Medium] This problem wasn't introduced by this patch, but the patch
  rewrites the exact line involved.

--

I found five things worth raising on this patch. Two are about the commit message not matching the diff. One is a small state problem the patch introduces. Two are older bugs that this patch touches but did not cause, and those are labelled as such inline.

rxrpc: Fix RACK-TLP implementation

This commit makes several fixes to the rxrpc RACK-TLP code. The in-recovery
flag moves from rxrpc_ack_summary to rxrpc_call. The RTO timer is now rearmed
in rxrpc_input_call_event() when no other RACK timer is set. TLP init moves
to the transition into FAST_RETRANSMIT, the segment_xmit_ts reset moves into
rxrpc_rack_detect_loss(), and the PTO flight_size check is adjusted.

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

[Severity: Low]
Does this match what the patch does? Only the "in" flag moves, and it
becomes call->cong_in_recovery. The "exiting" flag is still in the per-ACK
summary:

struct rxrpc_ack_summary {
	...
	bool		exiting_fast_or_rto_recovery:1;
	...
};

rxrpc_congestion_management() still sets it for each ACK, and
rxrpc_rack_update_reo_wnd() still reads it as
summary->exiting_fast_or_rto_recovery.

rxrpc_rack_timer_expired() passes a zeroed summary from its stack, so the
exiting flag is always false on the Reorder timer path.

Keeping the exiting flag per ACK looks deliberate, but could the commit
message say that only the "in" flag moves?

>  (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.

[Severity: Medium]
Is this item still accurate? The diff does not touch net/rxrpc/output.c,
and struct rxrpc_call has no tx_nr_resent member in this tree.

The previous commit in the series, "rxrpc: Fix counting of in-flight
transmitted packets", seems to have removed tx_nr_resent and tx_nr_sent. It
replaced them with the per-txqueue in_flight bitmap and call->tx_in_flight.
rxrpc_prepare_data_packet() already does this:

		if (!__test_and_set_bit(ix, &tq->in_flight))
			call->tx_in_flight++;

This patch is Cc'd to stable. Could item (6) be dropped, or reworded to
refer to the previous commit?

> diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h
> index d0e18eb6301e9..d3b1b095192da 100644
> --- a/net/rxrpc/ar-internal.h
> +++ b/net/rxrpc/ar-internal.h
> @@ -811,6 +811,7 @@ struct rxrpc_call {
>  #define RXRPC_TX_SMSS		RXRPC_JUMBO_DATALEN
>  #define RXRPC_MIN_CWND		4
>  	enum rxrpc_ca_state	cong_ca_state;	/* Congestion control state */
> +	bool			cong_in_recovery:1; /* If we're in Fast/RTO recovery */

[Severity: Medium]
This isn't a bug introduced by this patch, but does anything set this flag
during RTO recovery? rxrpc_congestion_management() sets it to true in only
two places: the PACKET_LOSS to FAST_RETRANSMIT transition, and the
RXRPC_CA_FAST_RETRANSMIT case.

The RACK RTO path never touches it:

rxrpc_rack_timer_expired()
    case RXRPC_CALL_RACKTIMER_RTO:
        rxrpc_rack_mark_losses_on_rto(call);

The other RTO hook in rxrpc_congestion_management() looks unreachable.
RXRPC_CALL_RETRANS_TIMEOUT is tested and cleared there but never set
anywhere:

	if (test_and_clear_bit(RXRPC_CALL_RETRANS_TIMEOUT, &call->flags)) {

So after an RTO, rxrpc_rack_update_reo_wnd() uses a non-zero reordering
window. RFC 8985 section 6.2 step 4 uses 0 during RTO recovery. The old
summary->in_fast_or_rto_recovery flag had the same gap.

>  	u8			cong_extra;	/* Extra to send for congestion management */

[ ... ]

> diff --git a/net/rxrpc/input.c b/net/rxrpc/input.c
> index f497eba59cf6e..815749c7abeb8 100644
> --- a/net/rxrpc/input.c
> +++ b/net/rxrpc/input.c
> @@ -34,6 +34,7 @@ static void rxrpc_congestion_management(struct rxrpc_call *call,
>  {
>  	summary->change = rxrpc_cong_no_change;
>  	summary->in_flight = call->tx_in_flight;
> +	call->cong_in_recovery = false;
>  
>  	if (test_and_clear_bit(RXRPC_CALL_RETRANS_TIMEOUT, &call->flags)) {
>  		summary->retrans_timeo = true;
> @@ -106,12 +107,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;

[Severity: Low]
Should cong_in_recovery be cleared when this case leaves recovery? The flag
is set here before the exit check, and the exit path is:

		} else {
			summary->change = rxrpc_cong_progress;
			call->cong_cwnd = call->cong_ssthresh;
			if (call->acks_nr_snacks == 0) {
				summary->exiting_fast_or_rto_recovery = true;
				goto resume_normality;
			}
		}

resume_normality sets cong_ca_state to RXRPC_CA_SLOW_START or
RXRPC_CA_CONGEST_AVOIDANCE, but leaves call->cong_in_recovery set.

The flag now lives on the call. Until the next ACK, it says the call is in
Fast/RTO recovery while cong_ca_state says it is not.

Suppose a Reorder timer armed by an earlier ACK fires in that window:

rxrpc_rack_timer_expired()
  rxrpc_rack_detect_loss_and_arm_timer()
    rxrpc_rack_detect_loss()
      rxrpc_rack_update_reo_wnd()
        if (call->cong_in_recovery)
            return 0;

If rack_reordering_seen is false, call->rack_reo_wnd is set to 0, and
rxrpc_rack_mark_losses_on_rto() later uses that value.

acks_nr_snacks is 0 on this path, so the timer has no NACKed segments to
mark lost. The practical effect looks small.

> +		if (!summary->new_low_snack) {
>  			if (summary->nr_new_sacks == 0)
>  				call->cong_cwnd += 1;

[ ... ]

> diff --git a/net/rxrpc/input_rack.c b/net/rxrpc/input_rack.c
> index e43ce032a2618..1f6b51fa9721f 100644
> --- a/net/rxrpc/input_rack.c
> +++ b/net/rxrpc/input_rack.c

[ ... ]

> @@ -230,6 +229,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;

[Severity: Medium]
This is a pre-existing issue, not one introduced by this patch, but the
patch moves this line. What happens if a segment marked lost here is acked
before it is retransmitted?

rxrpc_get_xmit_ts() turns the sentinel into KTIME_MAX:

	if (tq->segment_xmit_ts[ix] == UINT_MAX)
		return KTIME_MAX;

If the original packet was only reordered, a later ACK can still ack it.
One way is the old NAK to ACK flip, rxrpc_input_soft_ack_tq() ->
rxrpc_input_rack(). Another is the soft NAK to hard ACK case,
rxrpc_rotate_tx_window() -> rxrpc_input_rack_one().

rxrpc_rack_update() then uses KTIME_MAX as if it were a real transmit
time. segment_retransmitted is clear for this segment, so the serial and
min_rtt checks are skipped:

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

rack_rtt ends up hugely negative, and rack_rtt_ts and rack_xmit_ts end up
at KTIME_MAX. Only rxrpc_rack_update() writes these fields, and no later
timestamp can be greater than KTIME_MAX. So the bad values stay for the
rest of the call.

After that, lost_after in rxrpc_rack_detect_loss() is very negative, so
every reported NACK is marked lost at once. The deadline in
rxrpc_rack_mark_losses_on_rto() is also skewed.

Could this cause spurious retransmissions for the rest of the call? No
concurrency seems to be needed. rxrpc_input_call_event() handles several
queued ACKs and Reorder timer expiries in its rx_queue loop before
rxrpc_resend() runs.

>  					trace_rxrpc_rack_detect_loss(call, summary, seq);
>  				} else {
>  					timeout = max(remaining, timeout);

[ ... ]

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