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_;
