gfe-relnote: In QUIC BBR(v1 and v2), do not add QuicBandwidth::Zero() into the max bandwidth filter. Protected by --gfe2_reloadable_flag_quic_bbr_fix_zero_bw_on_loss_only_event.
This is an attempt to fix b/151239871.
PiperOrigin-RevId: 300605878
Change-Id: I0dc04fe0b6735e88777de510bda072da4c2854c6
diff --git a/quic/core/congestion_control/bbr2_misc.cc b/quic/core/congestion_control/bbr2_misc.cc
index 533e9a0..626540a 100644
--- a/quic/core/congestion_control/bbr2_misc.cc
+++ b/quic/core/congestion_control/bbr2_misc.cc
@@ -98,10 +98,29 @@
sample.last_packet_send_state.is_app_limited;
}
- if (!sample.sample_is_app_limited ||
- sample.sample_max_bandwidth > MaxBandwidth()) {
- congestion_event->sample_max_bandwidth = sample.sample_max_bandwidth;
- max_bandwidth_filter_.Update(congestion_event->sample_max_bandwidth);
+ // Avoid updating |max_bandwidth_filter_| if a) this is a loss-only event, or
+ // b) all packets in |acked_packets| did not generate valid samples. (e.g. ack
+ // of ack-only packets). In both cases, total_bytes_acked() will not change.
+ if (!fix_zero_bw_on_loss_only_event_ ||
+ (prior_bytes_acked != total_bytes_acked())) {
+ QUIC_BUG_IF((prior_bytes_acked != total_bytes_acked()) &&
+ sample.sample_max_bandwidth.IsZero())
+ << total_bytes_acked() - prior_bytes_acked << " bytes from "
+ << acked_packets.size()
+ << " packets have been acked, but sample_max_bandwidth is zero.";
+ if (!sample.sample_is_app_limited ||
+ sample.sample_max_bandwidth > MaxBandwidth()) {
+ congestion_event->sample_max_bandwidth = sample.sample_max_bandwidth;
+ max_bandwidth_filter_.Update(congestion_event->sample_max_bandwidth);
+ }
+ } else {
+ if (acked_packets.empty()) {
+ QUIC_RELOADABLE_FLAG_COUNT_N(quic_bbr_fix_zero_bw_on_loss_only_event, 3,
+ 4);
+ } else {
+ QUIC_RELOADABLE_FLAG_COUNT_N(quic_bbr_fix_zero_bw_on_loss_only_event, 4,
+ 4);
+ }
}
if (!sample.sample_rtt.IsInfinite()) {
diff --git a/quic/core/congestion_control/bbr2_misc.h b/quic/core/congestion_control/bbr2_misc.h
index 66702ee..d98bb1a 100644
--- a/quic/core/congestion_control/bbr2_misc.h
+++ b/quic/core/congestion_control/bbr2_misc.h
@@ -455,6 +455,9 @@
float cwnd_gain_;
float pacing_gain_;
+
+ const bool fix_zero_bw_on_loss_only_event_ =
+ GetQuicReloadableFlag(quic_bbr_fix_zero_bw_on_loss_only_event);
};
enum class Bbr2Mode : uint8_t {
diff --git a/quic/core/congestion_control/bbr2_simulator_test.cc b/quic/core/congestion_control/bbr2_simulator_test.cc
index 553dfcf..6f3829c 100644
--- a/quic/core/congestion_control/bbr2_simulator_test.cc
+++ b/quic/core/congestion_control/bbr2_simulator_test.cc
@@ -817,6 +817,44 @@
EXPECT_LT(2 * kDefaultMaxPacketSize, inflight_hi);
}
+// Ensures bandwidth estimate does not change after a loss only event.
+TEST_F(Bbr2DefaultTopologyTest, LossOnlyCongestionEvent) {
+ DefaultTopologyParams params;
+ CreateNetwork(params);
+
+ DriveOutOfStartup(params);
+ EXPECT_FALSE(sender_->ExportDebugState().last_sample_is_app_limited);
+
+ // Send some bursts, each burst increments round count by 1, since it only
+ // generates small, app-limited samples, the max_bandwidth_filter_ will not be
+ // updated.
+ SendBursts(params, 20, 512, QuicTime::Delta::FromSeconds(3));
+
+ // Run until we have something in flight.
+ sender_endpoint_.AddBytesToTransfer(50 * 1024 * 1024);
+ bool simulator_result = simulator_.RunUntilOrTimeout(
+ [&]() { return sender_unacked_map()->bytes_in_flight() > 0; },
+ QuicTime::Delta::FromSeconds(5));
+ ASSERT_TRUE(simulator_result);
+
+ const QuicBandwidth prior_bandwidth_estimate = sender_->BandwidthEstimate();
+ EXPECT_APPROX_EQ(params.BottleneckBandwidth(), prior_bandwidth_estimate,
+ 0.01f);
+
+ // Lose the least unacked packet.
+ LostPacketVector lost_packets;
+ lost_packets.emplace_back(
+ sender_connection()->sent_packet_manager().GetLeastUnacked(),
+ kDefaultMaxPacketSize);
+
+ QuicTime now = simulator_.GetClock()->Now() + params.RTT() * 0.25;
+ sender_->OnCongestionEvent(false, sender_unacked_map()->bytes_in_flight(),
+ now, {}, lost_packets);
+
+ // Bandwidth estimate should not change for the loss only event.
+ EXPECT_EQ(prior_bandwidth_estimate, sender_->BandwidthEstimate());
+}
+
// After quiescence, if the sender is in PROBE_RTT, it should transition to
// PROBE_BW immediately on the first sent packet after quiescence.
TEST_F(Bbr2DefaultTopologyTest, ProbeRttAfterQuiescenceImmediatelyExits) {
diff --git a/quic/core/congestion_control/bbr_sender.cc b/quic/core/congestion_control/bbr_sender.cc
index 86493fc..2961680 100644
--- a/quic/core/congestion_control/bbr_sender.cc
+++ b/quic/core/congestion_control/bbr_sender.cc
@@ -426,9 +426,29 @@
stats_->has_non_app_limited_sample = has_non_app_limited_sample_;
}
}
- if (!sample.sample_is_app_limited ||
- sample.sample_max_bandwidth > max_bandwidth_.GetBest()) {
- max_bandwidth_.Update(sample.sample_max_bandwidth, round_trip_count_);
+ // Avoid updating |max_bandwidth_| if a) this is a loss-only event, or b) all
+ // packets in |acked_packets| did not generate valid samples. (e.g. ack of
+ // ack-only packets). In both cases, sampler_.total_bytes_acked() will not
+ // change.
+ if (!fix_zero_bw_on_loss_only_event_ ||
+ (total_bytes_acked_before != sampler_.total_bytes_acked())) {
+ QUIC_BUG_IF((total_bytes_acked_before != sampler_.total_bytes_acked()) &&
+ sample.sample_max_bandwidth.IsZero())
+ << sampler_.total_bytes_acked() - total_bytes_acked_before
+ << " bytes from " << acked_packets.size()
+ << " packets have been acked, but sample_max_bandwidth is zero.";
+ if (!sample.sample_is_app_limited ||
+ sample.sample_max_bandwidth > max_bandwidth_.GetBest()) {
+ max_bandwidth_.Update(sample.sample_max_bandwidth, round_trip_count_);
+ }
+ } else {
+ if (acked_packets.empty()) {
+ QUIC_RELOADABLE_FLAG_COUNT_N(quic_bbr_fix_zero_bw_on_loss_only_event, 1,
+ 4);
+ } else {
+ QUIC_RELOADABLE_FLAG_COUNT_N(quic_bbr_fix_zero_bw_on_loss_only_event, 2,
+ 4);
+ }
}
if (!sample.sample_rtt.IsInfinite()) {
min_rtt_expired = MaybeUpdateMinRtt(event_time, sample.sample_rtt);
diff --git a/quic/core/congestion_control/bbr_sender.h b/quic/core/congestion_control/bbr_sender.h
index dbdf0b8..4b42600 100644
--- a/quic/core/congestion_control/bbr_sender.h
+++ b/quic/core/congestion_control/bbr_sender.h
@@ -16,10 +16,12 @@
#include "net/third_party/quiche/src/quic/core/congestion_control/windowed_filter.h"
#include "net/third_party/quiche/src/quic/core/crypto/quic_random.h"
#include "net/third_party/quiche/src/quic/core/quic_bandwidth.h"
+#include "net/third_party/quiche/src/quic/core/quic_packet_number.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_unacked_packet_map.h"
#include "net/third_party/quiche/src/quic/platform/api/quic_export.h"
+#include "net/third_party/quiche/src/quic/platform/api/quic_flags.h"
namespace quic {
@@ -388,6 +390,9 @@
// or it's time for high gain mode.
bool drain_to_target_;
+ const bool fix_zero_bw_on_loss_only_event_ =
+ GetQuicReloadableFlag(quic_bbr_fix_zero_bw_on_loss_only_event);
+
// True if network parameters are adjusted, and this will be reset if
// overshooting is detected and pacing rate gets slowed.
bool network_parameters_adjusted_;
diff --git a/quic/core/congestion_control/bbr_sender_test.cc b/quic/core/congestion_control/bbr_sender_test.cc
index 618dbc1..483f98b 100644
--- a/quic/core/congestion_control/bbr_sender_test.cc
+++ b/quic/core/congestion_control/bbr_sender_test.cc
@@ -10,6 +10,7 @@
#include <utility>
#include "net/third_party/quiche/src/quic/core/congestion_control/rtt_stats.h"
+#include "net/third_party/quiche/src/quic/core/quic_bandwidth.h"
#include "net/third_party/quiche/src/quic/core/quic_packets.h"
#include "net/third_party/quiche/src/quic/core/quic_types.h"
#include "net/third_party/quiche/src/quic/core/quic_utils.h"
@@ -1520,5 +1521,48 @@
EXPECT_GT(1024 * kTestLinkBandwidth, sender_->PacingRate(0));
}
+// Ensures bandwidth estimate does not change after a loss only event.
+// Regression test for b/151239871.
+TEST_F(BbrSenderTest, LossOnlyCongestionEvent) {
+ CreateDefaultSetup();
+
+ DriveOutOfStartup();
+ EXPECT_FALSE(sender_->ExportDebugState().last_sample_is_app_limited);
+
+ // Send some bursts, each burst increments round count by 1, since it only
+ // generates small, app-limited samples, the max_bandwidth_ will not be
+ // updated. At the end of all bursts, all estimates in max_bandwidth_ will
+ // look very old such that any Update() will reset all estimates.
+ SendBursts(20, 512, QuicTime::Delta::FromSeconds(3));
+
+ QuicUnackedPacketMap* unacked_packets =
+ QuicSentPacketManagerPeer::GetUnackedPacketMap(
+ QuicConnectionPeer::GetSentPacketManager(bbr_sender_.connection()));
+ // Run until we have something in flight.
+ bbr_sender_.AddBytesToTransfer(50 * 1024 * 1024);
+ bool simulator_result = simulator_.RunUntilOrTimeout(
+ [&]() { return unacked_packets->bytes_in_flight() > 0; },
+ QuicTime::Delta::FromSeconds(5));
+ ASSERT_TRUE(simulator_result);
+
+ const QuicBandwidth prior_bandwidth_estimate = sender_->BandwidthEstimate();
+ EXPECT_APPROX_EQ(kTestLinkBandwidth, prior_bandwidth_estimate, 0.01f);
+
+ // Lose the least unacked packet.
+ LostPacketVector lost_packets;
+ lost_packets.emplace_back(
+ bbr_sender_.connection()->sent_packet_manager().GetLeastUnacked(),
+ kDefaultMaxPacketSize);
+
+ QuicTime now = simulator_.GetClock()->Now() + kTestRtt * 0.25;
+ sender_->OnCongestionEvent(false, unacked_packets->bytes_in_flight(), now, {},
+ lost_packets);
+
+ // Bandwidth estimate should not change for the loss only event.
+ if (GetQuicReloadableFlag(quic_bbr_fix_zero_bw_on_loss_only_event)) {
+ EXPECT_EQ(prior_bandwidth_estimate, sender_->BandwidthEstimate());
+ }
+}
+
} // namespace test
} // namespace quic
