Internal QUICHE change PiperOrigin-RevId: 296522161 Change-Id: I078e53c1ad6b63b052fc1f0993ebfd5097aa69dd
diff --git a/quic/core/congestion_control/bandwidth_sampler.cc b/quic/core/congestion_control/bandwidth_sampler.cc index 6272664..1a56e62 100644 --- a/quic/core/congestion_control/bandwidth_sampler.cc +++ b/quic/core/congestion_control/bandwidth_sampler.cc
@@ -32,18 +32,19 @@ // Reset the current aggregation epoch as soon as the ack arrival rate is less // than or equal to the max bandwidth. if (aggregation_epoch_bytes_ <= - GetQuicFlag(FLAGS_quic_ack_aggregation_bandwidth_threshold) * - expected_bytes_acked) { + ack_aggregation_bandwidth_threshold_ * expected_bytes_acked) { QUIC_DVLOG(3) << "Starting a new aggregation epoch because " "aggregation_epoch_bytes_ " << aggregation_epoch_bytes_ << " is smaller than expected. " - "quic_ack_aggregation_bandwidth_threshold:" - << GetQuicFlag(FLAGS_quic_ack_aggregation_bandwidth_threshold) + "ack_aggregation_bandwidth_threshold_:" + << ack_aggregation_bandwidth_threshold_ << ", expected_bytes_acked:" << expected_bytes_acked << ", bandwidth_estimate:" << bandwidth_estimate << ", aggregation_duration:" - << (ack_time - aggregation_epoch_start_time_); + << (ack_time - aggregation_epoch_start_time_) + << ", new_aggregation_epoch:" << ack_time + << ", new_aggregation_bytes_acked:" << bytes_acked; // Reset to start measuring a new aggregation epoch. aggregation_epoch_bytes_ = bytes_acked; aggregation_epoch_start_time_ = ack_time; @@ -82,7 +83,8 @@ max_tracked_packets_(GetQuicFlag(FLAGS_quic_max_tracked_packet_count)), unacked_packet_map_(unacked_packet_map), max_ack_height_tracker_(max_height_tracker_window_length), - total_bytes_acked_after_last_ack_event_(0) { + total_bytes_acked_after_last_ack_event_(0), + overestimate_avoidance_(false) { if (remove_packets_once_per_congestion_event_) { QUIC_RELOADABLE_FLAG_COUNT( quic_bw_sampler_remove_packets_once_per_congestion_event2); @@ -92,6 +94,17 @@ } } +void BandwidthSampler::EnableOverestimateAvoidance() { + if (overestimate_avoidance_) { + return; + } + + overestimate_avoidance_ = true; + // TODO(wub): Change the default value of + // --quic_ack_aggregation_bandwidth_threshold to 2.0. + max_ack_height_tracker_.SetAckAggregationBandwidthThreshold(2.0); +} + BandwidthSampler::~BandwidthSampler() {} void BandwidthSampler::OnPacketSent( @@ -116,6 +129,12 @@ // importantly at the beginning of the connection. if (bytes_in_flight == 0) { last_acked_packet_ack_time_ = sent_time; + if (overestimate_avoidance_) { + recent_ack_points_.Clear(); + recent_ack_points_.Update(sent_time, total_bytes_acked_); + a0_candidates_.clear(); + a0_candidates_.push_back(recent_ack_points_.MostRecentPoint()); + } total_bytes_sent_at_last_acked_packet_ = total_bytes_sent_; // In this situation ack compression is not a concern, set send rate to @@ -240,9 +259,17 @@ } total_bytes_acked_after_last_ack_event_ = total_bytes_acked_; - return max_ack_height_tracker_.Update(bandwidth_estimate, round_trip_count, - last_acked_packet_ack_time_, - newly_acked_bytes); + QuicByteCount extra_acked = max_ack_height_tracker_.Update( + bandwidth_estimate, round_trip_count, last_acked_packet_ack_time_, + newly_acked_bytes); + // If |extra_acked| is zero, i.e. this ack event marks the start of a new ack + // aggregation epoch, save LessRecentPoint, which is the last ack point of the + // previous epoch, as a A0 candidate. + if (overestimate_avoidance_ && extra_acked == 0) { + a0_candidates_.push_back(recent_ack_points_.LessRecentPoint()); + QUIC_DVLOG(1) << "New a0_candidate:" << a0_candidates_.back(); + } + return extra_acked; } BandwidthSample BandwidthSampler::OnPacketAcknowledged( @@ -271,6 +298,9 @@ sent_packet.send_time_state.total_bytes_sent; last_acked_packet_sent_time_ = sent_packet.sent_time; last_acked_packet_ack_time_ = ack_time; + if (overestimate_avoidance_) { + recent_ack_points_.Update(ack_time, total_bytes_acked_); + } // Exit app-limited phase once a packet that was sent while the connection is // not app-limited is acknowledged. @@ -296,13 +326,22 @@ sent_packet.sent_time - sent_packet.last_acked_packet_sent_time); } + AckPoint a0; + if (overestimate_avoidance_ && + ChooseA0Point(sent_packet.send_time_state.total_bytes_acked, &a0)) { + QUIC_DVLOG(2) << "Using a0 point: " << a0; + } else { + a0.ack_time = sent_packet.last_acked_packet_ack_time, + a0.total_bytes_acked = sent_packet.send_time_state.total_bytes_acked; + } + // During the slope calculation, ensure that ack time of the current packet is // always larger than the time of the previous packet, otherwise division by // zero or integer underflow can occur. - if (ack_time <= sent_packet.last_acked_packet_ack_time) { + if (ack_time <= a0.ack_time) { // TODO(wub): Compare this code count before and after fixing clock jitter // issue. - if (sent_packet.last_acked_packet_ack_time == sent_packet.sent_time) { + if (a0.ack_time == sent_packet.sent_time) { // This is the 1st packet after quiescense. QUIC_CODE_COUNT_N(quic_prev_ack_time_larger_than_current_ack_time, 1, 2); } else { @@ -310,14 +349,13 @@ } QUIC_LOG_EVERY_N_SEC(ERROR, 60) << "Time of the previously acked packet:" - << sent_packet.last_acked_packet_ack_time.ToDebuggingValue() + << a0.ack_time.ToDebuggingValue() << " is larger than the ack time of the current packet:" << ack_time.ToDebuggingValue(); return BandwidthSample(); } QuicBandwidth ack_rate = QuicBandwidth::FromBytesAndTimeDelta( - total_bytes_acked_ - sent_packet.send_time_state.total_bytes_acked, - ack_time - sent_packet.last_acked_packet_ack_time); + total_bytes_acked_ - a0.total_bytes_acked, ack_time - a0.ack_time); BandwidthSample sample; sample.bandwidth = std::min(send_rate, ack_rate); @@ -329,6 +367,35 @@ return sample; } +bool BandwidthSampler::ChooseA0Point(QuicByteCount total_bytes_acked, + AckPoint* a0) { + if (a0_candidates_.empty()) { + QUIC_BUG << "No A0 point candicates. total_bytes_acked:" + << total_bytes_acked; + return false; + } + + if (a0_candidates_.size() == 1) { + *a0 = a0_candidates_.front(); + return true; + } + + for (size_t i = 1; i < a0_candidates_.size(); ++i) { + if (a0_candidates_[i].total_bytes_acked > total_bytes_acked) { + *a0 = a0_candidates_[i - 1]; + if (i > 1) { + a0_candidates_.pop_front_n(i - 1); + } + return true; + } + } + + // All candidates' total_bytes_acked is <= |total_bytes_acked|. + *a0 = a0_candidates_.back(); + a0_candidates_.pop_front_n(a0_candidates_.size() - 1); + return true; +} + SendTimeState BandwidthSampler::OnPacketLost(QuicPacketNumber packet_number, QuicPacketLength bytes_lost) { // TODO(vasilvv): see the comment for the case of missing packets in
diff --git a/quic/core/congestion_control/bandwidth_sampler.h b/quic/core/congestion_control/bandwidth_sampler.h index 90b20fc..210a219 100644 --- a/quic/core/congestion_control/bandwidth_sampler.h +++ b/quic/core/congestion_control/bandwidth_sampler.h
@@ -9,6 +9,7 @@ #include "net/third_party/quiche/src/quic/core/congestion_control/windowed_filter.h" #include "net/third_party/quiche/src/quic/core/packet_number_indexed_queue.h" #include "net/third_party/quiche/src/quic/core/quic_bandwidth.h" +#include "net/third_party/quiche/src/quic/core/quic_circular_deque.h" #include "net/third_party/quiche/src/quic/core/quic_packets.h" #include "net/third_party/quiche/src/quic/core/quic_time.h" #include "net/third_party/quiche/src/quic/core/quic_types.h" @@ -110,6 +111,14 @@ max_ack_height_filter_.Reset(new_height, new_time); } + void SetAckAggregationBandwidthThreshold(double threshold) { + ack_aggregation_bandwidth_threshold_ = threshold; + } + + double ack_aggregation_bandwidth_threshold() const { + return ack_aggregation_bandwidth_threshold_; + } + uint64_t num_ack_aggregation_epochs() const { return num_ack_aggregation_epochs_; } @@ -130,6 +139,8 @@ // The number of ack aggregation epochs ever started, including the ongoing // one. Stats only. uint64_t num_ack_aggregation_epochs_ = 0; + double ack_aggregation_bandwidth_threshold_ = + GetQuicFlag(FLAGS_quic_ack_aggregation_bandwidth_threshold); }; // An interface common to any class that can provide bandwidth samples from the @@ -245,8 +256,8 @@ // For that purpose, BandwidthSampler always keeps track of the most recently // acknowledged packet, and records it together with every outgoing packet. // When a packet gets acknowledged (A_1), it has not only information about when -// it itself was sent (S_1), but also the information about the latest -// acknowledged packet right before it was sent (S_0 and A_0). +// it itself was sent (S_1), but also the information about a previously +// acknowledged packet before it was sent (S_0 and A_0). // // Based on that data, send and ack rate are estimated as: // send_rate = (bytes(S_1) - bytes(S_0)) / (time(S_1) - time(S_0)) @@ -362,6 +373,57 @@ return one_bw_sample_per_ack_event_; } + // AckPoint represents a point on the ack line. + struct QUIC_NO_EXPORT AckPoint { + QuicTime ack_time = QuicTime::Zero(); + QuicByteCount total_bytes_acked = 0; + + friend QUIC_NO_EXPORT std::ostream& operator<<(std::ostream& os, + const AckPoint& ack_point) { + return os << ack_point.ack_time << ":" << ack_point.total_bytes_acked; + } + }; + + // RecentAckPoints maintains the most recent 2 ack points at distinct times. + class QUIC_NO_EXPORT RecentAckPoints { + public: + void Update(QuicTime ack_time, QuicByteCount total_bytes_acked) { + DCHECK_GE(total_bytes_acked, ack_points_[1].total_bytes_acked); + + if (ack_time < ack_points_[1].ack_time) { + // This can only happen when time goes backwards, we use the smaller + // timestamp for the most recent ack point in that case. + // TODO(wub): Add a QUIC_BUG if ack time stops going backwards. + ack_points_[1].ack_time = ack_time; + } else if (ack_time > ack_points_[1].ack_time) { + ack_points_[0] = ack_points_[1]; + ack_points_[1].ack_time = ack_time; + } + + ack_points_[1].total_bytes_acked = total_bytes_acked; + } + + void Clear() { ack_points_[0] = ack_points_[1] = AckPoint(); } + + const AckPoint& MostRecentPoint() const { return ack_points_[1]; } + + const AckPoint& LessRecentPoint() const { + if (ack_points_[0].total_bytes_acked != 0) { + return ack_points_[0]; + } + + return ack_points_[1]; + } + + private: + AckPoint ack_points_[2]; + }; + + void EnableOverestimateAvoidance(); + bool IsOverestimateAvoidanceEnabled() const { + return overestimate_avoidance_; + } + private: friend class test::BandwidthSamplerPeer; @@ -428,6 +490,20 @@ void SentPacketToSendTimeState(const ConnectionStateOnSentPacket& sent_packet, SendTimeState* send_time_state) const; + // Choose the best a0 from |a0_candidates_| to calculate the ack rate. + // |total_bytes_acked| is the total bytes acked when the packet being acked is + // sent. The best a0 is chosen as follows: + // - If there's only one candidate, use it. + // - If there are multiple candidates, let a[n] be the nth candidate, and + // a[n-1].total_bytes_acked <= |total_bytes_acked| < a[n].total_bytes_acked, + // use a[n-1]. + // - If all candidates's total_bytes_acked is > |total_bytes_acked|, use a[0]. + // This may happen when acks are received out of order, and ack[n] caused + // some candidates of ack[n-x] to be removed. + // - If all candidates's total_bytes_acked is <= |total_bytes_acked|, use + // a[a.size()-1]. + bool ChooseA0Point(QuicByteCount total_bytes_acked, AckPoint* a0); + // The total number of congestion controlled bytes sent during the connection. QuicByteCount total_bytes_sent_; @@ -466,6 +542,9 @@ // sent, indexed by the packet number. PacketNumberIndexedQueue<ConnectionStateOnSentPacket> connection_state_map_; + RecentAckPoints recent_ack_points_; + QuicCircularDeque<AckPoint> a0_candidates_; + // Maximum number of tracked packets. const QuicPacketCount max_tracked_packets_; @@ -493,6 +572,10 @@ const bool one_bw_sample_per_ack_event_ = remove_packets_once_per_congestion_event_ && GetQuicReloadableFlag(quic_one_bw_sample_per_ack_event2); + + // True if --quic_avoid_overestimate_bandwidth_with_aggregation=true and + // connection option 'BSAO' is set. + bool overestimate_avoidance_; }; } // namespace quic
diff --git a/quic/core/congestion_control/bandwidth_sampler_test.cc b/quic/core/congestion_control/bandwidth_sampler_test.cc index 13fa1c0..2529ba1 100644 --- a/quic/core/congestion_control/bandwidth_sampler_test.cc +++ b/quic/core/congestion_control/bandwidth_sampler_test.cc
@@ -45,6 +45,10 @@ round_trip_count_(0) { // Ensure that the clock does not start at zero. clock_.AdvanceTime(QuicTime::Delta::FromSeconds(1)); + if (GetQuicReloadableFlag( + quic_avoid_overestimate_bandwidth_with_aggregation)) { + sampler_.EnableOverestimateAvoidance(); + } } MockClock clock_; @@ -286,7 +290,7 @@ QuicBandwidth last_bandwidth = QuicBandwidth::Zero(); for (int i = 21; i <= 40; i++) { last_bandwidth = AckPacket(i); - EXPECT_EQ(expected_bandwidth, last_bandwidth); + EXPECT_EQ(expected_bandwidth, last_bandwidth) << "i is " << i; clock_.AdvanceTime(time_between_packets); } if (sampler_.remove_packets_once_per_congestion_event()) { @@ -690,7 +694,7 @@ // Send and ack packet 2, 3 and 4. round_trip_count_++; - est_bandwidth_upper_bound_ = first_packet_sending_rate * 0.9; + est_bandwidth_upper_bound_ = first_packet_sending_rate * 0.3; SendPacket(2); SendPacket(3); SendPacket(4); @@ -704,7 +708,12 @@ class MaxAckHeightTrackerTest : public QuicTest { protected: - MaxAckHeightTrackerTest() : tracker_(/*initial_filter_window=*/10) {} + MaxAckHeightTrackerTest() : tracker_(/*initial_filter_window=*/10) { + if (GetQuicReloadableFlag( + quic_avoid_overestimate_bandwidth_with_aggregation)) { + tracker_.SetAckAggregationBandwidthThreshold(1.8); + } + } // Run a full aggregation episode, which is one or more aggregated acks, // followed by a quiet period in which no ack happens. @@ -787,9 +796,15 @@ 1200, true); now_ = now_ - QuicTime::Delta::FromMilliseconds(1); - AggregationEpisode(bandwidth_ * 20, QuicTime::Delta::FromMilliseconds(6), - 1200, false); - EXPECT_EQ(2u, tracker_.num_ack_aggregation_epochs()); + if (tracker_.ack_aggregation_bandwidth_threshold() > 1.1) { + AggregationEpisode(bandwidth_ * 20, QuicTime::Delta::FromMilliseconds(6), + 1200, true); + EXPECT_EQ(3u, tracker_.num_ack_aggregation_epochs()); + } else { + AggregationEpisode(bandwidth_ * 20, QuicTime::Delta::FromMilliseconds(6), + 1200, false); + EXPECT_EQ(2u, tracker_.num_ack_aggregation_epochs()); + } } TEST_F(MaxAckHeightTrackerTest, VeryAggregatedSmallAcks) { @@ -799,9 +814,15 @@ true); now_ = now_ - QuicTime::Delta::FromMilliseconds(1); - AggregationEpisode(bandwidth_ * 20, QuicTime::Delta::FromMilliseconds(6), 300, - false); - EXPECT_EQ(2u, tracker_.num_ack_aggregation_epochs()); + if (tracker_.ack_aggregation_bandwidth_threshold() > 1.1) { + AggregationEpisode(bandwidth_ * 20, QuicTime::Delta::FromMilliseconds(6), + 300, true); + EXPECT_EQ(3u, tracker_.num_ack_aggregation_epochs()); + } else { + AggregationEpisode(bandwidth_ * 20, QuicTime::Delta::FromMilliseconds(6), + 300, false); + EXPECT_EQ(2u, tracker_.num_ack_aggregation_epochs()); + } } TEST_F(MaxAckHeightTrackerTest, SomewhatAggregatedLargeAck) { @@ -811,9 +832,15 @@ 1000, true); now_ = now_ - QuicTime::Delta::FromMilliseconds(1); - AggregationEpisode(bandwidth_ * 2, QuicTime::Delta::FromMilliseconds(50), - 1000, false); - EXPECT_EQ(2u, tracker_.num_ack_aggregation_epochs()); + if (tracker_.ack_aggregation_bandwidth_threshold() > 1.1) { + AggregationEpisode(bandwidth_ * 2, QuicTime::Delta::FromMilliseconds(50), + 1000, true); + EXPECT_EQ(3u, tracker_.num_ack_aggregation_epochs()); + } else { + AggregationEpisode(bandwidth_ * 2, QuicTime::Delta::FromMilliseconds(50), + 1000, false); + EXPECT_EQ(2u, tracker_.num_ack_aggregation_epochs()); + } } TEST_F(MaxAckHeightTrackerTest, SomewhatAggregatedSmallAcks) { @@ -823,9 +850,15 @@ true); now_ = now_ - QuicTime::Delta::FromMilliseconds(1); - AggregationEpisode(bandwidth_ * 2, QuicTime::Delta::FromMilliseconds(50), 100, - false); - EXPECT_EQ(2u, tracker_.num_ack_aggregation_epochs()); + if (tracker_.ack_aggregation_bandwidth_threshold() > 1.1) { + AggregationEpisode(bandwidth_ * 2, QuicTime::Delta::FromMilliseconds(50), + 100, true); + EXPECT_EQ(3u, tracker_.num_ack_aggregation_epochs()); + } else { + AggregationEpisode(bandwidth_ * 2, QuicTime::Delta::FromMilliseconds(50), + 100, false); + EXPECT_EQ(2u, tracker_.num_ack_aggregation_epochs()); + } } TEST_F(MaxAckHeightTrackerTest, NotAggregated) {
diff --git a/quic/core/congestion_control/bbr2_misc.h b/quic/core/congestion_control/bbr2_misc.h index 2226566..191790b 100644 --- a/quic/core/congestion_control/bbr2_misc.h +++ b/quic/core/congestion_control/bbr2_misc.h
@@ -359,6 +359,10 @@ return bandwidth_sampler_.max_ack_height(); } + void EnableOverestimateAvoidance() { + bandwidth_sampler_.EnableOverestimateAvoidance(); + } + void OnPacketNeutered(QuicPacketNumber packet_number) { bandwidth_sampler_.OnPacketNeutered(packet_number); }
diff --git a/quic/core/congestion_control/bbr2_sender.cc b/quic/core/congestion_control/bbr2_sender.cc index 108cb34..60fb374 100644 --- a/quic/core/congestion_control/bbr2_sender.cc +++ b/quic/core/congestion_control/bbr2_sender.cc
@@ -93,6 +93,13 @@ if (config.HasClientRequestedIndependentOption(kBBR9, perspective)) { flexible_app_limited_ = true; } + if (GetQuicReloadableFlag( + quic_avoid_overestimate_bandwidth_with_aggregation) && + config.HasClientRequestedIndependentOption(kBSAO, perspective)) { + QUIC_RELOADABLE_FLAG_COUNT_N( + quic_avoid_overestimate_bandwidth_with_aggregation, 4, 4); + model_.EnableOverestimateAvoidance(); + } } 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 d109d54..e80b5ac 100644 --- a/quic/core/congestion_control/bbr2_simulator_test.cc +++ b/quic/core/congestion_control/bbr2_simulator_test.cc
@@ -16,6 +16,7 @@ #include "net/third_party/quiche/src/quic/platform/api/quic_logging.h" #include "net/third_party/quiche/src/quic/platform/api/quic_optional.h" #include "net/third_party/quiche/src/quic/platform/api/quic_test.h" +#include "net/third_party/quiche/src/quic/test_tools/quic_config_peer.h" #include "net/third_party/quiche/src/quic/test_tools/quic_connection_peer.h" #include "net/third_party/quiche/src/quic/test_tools/quic_sent_packet_manager_peer.h" #include "net/third_party/quiche/src/quic/test_tools/quic_test_utils.h" @@ -304,6 +305,14 @@ aggregation_timeout); } + void SetConnectionOption(QuicTag option) { + QuicConfig config; + QuicTagVector options; + options.push_back(option); + QuicConfigPeer::SetReceivedConnectionOptions(&config, options); + sender_->SetFromConfig(config, Perspective::IS_SERVER); + } + bool Bbr2ModeIsOneOf(const std::vector<Bbr2Mode>& expected_modes) const { const Bbr2Mode mode = sender_->ExportDebugState().mode; for (Bbr2Mode expected_mode : expected_modes) { @@ -416,6 +425,10 @@ } TEST_F(Bbr2DefaultTopologyTest, SimpleTransfer2RTTAggregationBytes) { + if (GetQuicReloadableFlag( + quic_avoid_overestimate_bandwidth_with_aggregation)) { + SetConnectionOption(kBSAO); + } DefaultTopologyParams params; CreateNetwork(params); // 2 RTTs of aggregation, with a max of 10kb. @@ -425,11 +438,17 @@ DoSimpleTransfer(12 * 1024 * 1024, QuicTime::Delta::FromSeconds(35)); EXPECT_TRUE(Bbr2ModeIsOneOf({Bbr2Mode::PROBE_BW, Bbr2Mode::PROBE_RTT})); - EXPECT_LE(params.BottleneckBandwidth() * 0.99f, - sender_->ExportDebugState().bandwidth_hi); - // TODO(b/36022633): Bandwidth sampler overestimates with aggregation. - EXPECT_GE(params.BottleneckBandwidth() * 1.5f, - sender_->ExportDebugState().bandwidth_hi); + if (GetQuicReloadableFlag( + quic_avoid_overestimate_bandwidth_with_aggregation)) { + EXPECT_APPROX_EQ(params.BottleneckBandwidth(), + sender_->ExportDebugState().bandwidth_hi, 0.01f); + } else { + EXPECT_LE(params.BottleneckBandwidth() * 0.99f, + sender_->ExportDebugState().bandwidth_hi); + // TODO(b/36022633): Bandwidth sampler overestimates with aggregation. + EXPECT_GE(params.BottleneckBandwidth() * 1.5f, + sender_->ExportDebugState().bandwidth_hi); + } EXPECT_LE(sender_loss_rate_in_packets(), 0.05); // The margin here is high, because the aggregation greatly increases // smoothed rtt. @@ -438,6 +457,10 @@ } TEST_F(Bbr2DefaultTopologyTest, SimpleTransferAckDecimation) { + if (GetQuicReloadableFlag( + quic_avoid_overestimate_bandwidth_with_aggregation)) { + SetConnectionOption(kBSAO); + } // Enable Ack Decimation on the receiver. QuicConnectionPeer::SetAckMode(receiver_endpoint_.connection(), AckMode::ACK_DECIMATION); @@ -448,11 +471,17 @@ DoSimpleTransfer(12 * 1024 * 1024, QuicTime::Delta::FromSeconds(35)); EXPECT_TRUE(Bbr2ModeIsOneOf({Bbr2Mode::PROBE_BW, Bbr2Mode::PROBE_RTT})); - EXPECT_LE(params.BottleneckBandwidth() * 0.99f, - sender_->ExportDebugState().bandwidth_hi); - // TODO(b/36022633): Bandwidth sampler overestimates with aggregation. - EXPECT_GE(params.BottleneckBandwidth() * 1.1f, - sender_->ExportDebugState().bandwidth_hi); + if (GetQuicReloadableFlag( + quic_avoid_overestimate_bandwidth_with_aggregation)) { + EXPECT_APPROX_EQ(params.BottleneckBandwidth(), + sender_->ExportDebugState().bandwidth_hi, 0.01f); + } else { + EXPECT_LE(params.BottleneckBandwidth() * 0.99f, + sender_->ExportDebugState().bandwidth_hi); + // TODO(b/36022633): Bandwidth sampler overestimates with aggregation. + EXPECT_GE(params.BottleneckBandwidth() * 1.1f, + sender_->ExportDebugState().bandwidth_hi); + } EXPECT_LE(sender_loss_rate_in_packets(), 0.001); EXPECT_FALSE(sender_->ExportDebugState().last_sample_is_app_limited); // The margin here is high, because the aggregation greatly increases
diff --git a/quic/core/congestion_control/bbr_sender.cc b/quic/core/congestion_control/bbr_sender.cc index da723c4..fba16b5 100644 --- a/quic/core/congestion_control/bbr_sender.cc +++ b/quic/core/congestion_control/bbr_sender.cc
@@ -330,6 +330,13 @@ max_congestion_window_with_network_parameters_adjusted_ = 100 * kDefaultTCPMSS; } + if (GetQuicReloadableFlag( + quic_avoid_overestimate_bandwidth_with_aggregation) && + config.HasClientRequestedIndependentOption(kBSAO, perspective)) { + QUIC_RELOADABLE_FLAG_COUNT_N( + quic_avoid_overestimate_bandwidth_with_aggregation, 3, 4); + sampler_.EnableOverestimateAvoidance(); + } } void BbrSender::AdjustNetworkParameters(const NetworkParams& params) {
diff --git a/quic/core/congestion_control/bbr_sender_test.cc b/quic/core/congestion_control/bbr_sender_test.cc index 096b7fa..2b262c9 100644 --- a/quic/core/congestion_control/bbr_sender_test.cc +++ b/quic/core/congestion_control/bbr_sender_test.cc
@@ -430,20 +430,32 @@ // Flaky in Chromium: https://crbug.com/1042575 TEST_F(BbrSenderTest, QUIC_TEST_DISABLED_IN_CHROME(SimpleTransfer2RTTAggregationBytes)) { + if (GetQuicReloadableFlag( + quic_avoid_overestimate_bandwidth_with_aggregation)) { + SetConnectionOption(kBSAO); + } CreateDefaultSetup(); // 2 RTTs of aggregation, with a max of 10kb. EnableAggregation(10 * 1024, 2 * kTestRtt); // Transfer 12MB. DoSimpleTransfer(12 * 1024 * 1024, QuicTime::Delta::FromSeconds(35)); - EXPECT_EQ(BbrSender::PROBE_BW, sender_->ExportDebugState().mode); - // It's possible to read a bandwidth as much as 50% too high with aggregation. - EXPECT_LE(kTestLinkBandwidth * 0.99f, - sender_->ExportDebugState().max_bandwidth); - // TODO(ianswett): Tighten this bound once we understand why BBR is - // overestimating bandwidth with aggregation. b/36022633 - EXPECT_GE(kTestLinkBandwidth * 1.5f, - sender_->ExportDebugState().max_bandwidth); + EXPECT_TRUE(sender_->ExportDebugState().mode == BbrSender::PROBE_BW || + sender_->ExportDebugState().mode == BbrSender::PROBE_RTT); + if (GetQuicReloadableFlag( + quic_avoid_overestimate_bandwidth_with_aggregation)) { + EXPECT_APPROX_EQ(kTestLinkBandwidth, + sender_->ExportDebugState().max_bandwidth, 0.01f); + } else { + // It's possible to read a bandwidth as much as 50% too high with + // aggregation. + EXPECT_LE(kTestLinkBandwidth * 0.93f, + sender_->ExportDebugState().max_bandwidth); + // TODO(ianswett): Tighten this bound once we understand why BBR is + // overestimating bandwidth with aggregation. b/36022633 + EXPECT_GE(kTestLinkBandwidth * 1.5f, + sender_->ExportDebugState().max_bandwidth); + } // The margin here is high, because the aggregation greatly increases // smoothed rtt. EXPECT_GE(kTestRtt * 4, rtt_stats_->smoothed_rtt()); @@ -452,6 +464,10 @@ // Test a simple long data transfer with 2 rtts of aggregation. TEST_F(BbrSenderTest, SimpleTransferAckDecimation) { + if (GetQuicReloadableFlag( + quic_avoid_overestimate_bandwidth_with_aggregation)) { + SetConnectionOption(kBSAO); + } // Decrease the CWND gain so extra CWND is required with stretch acks. SetQuicFlag(FLAGS_quic_bbr_cwnd_gain, 1.0); sender_ = new BbrSender( @@ -470,13 +486,21 @@ // Transfer 12MB. DoSimpleTransfer(12 * 1024 * 1024, QuicTime::Delta::FromSeconds(35)); EXPECT_EQ(BbrSender::PROBE_BW, sender_->ExportDebugState().mode); - // It's possible to read a bandwidth as much as 50% too high with aggregation. - EXPECT_LE(kTestLinkBandwidth * 0.99f, - sender_->ExportDebugState().max_bandwidth); - // TODO(ianswett): Tighten this bound once we understand why BBR is - // overestimating bandwidth with aggregation. b/36022633 - EXPECT_GE(kTestLinkBandwidth * 1.5f, - sender_->ExportDebugState().max_bandwidth); + + if (GetQuicReloadableFlag( + quic_avoid_overestimate_bandwidth_with_aggregation)) { + EXPECT_APPROX_EQ(kTestLinkBandwidth, + sender_->ExportDebugState().max_bandwidth, 0.01f); + } else { + // It's possible to read a bandwidth as much as 50% too high with + // aggregation. + EXPECT_LE(kTestLinkBandwidth * 0.93f, + sender_->ExportDebugState().max_bandwidth); + // TODO(ianswett): Tighten this bound once we understand why BBR is + // overestimating bandwidth with aggregation. b/36022633 + EXPECT_GE(kTestLinkBandwidth * 1.5f, + sender_->ExportDebugState().max_bandwidth); + } // TODO(ianswett): Expect 0 packets are lost once BBR no longer measures // bandwidth higher than the link rate. EXPECT_FALSE(sender_->ExportDebugState().last_sample_is_app_limited); @@ -488,6 +512,10 @@ // Test a simple long data transfer with 2 rtts of aggregation. TEST_F(BbrSenderTest, SimpleTransfer2RTTAggregationBytes20RTTWindow) { + if (GetQuicReloadableFlag( + quic_avoid_overestimate_bandwidth_with_aggregation)) { + SetConnectionOption(kBSAO); + } // Disable Ack Decimation on the receiver, because it can increase srtt. QuicConnectionPeer::SetAckMode(receiver_.connection(), AckMode::TCP_ACKING); CreateDefaultSetup(); @@ -499,13 +527,20 @@ DoSimpleTransfer(12 * 1024 * 1024, QuicTime::Delta::FromSeconds(35)); EXPECT_TRUE(sender_->ExportDebugState().mode == BbrSender::PROBE_BW || sender_->ExportDebugState().mode == BbrSender::PROBE_RTT); - // It's possible to read a bandwidth as much as 50% too high with aggregation. - EXPECT_LE(kTestLinkBandwidth * 0.99f, - sender_->ExportDebugState().max_bandwidth); - // TODO(ianswett): Tighten this bound once we understand why BBR is - // overestimating bandwidth with aggregation. b/36022633 - EXPECT_GE(kTestLinkBandwidth * 1.5f, - sender_->ExportDebugState().max_bandwidth); + if (GetQuicReloadableFlag( + quic_avoid_overestimate_bandwidth_with_aggregation)) { + EXPECT_APPROX_EQ(kTestLinkBandwidth, + sender_->ExportDebugState().max_bandwidth, 0.01f); + } else { + // It's possible to read a bandwidth as much as 50% too high with + // aggregation. + EXPECT_LE(kTestLinkBandwidth * 0.93f, + sender_->ExportDebugState().max_bandwidth); + // TODO(ianswett): Tighten this bound once we understand why BBR is + // overestimating bandwidth with aggregation. b/36022633 + EXPECT_GE(kTestLinkBandwidth * 1.5f, + sender_->ExportDebugState().max_bandwidth); + } // TODO(ianswett): Expect 0 packets are lost once BBR no longer measures // bandwidth higher than the link rate. // The margin here is high, because the aggregation greatly increases @@ -516,6 +551,10 @@ // Test a simple long data transfer with 2 rtts of aggregation. TEST_F(BbrSenderTest, SimpleTransfer2RTTAggregationBytes40RTTWindow) { + if (GetQuicReloadableFlag( + quic_avoid_overestimate_bandwidth_with_aggregation)) { + SetConnectionOption(kBSAO); + } // Disable Ack Decimation on the receiver, because it can increase srtt. QuicConnectionPeer::SetAckMode(receiver_.connection(), AckMode::TCP_ACKING); CreateDefaultSetup(); @@ -527,13 +566,20 @@ DoSimpleTransfer(12 * 1024 * 1024, QuicTime::Delta::FromSeconds(35)); EXPECT_TRUE(sender_->ExportDebugState().mode == BbrSender::PROBE_BW || sender_->ExportDebugState().mode == BbrSender::PROBE_RTT); - // It's possible to read a bandwidth as much as 50% too high with aggregation. - EXPECT_LE(kTestLinkBandwidth * 0.99f, - sender_->ExportDebugState().max_bandwidth); - // TODO(ianswett): Tighten this bound once we understand why BBR is - // overestimating bandwidth with aggregation. b/36022633 - EXPECT_GE(kTestLinkBandwidth * 1.5f, - sender_->ExportDebugState().max_bandwidth); + if (GetQuicReloadableFlag( + quic_avoid_overestimate_bandwidth_with_aggregation)) { + EXPECT_APPROX_EQ(kTestLinkBandwidth, + sender_->ExportDebugState().max_bandwidth, 0.01f); + } else { + // It's possible to read a bandwidth as much as 50% too high with + // aggregation. + EXPECT_LE(kTestLinkBandwidth * 0.93f, + sender_->ExportDebugState().max_bandwidth); + // TODO(ianswett): Tighten this bound once we understand why BBR is + // overestimating bandwidth with aggregation. b/36022633 + EXPECT_GE(kTestLinkBandwidth * 1.5f, + sender_->ExportDebugState().max_bandwidth); + } // TODO(ianswett): Expect 0 packets are lost once BBR no longer measures // bandwidth higher than the link rate. // The margin here is high, because the aggregation greatly increases