gfe-relnote: Do not add peer_max_ack_delay if an immediate ACK is expected when calculating PTO timeout. Protected by existing gfe2_reloadable_flag_quic_enable_pto. PiperOrigin-RevId: 277922554 Change-Id: I228b6bd1a299b8cb9a3f71eef089b680af1a80c0
diff --git a/quic/core/crypto/crypto_protocol.h b/quic/core/crypto/crypto_protocol.h index 4d5a890..2d0f6d3 100644 --- a/quic/core/crypto/crypto_protocol.h +++ b/quic/core/crypto/crypto_protocol.h
@@ -194,6 +194,10 @@ // consecutive PTOs. const QuicTag kPTOS = TAG('P', 'T', 'O', 'S'); // Skip packet number before // sending the last PTO. +const QuicTag kPTOA = TAG('P', 'T', 'O', 'A'); // Do not add max ack delay + // when computing PTO timeout + // if an immediate ACK is + // expected. // Optional support of truncated Connection IDs. If sent by a peer, the value // is the minimum number of bytes allowed for the connection ID sent to the
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc index aa4ee06..d2ec4c8 100644 --- a/quic/core/quic_connection.cc +++ b/quic/core/quic_connection.cc
@@ -326,7 +326,6 @@ bytes_received_before_address_validation_(0), bytes_sent_before_address_validation_(0), address_validated_(false), - skip_packet_number_for_pto_(false), treat_queued_packets_as_sent_( GetQuicReloadableFlag(quic_treat_queued_packets_as_sent)), mtu_discovery_v2_(GetQuicReloadableFlag(quic_mtu_discovery_v2)) { @@ -445,16 +444,11 @@ } if (config.HasClientSentConnectionOption(k7PTO, perspective_)) { max_consecutive_ptos_ = 6; - QUIC_RELOADABLE_FLAG_COUNT_N(quic_enable_pto, 3, 4); + QUIC_RELOADABLE_FLAG_COUNT_N(quic_enable_pto, 3, 5); } if (config.HasClientSentConnectionOption(k8PTO, perspective_)) { max_consecutive_ptos_ = 7; - QUIC_RELOADABLE_FLAG_COUNT_N(quic_enable_pto, 4, 4); - } - if (GetQuicReloadableFlag(quic_skip_packet_number_for_pto) && - config.HasClientSentConnectionOption(kPTOS, perspective_)) { - QUIC_RELOADABLE_FLAG_COUNT(quic_skip_packet_number_for_pto); - skip_packet_number_for_pto_ = true; + QUIC_RELOADABLE_FLAG_COUNT_N(quic_enable_pto, 4, 5); } } if (config.HasClientSentConnectionOption(kNSTP, perspective_)) { @@ -2649,7 +2643,7 @@ const auto retransmission_mode = sent_packet_manager_.OnRetransmissionTimeout(); - if (skip_packet_number_for_pto_ && + if (sent_packet_manager_.skip_packet_number_for_pto() && retransmission_mode == QuicSentPacketManager::PTO_MODE && sent_packet_manager_.pending_timer_transmission_count() == 1) { // Skip a packet number when a single PTO packet is sent to elicit an
diff --git a/quic/core/quic_connection.h b/quic/core/quic_connection.h index f618ddf..7098cc8 100644 --- a/quic/core/quic_connection.h +++ b/quic/core/quic_connection.h
@@ -1496,9 +1496,6 @@ // EnforceAntiAmplificationLimit returns true. bool address_validated_; - // If true, skip packet number before sending the last PTO retransmission. - bool skip_packet_number_for_pto_; - // Used to store content of packets which cannot be sent because of write // blocked. Packets' encrypted buffers are copied and owned by // buffered_packets_. From unacked_packet_map (and congestion control)'s
diff --git a/quic/core/quic_constants.h b/quic/core/quic_constants.h index 3dc462c..6b22bbf 100644 --- a/quic/core/quic_constants.h +++ b/quic/core/quic_constants.h
@@ -245,6 +245,15 @@ // packet is lost due to early retransmission by time based loss detection. static const int kDefaultLossDelayShift = 2; +// Maximum number of retransmittable packets received before sending an ack. +const QuicPacketCount kDefaultRetransmittablePacketsBeforeAck = 2; +// Wait for up to 10 retransmittable packets before sending an ack. +const QuicPacketCount kMaxRetransmittablePacketsBeforeAck = 10; +// Minimum number of packets received before ack decimation is enabled. +// This intends to avoid the beginning of slow start, when CWNDs may be +// rapidly increasing. +const QuicPacketCount kMinReceivedBeforeAckDecimation = 100; + // Packet number of first sending packet of a connection. Please note, this // cannot be used as first received packet because peer can choose its starting // packet number.
diff --git a/quic/core/quic_received_packet_manager.cc b/quic/core/quic_received_packet_manager.cc index e21f4c4..908d04f 100644 --- a/quic/core/quic_received_packet_manager.cc +++ b/quic/core/quic_received_packet_manager.cc
@@ -25,14 +25,6 @@ // against an ack loss const size_t kMaxPacketsAfterNewMissing = 4; -// Maximum number of retransmittable packets received before sending an ack. -const QuicPacketCount kDefaultRetransmittablePacketsBeforeAck = 2; -// Minimum number of packets received before ack decimation is enabled. -// This intends to avoid the beginning of slow start, when CWNDs may be -// rapidly increasing. -const QuicPacketCount kMinReceivedBeforeAckDecimation = 100; -// Wait for up to 10 retransmittable packets before sending an ack. -const QuicPacketCount kMaxRetransmittablePacketsBeforeAck = 10; // One quarter RTT delay when doing ack decimation. const float kAckDecimationDelay = 0.25; // One eighth RTT delay when doing ack decimation.
diff --git a/quic/core/quic_sent_packet_manager.cc b/quic/core/quic_sent_packet_manager.cc index 2b52dd2..48a11c8 100644 --- a/quic/core/quic_sent_packet_manager.cc +++ b/quic/core/quic_sent_packet_manager.cc
@@ -102,6 +102,8 @@ consecutive_pto_count_(0), handshake_mode_disabled_(false), forward_secure_packet_acked_(false), + skip_packet_number_for_pto_(false), + always_include_max_ack_delay_for_pto_timeout_(true), neuter_handshake_packets_once_( GetQuicReloadableFlag(quic_neuter_handshake_packets_once)) { SetSendAlgorithm(congestion_control_type); @@ -148,15 +150,33 @@ if (GetQuicReloadableFlag(quic_enable_pto)) { if (config.HasClientSentConnectionOption(k2PTO, perspective)) { pto_enabled_ = true; - QUIC_RELOADABLE_FLAG_COUNT_N(quic_enable_pto, 2, 4); + QUIC_RELOADABLE_FLAG_COUNT_N(quic_enable_pto, 2, 5); } if (config.HasClientSentConnectionOption(k1PTO, perspective)) { pto_enabled_ = true; max_probe_packets_per_pto_ = 1; - QUIC_RELOADABLE_FLAG_COUNT_N(quic_enable_pto, 1, 4); + QUIC_RELOADABLE_FLAG_COUNT_N(quic_enable_pto, 1, 5); } } + if (GetQuicReloadableFlag(quic_skip_packet_number_for_pto) && + config.HasClientSentConnectionOption(kPTOS, perspective)) { + QUIC_RELOADABLE_FLAG_COUNT(quic_skip_packet_number_for_pto); + if (!pto_enabled_) { + QUIC_PEER_BUG + << "PTO is not enabled when receiving PTOS connection option."; + pto_enabled_ = true; + max_probe_packets_per_pto_ = 1; + } + skip_packet_number_for_pto_ = true; + } + + if (pto_enabled_ && + config.HasClientSentConnectionOption(kPTOA, perspective)) { + QUIC_RELOADABLE_FLAG_COUNT_N(quic_enable_pto, 5, 5); + always_include_max_ack_delay_for_pto_timeout_ = false; + } + // Configure congestion control. if (config.HasClientRequestedIndependentOption(kTBBR, perspective)) { SetSendAlgorithm(kBBR); @@ -430,6 +450,37 @@ } } +bool QuicSentPacketManager::ShouldAddMaxAckDelay() const { + DCHECK(pto_enabled_); + if (always_include_max_ack_delay_for_pto_timeout_) { + return true; + } + if (!unacked_packets_ + .GetLargestSentRetransmittableOfPacketNumberSpace(APPLICATION_DATA) + .IsInitialized() || + unacked_packets_.GetLargestSentRetransmittableOfPacketNumberSpace( + APPLICATION_DATA) < + FirstSendingPacketNumber() + kMinReceivedBeforeAckDecimation - 1) { + // Peer is doing TCP style acking. Expect an immediate ACK if more than 1 + // packet are outstanding. + if (unacked_packets_.packets_in_flight() >= + kDefaultRetransmittablePacketsBeforeAck) { + return false; + } + } else if (unacked_packets_.packets_in_flight() >= + kMaxRetransmittablePacketsBeforeAck) { + // Peer is doing ack decimation. Expect an immediate ACK if >= 10 + // packets are outstanding. + return false; + } + if (skip_packet_number_for_pto_ && consecutive_pto_count_ > 0) { + // An immediate ACK is expected when doing PTOS. Please note, this will miss + // cases when PTO fires and turns out to be spurious. + return false; + } + return true; +} + void QuicSentPacketManager::MarkForRetransmission( QuicPacketNumber packet_number, TransmissionType transmission_type) { @@ -974,7 +1025,7 @@ rtt_stats_.smoothed_rtt() + std::max(4 * rtt_stats_.mean_deviation(), QuicTime::Delta::FromMilliseconds(1)) + - peer_max_ack_delay_; + (ShouldAddMaxAckDelay() ? peer_max_ack_delay_ : QuicTime::Delta::Zero()); return pto_delay * (1 << consecutive_pto_count_); }
diff --git a/quic/core/quic_sent_packet_manager.h b/quic/core/quic_sent_packet_manager.h index b56b999..f15b3d5 100644 --- a/quic/core/quic_sent_packet_manager.h +++ b/quic/core/quic_sent_packet_manager.h
@@ -395,6 +395,10 @@ return forward_secure_packet_acked_; } + bool skip_packet_number_for_pto() const { + return skip_packet_number_for_pto_; + } + private: friend class test::QuicConnectionPeer; friend class test::QuicSentPacketManagerPeer; @@ -504,6 +508,10 @@ // switches to IETF QUIC with QUIC TLS. void NeuterHandshakePackets(); + // Indicates whether including peer_max_ack_delay_ when calculating PTO + // timeout. + bool ShouldAddMaxAckDelay() const; + // Newly serialized retransmittable packets are added to this map, which // contains owning pointers to any contained frames. If a packet is // retransmitted, this map will contain entries for both the old and the new @@ -617,6 +625,12 @@ // ENCRYPTION_FORWARD_SECURE packet. bool forward_secure_packet_acked_; + // If true, skip packet number before sending the last PTO retransmission. + bool skip_packet_number_for_pto_; + + // If true, always include peer_max_ack_delay_ when calculating PTO timeout. + bool always_include_max_ack_delay_for_pto_timeout_; + // Latched value of quic_neuter_handshake_packets_once. const bool neuter_handshake_packets_once_; };
diff --git a/quic/core/quic_sent_packet_manager_test.cc b/quic/core/quic_sent_packet_manager_test.cc index 9567fc4..f2a72e0 100644 --- a/quic/core/quic_sent_packet_manager_test.cc +++ b/quic/core/quic_sent_packet_manager_test.cc
@@ -2760,6 +2760,128 @@ EXPECT_TRUE(manager_.forward_secure_packet_acked()); } +TEST_F(QuicSentPacketManagerTest, PtoTimeoutIncludesMaxAckDelay) { + EnablePto(k1PTO); + // Use PTOS and PTOA. + QuicConfig config; + QuicTagVector options; + options.push_back(kPTOS); + options.push_back(kPTOA); + QuicConfigPeer::SetReceivedConnectionOptions(&config, options); + EXPECT_CALL(*send_algorithm_, SetFromConfig(_, _)); + EXPECT_CALL(*network_change_visitor_, OnCongestionChange()); + manager_.SetFromConfig(config); + EXPECT_TRUE(manager_.skip_packet_number_for_pto()); + EXPECT_CALL(*send_algorithm_, CanSend(_)).WillRepeatedly(Return(true)); + + EXPECT_CALL(*send_algorithm_, PacingRate(_)) + .WillRepeatedly(Return(QuicBandwidth::Zero())); + EXPECT_CALL(*send_algorithm_, GetCongestionWindow()) + .WillRepeatedly(Return(10 * kDefaultTCPMSS)); + RttStats* rtt_stats = const_cast<RttStats*>(manager_.GetRttStats()); + rtt_stats->UpdateRtt(QuicTime::Delta::FromMilliseconds(100), + QuicTime::Delta::Zero(), QuicTime::Zero()); + QuicTime::Delta srtt = rtt_stats->smoothed_rtt(); + + SendDataPacket(1, ENCRYPTION_FORWARD_SECURE); + // Verify PTO is correctly set and ack delay is included. + QuicTime::Delta expected_pto_delay = + srtt + 4 * rtt_stats->mean_deviation() + + QuicTime::Delta::FromMilliseconds(kDefaultDelayedAckTimeMs); + EXPECT_EQ(clock_.Now() + expected_pto_delay, + manager_.GetRetransmissionTime()); + + clock_.AdvanceTime(QuicTime::Delta::FromMilliseconds(10)); + SendDataPacket(2, ENCRYPTION_FORWARD_SECURE); + // Verify PTO is correctly set based on sent time of packet 2 but ack delay is + // not included as an immediate ACK is expected. + expected_pto_delay = expected_pto_delay - QuicTime::Delta::FromMilliseconds( + kDefaultDelayedAckTimeMs); + EXPECT_EQ(clock_.Now() + expected_pto_delay, + manager_.GetRetransmissionTime()); + EXPECT_EQ(0u, stats_.pto_count); + + // Invoke PTO. + clock_.AdvanceTime(expected_pto_delay); + manager_.OnRetransmissionTimeout(); + EXPECT_EQ(QuicTime::Delta::Zero(), manager_.TimeUntilSend(clock_.Now())); + EXPECT_EQ(1u, stats_.pto_count); + + // Verify 1 probe packets get sent and packet number gets skipped. + EXPECT_CALL(notifier_, RetransmitFrames(_, _)) + .WillOnce(WithArgs<1>(Invoke([this](TransmissionType type) { + RetransmitDataPacket(4, type, ENCRYPTION_FORWARD_SECURE); + }))); + manager_.MaybeSendProbePackets(); + // Verify PTO period gets set to twice the current value. Also, ack delay is + // not included. + QuicTime sent_time = clock_.Now(); + EXPECT_EQ(sent_time + expected_pto_delay * 2, + manager_.GetRetransmissionTime()); + + // Received ACK for packets 1 and 2. + uint64_t acked[] = {1, 2}; + ExpectAcksAndLosses(true, acked, QUIC_ARRAYSIZE(acked), nullptr, 0); + manager_.OnAckFrameStart(QuicPacketNumber(2), QuicTime::Delta::Infinite(), + clock_.Now()); + manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(3)); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(1), + ENCRYPTION_FORWARD_SECURE)); + expected_pto_delay = + rtt_stats->SmoothedOrInitialRtt() + + std::max(4 * rtt_stats->mean_deviation(), + QuicTime::Delta::FromMilliseconds(1)) + + QuicTime::Delta::FromMilliseconds(kDefaultDelayedAckTimeMs); + + // Verify PTO is correctly re-armed based on sent time of packet 4. Because of + // PTOS turns out to be spurious, ACK delay is included. + EXPECT_EQ(sent_time + expected_pto_delay, manager_.GetRetransmissionTime()); + + // Received ACK for packets 4. + ExpectAck(4); + manager_.OnAckFrameStart(QuicPacketNumber(4), QuicTime::Delta::Infinite(), + clock_.Now()); + manager_.OnAckRange(QuicPacketNumber(4), QuicPacketNumber(5)); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(4), + ENCRYPTION_FORWARD_SECURE)); + EXPECT_EQ(QuicTime::Zero(), manager_.GetRetransmissionTime()); + // Send more packets, such that peer will do ack decimation. + std::vector<uint64_t> acked2; + for (size_t i = 5; i <= 100; ++i) { + SendDataPacket(i, ENCRYPTION_FORWARD_SECURE); + acked2.push_back(i); + } + // Received ACK for all sent packets. + ExpectAcksAndLosses(true, &acked2[0], acked2.size(), nullptr, 0); + manager_.OnAckFrameStart(QuicPacketNumber(100), QuicTime::Delta::Infinite(), + clock_.Now()); + manager_.OnAckRange(QuicPacketNumber(5), QuicPacketNumber(101)); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(100), + ENCRYPTION_FORWARD_SECURE)); + + expected_pto_delay = + rtt_stats->SmoothedOrInitialRtt() + + std::max(4 * rtt_stats->mean_deviation(), + QuicTime::Delta::FromMilliseconds(1)) + + QuicTime::Delta::FromMilliseconds(kDefaultDelayedAckTimeMs); + for (size_t i = 101; i < 110; i++) { + SendDataPacket(i, ENCRYPTION_FORWARD_SECURE); + // Verify PTO timeout includes ACK delay as there are less than 10 packets + // outstanding. + EXPECT_EQ(clock_.Now() + expected_pto_delay, + manager_.GetRetransmissionTime()); + } + expected_pto_delay = expected_pto_delay - QuicTime::Delta::FromMilliseconds( + kDefaultDelayedAckTimeMs); + SendDataPacket(110, ENCRYPTION_FORWARD_SECURE); + // Verify ACK delay is excluded. + EXPECT_EQ(clock_.Now() + expected_pto_delay, + manager_.GetRetransmissionTime()); +} + } // namespace } // namespace test } // namespace quic
diff --git a/quic/core/quic_unacked_packet_map.cc b/quic/core/quic_unacked_packet_map.cc index a4497c3..d8beb43 100644 --- a/quic/core/quic_unacked_packet_map.cc +++ b/quic/core/quic_unacked_packet_map.cc
@@ -28,6 +28,7 @@ : perspective_(perspective), least_unacked_(FirstSendingPacketNumber()), bytes_in_flight_(0), + packets_in_flight_(0), last_inflight_packet_sent_time_(QuicTime::Zero()), last_crypto_packet_sent_time_(QuicTime::Zero()), session_notifier_(nullptr), @@ -70,6 +71,7 @@ } if (set_in_flight) { bytes_in_flight_ += bytes_sent; + ++packets_in_flight_; info.in_flight = true; largest_sent_retransmittable_packets_[GetPacketNumberSpace( info.encryption_level)] = packet_number; @@ -191,7 +193,9 @@ void QuicUnackedPacketMap::RemoveFromInFlight(QuicTransmissionInfo* info) { if (info->in_flight) { QUIC_BUG_IF(bytes_in_flight_ < info->bytes_sent); + QUIC_BUG_IF(packets_in_flight_ == 0); bytes_in_flight_ -= info->bytes_sent; + --packets_in_flight_; info->in_flight = false; } }
diff --git a/quic/core/quic_unacked_packet_map.h b/quic/core/quic_unacked_packet_map.h index 6b390b7..fd6510a 100644 --- a/quic/core/quic_unacked_packet_map.h +++ b/quic/core/quic_unacked_packet_map.h
@@ -94,6 +94,7 @@ // Returns the sum of bytes from all packets in flight. QuicByteCount bytes_in_flight() const { return bytes_in_flight_; } + QuicPacketCount packets_in_flight() const { return packets_in_flight_; } // Returns the smallest packet number of a serialized packet which has not // been acked by the peer. If there are no unacked packets, returns 0. @@ -134,6 +135,7 @@ size_t GetNumUnackedPacketsDebugOnly() const; // Returns true if there are multiple packets in flight. + // TODO(fayang): Remove this method and use packets_in_flight_ instead. bool HasMultipleInFlightPackets() const; // Returns true if there are any pending crypto packets. @@ -260,6 +262,7 @@ QuicPacketNumber least_unacked_; QuicByteCount bytes_in_flight_; + QuicPacketCount packets_in_flight_; // Time that the last inflight packet was sent. QuicTime last_inflight_packet_sent_time_;