Fix BBR MinRtt validity checks#6118
Conversation
|
@microsoft-github-policy-service agree |
There was a problem hiding this comment.
Pull request overview
This PR fixes BBR’s handling of an “invalid/uninitialized MinRtt” by removing an incorrect sentinel comparison (MinRtt == UINT32_MAX) and consistently using the existing validity flag (MinRttTimestampValid). This aligns BBR’s control logic with its internal state model so BDP- and pacing-based computations don’t consume MinRtt before any valid RTT sample exists.
Changes:
- Replace incorrect
MinRtt == UINT32_MAXchecks with!MinRttTimestampValidin target cwnd and send allowance calculations. - Prevent transitions to
BBR_STATE_PROBE_RTTunless a valid MinRtt sample has been established. - Update/add BBR unit tests to validate safe fallback behavior when MinRtt is invalid (including ensuring BBR stays in STARTUP instead of entering PROBE_RTT).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/core/bbr.c |
Uses MinRttTimestampValid to gate target cwnd, pacing send allowance, and ProbeRTT entry to avoid consuming an invalid MinRtt sentinel. |
src/core/unittest/BbrTest.cpp |
Updates the send allowance test expectation and adds coverage verifying target cwnd fallback + no premature ProbeRTT when MinRtt is invalid. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6118 +/- ##
==========================================
- Coverage 86.15% 85.55% -0.61%
==========================================
Files 60 60
Lines 18857 18911 +54
==========================================
- Hits 16247 16179 -68
- Misses 2610 2732 +122 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
guhetier
left a comment
There was a problem hiding this comment.
Thanks for the PR! LGTM, minor nitpick to clarify a comment.
Co-authored-by: Guillaume Hetier <hetier.guillaume@gmail.com>
|
Suggestion patched. Ready to merge :) |
Summary
Fix BBR's invalid MinRtt handling to consistently use the existing
MinRttTimestampValidstate instead of relying on an incorrect sentinel comparison.Motivation
BBR's model depends on two independent path estimates:
BtlBw)RTprop, represented here byMinRtt)The BBR paper describes
RTpropas a windowed minimum RTT estimate. BBR should only use that estimate after a valid RTT sample has established it. Until then, calculations based on BDP (BtlBw * RTprop) should use startup-safe fallback behavior instead of consuming an uninitialized sentinel value.In this implementation,
MinRttis initialized toUINT64_MAX, while validity is separately tracked byMinRttTimestampValid. However, some code paths checkedMinRtt == UINT32_MAXto decide whether the estimate was unavailable. SinceUINT64_MAX != UINT32_MAX, those paths could incorrectly treat an invalidMinRttas valid.This matters because an invalid RTprop estimate can affect:
ProbeRTT is meant to refresh an existing RTprop estimate after it expires. If no valid MinRtt sample has ever existed, entering ProbeRTT is not useful; BBR should remain in startup/model-building behavior until a valid RTT sample exists.
Change
This change replaces invalid MinRtt checks with
!Bbr->MinRttTimestampValid.That makes the code match the actual state model:
MinRttTimestampValid == FALSE: no valid MinRtt/RTprop sample exists yetMinRttTimestampValid == TRUE:MinRttcan safely participate in BDP, pacing, and ProbeRTT decisionsSpecifically, the change ensures:
Tests
Updated and added BBR unit coverage: