Repository navigation
Conversation
…recovery The cumulative-ACK removal loop in handleRcvdSegment skips the Outstanding decrement for segments that were previously SACKed, on the premise that SetPipe() already accounted for them. That premise only holds during loss recovery: SetPipe() returns immediately when recovery is not active, and nothing else removes those packets. A segment SACKed outside recovery (benign reordering) and then cumulatively ACKed therefore leaks its packet count into Outstanding permanently. On a long-lived connection over a reordering-but-lossless path the phantoms accumulate until the sender believes it is cwnd-limited while almost nothing is in flight, throttling bulk transfers (observed: a wedged sender with Outstanding=86, cwnd=86 and ~5 packets actually in flight, and a completed transfer running at ~8% of its believed window). Track packets newly SACKed while no recovery is in progress in a sender-local counter, and consume it when those segments are cumulatively ACKed. The counter resets wherever Outstanding is recomputed or reset wholesale (recovery entry, RTO, full window drain), so recovery-path accounting is unchanged. Two simpler shapes were measured and rejected: - decrementing Outstanding unconditionally in the removal loop's SACKed branch double-subtracts segments SetPipe() already accounted for (SACKed during recovery, cumulatively ACKed at/after exit) and breaks TestSACKRecovery's post-recovery pacing; - decrementing at SACK arrival (walkSACK) frees congestion window slots before the cumulative ACK, effectively unbounded limited transmit, and breaks TestRACKWithWindowFull with a transmission after a zero-window ACK. The walk that collects the segments a new SACK block may cover skips segments that end at or before the block's start: inserting the block cannot change whether those are covered. A partially overlapping segment is still checked, because adjacent blocks can merge. After a retransmission timeout Outstanding is zero and the scoreboard is empty, so the peer's SACKs for segments sent before the timeout look new although those segments are no longer counted. The walk therefore stops at writeNext, the first segment not sent since the timeout. Without that, TestSACKAfterTimeoutLeavesOutstanding sees Outstanding fall one packet short of what is in flight. The test disables RACK, which would start a recovery on that SACK and recompute Outstanding. The original regression test now runs on a manual clock. On a real clock RACK may declare the segment below the SACKed one lost, depending on how far apart the two were sent, and the recovery that follows hides the leak. Fixes google#14092 Co-authored-by: Tamir Duberstein <tamird@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is #14101 by @davidbell217, rebased onto master with David kept as author. David asked me to open it here.
Outside loss recovery nothing takes SACKed packets out of
Outstanding, so each leaves a phantom packet in flight, and under reordering the sender believes its window is full.Three changes from #14101. The walk for a new SACK block skips segments that end at or before it (tamird's review). It stops at
writeNext: after a timeout the peer's SACKs look new again, and those segments were subtracted although the timeout had zeroedOutstanding(new test). The original test runs on a manual clock: its RACK case passed without the fix in 8 of 30 runs.With RACK on and 25% of data packets held 15 µs, a 20 MB fetch took 2.51 s on master and 0.404 s with this (medians of 16, results).
Fixes #14092