If bytes in flight has dipped below 1.25*MaxBW in the last round, do not exit PROBE_UP due to excess queue buildup. Also exits PROBE_UP even if app limited if queueing or excess loss is detected. Activated by the B202 connection option. Protected by quic_reloadable_flag_quic_bbr2_no_probe_up_exit_if_no_queue. PiperOrigin-RevId: 399536464
diff --git a/quic/core/congestion_control/bbr2_misc.cc b/quic/core/congestion_control/bbr2_misc.cc index 8c9059e..b67b7df 100644 --- a/quic/core/congestion_control/bbr2_misc.cc +++ b/quic/core/congestion_control/bbr2_misc.cc
@@ -164,6 +164,10 @@ congestion_event->last_packet_send_state.total_bytes_acked; max_bytes_delivered_in_round_ = std::max(max_bytes_delivered_in_round_, bytes_delivered); + if (min_bytes_in_flight_in_round_ == 0 || + congestion_event->bytes_in_flight < min_bytes_in_flight_in_round_) { + min_bytes_in_flight_in_round_ = congestion_event->bytes_in_flight; + } } // |bandwidth_latest_| and |inflight_latest_| only increased within a round. @@ -376,6 +380,7 @@ bytes_lost_in_round_ = 0; loss_events_in_round_ = 0; max_bytes_delivered_in_round_ = 0; + min_bytes_in_flight_in_round_ = 0; } void Bbr2NetworkModel::cap_inflight_lo(QuicByteCount cap) {
diff --git a/quic/core/congestion_control/bbr2_misc.h b/quic/core/congestion_control/bbr2_misc.h index 0161a6a..5df19d5 100644 --- a/quic/core/congestion_control/bbr2_misc.h +++ b/quic/core/congestion_control/bbr2_misc.h
@@ -146,6 +146,7 @@ * PROBE_UP parameters. */ bool probe_up_includes_acks_after_cwnd_limited = false; + bool probe_up_dont_exit_if_no_queue_ = false; /* * PROBE_RTT parameters. @@ -474,6 +475,10 @@ return max_bytes_delivered_in_round_; } + QuicByteCount min_bytes_in_flight_in_round() const { + return min_bytes_in_flight_in_round_; + } + QuicPacketNumber end_of_app_limited_phase() const { return bandwidth_sampler_.end_of_app_limited_phase(); } @@ -544,6 +549,9 @@ // congestion event) was sent and acked, respectively. QuicByteCount max_bytes_delivered_in_round_ = 0; + // The minimum bytes in flight during this round. + QuicByteCount min_bytes_in_flight_in_round_ = 0; + // 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/quic/core/congestion_control/bbr2_probe_bw.cc b/quic/core/congestion_control/bbr2_probe_bw.cc index 83c2585..1c1963f 100644 --- a/quic/core/congestion_control/bbr2_probe_bw.cc +++ b/quic/core/congestion_control/bbr2_probe_bw.cc
@@ -36,8 +36,7 @@ } Bbr2Mode Bbr2ProbeBwMode::OnCongestionEvent( - QuicByteCount prior_in_flight, - QuicTime event_time, + QuicByteCount prior_in_flight, QuicTime event_time, const AckedPacketVector& /*acked_packets*/, const LostPacketVector& /*lost_packets*/, const Bbr2CongestionEvent& congestion_event) { @@ -187,12 +186,20 @@ << congestion_event.last_packet_send_state.total_bytes_acked << ")"; } } + // TODO(ianswett): Inflight too high is really checking for loss, not + // inflight. if (model_->IsInflightTooHigh(congestion_event, Params().probe_bw_full_loss_count)) { if (cycle_.is_sample_from_probing) { cycle_.is_sample_from_probing = false; - - if (!send_state.is_app_limited) { + if (!send_state.is_app_limited || + Params().probe_up_dont_exit_if_no_queue_) { + if (send_state.is_app_limited) { + // If there's excess loss or a queue is building, exit even if the + // last sample was app limited. + QUIC_RELOADABLE_FLAG_COUNT_N(quic_bbr2_no_probe_up_exit_if_no_queue, + 2, 2); + } const QuicByteCount inflight_target = sender_->GetTargetBytesInflight() * (1.0 - Params().beta); if (inflight_at_send >= inflight_target) { @@ -469,23 +476,32 @@ } else if (cycle_.rounds_in_phase > 0) { const QuicByteCount bdp = model_->BDP(); QuicByteCount queuing_threshold_extra_bytes = 2 * kDefaultTCPMSS; - if (Params().add_ack_height_to_queueing_threshold) { - queuing_threshold_extra_bytes += model_->MaxAckHeight(); + if (Params().probe_up_dont_exit_if_no_queue_) { + QUIC_RELOADABLE_FLAG_COUNT_N(quic_bbr2_no_probe_up_exit_if_no_queue, 1, + 2); + is_queuing = congestion_event.end_of_round_trip && + model_->min_bytes_in_flight_in_round() > + (bdp * Params().probe_bw_probe_inflight_gain + + queuing_threshold_extra_bytes); + } else { + if (Params().add_ack_height_to_queueing_threshold) { + queuing_threshold_extra_bytes += model_->MaxAckHeight(); + } + QuicByteCount queuing_threshold = + (Params().probe_bw_probe_inflight_gain * bdp) + + queuing_threshold_extra_bytes; + + is_queuing = congestion_event.bytes_in_flight >= queuing_threshold; + + QUIC_DVLOG(3) << sender_ + << " Checking if building up a queue. prior_in_flight:" + << prior_in_flight + << ", post_in_flight:" << congestion_event.bytes_in_flight + << ", threshold:" << queuing_threshold + << ", is_queuing:" << is_queuing + << ", max_bw:" << model_->MaxBandwidth() + << ", min_rtt:" << model_->MinRtt(); } - QuicByteCount queuing_threshold = - (Params().probe_bw_probe_inflight_gain * bdp) + - queuing_threshold_extra_bytes; - - is_queuing = congestion_event.bytes_in_flight >= queuing_threshold; - - QUIC_DVLOG(3) << sender_ - << " Checking if building up a queue. prior_in_flight:" - << prior_in_flight - << ", post_in_flight:" << congestion_event.bytes_in_flight - << ", threshold:" << queuing_threshold - << ", is_queuing:" << is_queuing - << ", max_bw:" << model_->MaxBandwidth() - << ", min_rtt:" << model_->MinRtt(); } if (is_risky || is_queuing) { @@ -495,8 +511,7 @@ } void Bbr2ProbeBwMode::EnterProbeDown(bool probed_too_high, - bool stopped_risky_probe, - QuicTime now) { + bool stopped_risky_probe, QuicTime now) { QUIC_DVLOG(2) << sender_ << " Phase change: " << cycle_.phase << " ==> " << CyclePhase::PROBE_DOWN << " after " << now - cycle_.phase_start_time << ", or " @@ -637,9 +652,7 @@ return os; } -const Bbr2Params& Bbr2ProbeBwMode::Params() const { - return sender_->Params(); -} +const Bbr2Params& Bbr2ProbeBwMode::Params() const { return sender_->Params(); } float Bbr2ProbeBwMode::PacingGainForPhase( Bbr2ProbeBwMode::CyclePhase phase) const {
diff --git a/quic/core/congestion_control/bbr2_sender.cc b/quic/core/congestion_control/bbr2_sender.cc index d811681..582451e 100644 --- a/quic/core/congestion_control/bbr2_sender.cc +++ b/quic/core/congestion_control/bbr2_sender.cc
@@ -175,6 +175,10 @@ ContainsQuicTag(connection_options, kBBQ0)) { params_.probe_up_includes_acks_after_cwnd_limited = true; } + if (GetQuicReloadableFlag(quic_bbr2_no_probe_up_exit_if_no_queue) && + ContainsQuicTag(connection_options, kB202)) { + params_.probe_up_dont_exit_if_no_queue_ = true; + } } Limits<QuicByteCount> Bbr2Sender::GetCwndLimitsByMode() const {
diff --git a/quic/core/congestion_control/bbr2_simulator_test.cc b/quic/core/congestion_control/bbr2_simulator_test.cc index c913a10..074e56b 100644 --- a/quic/core/congestion_control/bbr2_simulator_test.cc +++ b/quic/core/congestion_control/bbr2_simulator_test.cc
@@ -684,7 +684,7 @@ sender_->ExportDebugState().bandwidth_est, 0.6f); EXPECT_LE(sender_loss_rate_in_packets(), 0.35); - // Now increase the bottleneck bandwidth from 100Kbps to 1Mbps. + // Now increase the bottleneck bandwidth from 100Kbps to 10Mbps. params.test_link.bandwidth = QuicBandwidth::FromKBitsPerSecond(10000); TestLink()->set_bandwidth(params.test_link.bandwidth); @@ -692,11 +692,83 @@ [this]() { return sender_endpoint_.bytes_to_transfer() == 0; }, QuicTime::Delta::FromSeconds(50)); EXPECT_TRUE(simulator_result); - // Ensure at least 15% of full bandwidth is discovered. + // Ensure at least 10% of 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 B202 +TEST_F(Bbr2DefaultTopologyTest, QUIC_SLOW_TEST(BandwidthIncreaseB202)) { + SetQuicReloadableFlag(quic_bbr2_no_probe_up_exit_if_no_queue, true); + SetConnectionOption(kB202); + 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.1f); +} + +// Test Bbr2's reaction to a 100x bandwidth increase during a transfer with B202 +// in the presence of ACK aggregation. +TEST_F(Bbr2DefaultTopologyTest, + QUIC_SLOW_TEST(BandwidthIncreaseB202Aggregation)) { + SetQuicReloadableFlag(quic_bbr2_no_probe_up_exit_if_no_queue, true); + 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.45f); + 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 10% of full bandwidth is discovered. + EXPECT_APPROX_EQ(params.test_link.bandwidth, + sender_->ExportDebugState().bandwidth_hi, 0.92f); +} + // Test the number of losses incurred by the startup phase in a situation when // the buffer is less than BDP. TEST_F(Bbr2DefaultTopologyTest, PacketLossOnSmallBufferStartup) {
diff --git a/quic/core/crypto/crypto_protocol.h b/quic/core/crypto/crypto_protocol.h index 4522dfd..2eb697a 100644 --- a/quic/core/crypto/crypto_protocol.h +++ b/quic/core/crypto/crypto_protocol.h
@@ -159,6 +159,8 @@ const QuicTag kB201 = TAG('B', '2', '0', '1'); // In PROBE_UP, check if cwnd // limited before aggregation // epoch, instead of ack event. +const QuicTag kB202 = TAG('B', '2', '0', '2'); // Do not exit PROBE_UP if + // inflight dips below 1.25*BW. const QuicTag kNTLP = TAG('N', 'T', 'L', 'P'); // No tail loss probe const QuicTag k1TLP = TAG('1', 'T', 'L', 'P'); // 1 tail loss probe const QuicTag k1RTO = TAG('1', 'R', 'T', 'O'); // Send 1 packet upon RTO
diff --git a/quic/core/quic_flags_list.h b/quic/core/quic_flags_list.h index 8c750b5..5641006 100644 --- a/quic/core/quic_flags_list.h +++ b/quic/core/quic_flags_list.h
@@ -15,6 +15,8 @@ QUIC_FLAG(FLAGS_quic_restart_flag_quic_testonly_default_false, false) // A testonly restart flag that will always default to true. QUIC_FLAG(FLAGS_quic_restart_flag_quic_testonly_default_true, true) +// If bytes in flight has dipped below 1.25*MaxBW in the last round, do not exit PROBE_UP due to excess queue buildup. +QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_bbr2_no_probe_up_exit_if_no_queue, true) // If true and connection option B201 is used, check if cwnd limited before aggregation epoch, instead of ack event, in PROBE_UP phase. QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_bbr2_check_cwnd_limited_before_aggregation_epoch, true) // If true, GFE will explicitly configure its signature algorithm preference.