gfe-relnote: Notify SendAlgorithm on neutered packets in QUIC. Protected by (enabling blocked) --gfe2_reloadable_flag_quic_avoid_overestimate_bandwidth_with_aggregation.
This is split from cl/279746200 for easier review. I'll unblock this flag in that cl.
PiperOrigin-RevId: 296301871
Change-Id: I499357505e3b817a82b6f4f2172f7b890b684b32
diff --git a/quic/core/congestion_control/bandwidth_sampler.cc b/quic/core/congestion_control/bandwidth_sampler.cc
index 6ecd886..6272664 100644
--- a/quic/core/congestion_control/bandwidth_sampler.cc
+++ b/quic/core/congestion_control/bandwidth_sampler.cc
@@ -73,6 +73,7 @@
: total_bytes_sent_(0),
total_bytes_acked_(0),
total_bytes_lost_(0),
+ total_bytes_neutered_(0),
total_bytes_sent_at_last_acked_packet_(0),
last_acked_packet_sent_time_(QuicTime::Zero()),
last_acked_packet_ack_time_(QuicTime::Zero()),
@@ -147,6 +148,14 @@
"in it.";
}
+void BandwidthSampler::OnPacketNeutered(QuicPacketNumber packet_number) {
+ connection_state_map_.Remove(
+ packet_number, [&](const ConnectionStateOnSentPacket& sent_packet) {
+ QUIC_CODE_COUNT(quic_bandwidth_sampler_packet_neutered);
+ total_bytes_neutered_ += sent_packet.size;
+ });
+}
+
BandwidthSamplerInterface::CongestionEventSample
BandwidthSampler::OnCongestionEvent(QuicTime ack_time,
const AckedPacketVector& acked_packets,
@@ -390,6 +399,10 @@
return total_bytes_lost_;
}
+QuicByteCount BandwidthSampler::total_bytes_neutered() const {
+ return total_bytes_neutered_;
+}
+
bool BandwidthSampler::is_app_limited() const {
return is_app_limited_;
}
diff --git a/quic/core/congestion_control/bandwidth_sampler.h b/quic/core/congestion_control/bandwidth_sampler.h
index df63fa5..90b20fc 100644
--- a/quic/core/congestion_control/bandwidth_sampler.h
+++ b/quic/core/congestion_control/bandwidth_sampler.h
@@ -149,6 +149,8 @@
QuicByteCount bytes_in_flight,
HasRetransmittableData has_retransmittable_data) = 0;
+ virtual void OnPacketNeutered(QuicPacketNumber packet_number) = 0;
+
// Notifies the sampler that the |packet_number| is acknowledged. Returns a
// bandwidth sample. If no bandwidth sample is available,
// QuicBandwidth::Zero() is returned.
@@ -206,10 +208,11 @@
// Remove all the packets lower than the specified packet number.
virtual void RemoveObsoletePackets(QuicPacketNumber least_unacked) = 0;
- // Total number of bytes sent/acked/lost in the connection.
+ // Total number of bytes sent/acked/lost/neutered in the connection.
virtual QuicByteCount total_bytes_sent() const = 0;
virtual QuicByteCount total_bytes_acked() const = 0;
virtual QuicByteCount total_bytes_lost() const = 0;
+ virtual QuicByteCount total_bytes_neutered() const = 0;
// Application-limited information exported for debugging.
virtual bool is_app_limited() const = 0;
@@ -308,6 +311,7 @@
QuicByteCount bytes,
QuicByteCount bytes_in_flight,
HasRetransmittableData has_retransmittable_data) override;
+ void OnPacketNeutered(QuicPacketNumber packet_number) override;
BandwidthSample OnPacketAcknowledged(QuicTime ack_time,
QuicPacketNumber packet_number) override;
CongestionEventSample OnCongestionEvent(
@@ -329,6 +333,7 @@
QuicByteCount total_bytes_sent() const override;
QuicByteCount total_bytes_acked() const override;
QuicByteCount total_bytes_lost() const override;
+ QuicByteCount total_bytes_neutered() const override;
bool is_app_limited() const override;
@@ -432,6 +437,9 @@
// The total number of congestion controlled bytes which were lost.
QuicByteCount total_bytes_lost_;
+ // The total number of congestion controlled bytes which have been neutered.
+ QuicByteCount total_bytes_neutered_;
+
// The value of |total_bytes_sent_| at the time the last acknowledged packet
// was sent. Valid only when |last_acked_packet_sent_time_| is valid.
QuicByteCount total_bytes_sent_at_last_acked_packet_;
diff --git a/quic/core/congestion_control/bandwidth_sampler_test.cc b/quic/core/congestion_control/bandwidth_sampler_test.cc
index 9a3c456..13fa1c0 100644
--- a/quic/core/congestion_control/bandwidth_sampler_test.cc
+++ b/quic/core/congestion_control/bandwidth_sampler_test.cc
@@ -566,16 +566,39 @@
EXPECT_EQ(0u, BandwidthSamplerPeer::GetNumberOfTrackedPackets(sampler_));
}
+TEST_F(BandwidthSamplerTest, NeuterPacket) {
+ SendPacket(1);
+ EXPECT_EQ(0u, sampler_.total_bytes_neutered());
+
+ clock_.AdvanceTime(QuicTime::Delta::FromMilliseconds(10));
+ sampler_.OnPacketNeutered(QuicPacketNumber(1));
+ EXPECT_LT(0u, sampler_.total_bytes_neutered());
+ EXPECT_EQ(0u, sampler_.total_bytes_acked());
+
+ // If packet 1 is acked it should not produce a bandwidth sample.
+ clock_.AdvanceTime(QuicTime::Delta::FromMilliseconds(10));
+ BandwidthSampler::CongestionEventSample sample = sampler_.OnCongestionEvent(
+ clock_.Now(),
+ {AckedPacket(QuicPacketNumber(1), kRegularPacketSize, clock_.Now())}, {},
+ max_bandwidth_, est_bandwidth_upper_bound_, round_trip_count_);
+ EXPECT_EQ(0u, sampler_.total_bytes_acked());
+ EXPECT_EQ(QuicBandwidth::Zero(), sample.sample_max_bandwidth);
+ EXPECT_FALSE(sample.sample_is_app_limited);
+ EXPECT_EQ(QuicTime::Delta::Infinite(), sample.sample_rtt);
+ EXPECT_EQ(0u, sample.sample_max_inflight);
+ EXPECT_EQ(0u, sample.extra_acked);
+}
+
TEST_F(BandwidthSamplerTest, CongestionEventSampleDefaultValues) {
// Make sure a default constructed CongestionEventSample has the correct
// initial values for BandwidthSampler::OnCongestionEvent() to work.
BandwidthSampler::CongestionEventSample sample;
- DCHECK_EQ(QuicBandwidth::Zero(), sample.sample_max_bandwidth);
- DCHECK(!sample.sample_is_app_limited);
- DCHECK_EQ(QuicTime::Delta::Infinite(), sample.sample_rtt);
- DCHECK_EQ(0u, sample.sample_max_inflight);
- DCHECK_EQ(0u, sample.extra_acked);
+ EXPECT_EQ(QuicBandwidth::Zero(), sample.sample_max_bandwidth);
+ EXPECT_FALSE(sample.sample_is_app_limited);
+ EXPECT_EQ(QuicTime::Delta::Infinite(), sample.sample_rtt);
+ EXPECT_EQ(0u, sample.sample_max_inflight);
+ EXPECT_EQ(0u, sample.extra_acked);
}
// 1) Send 2 packets, 2) Ack both in 1 event, 3) Repeat.
diff --git a/quic/core/congestion_control/bbr2_misc.h b/quic/core/congestion_control/bbr2_misc.h
index 1d6fb35..2226566 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 OnPacketNeutered(QuicPacketNumber packet_number) {
+ bandwidth_sampler_.OnPacketNeutered(packet_number);
+ }
+
uint64_t num_ack_aggregation_epochs() const {
return bandwidth_sampler_.num_ack_aggregation_epochs();
}
diff --git a/quic/core/congestion_control/bbr2_sender.cc b/quic/core/congestion_control/bbr2_sender.cc
index d795ad1..108cb34 100644
--- a/quic/core/congestion_control/bbr2_sender.cc
+++ b/quic/core/congestion_control/bbr2_sender.cc
@@ -284,6 +284,10 @@
is_retransmittable);
}
+void Bbr2Sender::OnPacketNeutered(QuicPacketNumber packet_number) {
+ model_.OnPacketNeutered(packet_number);
+}
+
bool Bbr2Sender::CanSend(QuicByteCount bytes_in_flight) {
const bool result = bytes_in_flight < GetCongestionWindow();
return result;
diff --git a/quic/core/congestion_control/bbr2_sender.h b/quic/core/congestion_control/bbr2_sender.h
index 2bba285..64b6fcb 100644
--- a/quic/core/congestion_control/bbr2_sender.h
+++ b/quic/core/congestion_control/bbr2_sender.h
@@ -64,6 +64,8 @@
QuicByteCount bytes,
HasRetransmittableData is_retransmittable) override;
+ void OnPacketNeutered(QuicPacketNumber packet_number) override;
+
void OnRetransmissionTimeout(bool /*packets_retransmitted*/) override {}
void OnConnectionMigration() override {}
diff --git a/quic/core/congestion_control/bbr_sender.cc b/quic/core/congestion_control/bbr_sender.cc
index 3fd6139..da723c4 100644
--- a/quic/core/congestion_control/bbr_sender.cc
+++ b/quic/core/congestion_control/bbr_sender.cc
@@ -181,6 +181,10 @@
is_retransmittable);
}
+void BbrSender::OnPacketNeutered(QuicPacketNumber packet_number) {
+ sampler_.OnPacketNeutered(packet_number);
+}
+
bool BbrSender::CanSend(QuicByteCount bytes_in_flight) {
return bytes_in_flight < GetCongestionWindow();
}
diff --git a/quic/core/congestion_control/bbr_sender.h b/quic/core/congestion_control/bbr_sender.h
index 87d3272..a58b8d5 100644
--- a/quic/core/congestion_control/bbr_sender.h
+++ b/quic/core/congestion_control/bbr_sender.h
@@ -118,6 +118,7 @@
QuicPacketNumber packet_number,
QuicByteCount bytes,
HasRetransmittableData is_retransmittable) override;
+ void OnPacketNeutered(QuicPacketNumber packet_number) override;
void OnRetransmissionTimeout(bool /*packets_retransmitted*/) override {}
void OnConnectionMigration() override {}
bool CanSend(QuicByteCount bytes_in_flight) override;
diff --git a/quic/core/congestion_control/send_algorithm_interface.h b/quic/core/congestion_control/send_algorithm_interface.h
index a049e67..b242951 100644
--- a/quic/core/congestion_control/send_algorithm_interface.h
+++ b/quic/core/congestion_control/send_algorithm_interface.h
@@ -110,6 +110,9 @@
QuicByteCount bytes,
HasRetransmittableData is_retransmittable) = 0;
+ // Inform that |packet_number| has been neutered.
+ virtual void OnPacketNeutered(QuicPacketNumber packet_number) = 0;
+
// Called when the retransmission timeout fires. Neither OnPacketAbandoned
// nor OnPacketLost will be called for these packets.
virtual void OnRetransmissionTimeout(bool packets_retransmitted) = 0;
diff --git a/quic/core/congestion_control/tcp_cubic_sender_bytes.h b/quic/core/congestion_control/tcp_cubic_sender_bytes.h
index 9144b8a..7a36f76 100644
--- a/quic/core/congestion_control/tcp_cubic_sender_bytes.h
+++ b/quic/core/congestion_control/tcp_cubic_sender_bytes.h
@@ -61,6 +61,7 @@
QuicPacketNumber packet_number,
QuicByteCount bytes,
HasRetransmittableData is_retransmittable) override;
+ void OnPacketNeutered(QuicPacketNumber /*packet_number*/) override {}
void OnRetransmissionTimeout(bool packets_retransmitted) override;
bool CanSend(QuicByteCount bytes_in_flight) override;
QuicBandwidth PacingRate(QuicByteCount bytes_in_flight) const override;
diff --git a/quic/core/crypto/crypto_protocol.h b/quic/core/crypto/crypto_protocol.h
index 54a91e2..aa383b9 100644
--- a/quic/core/crypto/crypto_protocol.h
+++ b/quic/core/crypto/crypto_protocol.h
@@ -121,6 +121,9 @@
const QuicTag kIW20 = TAG('I', 'W', '2', '0'); // Force ICWND to 20
const QuicTag kIW50 = TAG('I', 'W', '5', '0'); // Force ICWND to 50
const QuicTag kB2ON = TAG('B', '2', 'O', 'N'); // Enable BBRv2
+const QuicTag kBSAO = TAG('B', 'S', 'A', 'O'); // Avoid Overestimation in
+ // Bandwidth Sampler with ack
+ // aggregation
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_connection_test.cc b/quic/core/quic_connection_test.cc
index 28524c8..c17424e 100644
--- a/quic/core/quic_connection_test.cc
+++ b/quic/core/quic_connection_test.cc
@@ -1053,6 +1053,7 @@
EXPECT_CALL(*send_algorithm_, CanSend(_)).WillRepeatedly(Return(true));
EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _))
.Times(AnyNumber());
+ EXPECT_CALL(*send_algorithm_, OnPacketNeutered(_)).Times(AnyNumber());
EXPECT_CALL(*send_algorithm_, GetCongestionWindow())
.WillRepeatedly(Return(kDefaultTCPMSS));
EXPECT_CALL(*send_algorithm_, PacingRate(_))
diff --git a/quic/core/quic_sent_packet_manager.cc b/quic/core/quic_sent_packet_manager.cc
index e4f46d8..a055299 100644
--- a/quic/core/quic_sent_packet_manager.cc
+++ b/quic/core/quic_sent_packet_manager.cc
@@ -431,11 +431,25 @@
}
void QuicSentPacketManager::NeuterUnencryptedPackets() {
- unacked_packets_.NeuterUnencryptedPackets();
+ for (QuicPacketNumber packet_number :
+ unacked_packets_.NeuterUnencryptedPackets()) {
+ if (avoid_overestimate_bandwidth_with_aggregation_) {
+ QUIC_RELOADABLE_FLAG_COUNT_N(
+ quic_avoid_overestimate_bandwidth_with_aggregation, 1, 4);
+ send_algorithm_->OnPacketNeutered(packet_number);
+ }
+ }
}
void QuicSentPacketManager::NeuterHandshakePackets() {
- unacked_packets_.NeuterHandshakePackets();
+ for (QuicPacketNumber packet_number :
+ unacked_packets_.NeuterHandshakePackets()) {
+ if (avoid_overestimate_bandwidth_with_aggregation_) {
+ QUIC_RELOADABLE_FLAG_COUNT_N(
+ quic_avoid_overestimate_bandwidth_with_aggregation, 2, 4);
+ send_algorithm_->OnPacketNeutered(packet_number);
+ }
+ }
}
bool QuicSentPacketManager::ShouldAddMaxAckDelay() const {
diff --git a/quic/core/quic_sent_packet_manager.h b/quic/core/quic_sent_packet_manager.h
index d8fc069..59bb356 100644
--- a/quic/core/quic_sent_packet_manager.h
+++ b/quic/core/quic_sent_packet_manager.h
@@ -648,6 +648,9 @@
// True if any 1-RTT packet gets sent.
bool one_rtt_packet_sent_;
+
+ const bool avoid_overestimate_bandwidth_with_aggregation_ =
+ GetQuicReloadableFlag(quic_avoid_overestimate_bandwidth_with_aggregation);
};
} // namespace quic
diff --git a/quic/core/quic_sent_packet_manager_test.cc b/quic/core/quic_sent_packet_manager_test.cc
index 08e918e..60c84a7 100644
--- a/quic/core/quic_sent_packet_manager_test.cc
+++ b/quic/core/quic_sent_packet_manager_test.cc
@@ -111,6 +111,7 @@
.WillRepeatedly(Return(QuicBandwidth::Zero()));
EXPECT_CALL(*send_algorithm_, InSlowStart()).Times(AnyNumber());
EXPECT_CALL(*send_algorithm_, InRecovery()).Times(AnyNumber());
+ EXPECT_CALL(*send_algorithm_, OnPacketNeutered(_)).Times(AnyNumber());
EXPECT_CALL(*network_change_visitor_, OnPathMtuIncreased(1000))
.Times(AnyNumber());
EXPECT_CALL(notifier_, IsFrameOutstanding(_)).WillRepeatedly(Return(true));
@@ -3343,6 +3344,31 @@
manager_.GetRetransmissionTime());
}
+TEST_F(QuicSentPacketManagerTest, SetHandshakeConfirmed) {
+ QuicSentPacketManagerPeer::SetPerspective(&manager_, Perspective::IS_CLIENT);
+ manager_.EnableMultiplePacketNumberSpacesSupport();
+
+ SendDataPacket(1, ENCRYPTION_INITIAL);
+
+ SendDataPacket(2, ENCRYPTION_HANDSHAKE);
+
+ EXPECT_CALL(notifier_, OnFrameAcked(_, _, _))
+ .WillOnce(
+ Invoke([](const QuicFrame& /*frame*/, QuicTime::Delta ack_delay_time,
+ QuicTime receive_timestamp) {
+ EXPECT_TRUE(ack_delay_time.IsZero());
+ EXPECT_EQ(receive_timestamp, QuicTime::Zero());
+ return true;
+ }));
+
+ if (GetQuicReloadableFlag(
+ quic_avoid_overestimate_bandwidth_with_aggregation)) {
+ EXPECT_CALL(*send_algorithm_, OnPacketNeutered(QuicPacketNumber(2)))
+ .Times(1);
+ }
+ manager_.SetHandshakeConfirmed();
+}
+
// Regresstion test for b/148841700.
TEST_F(QuicSentPacketManagerTest, NeuterUnencryptedPackets) {
SendCryptoPacket(1);
@@ -3357,6 +3383,11 @@
EXPECT_CALL(notifier_, OnFrameAcked(_, _, _)).Times(0);
}
EXPECT_CALL(notifier_, IsFrameOutstanding(_)).WillRepeatedly(Return(false));
+ if (GetQuicReloadableFlag(
+ quic_avoid_overestimate_bandwidth_with_aggregation)) {
+ EXPECT_CALL(*send_algorithm_, OnPacketNeutered(QuicPacketNumber(1)))
+ .Times(1);
+ }
manager_.NeuterUnencryptedPackets();
}
diff --git a/quic/core/quic_unacked_packet_map.cc b/quic/core/quic_unacked_packet_map.cc
index c80593f..c856353 100644
--- a/quic/core/quic_unacked_packet_map.cc
+++ b/quic/core/quic_unacked_packet_map.cc
@@ -207,17 +207,21 @@
RemoveFromInFlight(info);
}
-void QuicUnackedPacketMap::NeuterUnencryptedPackets() {
+QuicInlinedVector<QuicPacketNumber, 2>
+QuicUnackedPacketMap::NeuterUnencryptedPackets() {
+ QuicInlinedVector<QuicPacketNumber, 2> neutered_packets;
QuicPacketNumber packet_number = GetLeastUnacked();
for (QuicUnackedPacketMap::iterator it = unacked_packets_.begin();
it != unacked_packets_.end(); ++it, ++packet_number) {
if (!it->retransmittable_frames.empty() &&
it->encryption_level == ENCRYPTION_INITIAL) {
+ QUIC_DVLOG(2) << "Neutering unencrypted packet " << packet_number;
// Once the connection swithes to forward secure, no unencrypted packets
// will be sent. The data has been abandoned in the cryto stream. Remove
// it from in flight.
RemoveFromInFlight(packet_number);
it->state = NEUTERED;
+ neutered_packets.push_back(packet_number);
if (GetQuicReloadableFlag(quic_neuter_unencrypted_control_frames) ||
supports_multiple_packet_number_spaces_) {
if (GetQuicReloadableFlag(quic_neuter_unencrypted_control_frames)) {
@@ -234,18 +238,23 @@
if (supports_multiple_packet_number_spaces_) {
last_inflight_packets_sent_time_[INITIAL_DATA] = QuicTime::Zero();
}
+ return neutered_packets;
}
-void QuicUnackedPacketMap::NeuterHandshakePackets() {
+QuicInlinedVector<QuicPacketNumber, 2>
+QuicUnackedPacketMap::NeuterHandshakePackets() {
+ QuicInlinedVector<QuicPacketNumber, 2> neutered_packets;
QuicPacketNumber packet_number = GetLeastUnacked();
for (QuicUnackedPacketMap::iterator it = unacked_packets_.begin();
it != unacked_packets_.end(); ++it, ++packet_number) {
if (!it->retransmittable_frames.empty() &&
GetPacketNumberSpace(it->encryption_level) == HANDSHAKE_DATA) {
+ QUIC_DVLOG(2) << "Neutering handshake packet " << packet_number;
RemoveFromInFlight(packet_number);
// Notify session that the data has been delivered (but do not notify
// send algorithm).
it->state = NEUTERED;
+ neutered_packets.push_back(packet_number);
// TODO(b/148868195): use NotifyFramesNeutered.
NotifyFramesAcked(*it, QuicTime::Delta::Zero(), QuicTime::Zero());
}
@@ -253,6 +262,7 @@
if (supports_multiple_packet_number_spaces()) {
last_inflight_packets_sent_time_[HANDSHAKE_DATA] = QuicTime::Zero();
}
+ return neutered_packets;
}
bool QuicUnackedPacketMap::HasInFlightPackets() const {
diff --git a/quic/core/quic_unacked_packet_map.h b/quic/core/quic_unacked_packet_map.h
index d155fcc..8492fb0 100644
--- a/quic/core/quic_unacked_packet_map.h
+++ b/quic/core/quic_unacked_packet_map.h
@@ -66,13 +66,13 @@
void RemoveFromInFlight(QuicPacketNumber packet_number);
// Called to neuter all unencrypted packets to ensure they do not get
- // retransmitted.
- void NeuterUnencryptedPackets();
+ // retransmitted. Returns a vector of neutered packet numbers.
+ QuicInlinedVector<QuicPacketNumber, 2> NeuterUnencryptedPackets();
// Called to neuter packets in handshake packet number space to ensure they do
- // not get retransmitted.
+ // not get retransmitted. Returns a vector of neutered packet numbers.
// TODO(fayang): Consider to combine this with NeuterUnencryptedPackets.
- void NeuterHandshakePackets();
+ QuicInlinedVector<QuicPacketNumber, 2> NeuterHandshakePackets();
// Returns true if |packet_number| has retransmittable frames. This will
// return false if all frames of this packet are either non-retransmittable or
diff --git a/quic/test_tools/quic_test_utils.h b/quic/test_tools/quic_test_utils.h
index 7ecac5b..035e15b 100644
--- a/quic/test_tools/quic_test_utils.h
+++ b/quic/test_tools/quic_test_utils.h
@@ -940,6 +940,7 @@
QuicPacketNumber,
QuicByteCount,
HasRetransmittableData));
+ MOCK_METHOD1(OnPacketNeutered, void(QuicPacketNumber));
MOCK_METHOD1(OnRetransmissionTimeout, void(bool));
MOCK_METHOD0(OnConnectionMigration, void());
MOCK_METHOD0(RevertRetransmissionTimeout, void());
