fix: Handle migrating to a path that is an ECN blackhole #2081
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
larseggert
requested review from
KershawChang,
martinthomson and
mxinden
as code owners
Failed Interop TestsQUIC Interop Runner, client vs. server 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
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2081 +/- ##
==========================================
- Coverage 95.36% 95.33% -0.03%
==========================================
Files 112 112
Lines 36530 36559 +29
==========================================
+ Hits 34836 34853 +17
- Misses 1694 1706 +12 ☔ View full report in Codecov by Sentry. |
Benchmark resultsPerformance differences relative to 6f8823b. coalesce_acked_from_zero 1+1 entries: No change in performance detected. time: [98.759 ns 99.066 ns 99.387 ns]
change: [-0.1599% +0.2674% +0.6775%] (p = 0.24 > 0.05)
coalesce_acked_from_zero 3+1 entries: No change in performance detected. time: [116.60 ns 116.98 ns 117.38 ns]
change: [-0.7533% -0.3342% +0.0824%] (p = 0.13 > 0.05)
coalesce_acked_from_zero 10+1 entries: No change in performance detected. time: [116.23 ns 116.72 ns 117.29 ns]
change: [-0.6390% -0.1545% +0.3398%] (p = 0.54 > 0.05)
coalesce_acked_from_zero 1000+1 entries: No change in performance detected. time: [97.210 ns 97.373 ns 97.560 ns]
change: [-1.7541% -0.4468% +0.8628%] (p = 0.52 > 0.05)
RxStreamOrderer::inbound_frame(): Change within noise threshold. time: [111.71 ms 111.77 ms 111.83 ms]
change: [+0.1545% +0.2250% +0.2987%] (p = 0.00 < 0.05)
transfer/pacing-false/varying-seeds: No change in performance detected. time: [26.640 ms 27.617 ms 28.593 ms]
change: [-2.3511% +2.6598% +7.8897%] (p = 0.31 > 0.05)
transfer/pacing-true/varying-seeds: No change in performance detected. time: [34.837 ms 36.351 ms 37.894 ms]
change: [-7.1497% -1.4254% +4.9791%] (p = 0.65 > 0.05)
transfer/pacing-false/same-seed: Change within noise threshold. time: [31.527 ms 32.281 ms 33.025 ms]
change: [-6.0224% -3.2041% -0.3743%] (p = 0.04 < 0.05)
transfer/pacing-true/same-seed: No change in performance detected. time: [41.827 ms 44.834 ms 47.837 ms]
change: [-13.257% -5.5038% +3.1280%] (p = 0.22 > 0.05)
1-conn/1-100mb-resp (aka. Download)/client: Change within noise threshold. time: [113.45 ms 113.81 ms 114.17 ms]
thrpt: [875.87 MiB/s 878.64 MiB/s 881.46 MiB/s]
change:
time: [-1.5709% -1.1038% -0.6362%] (p = 0.00 < 0.05)
thrpt: [+0.6403% +1.1161% +1.5960%]
1-conn/10_000-parallel-1b-resp (aka. RPS)/client: No change in performance detected. time: [315.68 ms 319.35 ms 322.95 ms]
thrpt: [30.965 Kelem/s 31.314 Kelem/s 31.678 Kelem/s]
change:
time: [-0.7908% +0.9847% +2.7068%] (p = 0.26 > 0.05)
thrpt: [-2.6354% -0.9751% +0.7971%]
1-conn/1-1b-resp (aka. HPS)/client: 💚 Performance has improved. time: [32.520 ms 32.695 ms 32.884 ms]
thrpt: [30.410 elem/s 30.585 elem/s 30.750 elem/s]
change:
time: [-21.047% -19.603% -18.110%] (p = 0.00 < 0.05)
thrpt: [+22.114% +24.383% +26.658%]
Client/server transfer resultsTransfer of 33554432 bytes over loopback.
|
mxinden
approved these changes
Aug 30, 2024
There was a problem hiding this comment.
This looks good to me. Thank you for the patch.
As discussed before, we could as well do ECN validation after connection establishment and path validation. Assuming that ECN blackholes are rare, the only upside would be simpler code. Downside would be delay in ECN signals. I assume ECN signals early on is still worth the additional code complexity, correct?
A couple of minor suggestions below.
Co-authored-by: Max Inden <mail@max-inden.de> Signed-off-by: Lars Eggert <lars@eggert.org>
Co-authored-by: Max Inden <mail@max-inden.de> Signed-off-by: Lars Eggert <lars@eggert.org>
Co-authored-by: Max Inden <mail@max-inden.de> Signed-off-by: Lars Eggert <lars@eggert.org>
Co-authored-by: Max Inden <mail@max-inden.de> Signed-off-by: Lars Eggert <lars@eggert.org>
Co-authored-by: Max Inden <mail@max-inden.de> Signed-off-by: Lars Eggert <lars@eggert.org>
Co-authored-by: Max Inden <mail@max-inden.de> Signed-off-by: Lars Eggert <lars@eggert.org>
Co-authored-by: Max Inden <mail@max-inden.de> Signed-off-by: Lars Eggert <lars@eggert.org>
mxinden
reviewed
Sep 2, 2024
mxinden
approved these changes
Sep 2, 2024
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 will send
MAX_PATH_PROBESprobes with ECN and then anotherMAX_PATH_PROBESprobes without ECN, so withMAX_PATH_PROBES = 3, a total of 6 for a non-functional path. Is that OK?