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

[netdev-bot+sashiko@kernel.org]

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