gfe-relnote: In QUIC, skipping a packet number, when a single PTO packet will be sent, to elicit an immediate ACK. Protected by gfe2_reloadable_flag_quic_skip_packet_number_for_pto. PiperOrigin-RevId: 268899762 Change-Id: I5c4313b8c9b2a01392f4728bd6364071657fa0d5
diff --git a/quic/core/crypto/crypto_protocol.h b/quic/core/crypto/crypto_protocol.h index 9b3559d..8adf0ae 100644 --- a/quic/core/crypto/crypto_protocol.h +++ b/quic/core/crypto/crypto_protocol.h
@@ -190,6 +190,8 @@ // consecutive PTOs. const QuicTag k8PTO = TAG('8', 'P', 'T', 'O'); // Closes connection on 8 // consecutive PTOs. +const QuicTag kPTOS = TAG('P', 'T', 'O', 'S'); // Skip packet number before + // sending the last PTO. // 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 ebd2f45..49fc498 100644 --- a/quic/core/quic_connection.cc +++ b/quic/core/quic_connection.cc
@@ -325,7 +325,8 @@ max_consecutive_ptos_(0), bytes_received_before_address_validation_(0), bytes_sent_before_address_validation_(0), - address_validated_(false) { + address_validated_(false), + skip_packet_number_for_pto_(false) { QUIC_DLOG(INFO) << ENDPOINT << "Created connection with server connection ID " << server_connection_id << " and version: " << ParsedQuicVersionToString(version()); @@ -432,6 +433,11 @@ 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; + } } if (config.HasClientSentConnectionOption(kNSTP, perspective_)) { no_stop_waiting_frames_ = true; @@ -2585,6 +2591,15 @@ const auto retransmission_mode = sent_packet_manager_.OnRetransmissionTimeout(); + if (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 + // immediate ACK. + packet_generator_.SkipNPacketNumbers( + 1, sent_packet_manager_.GetLeastUnacked(), + sent_packet_manager_.EstimateMaxPacketsInFlight(max_packet_length())); + } WriteIfNotBlocked(); // A write failure can result in the connection being closed, don't attempt to
diff --git a/quic/core/quic_connection.h b/quic/core/quic_connection.h index e214a99..c811e96 100644 --- a/quic/core/quic_connection.h +++ b/quic/core/quic_connection.h
@@ -1483,6 +1483,9 @@ // packet has been successfully processed. Only used when // EnforceAntiAmplificationLimit returns true. bool address_validated_; + + // If true, skip packet number before sending the last PTO retransmission. + bool skip_packet_number_for_pto_; }; } // namespace quic
diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc index 85489bc..2d07981 100644 --- a/quic/core/quic_connection_test.cc +++ b/quic/core/quic_connection_test.cc
@@ -9160,6 +9160,37 @@ connection_.GetRetransmissionAlarm()->Fire(); } +TEST_P(QuicConnectionTest, PtoSkipsPacketNumber) { + if (!connection_.session_decides_what_to_write() || + !GetQuicReloadableFlag(quic_fix_rto_retransmission3)) { + return; + } + SetQuicReloadableFlag(quic_enable_pto, true); + SetQuicReloadableFlag(quic_skip_packet_number_for_pto, true); + QuicConfig config; + QuicTagVector connection_options; + connection_options.push_back(k1PTO); + connection_options.push_back(kPTOS); + config.SetConnectionOptionsToSend(connection_options); + EXPECT_CALL(*send_algorithm_, SetFromConfig(_, _)); + connection_.SetFromConfig(config); + EXPECT_FALSE(connection_.GetRetransmissionAlarm()->IsSet()); + + QuicStreamId stream_id = 2; + QuicPacketNumber last_packet; + SendStreamDataToPeer(stream_id, "foooooo", 0, NO_FIN, &last_packet); + SendStreamDataToPeer(stream_id, "foooooo", 7, NO_FIN, &last_packet); + EXPECT_EQ(QuicPacketNumber(2), last_packet); + EXPECT_TRUE(connection_.GetRetransmissionAlarm()->IsSet()); + + // Fire PTO and verify the PTO retransmission skips one packet number. + EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(1); + connection_.GetRetransmissionAlarm()->Fire(); + EXPECT_EQ(1u, writer_->stream_frames().size()); + EXPECT_EQ(QuicPacketNumber(4), writer_->last_packet_header().packet_number); + EXPECT_TRUE(connection_.GetRetransmissionAlarm()->IsSet()); +} + } // namespace } // namespace test } // namespace quic
diff --git a/quic/core/quic_packet_creator.cc b/quic/core/quic_packet_creator.cc index 0b7a9f2..4c381d4 100644 --- a/quic/core/quic_packet_creator.cc +++ b/quic/core/quic_packet_creator.cc
@@ -159,6 +159,33 @@ framer_->transport_version(), QuicPacketNumber(delta * 4)); } +void QuicPacketCreator::SkipNPacketNumbers( + QuicPacketCount count, + QuicPacketNumber least_packet_awaited_by_peer, + QuicPacketCount max_packets_in_flight) { + if (!queued_frames_.empty()) { + // Don't change creator state if there are frames queued. + QUIC_BUG << "Called SkipNPacketNumbers with " << queued_frames_.size() + << " queued_frames. First frame type:" + << queued_frames_.front().type + << " last frame type:" << queued_frames_.back().type; + return; + } + if (packet_.packet_number > packet_.packet_number + count) { + // Skipping count packet numbers causes packet number wrapping around, + // reject it. + QUIC_LOG(WARNING) << "Skipping " << count + << " packet numbers causes packet number wrapping " + "around, least_packet_awaited_by_peer: " + << least_packet_awaited_by_peer + << " packet_number:" << packet_.packet_number; + return; + } + packet_.packet_number += count; + // Packet number changes, update packet number length if necessary. + UpdatePacketNumberLength(least_packet_awaited_by_peer, max_packets_in_flight); +} + bool QuicPacketCreator::ConsumeCryptoData(EncryptionLevel level, size_t write_length, QuicStreamOffset offset,
diff --git a/quic/core/quic_packet_creator.h b/quic/core/quic_packet_creator.h index 8e3ea3f..9f00842 100644 --- a/quic/core/quic_packet_creator.h +++ b/quic/core/quic_packet_creator.h
@@ -82,6 +82,11 @@ void UpdatePacketNumberLength(QuicPacketNumber least_packet_awaited_by_peer, QuicPacketCount max_packets_in_flight); + // Skip |count| packet numbers. + void SkipNPacketNumbers(QuicPacketCount count, + QuicPacketNumber least_packet_awaited_by_peer, + QuicPacketCount max_packets_in_flight); + // The overhead the framing will add for a packet with one frame. static size_t StreamFramePacketOverhead( QuicTransportVersion version,
diff --git a/quic/core/quic_packet_creator_test.cc b/quic/core/quic_packet_creator_test.cc index e7c82d1..d0cfdae 100644 --- a/quic/core/quic_packet_creator_test.cc +++ b/quic/core/quic_packet_creator_test.cc
@@ -1181,6 +1181,36 @@ QuicPacketCreatorPeer::GetPacketNumberLength(&creator_)); } +TEST_P(QuicPacketCreatorTest, SkipNPacketNumbers) { + QuicPacketCreatorPeer::SetPacketNumber(&creator_, 1); + if (VersionHasIetfInvariantHeader(GetParam().version.transport_version) && + !GetParam().version.SendsVariableLengthPacketNumberInLongHeader()) { + EXPECT_EQ(PACKET_4BYTE_PACKET_NUMBER, + QuicPacketCreatorPeer::GetPacketNumberLength(&creator_)); + creator_.set_encryption_level(ENCRYPTION_FORWARD_SECURE); + } else { + EXPECT_EQ(PACKET_1BYTE_PACKET_NUMBER, + QuicPacketCreatorPeer::GetPacketNumberLength(&creator_)); + } + creator_.SkipNPacketNumbers(63, QuicPacketNumber(2), + 10000 / kDefaultMaxPacketSize); + EXPECT_EQ(QuicPacketNumber(64), creator_.packet_number()); + EXPECT_EQ(PACKET_1BYTE_PACKET_NUMBER, + QuicPacketCreatorPeer::GetPacketNumberLength(&creator_)); + + creator_.SkipNPacketNumbers(64 * 255, QuicPacketNumber(2), + 10000 / kDefaultMaxPacketSize); + EXPECT_EQ(QuicPacketNumber(64 * 256), creator_.packet_number()); + EXPECT_EQ(PACKET_2BYTE_PACKET_NUMBER, + QuicPacketCreatorPeer::GetPacketNumberLength(&creator_)); + + creator_.SkipNPacketNumbers(64 * 256 * 255, QuicPacketNumber(2), + 10000 / kDefaultMaxPacketSize); + EXPECT_EQ(QuicPacketNumber(64 * 256 * 256), creator_.packet_number()); + EXPECT_EQ(PACKET_4BYTE_PACKET_NUMBER, + QuicPacketCreatorPeer::GetPacketNumberLength(&creator_)); +} + TEST_P(QuicPacketCreatorTest, SerializeFrame) { if (!GetParam().version_serialization) { creator_.StopSendingVersion();
diff --git a/quic/core/quic_packet_generator.cc b/quic/core/quic_packet_generator.cc index 160222f..d4e879f 100644 --- a/quic/core/quic_packet_generator.cc +++ b/quic/core/quic_packet_generator.cc
@@ -337,6 +337,14 @@ max_packets_in_flight); } +void QuicPacketGenerator::SkipNPacketNumbers( + QuicPacketCount count, + QuicPacketNumber least_packet_awaited_by_peer, + QuicPacketCount max_packets_in_flight) { + packet_creator_.SkipNPacketNumbers(count, least_packet_awaited_by_peer, + max_packets_in_flight); +} + void QuicPacketGenerator::SetServerConnectionIdLength(uint32_t length) { if (length == 0) { packet_creator_.SetServerConnectionIdIncluded(CONNECTION_ID_ABSENT);
diff --git a/quic/core/quic_packet_generator.h b/quic/core/quic_packet_generator.h index c6902e0..69c69ea 100644 --- a/quic/core/quic_packet_generator.h +++ b/quic/core/quic_packet_generator.h
@@ -172,6 +172,11 @@ void UpdatePacketNumberLength(QuicPacketNumber least_packet_awaited_by_peer, QuicPacketCount max_packets_in_flight); + // Skip |count| packet numbers. + void SkipNPacketNumbers(QuicPacketCount count, + QuicPacketNumber least_packet_awaited_by_peer, + QuicPacketCount max_packets_in_flight); + // Set the minimum number of bytes for the server connection id length; void SetServerConnectionIdLength(uint32_t length);