Add a new inflight_hi_limited_in_round to QUIC BBRv2 to replace the more complex logic about when to raise inflight_hi there is today. This is more in line with BBRv2's intended inflight_hi growth, but avoids a number of problems around being app-limited or aggregation. Activated by the BBHI connection option. Protected by gfe2_reloadable_quic_bbr2_simplify_inflight_hi. PiperOrigin-RevId: 490084354
diff --git a/quiche/quic/core/congestion_control/bbr2_misc.cc b/quiche/quic/core/congestion_control/bbr2_misc.cc index 6cc2852..f85c602 100644 --- a/quiche/quic/core/congestion_control/bbr2_misc.cc +++ b/quiche/quic/core/congestion_control/bbr2_misc.cc
@@ -81,6 +81,9 @@ if (bytes_in_flight < min_bytes_in_flight_in_round_) { min_bytes_in_flight_in_round_ = bytes_in_flight; } + if (bytes_in_flight + bytes >= inflight_hi_) { + inflight_hi_limited_in_round_ = true; + } round_trip_counter_.OnPacketSent(packet_number); bandwidth_sampler_.OnPacketSent(sent_time, packet_number, bytes, @@ -383,6 +386,7 @@ loss_events_in_round_ = 0; max_bytes_delivered_in_round_ = 0; min_bytes_in_flight_in_round_ = std::numeric_limits<uint64_t>::max(); + inflight_hi_limited_in_round_ = false; } void Bbr2NetworkModel::cap_inflight_lo(QuicByteCount cap) {
diff --git a/quiche/quic/core/congestion_control/bbr2_misc.h b/quiche/quic/core/congestion_control/bbr2_misc.h index 39d4747..579a7b5 100644 --- a/quiche/quic/core/congestion_control/bbr2_misc.h +++ b/quiche/quic/core/congestion_control/bbr2_misc.h
@@ -157,6 +157,7 @@ bool probe_up_includes_acks_after_cwnd_limited = false; bool probe_up_dont_exit_if_no_queue_ = false; bool probe_up_ignore_inflight_hi = true; + bool probe_up_simplify_inflight_hi = false; /* * PROBE_RTT parameters. @@ -489,6 +490,10 @@ return min_bytes_in_flight_in_round_; } + bool inflight_hi_limited_in_round() const { + return inflight_hi_limited_in_round_; + } + QuicPacketNumber end_of_app_limited_phase() const { return bandwidth_sampler_.end_of_app_limited_phase(); } @@ -563,6 +568,9 @@ QuicByteCount min_bytes_in_flight_in_round_ = std::numeric_limits<uint64_t>::max(); + // True if sending was limited by inflight_hi anytime in the current round. + bool inflight_hi_limited_in_round_ = false; + // Max bandwidth in the current round. Updated once per congestion event. QuicBandwidth bandwidth_latest_ = QuicBandwidth::Zero(); // Max bandwidth of recent rounds. Updated once per round.
diff --git a/quiche/quic/core/congestion_control/bbr2_probe_bw.cc b/quiche/quic/core/congestion_control/bbr2_probe_bw.cc index 88e87b6..9b9ab3c 100644 --- a/quiche/quic/core/congestion_control/bbr2_probe_bw.cc +++ b/quiche/quic/core/congestion_control/bbr2_probe_bw.cc
@@ -354,20 +354,52 @@ // the number of bytes delivered in a round is larger inflight_hi. return; } - if (Params().probe_bw_check_cwnd_limited_before_aggregation_epoch) { - if (!model_->cwnd_limited_before_aggregation_epoch()) { - QUIC_DVLOG(3) << sender_ - << " Raising inflight_hi early return: Not cwnd limited " - "before aggregation epoch."; - // Not fully utilizing cwnd, so can't safely grow. + if (Params().probe_up_simplify_inflight_hi) { + // Raise inflight_hi exponentially if it was utilized this round. + cycle_.probe_up_acked += congestion_event.bytes_acked; + if (!congestion_event.end_of_round_trip) { return; } - } else if (Params().probe_up_includes_acks_after_cwnd_limited) { - // Don't continue adding bytes to probe_up_acked if the sender was not - // app-limited after being inflight_hi limited at least once. - if (!cycle_.probe_up_app_limited_since_inflight_hi_limited_ || - congestion_event.last_packet_send_state.is_app_limited) { - cycle_.probe_up_app_limited_since_inflight_hi_limited_ = false; + if (!model_->inflight_hi_limited_in_round() || + model_->loss_events_in_round() > 0) { + cycle_.probe_up_acked = 0; + return; + } + } else { + if (Params().probe_bw_check_cwnd_limited_before_aggregation_epoch) { + if (!model_->cwnd_limited_before_aggregation_epoch()) { + QUIC_DVLOG(3) << sender_ + << " Raising inflight_hi early return: Not cwnd limited " + "before aggregation epoch."; + // Not fully utilizing cwnd, so can't safely grow. + return; + } + } else if (Params().probe_up_includes_acks_after_cwnd_limited) { + // Don't continue adding bytes to probe_up_acked if the sender was not + // app-limited after being inflight_hi limited at least once. + if (!cycle_.probe_up_app_limited_since_inflight_hi_limited_ || + congestion_event.last_packet_send_state.is_app_limited) { + cycle_.probe_up_app_limited_since_inflight_hi_limited_ = false; + if (congestion_event.prior_bytes_in_flight < + congestion_event.prior_cwnd) { + QUIC_DVLOG(3) + << sender_ + << " Raising inflight_hi early return: Not cwnd limited."; + // Not fully utilizing cwnd, so can't safely grow. + return; + } + + if (congestion_event.prior_cwnd < model_->inflight_hi()) { + QUIC_DVLOG(3) << sender_ + << " Raising inflight_hi early return: inflight_hi not " + "fully used."; + // Not fully using inflight_hi, so don't grow it. + return; + } + } + // Start a new period of adding bytes_acked, because inflight_hi limited. + cycle_.probe_up_app_limited_since_inflight_hi_limited_ = true; + } else { if (congestion_event.prior_bytes_in_flight < congestion_event.prior_cwnd) { QUIC_DVLOG(3) << sender_ @@ -375,36 +407,21 @@ // Not fully utilizing cwnd, so can't safely grow. return; } - - if (congestion_event.prior_cwnd < model_->inflight_hi()) { - QUIC_DVLOG(3) - << sender_ - << " Raising inflight_hi early return: inflight_hi not fully used."; - // Not fully using inflight_hi, so don't grow it. - return; - } } - // Start a new period of adding bytes_acked, because inflight_hi limited. - cycle_.probe_up_app_limited_since_inflight_hi_limited_ = true; - } else { - if (congestion_event.prior_bytes_in_flight < congestion_event.prior_cwnd) { - QUIC_DVLOG(3) << sender_ - << " Raising inflight_hi early return: Not cwnd limited."; - // Not fully utilizing cwnd, so can't safely grow. + + if (congestion_event.prior_cwnd < model_->inflight_hi()) { + QUIC_DVLOG(3) + << sender_ + << " Raising inflight_hi early return: inflight_hi not fully used."; + // Not fully using inflight_hi, so don't grow it. return; } + + // Increase inflight_hi by the number of probe_up_bytes within + // probe_up_acked. + cycle_.probe_up_acked += congestion_event.bytes_acked; } - if (congestion_event.prior_cwnd < model_->inflight_hi()) { - QUIC_DVLOG(3) - << sender_ - << " Raising inflight_hi early return: inflight_hi not fully used."; - // Not fully using inflight_hi, so don't grow it. - return; - } - - // Increase inflight_hi by the number of probe_up_bytes within probe_up_acked. - cycle_.probe_up_acked += congestion_event.bytes_acked; if (cycle_.probe_up_acked >= cycle_.probe_up_bytes) { uint64_t delta = cycle_.probe_up_acked / cycle_.probe_up_bytes; cycle_.probe_up_acked -= delta * cycle_.probe_up_bytes;
diff --git a/quiche/quic/core/congestion_control/bbr2_sender.cc b/quiche/quic/core/congestion_control/bbr2_sender.cc index 33fe2e6..86e00ed 100644 --- a/quiche/quic/core/congestion_control/bbr2_sender.cc +++ b/quiche/quic/core/congestion_control/bbr2_sender.cc
@@ -203,6 +203,14 @@ // Derived constant to ensure fairness. params_.probe_bw_probe_down_pacing_gain = 0.91; } + if (GetQuicReloadableFlag(quic_bbr2_simplify_inflight_hi) && + ContainsQuicTag(connection_options, kBBHI)) { + QUIC_RELOADABLE_FLAG_COUNT(quic_bbr2_simplify_inflight_hi); + params_.probe_up_simplify_inflight_hi = true; + // Simplify inflight_hi is intended as an alternative to ignoring it, + // so ensure we're not ignoring it. + params_.probe_up_ignore_inflight_hi = false; + } } Limits<QuicByteCount> Bbr2Sender::GetCwndLimitsByMode() const {
diff --git a/quiche/quic/core/congestion_control/bbr2_simulator_test.cc b/quiche/quic/core/congestion_control/bbr2_simulator_test.cc index d1cf602..08237bb 100644 --- a/quiche/quic/core/congestion_control/bbr2_simulator_test.cc +++ b/quiche/quic/core/congestion_control/bbr2_simulator_test.cc
@@ -989,6 +989,120 @@ sender_->ExportDebugState().bandwidth_hi, 0.91f); } +// Test Bbr2's reaction to a 100x bandwidth increase during a transfer with BBHI +TEST_F(Bbr2DefaultTopologyTest, QUIC_SLOW_TEST(BandwidthIncreaseBBHI)) { + SetQuicReloadableFlag(quic_bbr2_simplify_inflight_hi, true); + SetConnectionOption(kBBHI); + DefaultTopologyParams params; + params.local_link.bandwidth = QuicBandwidth::FromKBitsPerSecond(15000); + params.test_link.bandwidth = QuicBandwidth::FromKBitsPerSecond(100); + CreateNetwork(params); + + sender_endpoint_.AddBytesToTransfer(10 * 1024 * 1024); + + simulator_.RunFor(QuicTime::Delta::FromSeconds(15)); + EXPECT_TRUE(Bbr2ModeIsOneOf({Bbr2Mode::PROBE_BW, Bbr2Mode::PROBE_RTT})); + QUIC_LOG(INFO) << "Bandwidth increasing at time " << SimulatedNow(); + + EXPECT_APPROX_EQ(params.test_link.bandwidth, + sender_->ExportDebugState().bandwidth_est, 0.1f); + EXPECT_LE(sender_loss_rate_in_packets(), 0.30); + + // Now increase the bottleneck bandwidth from 100Kbps to 10Mbps. + params.test_link.bandwidth = QuicBandwidth::FromKBitsPerSecond(10000); + TestLink()->set_bandwidth(params.test_link.bandwidth); + + bool simulator_result = simulator_.RunUntilOrTimeout( + [this]() { return sender_endpoint_.bytes_to_transfer() == 0; }, + QuicTime::Delta::FromSeconds(50)); + EXPECT_TRUE(simulator_result); + // Ensure the full bandwidth is discovered. + EXPECT_APPROX_EQ(params.test_link.bandwidth, + sender_->ExportDebugState().bandwidth_hi, 0.02f); +} + +// Test Bbr2's reaction to a 100x bandwidth increase during a transfer with BBHI +// in the presence of ACK aggregation. +TEST_F(Bbr2DefaultTopologyTest, + QUIC_SLOW_TEST(BandwidthIncreaseBBHIAggregation)) { + SetQuicReloadableFlag(quic_bbr2_simplify_inflight_hi, true); + SetConnectionOption(kBBHI); + DefaultTopologyParams params; + params.local_link.bandwidth = QuicBandwidth::FromKBitsPerSecond(15000); + params.test_link.bandwidth = QuicBandwidth::FromKBitsPerSecond(100); + CreateNetwork(params); + + // 2 RTTs of aggregation, with a max of 10kb. + EnableAggregation(10 * 1024, 2 * params.RTT()); + + // Reduce the payload to 2MB because 10MB takes too long. + sender_endpoint_.AddBytesToTransfer(2 * 1024 * 1024); + + simulator_.RunFor(QuicTime::Delta::FromSeconds(15)); + EXPECT_TRUE(Bbr2ModeIsOneOf({Bbr2Mode::PROBE_BW, Bbr2Mode::PROBE_RTT})); + QUIC_LOG(INFO) << "Bandwidth increasing at time " << SimulatedNow(); + + // This is much farther off when aggregation is present, + // Ideally BSAO or another option would fix this. + EXPECT_APPROX_EQ(params.test_link.bandwidth, + sender_->ExportDebugState().bandwidth_est, 0.60f); + EXPECT_LE(sender_loss_rate_in_packets(), 0.35); + + // Now increase the bottleneck bandwidth from 100Kbps to 10Mbps. + params.test_link.bandwidth = QuicBandwidth::FromKBitsPerSecond(10000); + TestLink()->set_bandwidth(params.test_link.bandwidth); + + bool simulator_result = simulator_.RunUntilOrTimeout( + [this]() { return sender_endpoint_.bytes_to_transfer() == 0; }, + QuicTime::Delta::FromSeconds(50)); + EXPECT_TRUE(simulator_result); + // Ensure the full bandwidth is discovered. + EXPECT_APPROX_EQ(params.test_link.bandwidth, + sender_->ExportDebugState().bandwidth_hi, 0.90f); +} + +// Test Bbr2's reaction to a 100x bandwidth increase during a transfer with BBHI +// and B202, which changes the exit criteria to be based on +// min_bytes_in_flight_in_round, in the presence of ACK aggregation. +TEST_F(Bbr2DefaultTopologyTest, + QUIC_SLOW_TEST(BandwidthIncreaseBBHI_B202Aggregation)) { + SetQuicReloadableFlag(quic_bbr2_simplify_inflight_hi, true); + SetConnectionOption(kBBHI); + SetConnectionOption(kB202); + DefaultTopologyParams params; + params.local_link.bandwidth = QuicBandwidth::FromKBitsPerSecond(15000); + params.test_link.bandwidth = QuicBandwidth::FromKBitsPerSecond(100); + CreateNetwork(params); + + // 2 RTTs of aggregation, with a max of 10kb. + EnableAggregation(10 * 1024, 2 * params.RTT()); + + // Reduce the payload to 2MB because 10MB takes too long. + sender_endpoint_.AddBytesToTransfer(2 * 1024 * 1024); + + simulator_.RunFor(QuicTime::Delta::FromSeconds(15)); + EXPECT_TRUE(Bbr2ModeIsOneOf({Bbr2Mode::PROBE_BW, Bbr2Mode::PROBE_RTT})); + QUIC_LOG(INFO) << "Bandwidth increasing at time " << SimulatedNow(); + + // This is much farther off when aggregation is present, + // Ideally BSAO or another option would fix this. + EXPECT_APPROX_EQ(params.test_link.bandwidth, + sender_->ExportDebugState().bandwidth_est, 0.60f); + EXPECT_LE(sender_loss_rate_in_packets(), 0.35); + + // Now increase the bottleneck bandwidth from 100Kbps to 10Mbps. + params.test_link.bandwidth = QuicBandwidth::FromKBitsPerSecond(10000); + TestLink()->set_bandwidth(params.test_link.bandwidth); + + bool simulator_result = simulator_.RunUntilOrTimeout( + [this]() { return sender_endpoint_.bytes_to_transfer() == 0; }, + QuicTime::Delta::FromSeconds(50)); + EXPECT_TRUE(simulator_result); + // Ensure at least 18% of the bandwidth is discovered. + EXPECT_APPROX_EQ(params.test_link.bandwidth, + sender_->ExportDebugState().bandwidth_hi, 0.85f); +} + // Test Bbr2's reaction to a 100x bandwidth increase during a transfer with B204 TEST_F(Bbr2DefaultTopologyTest, QUIC_SLOW_TEST(BandwidthIncreaseB204)) { SetConnectionOption(kB204);
diff --git a/quiche/quic/core/crypto/crypto_protocol.h b/quiche/quic/core/crypto/crypto_protocol.h index d72183c..bc9556d 100644 --- a/quiche/quic/core/crypto/crypto_protocol.h +++ b/quiche/quic/core/crypto/crypto_protocol.h
@@ -127,6 +127,9 @@ const QuicTag kBBQ0 = TAG('B', 'B', 'Q', '0'); // Increase bytes_acked in // PROBE_UP when app limited. const QuicTag kBBPD = TAG('B', 'B', 'P', 'D'); // Use 0.91 PROBE_DOWN gain. +const QuicTag kBBHI = TAG('B', 'B', 'H', 'I'); // Increase inflight_hi in + // PROBE_UP if ever inflight_hi + // limited in round const QuicTag kRENO = TAG('R', 'E', 'N', 'O'); // Reno Congestion Control const QuicTag kTPCC = TAG('P', 'C', 'C', '\0'); // Performance-Oriented // Congestion Control
diff --git a/quiche/quic/core/quic_flags_list.h b/quiche/quic/core/quic_flags_list.h index 7f855cb..d88f628 100644 --- a/quiche/quic/core/quic_flags_list.h +++ b/quiche/quic/core/quic_flags_list.h
@@ -95,6 +95,8 @@ QUIC_FLAG(quic_reloadable_flag_quic_default_to_bbr, false) // When true, support draft-ietf-quic-v2-01 QUIC_FLAG(quic_reloadable_flag_quic_enable_version_2_draft_01, false) +// When true, the BBHI copt causes QUIC BBRv2 to use a simpler algorithm for raising inflight_hi in PROBE_UP. +QUIC_FLAG(quic_reloadable_flag_quic_bbr2_simplify_inflight_hi, true) // When true, the BBR4 copt sets the extra_acked window to 20 RTTs and BBR5 sets it to 40 RTTs. QUIC_FLAG(quic_reloadable_flag_quic_bbr2_extra_acked_window, true)