fix: dynamically adjust time threshold on packet reordering#2145
Merged
fix: dynamically adjust time threshold on packet reordering#2145
Conversation
toidiu
commented
Aug 29, 2025
toidiu
commented
Aug 29, 2025
| // the logic. | ||
| #[rstest] | ||
| fn time_thresholds_on_reordering( | ||
| #[values("bbr2_gcongestion")] cc_algorithm_name: &str, |
Contributor
There was a problem hiding this comment.
Was the intent to run this test against multiple congestion algorithms and have alternate assertions for each algorithm?
Contributor
Author
There was a problem hiding this comment.
Yep, there is a TODO comment to add other algos. I spent quite some time trying to fix congestion but the logic is too different and would require a large change. I didn't want to block on that.
This will prob be part of the "lets merge the two modules" work.
Contributor
There was a problem hiding this comment.
The TODO should be more clear about the desire to run this test against other algorithms.
toidiu
force-pushed
the
akothari/dynamic_time_threshold_on_reorder
branch
from
5eabddd to
3d72f21
Compare
1 task
| // | ................ | ..................... | | ||
| // THRESH_GAP THRESH_GAP | ||
| // ``` | ||
| // Initial RTT in millis. |
Contributor
There was a problem hiding this comment.
Comment seems out of place. The next line is THRESH_GAP, not RTT related.
Contributor
|
Looks good. |
Dynamic time threshold logic from #470 was accidentally removed. The goal is to make time-based detection less sensitive when a spurious loss is detected.
Recovery logic was returning the wrong spurious loss count if no new packets were acked. This fixes the bug. I also added a new test for time-based loss detection, specifically testing that the value increases after a spurious loss event.
toidiu
force-pushed
the
akothari/dynamic_time_threshold_on_reorder
branch
from
3d72f21 to
a2484bd
Compare
antoniovicente
approved these changes
Sep 2, 2025
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
2 participants
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.
Dynamic time threshold logic from
#470 was accidentally removed.
The goal is to make time-based detection less sensitive when a spurious loss is detected.
Reviewing
It would be best to review this change as separate commits:
Testing
I also added the same test to the previous version (0.21.0) of code to confirm the behavior.
network simulator testing
I did some simulation testing to measure the effectiveness of this fix for BBRv3 in the gconsgestion branch. Because reordering is hard to control for there is a lot of variability between runs. However, across ~5 runs each the branch with the fix seems more resilient to packet reordering.
Setup
tc based simulator with
htb quantum 1514 rate 20mbit+ queuepfifo limit 10800)netem limit 2000000 delay 52ms 2ms distribution pareto). (To eliminate reordering also applyrate 100gbit)Before fix
After fix
