Internal QUICHE change PiperOrigin-RevId: 351401173 Change-Id: I8d4a5e264aa86b56a94c110b1e832abed172bd4e
diff --git a/quic/core/congestion_control/rtt_stats.cc b/quic/core/congestion_control/rtt_stats.cc index ecea766..8754966 100644 --- a/quic/core/congestion_control/rtt_stats.cc +++ b/quic/core/congestion_control/rtt_stats.cc
@@ -128,4 +128,17 @@ return QuicTime::Delta::FromMicroseconds(sqrt(m2)); } +void RttStats::CloneFrom(const RttStats& stats) { + latest_rtt_ = stats.latest_rtt_; + min_rtt_ = stats.min_rtt_; + smoothed_rtt_ = stats.smoothed_rtt_; + previous_srtt_ = stats.previous_srtt_; + mean_deviation_ = stats.mean_deviation_; + standard_deviation_calculator_ = stats.standard_deviation_calculator_; + calculate_standard_deviation_ = stats.calculate_standard_deviation_; + initial_rtt_ = stats.initial_rtt_; + last_update_time_ = stats.last_update_time_; + ignore_max_ack_delay_ = stats.ignore_max_ack_delay_; +} + } // namespace quic
diff --git a/quic/core/congestion_control/rtt_stats.h b/quic/core/congestion_control/rtt_stats.h index d1e5957..1911e38 100644 --- a/quic/core/congestion_control/rtt_stats.h +++ b/quic/core/congestion_control/rtt_stats.h
@@ -111,6 +111,8 @@ calculate_standard_deviation_ = true; } + void CloneFrom(const RttStats& stats); + private: friend class test::RttStatsPeer;
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc index 0a093f1..67177a5 100644 --- a/quic/core/quic_connection.cc +++ b/quic/core/quic_connection.cc
@@ -4584,7 +4584,10 @@ } } visitor_->OnConnectionMigration(addr_change_type); - sent_packet_manager_.OnConnectionMigration(addr_change_type); + if (addr_change_type != PORT_CHANGE && + addr_change_type != IPV4_SUBNET_CHANGE) { + sent_packet_manager_.OnConnectionMigration(/*reset_send_algorithm=*/false); + } } bool QuicConnection::IsCurrentPacketConnectivityProbing() const {
diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc index dc434a5..609f28c 100644 --- a/quic/core/quic_connection_test.cc +++ b/quic/core/quic_connection_test.cc
@@ -1632,6 +1632,16 @@ QuicSocketAddress()); EXPECT_FALSE(connection_.effective_peer_address().IsInitialized()); + RttStats* rtt_stats = const_cast<RttStats*>(manager_->GetRttStats()); + QuicTime::Delta default_init_rtt = rtt_stats->initial_rtt(); + rtt_stats->set_initial_rtt(default_init_rtt * 2); + EXPECT_EQ(2 * default_init_rtt, rtt_stats->initial_rtt()); + + QuicSentPacketManagerPeer::SetConsecutiveRtoCount(manager_, 1); + EXPECT_EQ(1u, manager_->GetConsecutiveRtoCount()); + QuicSentPacketManagerPeer::SetConsecutiveTlpCount(manager_, 2); + EXPECT_EQ(2u, manager_->GetConsecutiveTlpCount()); + const QuicSocketAddress kNewPeerAddress = QuicSocketAddress(QuicIpAddress::Loopback6(), /*port=*/23456); EXPECT_CALL(visitor_, OnStreamFrame(_)) @@ -1660,6 +1670,10 @@ ENCRYPTION_FORWARD_SECURE); EXPECT_EQ(kNewPeerAddress, connection_.peer_address()); EXPECT_EQ(kNewPeerAddress, connection_.effective_peer_address()); + // PORT_CHANGE shouldn't state change in sent packet manager. + EXPECT_EQ(2 * default_init_rtt, rtt_stats->initial_rtt()); + EXPECT_EQ(1u, manager_->GetConsecutiveRtoCount()); + EXPECT_EQ(2u, manager_->GetConsecutiveTlpCount()); } TEST_P(QuicConnectionTest, EffectivePeerAddressChangeAtServer) {
diff --git a/quic/core/quic_sent_packet_manager.cc b/quic/core/quic_sent_packet_manager.cc index 08b1371..ce49cc7 100644 --- a/quic/core/quic_sent_packet_manager.cc +++ b/quic/core/quic_sent_packet_manager.cc
@@ -10,6 +10,7 @@ #include "quic/core/congestion_control/general_loss_algorithm.h" #include "quic/core/congestion_control/pacing_sender.h" +#include "quic/core/congestion_control/send_algorithm_interface.h" #include "quic/core/crypto/crypto_protocol.h" #include "quic/core/frames/quic_ack_frequency_frame.h" #include "quic/core/proto/cached_network_parameters_proto.h" @@ -1244,7 +1245,8 @@ QuicTime::Delta ack_delay_time, QuicTime ack_receive_time) { // We rely on ack_delay_time to compute an RTT estimate, so we - // only update rtt when the largest observed gets acked. + // only update rtt when the largest observed gets acked and the acked packet + // is not useless. if (!unacked_packets_.IsUnacked(largest_acked)) { return false; } @@ -1258,6 +1260,9 @@ << largest_acked; return false; } + if (transmission_info.state == NOT_CONTRIBUTING_RTT) { + return false; + } if (transmission_info.sent_time > ack_receive_time) { QUIC_CODE_COUNT(quic_receive_acked_before_sending); } @@ -1544,17 +1549,37 @@ pacing_sender_.set_sender(send_algorithm); } -void QuicSentPacketManager::OnConnectionMigration(AddressChangeType type) { - if (type == PORT_CHANGE || type == IPV4_SUBNET_CHANGE) { - // Rtt and cwnd do not need to be reset when the peer address change is - // considered to be caused by NATs. - return; - } +std::unique_ptr<SendAlgorithmInterface> +QuicSentPacketManager::OnConnectionMigration(bool reset_send_algorithm) { consecutive_rto_count_ = 0; consecutive_tlp_count_ = 0; consecutive_pto_count_ = 0; rtt_stats_.OnConnectionMigration(); - send_algorithm_->OnConnectionMigration(); + if (!reset_send_algorithm) { + send_algorithm_->OnConnectionMigration(); + return nullptr; + } + + std::unique_ptr<SendAlgorithmInterface> old_send_algorithm = + std::move(send_algorithm_); + SetSendAlgorithm(old_send_algorithm->GetCongestionControlType()); + // Treat all in flight packets sent to the old peer address as lost and + // retransmit them. + QuicPacketNumber packet_number = unacked_packets_.GetLeastUnacked(); + for (auto it = unacked_packets_.begin(); it != unacked_packets_.end(); + ++it, ++packet_number) { + if (it->in_flight) { + // Proactively retransmit any packet which is in flight on the old path. + // As a result, these packets will not contribute to congestion control. + unacked_packets_.RemoveFromInFlight(packet_number); + // Retransmitting these packets with PATH_CHANGE_RETRANSMISSION will mark + // them as useless, thus not contributing to RTT stats. + MarkForRetransmission(packet_number, PATH_RETRANSMISSION); + } else { + it->state = NOT_CONTRIBUTING_RTT; + } + } + return old_send_algorithm; } void QuicSentPacketManager::OnAckFrameStart(QuicPacketNumber largest_acked,
diff --git a/quic/core/quic_sent_packet_manager.h b/quic/core/quic_sent_packet_manager.h index a32742f..0fe207b 100644 --- a/quic/core/quic_sent_packet_manager.h +++ b/quic/core/quic_sent_packet_manager.h
@@ -237,6 +237,10 @@ const RttStats* GetRttStats() const { return &rtt_stats_; } + void SetRttStats(const RttStats& rtt_stats) { + rtt_stats_.CloneFrom(rtt_stats); + } + // Returns the estimated bandwidth calculated by the congestion algorithm. QuicBandwidth BandwidthEstimate() const { return send_algorithm_->BandwidthEstimate(); @@ -295,8 +299,12 @@ return unacked_packets_.bytes_in_flight(); } - // Called when peer address changes and the connection migrates. - void OnConnectionMigration(AddressChangeType type); + // Called when peer address changes. Must be called IFF the address change is + // not NAT rebinding. If reset_send_algorithm is true, switch to a new send + // algorithm object and retransmit all the in-flight packets. Return the send + // algorithm object used on the previous path. + std::unique_ptr<SendAlgorithmInterface> OnConnectionMigration( + bool reset_send_algorithm); // Called when an ack frame is initially parsed. void OnAckFrameStart(QuicPacketNumber largest_acked,
diff --git a/quic/core/quic_sent_packet_manager_test.cc b/quic/core/quic_sent_packet_manager_test.cc index 968465c..b2ab341 100644 --- a/quic/core/quic_sent_packet_manager_test.cc +++ b/quic/core/quic_sent_packet_manager_test.cc
@@ -2235,14 +2235,21 @@ EXPECT_EQ(2u, manager_.GetConsecutiveTlpCount()); EXPECT_CALL(*send_algorithm_, OnConnectionMigration()); - manager_.OnConnectionMigration(IPV4_TO_IPV4_CHANGE); + EXPECT_EQ(nullptr, + manager_.OnConnectionMigration(/*reset_send_algorithm=*/false)); EXPECT_EQ(default_init_rtt, rtt_stats->initial_rtt()); EXPECT_EQ(0u, manager_.GetConsecutiveRtoCount()); EXPECT_EQ(0u, manager_.GetConsecutiveTlpCount()); } -TEST_F(QuicSentPacketManagerTest, ConnectionMigrationIPSubnetChange) { +// Tests that ResetCongestionControlUponPeerAddressChange() resets send +// algorithm and RTT. And unACK'ed packets are handled correctly. +TEST_F(QuicSentPacketManagerTest, + ConnectionMigrationUnspecifiedChangeResetSendAlgorithm) { + auto loss_algorithm = std::make_unique<MockLossAlgorithm>(); + QuicSentPacketManagerPeer::SetLossAlgorithm(&manager_, loss_algorithm.get()); + RttStats* rtt_stats = const_cast<RttStats*>(manager_.GetRttStats()); QuicTime::Delta default_init_rtt = rtt_stats->initial_rtt(); rtt_stats->set_initial_rtt(default_init_rtt * 2); @@ -2253,29 +2260,128 @@ QuicSentPacketManagerPeer::SetConsecutiveTlpCount(&manager_, 2); EXPECT_EQ(2u, manager_.GetConsecutiveTlpCount()); - manager_.OnConnectionMigration(IPV4_SUBNET_CHANGE); + SendDataPacket(1, ENCRYPTION_FORWARD_SECURE); - EXPECT_EQ(2 * default_init_rtt, rtt_stats->initial_rtt()); - EXPECT_EQ(1u, manager_.GetConsecutiveRtoCount()); - EXPECT_EQ(2u, manager_.GetConsecutiveTlpCount()); -} + RttStats old_rtt_stats; + old_rtt_stats.CloneFrom(*manager_.GetRttStats()); -TEST_F(QuicSentPacketManagerTest, ConnectionMigrationPortChange) { - RttStats* rtt_stats = const_cast<RttStats*>(manager_.GetRttStats()); - QuicTime::Delta default_init_rtt = rtt_stats->initial_rtt(); - rtt_stats->set_initial_rtt(default_init_rtt * 2); - EXPECT_EQ(2 * default_init_rtt, rtt_stats->initial_rtt()); + // Packet1 will be mark for retransmission upon migration. + EXPECT_CALL(notifier_, OnFrameLost(_)); + std::unique_ptr<SendAlgorithmInterface> old_send_algorithm = + manager_.OnConnectionMigration(/*reset_send_algorithm=*/true); - QuicSentPacketManagerPeer::SetConsecutiveRtoCount(&manager_, 1); - EXPECT_EQ(1u, manager_.GetConsecutiveRtoCount()); - QuicSentPacketManagerPeer::SetConsecutiveTlpCount(&manager_, 2); - EXPECT_EQ(2u, manager_.GetConsecutiveTlpCount()); + EXPECT_NE(old_send_algorithm.get(), manager_.GetSendAlgorithm()); + EXPECT_EQ(old_send_algorithm->GetCongestionControlType(), + manager_.GetSendAlgorithm()->GetCongestionControlType()); + EXPECT_EQ(default_init_rtt, rtt_stats->initial_rtt()); + EXPECT_EQ(0u, manager_.GetConsecutiveRtoCount()); + EXPECT_EQ(0u, manager_.GetConsecutiveTlpCount()); + // Packets sent earlier shouldn't be regarded as in flight. + EXPECT_EQ(0u, BytesInFlight()); - manager_.OnConnectionMigration(PORT_CHANGE); + // Replace the new send algorithm with the mock object. + manager_.SetSendAlgorithm(old_send_algorithm.release()); - EXPECT_EQ(2 * default_init_rtt, rtt_stats->initial_rtt()); - EXPECT_EQ(1u, manager_.GetConsecutiveRtoCount()); - EXPECT_EQ(2u, manager_.GetConsecutiveTlpCount()); + clock_.AdvanceTime(QuicTime::Delta::FromMilliseconds(10)); + // Application retransmit the data as LOSS_RETRANSMISSION. + RetransmitDataPacket(2, LOSS_RETRANSMISSION, ENCRYPTION_FORWARD_SECURE); + EXPECT_EQ(kDefaultLength, BytesInFlight()); + + clock_.AdvanceTime(QuicTime::Delta::FromMilliseconds(10)); + // Receiving an ACK for packet1 20s later shouldn't update the RTT, and + // shouldn't be treated as spurious retransmission. + EXPECT_CALL(*send_algorithm_, + OnCongestionEvent(/*rtt_updated=*/false, kDefaultLength, _, _, _)) + .WillOnce(testing::WithArg<3>( + Invoke([](const AckedPacketVector& acked_packets) { + EXPECT_EQ(1u, acked_packets.size()); + EXPECT_EQ(QuicPacketNumber(1), acked_packets[0].packet_number); + // The bytes in packet1 shouldn't contribute to congestion control. + EXPECT_EQ(0u, acked_packets[0].bytes_acked); + }))); + EXPECT_CALL(*network_change_visitor_, OnCongestionChange()); + manager_.OnAckFrameStart(QuicPacketNumber(1), QuicTime::Delta::Infinite(), + clock_.Now()); + manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)); + EXPECT_CALL(*loss_algorithm, DetectLosses(_, _, _, _, _, _)); + EXPECT_CALL(*loss_algorithm, SpuriousLossDetected(_, _, _, _, _)).Times(0u); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(1), + ENCRYPTION_FORWARD_SECURE)); + EXPECT_TRUE(manager_.GetRttStats()->latest_rtt().IsZero()); + + // Receiving an ACK for packet2 should update RTT and congestion control. + manager_.OnAckFrameStart(QuicPacketNumber(2), QuicTime::Delta::Infinite(), + clock_.Now()); + manager_.OnAckRange(QuicPacketNumber(2), QuicPacketNumber(3)); + EXPECT_CALL(*loss_algorithm, DetectLosses(_, _, _, _, _, _)); + EXPECT_CALL(*send_algorithm_, + OnCongestionEvent(/*rtt_updated=*/true, kDefaultLength, _, _, _)) + .WillOnce(testing::WithArg<3>( + Invoke([](const AckedPacketVector& acked_packets) { + EXPECT_EQ(1u, acked_packets.size()); + EXPECT_EQ(QuicPacketNumber(2), acked_packets[0].packet_number); + // The bytes in packet2 should contribute to congestion control. + EXPECT_EQ(kDefaultLength, acked_packets[0].bytes_acked); + }))); + EXPECT_CALL(*network_change_visitor_, OnCongestionChange()); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(2), + ENCRYPTION_FORWARD_SECURE)); + EXPECT_EQ(0u, BytesInFlight()); + EXPECT_EQ(QuicTime::Delta::FromMilliseconds(10), + manager_.GetRttStats()->latest_rtt()); + + SendDataPacket(3, ENCRYPTION_FORWARD_SECURE); + // Trigger loss timeout and makr packet3 for retransmission. + EXPECT_CALL(*loss_algorithm, GetLossTimeout()) + .WillOnce(Return(clock_.Now() + QuicTime::Delta::FromMilliseconds(10))); + EXPECT_CALL(*loss_algorithm, DetectLosses(_, _, _, _, _, _)) + .WillOnce(WithArgs<5>(Invoke([](LostPacketVector* packet_lost) { + packet_lost->emplace_back(QuicPacketNumber(3u), kDefaultLength); + return LossDetectionInterface::DetectionStats(); + }))); + EXPECT_CALL(notifier_, OnFrameLost(_)); + EXPECT_CALL(*send_algorithm_, + OnCongestionEvent(false, kDefaultLength, _, _, _)); + EXPECT_CALL(*network_change_visitor_, OnCongestionChange()); + manager_.OnRetransmissionTimeout(); + EXPECT_EQ(0u, BytesInFlight()); + + // Migrate again with unACK'ed but not in-flight packet. + // Packet3 shouldn't be marked for retransmission again as it is not in + // flight. + old_send_algorithm = + manager_.OnConnectionMigration(/*reset_send_algorithm=*/true); + + EXPECT_NE(old_send_algorithm.get(), manager_.GetSendAlgorithm()); + EXPECT_EQ(old_send_algorithm->GetCongestionControlType(), + manager_.GetSendAlgorithm()->GetCongestionControlType()); + EXPECT_EQ(default_init_rtt, rtt_stats->initial_rtt()); + EXPECT_EQ(0u, manager_.GetConsecutiveRtoCount()); + EXPECT_EQ(0u, manager_.GetConsecutiveTlpCount()); + EXPECT_EQ(0u, BytesInFlight()); + EXPECT_TRUE(manager_.GetRttStats()->latest_rtt().IsZero()); + + manager_.SetSendAlgorithm(old_send_algorithm.release()); + + clock_.AdvanceTime(QuicTime::Delta::FromMilliseconds(30)); + // Receiving an ACK for packet3 shouldn't update RTT. Though packet 3 was + // marked lost, this spurious retransmission shouldn't be reported to the loss + // algorithm. + manager_.OnAckFrameStart(QuicPacketNumber(3), QuicTime::Delta::Infinite(), + clock_.Now()); + manager_.OnAckRange(QuicPacketNumber(3), QuicPacketNumber(4)); + EXPECT_CALL(*loss_algorithm, DetectLosses(_, _, _, _, _, _)); + EXPECT_CALL(*loss_algorithm, SpuriousLossDetected(_, _, _, _, _)).Times(0u); + EXPECT_CALL(*send_algorithm_, + OnCongestionEvent(/*rtt_updated=*/false, 0, _, _, _)); + EXPECT_CALL(*network_change_visitor_, OnCongestionChange()); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(3), + ENCRYPTION_FORWARD_SECURE)); + EXPECT_EQ(0u, BytesInFlight()); + EXPECT_TRUE(manager_.GetRttStats()->latest_rtt().IsZero()); } TEST_F(QuicSentPacketManagerTest, PathMtuIncreased) {
diff --git a/quic/core/quic_types.cc b/quic/core/quic_types.cc index 372f953..2cb0625 100644 --- a/quic/core/quic_types.cc +++ b/quic/core/quic_types.cc
@@ -214,6 +214,7 @@ RETURN_STRING_LITERAL(TLP_RETRANSMISSION); RETURN_STRING_LITERAL(PTO_RETRANSMISSION); RETURN_STRING_LITERAL(PROBING_RETRANSMISSION); + RETURN_STRING_LITERAL(PATH_RETRANSMISSION); default: // Some varz rely on this behavior for statistic collection. if (transmission_type == LAST_TRANSMISSION_TYPE + 1) {
diff --git a/quic/core/quic_types.h b/quic/core/quic_types.h index 2d89ce6..4c76ab6 100644 --- a/quic/core/quic_types.h +++ b/quic/core/quic_types.h
@@ -173,7 +173,9 @@ TLP_RETRANSMISSION, // Tail loss probes. PTO_RETRANSMISSION, // Retransmission due to probe timeout. PROBING_RETRANSMISSION, // Retransmission in order to probe bandwidth. - LAST_TRANSMISSION_TYPE = PROBING_RETRANSMISSION, + PATH_RETRANSMISSION, // Retransmission proactively due to underlying + // network change. + LAST_TRANSMISSION_TYPE = PATH_RETRANSMISSION, }; QUIC_EXPORT_PRIVATE std::string TransmissionTypeToString( @@ -530,10 +532,11 @@ PTO_RETRANSMITTED, // This packet has been retransmitted for probing purpose. PROBE_RETRANSMITTED, - // Do not collect RTT sample if this packet is the largest_acked of an - // incoming ACK. + // This packet is sent on a different path or is a PING only packet. + // Do not update RTT stats and congestion control if the packet is the + // largest_acked of an incoming ACK. NOT_CONTRIBUTING_RTT, - LAST_PACKET_STATE = PROBE_RETRANSMITTED, + LAST_PACKET_STATE = NOT_CONTRIBUTING_RTT, }; enum PacketHeaderFormat : uint8_t {
diff --git a/quic/core/quic_unacked_packet_map.cc b/quic/core/quic_unacked_packet_map.cc index 31c386a..acb30d5 100644 --- a/quic/core/quic_unacked_packet_map.cc +++ b/quic/core/quic_unacked_packet_map.cc
@@ -10,6 +10,7 @@ #include "quic/core/quic_connection_stats.h" #include "quic/core/quic_packet_number.h" +#include "quic/core/quic_types.h" #include "quic/core/quic_utils.h" #include "quic/platform/api/quic_bug_tracker.h" #include "quic/platform/api/quic_flag_utils.h"
diff --git a/quic/core/quic_utils.cc b/quic/core/quic_utils.cc index 5d4d512..cf291c7 100644 --- a/quic/core/quic_utils.cc +++ b/quic/core/quic_utils.cc
@@ -170,7 +170,7 @@ RETURN_STRING_LITERAL(RTO_RETRANSMITTED); RETURN_STRING_LITERAL(PTO_RETRANSMITTED); RETURN_STRING_LITERAL(PROBE_RETRANSMITTED); - RETURN_STRING_LITERAL(NOT_CONTRIBUTING_RTT) + RETURN_STRING_LITERAL(NOT_CONTRIBUTING_RTT); } return "INVALID_SENT_PACKET_STATE"; } @@ -351,6 +351,8 @@ return PTO_RETRANSMITTED; case PROBING_RETRANSMISSION: return PROBE_RETRANSMITTED; + case PATH_RETRANSMISSION: + return NOT_CONTRIBUTING_RTT; default: QUIC_BUG << retransmission_type << " is not a retransmission_type"; return UNACKABLE;
diff --git a/quic/core/quic_utils_test.cc b/quic/core/quic_utils_test.cc index f99a332..f57173d 100644 --- a/quic/core/quic_utils_test.cc +++ b/quic/core/quic_utils_test.cc
@@ -10,6 +10,7 @@ #include "absl/strings/string_view.h" #include "quic/core/crypto/crypto_protocol.h" #include "quic/core/quic_connection_id.h" +#include "quic/core/quic_types.h" #include "quic/platform/api/quic_test.h" #include "quic/test_tools/quic_test_utils.h" @@ -135,6 +136,8 @@ EXPECT_EQ(PTO_RETRANSMITTED, state); } else if (i == PROBING_RETRANSMISSION) { EXPECT_EQ(PROBE_RETRANSMITTED, state); + } else if (i == PATH_RETRANSMISSION) { + EXPECT_EQ(NOT_CONTRIBUTING_RTT, state); } else { DCHECK(false) << "No corresponding packet state according to transmission type: "