fix: Avoid spurious PMTUD resets #2293
Open
Conversation
This file contains 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
Previously, after PMTUD had completed, we could end up restarting PMTUD when packet loss counters for packets larger than the current PMTU exceeded the limit. We're now making sure to not do that.
larseggert
requested review from
KershawChang,
martinthomson and
mxinden
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2293 +/- ##
=======================================
Coverage 93.33% 93.34%
=======================================
Files 114 114
Lines 36896 36943 +47
Branches 36896 36943 +47
=======================================
+ Hits 34438 34484 +46
- Misses 1675 1676 +1
Partials 783 783 ☔ View full report in Codecov by Sentry. |
Failed Interop TestsQUIC Interop Runner, client vs. server, differences relative to bb45c74. neqo-latest as client
neqo-latest as server
All resultsSucceeded Interop TestsQUIC Interop Runner, client vs. server neqo-latest as client
neqo-latest as server
Unsupported Interop TestsQUIC Interop Runner, client vs. server neqo-latest as client
neqo-latest as server
|
larseggert
changed the title
Comment on lines
+542
to
+543
| "Outbound interface {name} for destination {} has MTU {mtu}", | ||
| remote.ip() |
There was a problem hiding this comment.
Suggested change
| "Outbound interface {name} for destination {} has MTU {mtu}", | |
| remote.ip() | |
| "Outbound interface {name} for destination {ip} has MTU {mtu}", | |
| ip = remote.ip() |
| Probe::Needed | Probe::Sent => { | ||
| // We saw multiple losses of packets > the current MTU during PMTU discovery, so | ||
| // we're done. | ||
| if largest_ok_mtu > self.mtu { |
There was a problem hiding this comment.
You mentioned that the reason for this change was that we were resetting PMTUD too often, but this will stop a probe when it looks like we should be setting it higher.
I'm not following this. The logic here is that we've detected losses, but they are on larger packets than we are currently sending. That is, we are probing and have seem to have three packet sizes, decreasing in size:
- A large MTU, where we have too many losses.
- One step lower in size, where we have fewer than
MAX_PROBESof losses. - The current MTU.
However, the current MTU is going to be the same as this second value. That is, when we are probing, we'll have self.mtu == largest_ok_mtu when the probed size. The probed size is only ever one step larger than the current MTU. That means that this test will always fail.
Is this extra condition is even needed? If we ever get MAX_PROBES losses, I can think of the following cases:
- Those losses correspond to probes (at one step up from
largest_ok_mtu) in which case you want to fail the probe and settle at the current MTU. - Those losses are just losses. In which case you might want to drop the MTU down, but maybe not.
- You probed at a larger MTU in the past and these packets are just now being marked as lost.
I can see a case for a >= here, perhaps. I don't understand the guard otherwise. As it is, it looks like we'll never stop probing due to losses.
| // Two packets of size 4000 were lost, which should increase loss counts >= 4000 by two. | ||
| let expected_lc = search_table_inc(&pmtud, &expected_lc, 4000); | ||
| let expected_lc = search_table_inc(&pmtud, &expected_lc, 4000); | ||
| // Two packets of size 4000 were lost, which should increase loss counts >= 4000 by one. |
There was a problem hiding this comment.
Suggested change
| // Two packets of size 4000 were lost, which should increase loss counts >= 4000 by one. | |
| // Two packets of size 4000 were lost, which should increase loss counts >= 4000 by two. |
| // by one. There have now been MAX_PROBES losses of packets >= 8191, so the PMTUD process | ||
| // should have restarted. | ||
| // by one. There have now been MAX_PROBES losses of packets >= 8191, but that is larger than | ||
| // the current MTU, so nothing will happen. | ||
| pmtud.on_packets_lost(&[make_sentpacket(0, now, 9000)], &mut stats, now); |
There was a problem hiding this comment.
Isn't this pure contrivance? We don't probe in this odd pattern.
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.
Previously, after PMTUD had completed, we could
end up restarting PMTUD when packet loss counters
for packets larger than the current PMTU exceeded
the limit. We're now making sure to not do that.