Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce black hole detection false positives #1858

Merged
merged 7 commits into from May 17, 2024
Merged

Reduce black hole detection false positives #1858

merged 7 commits into from May 17, 2024

Conversation

Ralith
Copy link
Collaborator

@Ralith Ralith commented May 10, 2024

Fixes #1855.

TODO:

  • Document/justify the new design
  • Handle out-of-order acknowledgements
  • Test new behavior

@Ralith Ralith force-pushed the better-black-holes branch 2 times, most recently from 30f1c05 to 3d5b28d Compare May 11, 2024 19:49
@Ralith Ralith marked this pull request as ready for review May 11, 2024 19:49
@Ralith
Copy link
Collaborator Author

Ralith commented May 11, 2024

Redesigned per chat to be robust against two-pass ACK procssing, and a bit simpler than the original plan besides. Still need to add tests and some high-level overview docs, but I think I'm happy with the shape of the code.

@Ralith Ralith force-pushed the better-black-holes branch 3 times, most recently from 4764764 to 6bfa3c4 Compare May 11, 2024 20:04
Copy link
Collaborator

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initial feedback. Not sure yet I fully grok the last commit, see also my comment in the issue.

quinn-proto/src/connection/paths.rs Outdated Show resolved Hide resolved
quinn-proto/src/connection/mtud.rs Outdated Show resolved Hide resolved
quinn-proto/src/connection/mtud.rs Outdated Show resolved Hide resolved
quinn-proto/src/connection/mtud.rs Outdated Show resolved Hide resolved
@Ralith Ralith force-pushed the better-black-holes branch 3 times, most recently from eb4e619 to 36c732a Compare May 13, 2024 04:05
@Ralith
Copy link
Collaborator Author

Ralith commented May 13, 2024

Addressed feedback and revised documentation. Test coverage of the new capabilities still TODO.

@Ralith
Copy link
Collaborator Author

Ralith commented May 14, 2024

Added some tests; feel free to suggest any interesting cases I may have missed.

quinn-proto/src/connection/mtud.rs Outdated Show resolved Hide resolved
quinn-proto/src/connection/mtud.rs Outdated Show resolved Hide resolved
quinn-proto/src/connection/mtud.rs Outdated Show resolved Hide resolved
quinn-proto/src/connection/mtud.rs Outdated Show resolved Hide resolved
quinn-proto/src/connection/mtud.rs Outdated Show resolved Hide resolved
@Ralith Ralith force-pushed the better-black-holes branch 2 times, most recently from c68f0c9 to cfcf5a8 Compare May 15, 2024 17:08
In `last_burst_was_suspicious`, if the last burst contains
non-suspicious packets, we bail out early. If it doesn't,
`largest_suspicious_packet_lost` is necessarily equal to
`largest_non_probe_lost`. No other code reads this field, so it is
redundant to `largest_non_probe_lost`.
Copy link
Collaborator

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I think I'm close to reaching the end of this iterative feedback stream...

quinn-proto/src/connection/mtud.rs Outdated Show resolved Hide resolved
quinn-proto/src/connection/mtud.rs Show resolved Hide resolved
@djc
Copy link
Collaborator

djc commented May 16, 2024

(Maybe add a quinn-proto version bump here? Between #1857, #1859 and this it feels like a release might be good.)

@Ralith
Copy link
Collaborator Author

Ralith commented May 16, 2024

Sorry, I think I'm close to reaching the end of this iterative feedback stream...

No worries, this isn't urgent and I'm as happy as ever to get good feedback!

Exactly MTU-sized datagrams aren't necessarily transmitted often, if
ever. As a result, black hole detection could previously reset the MTU
regularly at any nonzero packet loss rate.
@Ralith Ralith merged commit 9c2c553 into main May 17, 2024
8 checks passed
@Ralith Ralith deleted the better-black-holes branch May 17, 2024 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Black hole detection false-positives
2 participants