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());