Repository navigation
Conversation
| // have been decimated by the ACKs that triggered the detection. An | ||
| // Ssthresh that was never reduced (InitialSsthresh) holds no pipe | ||
| // estimate to restore and is skipped. | ||
| s.pipePrev = s.SndCwnd |
There was a problem hiding this comment.
Could timeout recovery use the number of packets in flight here? In the packet test, an earlier timeout set the slow-start threshold to 2. ACKs then grew the congestion window to 10. Only two packets were in flight when the next timeout fired. Once the sender detected that timeout was spurious, this assignment restored the threshold to 10. RFC 4015 §3.1, step 0 gives max(2, 2) = 2.
Changing the timeout capture to use the outstanding count kept the threshold at 2. The control that echoed the retransmission’s timestamp also stayed at 2 on both versions. Could you add this application-limited case to the timeout tests? Packet results
Posted by Codex on behalf of @tamird.
There was a problem hiding this comment.
Thanks, yes. A timeout now captures the packets in flight (Outstanding) as FlightSize. ACK-driven detection (fast retransmit, RACK, TLP) still captures cwnd, as those ACKs may already have cut Outstanding.
I added your packet test as TestEifelFiniteApplicationFlight, minus the logging, with you as co-author. It fails on the previous head (threshold 10, want 2) and passes now. One change: with #17023 (step 8) a spurious timeout doesn't resend the second packet, so only repair_ack requires that resend.
This also moves the existing RTO test, which times out 5 packets with a window of 10: ssthresh is now restored to 5, not 10. RFC 4015 would restore the initial, unlimited ssthresh there, but this PR has always skipped that value. Happy to follow the RFC there too.
All 11 TCP packages pass with go test, alone and stacked with #17023, #16851 and #16258, and Bazel (nogo included) passes on Linux.
There was a problem hiding this comment.
Yes, I’d restore InitialSsthresh in the timeout case. RFC 4015 §3.1 step (0) saves max(FlightSize, ssthresh), so a false alarm with five packets in flight should preserve the initial threshold. Step (9) bounds the immediate send separately through cwnd. Could you update the existing five-packet RTO assertion? The finite-threshold two-packet case now matches the trace.
Posted by Codex on behalf of @tamird.
There was a problem hiding this comment.
Done. A timeout now captures max(FlightSize, ssthresh) as is, so a spurious timeout restores an untouched InitialSsthresh, and the five-packet RTO test now expects that. Step (9) still sets cwnd, so that test still expects 4. The ACK-driven paths are back to #14458's code unchanged, so they still skip an untouched InitialSsthresh; only the timeout has its own capture. I also merged the two doc comments on TestEifelFiniteApplicationFlight.
The five-packet test fails on the previous head (threshold 5, want InitialSsthresh) and passes now, with your two-packet case still at 2. All 11 TCP packages pass with go test, alone and stacked with #17023, #16851 and #16258, and Bazel (nogo included) passes on Linux.
963e83a to
d0439e8
Compare
|
Thanks @adios-404 for carrying this forward and @tamird for the fix and test. Happy for this PR to supersede #14458. I'll close the original so review can continue here. |
c02d094 to
7e3d55e
Compare
|
Two more changes, so the RFC 4015 response can't reverse a loss response that must stand. With #16851, which keeps the detection state through recovery, the response to a loss found after Eifel has flagged the recovery as spurious would be undone when recovery ends. A loss found by a tail loss probe could be undone too: The two new tests fail without these changes (the first needs #16851), and removing any part fails its own case. All 11 TCP packages and Bazel pass, alone and stacked with #17023, #16851 and #16258. |
|
One more interaction, with #16081, which keeps timeout recovery from entering RACK recovery. With both applied, a loss that RACK finds after Eifel has flagged the timeout as spurious gets no response at all: #16081 skips the RACK entry, so the pending response is never cancelled, and the end of recovery restores ssthresh (unlimited in my test, want 2). In a local stack, cancelling the pending response wherever RACK finds a loss inside a recovery already flagged as spurious, whether or not it enters recovery, fixes it. The "rack" case of |
The four-way packet replay now covers the revised google#17022 source, both alone and with google#16851. Keep the previous replies in history and send reviewers to the current results.
tamird
left a comment
tamird
left a comment
There was a problem hiding this comment.
@adios-404, please fold the RACK case into this PR. With #16081 applied, I reproduced ssthresh going back to unlimited at the end of recovery. Clearing the pending Eifel response when RACK marks the packet lost kept ssthresh at 2, both on this PR alone and with #16081 applied.
I’d keep the check that Eifel detected one spurious timeout and the final ssthresh assertion. The intermediate recovery state legitimately differs between the two stacks. The original-timestamp and retransmission-timestamp controls passed on both. Tested change · Packet test results
— Codex, on behalf of @tamird
7e3d55e to
62277fb
Compare
|
Folded in your change as is: RACK now cancels the pending response where it marks the packet lost, and the "rack" case checks only the spurious-timeout count and the final ssthresh. The commit message says why. Each cancel guards its own case. With #16081, removing the RACK one leaves ssthresh unlimited in "rack" (want 2); with #16851, removing the one in the capture functions does the same in "timeout". All 11 TCP packages pass alone, merged with master, with #16081, and with #16081 and #16851; Bazel passes on Linux. One more interaction: with #17023 as well, #16081's timeout-recovery guard blocks the fast retransmit that #17023 allows after a spurious timeout, so two #17023 tests and the "rack" case wait for another timeout. Letting that guard through once step (8) has run fixes all three; I'll take that to #17023. |
62277fb to
f38afb6
Compare
|
Rebased onto master now that #16851 has merged. The only conflict was in the timeout path, where |
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. A timeout captures RFC 4015's max(FlightSize, ssthresh) as is, with the packets in flight as FlightSize: no ACK triggered the timeout, so none has just taken packets out of that count, and an application-limited sender can have far fewer packets in flight than cwnd. A spurious timeout thus restores an untouched, unlimited ssthresh. Loss detected from ACKs uses cwnd as FlightSize instead and skips an untouched ssthresh, as the sender is cwnd-limited then and those ACKs may already have taken packets out of the count. detectSpuriousRecovery also ignores ACKs without a usable timestamp echo. Since google#16601 its caller checks this too; the guard keeps the response safe if another caller is added. Eifel cannot judge a loss found after it has flagged the recovery as spurious, so treat that loss as genuine: cancel the pending response, so that the end of recovery does not reverse the loss's reduction. RACK cancels it where it marks the packet lost, before either caller decides whether to enter fast recovery, so the cancellation holds even when the sender stays in timeout recovery (google#16081). Since google#16851, recovery keeps the detection state, as RFC 3522 requires. A loss confirmed by a tail loss probe has nothing to restore either: since google#16596 its response reduces the window without entering recovery, so nothing is captured for it and no recovery exit can reverse 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. TestEifelFiniteApplicationFlight times out an application-limited flight of two packets after an earlier timeout left ssthresh at 2: the response must restore ssthresh to 2, not to cwnd. TestEifelGenuineLossAfterSpuriousDetection follows a spurious timeout with a genuine loss, found by a second timeout or by RACK, and checks that its reduction survives the end of recovery; both cases fail without the cancellations, and with google#16081 applied, the RACK case fails without RACK's. TestTailLossProbeResponseIsKept repairs a lost tail with a probe after an earlier DSACK and checks that its reduction is kept; since google#16596 it passes without this change too. Fixes google#14102 Co-authored-by: Tamir Duberstein <tamird@gmail.com>
f38afb6 to
5f4e4e2
Compare
|
Rebased onto master now that #16596 has merged. The only conflict was in On Linux (Bazel), all 27 TCP targets pass, alone and with #16081 or #16258. Both extended spurious-recovery tests still fail without the fix, and each case of
Related: #17665 fixes a false spurious verdict that this response would act on. |
This is #14458 by @davidbell217, rebased onto master with David kept as author; besides review changes, only tcp_sack_test.go changed, to fit #16601's table-driven test. kerumeto approved #14458; David has since closed it in favour of this PR.
When Eifel detection finds a recovery was spurious, the sender restores ssthresh and slow-starts back toward it (RFC 4015 step 9). Detection needs timestamps.
With RACK off and 1% of data packets held 200 µs, a 20 MB fetch took 1.33 s on master and 0.268 s with this (medians of 16 runs, results). Retransmissions rose from 95.5 to 156.5, with no drops or timeouts. In juxuanu's model of a lossy Wi-Fi path, a similar undo gained about 6% (tailscale/tailscale#18307).
This is rebased onto #16851 (detector fixes), now merged. #16258 stops other connections' D-SACKs from triggering this; TCP tests pass with it.
Fixes #14102