Re: [PATCH net 2/2] 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: 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