Fixing RTO timer during SACK loss recovery
AbandonedPublic
Actions

Authored by rscheff on Jan 14 2020, 12:00 AM.

Details

Reviewers

rgrimes
cc
tuexen
rrs
gnn

Group Reviewers

transport

Summary

Since its inception, a partial ack (where the right edge of continous
data moves forwards) would clear the RTO timer. Thus if some sack
retransmission other than the initial segment retransmitted is lost,
the session would then be at the mercy of some other - longer waiting -
timer to make forward progress. The very first retransmission is typically the
most prone to encounter a still filled up queue. But that retransmissions is
covered by the still running RTO timer from the in-sequence phase at that point.

It is counter-intuitive to disable the RTO timer completely, once the very
first of possibly many dozends of packets that need to be retransmitted, makes
it to the receiver. It is more logical to restart the RTO timer when some
additional in-sequence data is acked.

Short of lost retransmission detection, this will help with a more timely
loss recovery, when SACK retransmissions still encounter a congested
queue

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 28647
Build 26674: arc lint + arc unit

Event Timeline

rscheff created this revision.Jan 14 2020, 12:00 AM

Herald added a reviewer: transport. · View Herald TranscriptJan 14 2020, 12:00 AM

Harbormaster completed remote builds in B28647: Diff 66710.Jan 14 2020, 12:00 AM

Just found, that RFC2582, last paragraph of section 4 mentions the "slow-but-steady" reset (re-arm) of the RTO after each partial ack in the context of SACK:

one possibility for a more optimal
algorithm might be one that recovered more quickly from multiple
packet drops, and combined this with the Slow-but-Steady variant in
terms of resetting the retransmit timers.  We note, however, that
there is a limitation to the potential performance in this case in
the absence of the SACK option.

Note this was found in FBSD11.

Head seems to send out one rescue retransmission already (before explicitly implemented with 6675 support?), restarting the RTO timer again.

Maybe MFC this?

Tested in both bsd11 and bsd12. Without this fix, I can find a 2nd retransmission triggered by the persistent timer, not the RTO timer. But this means the persistent timer is still alive. Suggest revise this change.

Looks the persistent timer may be still alive. Suggest revise this change.

This revision now requires changes to proceed.Jan 27 2020, 9:18 PM

Merge branch 'master' into fix-sack-rto
ensure that persist and retransmit timers are not running in parallel

Harbormaster completed remote builds in B28956: Diff 67370.Jan 27 2020, 9:30 PM

and also resetting the timeout backoff counter

Harbormaster completed remote builds in B28957: Diff 67371.Jan 27 2020, 9:35 PM

I suspect this snippet here is not working as intended:

tcp_output.c-1516-timer:
tcp_output.c-1517-              if (!tcp_timer_active(tp, TT_REXMT) &&
tcp_output.c-1518-                  ((sack_rxmit && tp->snd_nxt != tp->snd_max) ||
tcp_output.c-1519-                   (tp->snd_nxt != tp->snd_una))) {
tcp_output.c-1520-                      if (tcp_timer_active(tp, TT_PERSIST)) {
tcp_output.c-1521-                              tcp_timer_activate(tp, TT_PERSIST, 0);
tcp_output.c-1522-                              tp->t_rxtshift = 0;
tcp_output.c-1523-                      }
tcp_output.c:1524:                      tcp_timer_activate(tp, TT_REXMT, tp->t_rxtcur);

Because during SACK retransmissions, snd_nxt is not used by tcp_output to figure out which data to send next - unlike NewReno. So, most likely, this

(sack_rxmit && tp->snd_nxt != tp->snd_max)

never evaluates true...

Turned out that this change is unnecessary, when SACK loss recovery works properly.

Herald added a subscriber: melifaro. · View Herald TranscriptSep 25 2020, 2:38 PM

Revision Contents
Changeset List

Path

Size

sys/

netinet/

tcp_sack.c

2 lines

Diff 66710

View Options

Fixing RTO timer during SACK loss recoveryAbandonedPublicActions