Page MenuHomeFreeBSD

tcp: do not purge SACK scoreboard on first RTO
ClosedPublic

Authored by rscheff on Dec 5 2023, 3:15 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Feb 10, 9:40 PM
Unknown Object (File)
Sat, Feb 3, 8:19 AM
Unknown Object (File)
Jan 20 2024, 1:43 AM
Unknown Object (File)
Jan 20 2024, 1:39 AM
Unknown Object (File)
Jan 11 2024, 9:51 AM
Unknown Object (File)
Jan 6 2024, 8:05 PM
Unknown Object (File)
Jan 1 2024, 6:35 PM
Unknown Object (File)
Dec 19 2023, 8:18 PM
Subscribers

Details

Summary

The errate to RFC2018 have an important change - on
RTO, the SACK data SHOULD be purged, not MUST be purged.

Keeping the SACK scoreboard intact after the first RTO,
and discarding all data only on subsequent RTOs allows
a more timely and efficient loss recovery under many
cirumstances, where the RTO was due to an unrecoverable
lost retransmissions (LRD requires additional data to be
sent and SACKed, and a sufficiently large cwnd for this
to happen).

MFC after: 10 weeks

Test Plan

after some pathological losses, induce an RTO and check that
previously SACKed data is not retransmitted again.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

This revision is now accepted and ready to land.Dec 7 2023, 12:28 PM
  • perform clean SACK retransmissions after RTO
This revision now requires review to proceed.Dec 8 2023, 12:01 AM

In order for clean SACK LR after the RTO, we don't want to exit recovery, and we also need to clear the number of retransmitted sack bytes, for the cwnd calculation to work. Without these, the stack would oscillate between SACK and non-SACK retransmissions (snd_una), resulting in many duplicate sent packets.

There are two incidental edits here (missed one magic number and replaced with the proper enum, and a whitespace change).

Note that this approach is a bit brittle in my testing (with a pathological loss pattern of 3 lost - 1 delivered - 3 lost ..., as after the RTO, every packet is clocked out by a preceeding ACK (cwnd remains at 1, since I'm not resetting any of the prr variables, which could allow some cwnd growth during loss recovery).

Maybe split the editorial changes in a separate commit...

sys/netinet/tcp_sack.c
924

Nit: no double space before p->start.

This revision is now accepted and ready to land.Jan 5 2024, 2:21 PM
sys/netinet/siftr.c
521 ↗(On Diff #131159)

Strongly suggest not adding this file in this change for tracking purposes. It is so benign that can wait.

cc requested changes to this revision.Jan 5 2024, 3:42 PM
This revision now requires changes to proceed.Jan 5 2024, 3:42 PM
  • remove editorial change in siftr.c; keep change of magic number to enum for SACK
  • split off editorial changes to MFC independently
This revision was not accepted when it landed; it landed in state Needs Review.Jan 6 2024, 7:44 PM
This revision was automatically updated to reflect the committed changes.