tcp: implement the RFC 4015 response to spurious loss recovery - #14458
tcp: implement the RFC 4015 response to spurious loss recovery#14458davidbell217 wants to merge 1 commit into
Conversation
be53c9a to
190e04d
Compare
kerumeto
left a comment
kerumeto
left a comment
There was a problem hiding this comment.
Thanks for the nice description and clean code. Just a few small comments to address from side.
Thanks for catching the missing Timestamps check!
| } | ||
| // Outstanding can be transiently negative while an ACK for data sent | ||
| // before an RTO is being processed; FlightSize is never negative. | ||
| s.SndCwnd = max(s.Outstanding, 0) + InitialCwnd |
There was a problem hiding this comment.
Is it possible to get the number of bytes ACKED by this segment so we can follow
min (bytes_acked, IW)?
Otherwise, InitialCwnd should be sufficiently small
There was a problem hiding this comment.
Done. undoSpuriousRecovery now takes the packet count the exit ACK removed from flight (the same originalOutstanding - s.Outstanding that cc.Update consumes, so it excludes segments already SACKed) and sets cwnd = FlightSize + min(acked, IW). The tests now assert the exact cwnd: 4 on the RTO exit and 2 on the SACK exit (two of the four covered packets were already SACKed). Both fail with the flat IW form.
| // | ||
| // +checklocks:s.ep.mu | ||
| func (s *sender) capturePipePrev() { | ||
| // Like RetransmitTS (RFC3522 Section 3.2 step 2), the capture must not |
There was a problem hiding this comment.
Maybe we can just directly mention RFC4015 Section 3.1 here:
"The algorithm MUST NOT be reinitiated after a timeout-based loss recovery has already been started but not completed"
There was a problem hiding this comment.
Done, quoted Section 3.1 directly, with a note that netstack extends the rule to fast recovery since its detection covers that too.
| func (s *sender) undoSpuriousRecovery() { | ||
| if s.pipePrev == 0 { | ||
| return | ||
| } |
There was a problem hiding this comment.
The RFC states:
"If the acceptable ACK has the ECN-Echo flag [[RFC3168] set"
Then we should proceed to do this recovery. Although we do not currently support ECN (#995), I think its worth noting here that we should skip reversing the congestion window if the flag is set. This will help in tracking missing functionality.
There was a problem hiding this comment.
Added a TODO(gvisor.dev/issue/995) at the reversal. I left it as a note rather than a live TCPFlagEce check: netstack does not negotiate ECN, and RFC 3168 says a non-ECN connection must ignore ECE, so acting on the flag today would be its own deviation. Happy to make it a real check instead if you would rather have it in place now.
190e04d to
88cc357
Compare
RFC 3522 Eifel detection sets sender.spuriousRecovery and the SpuriousRecovery metrics, but nothing consumed the signal: leaveRecovery unconditionally deflated SndCwnd to Ssthresh, so a recovery the sender itself proved spurious still cost a permanent multiplicative decrease, and under sustained path jitter the sender ratchets its throughput down episode after episode. Capture pipe_prev (RFC 4015 Section 3 step (0)) at every loss-recovery entry point before the congestion controller reduces Ssthresh, and on exit from a recovery detected spurious restore Ssthresh to it and slow-start back from FlightSize + min(bytes_acked, IW) (step (9)) instead of keeping the deflation. The response is applied at the single point every recovery exits through — the Open transition in handleRcvdSegment — after the ACK-removal loop, when Outstanding is the true FlightSize, so the send it allows is bounded by IW. The capture is consumed on use, so the response applies at most once per episode. detectSpuriousRecovery now ignores ACKs carrying no Timestamps option: a TSEcr of zero previously compared as smaller than RetransmitTS and marked a genuine recovery spurious, which was harmless while the flag only fed a metric but wrong once a response consumes it. Extend the two spurious-recovery e2e tests to drive the connection out of recovery on each exit path and assert Ssthresh/SndCwnd are restored; both extensions fail without the fix. Fixes google#14102 Assisted-by: Claude Code
88cc357 to
1af4ae8
Compare
2 participants
Fixes #14102 (cc @nybidari).
Implements the RFC 4015 congestion control response to spurious recovery, per the notes on the issue:
pipe_previs captured at all five recovery entry points (detectLoss, RACK reorder timer, TLP loss, RTO expiry) beforeHandleLossDetected()/HandleRTOExpired()cutSsthresh— atenterRecovery()it's already too late.SndCwndstands in for FlightSize (both packet-counted;Outstandingis already decimated by the ACKs that trigger detection). A never-reducedSsthresh(InitialSsthresh) is not restored: it holds no pipe estimate, and restoring it re-created the collapse on our fixture.Ssthresh = pipe_prev(only if that raises it) andSndCwnd = FlightSize + min(bytes_acked, IW). cwnd is not restored directly — a direct restore burst-collapsed 24/24 runs, as warned in the issue.leaveRecovery, pure RTO directly) ends at theOpentransition inhandleRcvdSegment, so the response applies exactly there — after the ACK-removal loop, whereOutstandingis the true FlightSize and the allowed send is bounded by IW. The capture is consumed on use, so it applies at most once per episode.sender; nothing added toTCPSenderState.TestDetectSpuriousRecoveryWithRTOandTestSACKDetectSpuriousRecoveryWithDupACKnow drive the connection out of recovery and assertSsthresh/SndCwndare restored. Both fail without the fix (Ssthresh = 2, want = 10, one per exit path).One enabling detection fix:
detectSpuriousRecoverynow ignores ACKs with no Timestamps option (!SendTSOk || TSEcr == 0). PreviouslyTSEcr = 0compared as< RetransmitTSand marked a genuine recovery spurious — harmless while only a metric consumed the flag, butTestSACKRecovery(whose crafted ACKs carry no TS option) fails once a response does.Benchmark, on the fixture from #14102 (195 ms RTT, ±15 ms jitter, 3% of packets delayed +30 ms, no loss; 24 runs/arm; #14092's fix applied in both arms; pinned = run exceeds 15 s):
Episodes still fire either way; the response stops them compounding. A clean fixture (3% reorder, no jitter) shows no regression.
Scope: RFC 4015 specifies the response for spurious timeouts; applying it to fast/SACK recovery too matches netstack's detection, which deliberately doesn't differentiate. Step (8) (
SND.NXT <- SND.MAX), steps (10)–(11) (RTT/RTO restoration), and the ECE guard (netstack has no ECN congestion response) are out of scope.