Parse and frame the updated ACK_FREQUENCY frame and implement the logic for receiving it. In a change from draft-iyengar-quic-delayed-ack-01, previously missing packets are always acknowledged regardless of reordering_threshold. Protected by FLAGS_quic_reloadable_flag_quic_receive_ack_frequency. PiperOrigin-RevId: 765276193
diff --git a/quiche/quic/core/frames/quic_ack_frequency_frame.cc b/quiche/quic/core/frames/quic_ack_frequency_frame.cc index 7faf7f1..bcb6888 100644 --- a/quiche/quic/core/frames/quic_ack_frequency_frame.cc +++ b/quiche/quic/core/frames/quic_ack_frequency_frame.cc
@@ -5,25 +5,27 @@ #include "quiche/quic/core/frames/quic_ack_frequency_frame.h" #include <cstdint> -#include <limits> #include <ostream> namespace quic { QuicAckFrequencyFrame::QuicAckFrequencyFrame( QuicControlFrameId control_frame_id, uint64_t sequence_number, - uint64_t packet_tolerance, QuicTime::Delta max_ack_delay) + uint64_t ack_eliciting_threshold, QuicTime::Delta requested_max_ack_delay, + uint64_t reordering_threshold) : control_frame_id(control_frame_id), sequence_number(sequence_number), - packet_tolerance(packet_tolerance), - max_ack_delay(max_ack_delay) {} + ack_eliciting_threshold(ack_eliciting_threshold), + requested_max_ack_delay(requested_max_ack_delay), + reordering_threshold(reordering_threshold) {} std::ostream& operator<<(std::ostream& os, const QuicAckFrequencyFrame& frame) { os << "{ control_frame_id: " << frame.control_frame_id << ", sequence_number: " << frame.sequence_number - << ", packet_tolerance: " << frame.packet_tolerance - << ", max_ack_delay_ms: " << frame.max_ack_delay.ToMilliseconds() - << ", ignore_order: " << frame.ignore_order << " }\n"; + << ", ack_eliciting_threshold: " << frame.ack_eliciting_threshold + << ", requested_max_ack_delay_ms: " + << frame.requested_max_ack_delay.ToMilliseconds() + << ", reordering_threshold: " << frame.reordering_threshold << " }\n"; return os; }
diff --git a/quiche/quic/core/frames/quic_ack_frequency_frame.h b/quiche/quic/core/frames/quic_ack_frequency_frame.h index 660b1a7..22fe058 100644 --- a/quiche/quic/core/frames/quic_ack_frequency_frame.h +++ b/quiche/quic/core/frames/quic_ack_frequency_frame.h
@@ -11,7 +11,7 @@ #include "quiche/quic/core/quic_constants.h" #include "quiche/quic/core/quic_time.h" #include "quiche/quic/core/quic_types.h" -#include "quiche/quic/platform/api/quic_export.h" +#include "quiche/common/platform/api/quiche_export.h" namespace quic { @@ -21,28 +21,32 @@ std::ostream& os, const QuicAckFrequencyFrame& ack_frequency_frame); QuicAckFrequencyFrame() = default; + // Set a default for reordering_threshold so it does not break Envoy tests. QuicAckFrequencyFrame(QuicControlFrameId control_frame_id, - uint64_t sequence_number, uint64_t packet_tolerance, - QuicTime::Delta max_ack_delay); + uint64_t sequence_number, + uint64_t ack_eliciting_threshold, + QuicTime::Delta requested_max_ack_delay, + uint64_t reordering_threshold = 1); // A unique identifier of this control frame. 0 when this frame is // received, and non-zero when sent. QuicControlFrameId control_frame_id = kInvalidControlFrameId; - // If true, do not ack immediately upon observeation of packet reordering. - bool ignore_order = false; - // Sequence number assigned to the ACK_FREQUENCY frame by the sender to allow // receivers to ignore obsolete frames. uint64_t sequence_number = 0; - // The maximum number of ack-eliciting packets after which the receiver sends - // an acknowledgement. Invald if == 0. - uint64_t packet_tolerance = 2; + // The maximum number of ack-eliciting packets that do not require an + // acknowledgement. + uint64_t ack_eliciting_threshold = 1; // The maximum time that ack packets can be delayed. - QuicTime::Delta max_ack_delay = + QuicTime::Delta requested_max_ack_delay = QuicTime::Delta::FromMilliseconds(kDefaultPeerDelayedAckTimeMs); + + // The number of out-of-order packets necessary to trigger an immediate + // acknowledgement. If zero, OOO packets are not acked immediately. + uint64_t reordering_threshold = 1; }; } // namespace quic
diff --git a/quiche/quic/core/frames/quic_frames_test.cc b/quiche/quic/core/frames/quic_frames_test.cc index e6a843d..eeed473 100644 --- a/quiche/quic/core/frames/quic_frames_test.cc +++ b/quiche/quic/core/frames/quic_frames_test.cc
@@ -327,12 +327,13 @@ EXPECT_TRUE(IsControlFrame(frame.type)); } -TEST_F(QuicFramesTest, QuicAckFreuqncyFrameToString) { +TEST_F(QuicFramesTest, QuicAckFrequencyFrameToString) { QuicAckFrequencyFrame ack_frequency_frame; ack_frequency_frame.sequence_number = 1; - ack_frequency_frame.packet_tolerance = 2; - ack_frequency_frame.max_ack_delay = QuicTime::Delta::FromMilliseconds(25); - ack_frequency_frame.ignore_order = false; + ack_frequency_frame.ack_eliciting_threshold = 2; + ack_frequency_frame.requested_max_ack_delay = + QuicTime::Delta::FromMilliseconds(25); + ack_frequency_frame.reordering_threshold = false; QuicFrame frame(&ack_frequency_frame); ASSERT_EQ(ACK_FREQUENCY_FRAME, frame.type); SetControlFrameId(6, &frame); @@ -340,8 +341,8 @@ std::ostringstream stream; stream << *frame.ack_frequency_frame; EXPECT_EQ( - "{ control_frame_id: 6, sequence_number: 1, packet_tolerance: 2, " - "max_ack_delay_ms: 25, ignore_order: 0 }\n", + "{ control_frame_id: 6, sequence_number: 1, ack_eliciting_threshold: 2, " + "requested_max_ack_delay_ms: 25, reordering_threshold: 0 }\n", stream.str()); EXPECT_TRUE(IsControlFrame(frame.type)); }
diff --git a/quiche/quic/core/quic_connection.cc b/quiche/quic/core/quic_connection.cc index 214414a..74ac6e4 100644 --- a/quiche/quic/core/quic_connection.cc +++ b/quiche/quic/core/quic_connection.cc
@@ -607,6 +607,8 @@ if (config.GetMinAckDelayDraft10ToSendMs() <= config.GetMaxAckDelayToSendMs()) { // MinAckDelay is valid. set_can_receive_ack_frequency_immediate_ack(true); + local_min_ack_delay_ = QuicTime::Delta::FromMilliseconds( + config.GetMinAckDelayDraft10ToSendMs()); } else { QUIC_BUG(quic_bug_min_ack_delay_too_high) << "MinAckDelay higher than MaxAckDelay"; @@ -2116,6 +2118,15 @@ QUIC_LOG_EVERY_N_SEC(ERROR, 120) << "Get unexpected AckFrequencyFrame."; return false; } + if (frame.requested_max_ack_delay < local_min_ack_delay_) { + QUIC_LOG_EVERY_N_SEC(ERROR, 120) + << "Received AckFrequencyFrame with requested_max_ack_delay " + << frame.requested_max_ack_delay + << " which is less than the minimum ack delay " << local_min_ack_delay_; + CloseConnection(IETF_QUIC_PROTOCOL_VIOLATION, "MaxAckDelay too small", + ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET); + return false; + } if (auto packet_number_space = QuicUtils::GetPacketNumberSpace( last_received_packet_info_.decrypted_level) == APPLICATION_DATA) { @@ -4087,10 +4098,11 @@ QUIC_RELOADABLE_FLAG_COUNT_N(quic_can_send_ack_frequency, 2, 3); auto ack_frequency_frame = sent_packet_manager_.GetUpdatedAckFrequencyFrame(); - // This AckFrequencyFrame is meant to only update the max_ack_delay. Set - // packet tolerance to the default value for now. - ack_frequency_frame.packet_tolerance = + // This AckFrequencyFrame is meant to only update the max_ack_delay. All + // other values are set to the default. + ack_frequency_frame.ack_eliciting_threshold = kDefaultRetransmittablePacketsBeforeAck; + ack_frequency_frame.reordering_threshold = 1; visitor_->SendAckFrequency(ack_frequency_frame); if (!connected_) { return;
diff --git a/quiche/quic/core/quic_connection.h b/quiche/quic/core/quic_connection.h index a2f7bfa..e2073c5 100644 --- a/quiche/quic/core/quic_connection.h +++ b/quiche/quic/core/quic_connection.h
@@ -2530,6 +2530,9 @@ QuicTime::Delta multi_port_probing_interval_; + // The minimum ack delay time advertised to the peer via transport parameter. + QuicTime::Delta local_min_ack_delay_ = QuicTime::Delta::Zero(); + std::unique_ptr<MultiPortStats> multi_port_stats_; // If true, connection will migrate to multi-port path upon path degrading.
diff --git a/quiche/quic/core/quic_connection_test.cc b/quiche/quic/core/quic_connection_test.cc index 1950e10..915f65d 100644 --- a/quiche/quic/core/quic_connection_test.cc +++ b/quiche/quic/core/quic_connection_test.cc
@@ -4,8 +4,6 @@ #include "quiche/quic/core/quic_connection.h" -#include <errno.h> - #include <algorithm> #include <cstdint> #include <memory> @@ -3592,7 +3590,7 @@ EXPECT_CALL(visitor_, OnSuccessfulVersionNegotiation(_)); QuicAckFrequencyFrame ack_frequency_frame; - ack_frequency_frame.packet_tolerance = 3; + ack_frequency_frame.ack_eliciting_threshold = 2; ProcessFramePacketAtLevel(1, QuicFrame(&ack_frequency_frame), ENCRYPTION_FORWARD_SECURE); @@ -13299,8 +13297,8 @@ // Send packet 101. SendStreamDataToPeer(/*id=*/1, "bar", /*offset=*/3, NO_FIN, nullptr); - EXPECT_EQ(captured_frame.packet_tolerance, 10u); - EXPECT_EQ(captured_frame.max_ack_delay, + EXPECT_EQ(captured_frame.ack_eliciting_threshold, 10u); + EXPECT_EQ(captured_frame.requested_max_ack_delay, QuicTime::Delta::FromMilliseconds(GetDefaultDelayedAckTimeMs())); // Sending packet 102 does not trigger sending another AckFrequencyFrame. @@ -13339,8 +13337,8 @@ connection_.OnHandshakeComplete(); - EXPECT_EQ(captured_frame.packet_tolerance, 2u); - EXPECT_EQ(captured_frame.max_ack_delay, + EXPECT_EQ(captured_frame.ack_eliciting_threshold, 2u); + EXPECT_EQ(captured_frame.requested_max_ack_delay, QuicTime::Delta::FromMilliseconds(GetDefaultDelayedAckTimeMs())); }
diff --git a/quiche/quic/core/quic_control_frame_manager.cc b/quiche/quic/core/quic_control_frame_manager.cc index 9dd73a1..b5dbbba 100644 --- a/quiche/quic/core/quic_control_frame_manager.cc +++ b/quiche/quic/core/quic_control_frame_manager.cc
@@ -137,11 +137,12 @@ QuicControlFrameId control_frame_id = ++last_control_frame_id_; // Using the control_frame_id for sequence_number here leaves gaps in // sequence_number. - WriteOrBufferQuicFrame( - QuicFrame(new QuicAckFrequencyFrame(control_frame_id, - /*sequence_number=*/control_frame_id, - ack_frequency_frame.packet_tolerance, - ack_frequency_frame.max_ack_delay))); + WriteOrBufferQuicFrame(QuicFrame( + new QuicAckFrequencyFrame(control_frame_id, + /*sequence_number=*/control_frame_id, + ack_frequency_frame.ack_eliciting_threshold, + ack_frequency_frame.requested_max_ack_delay, + ack_frequency_frame.reordering_threshold))); } void QuicControlFrameManager::WriteOrBufferNewConnectionId(
diff --git a/quiche/quic/core/quic_control_frame_manager_test.cc b/quiche/quic/core/quic_control_frame_manager_test.cc index a007d92..d58ffed 100644 --- a/quiche/quic/core/quic_control_frame_manager_test.cc +++ b/quiche/quic/core/quic_control_frame_manager_test.cc
@@ -421,8 +421,9 @@ TEST_F(QuicControlFrameManagerTest, SendAndAckAckFrequencyFrame) { // Send AckFrequencyFrame QuicAckFrequencyFrame frame_to_send; - frame_to_send.packet_tolerance = 10; - frame_to_send.max_ack_delay = QuicTime::Delta::FromMilliseconds(24); + frame_to_send.ack_eliciting_threshold = 10; + frame_to_send.requested_max_ack_delay = QuicTime::Delta::FromMilliseconds(24); + frame_to_send.reordering_threshold = 3; EXPECT_CALL(*session_, WriteControlFrame(_, _)) .WillOnce(Invoke(&ClearControlFrameWithTransmissionType)); manager_->WriteOrBufferAckFrequency(frame_to_send);
diff --git a/quiche/quic/core/quic_framer.cc b/quiche/quic/core/quic_framer.cc index a506624..e01b1af 100644 --- a/quiche/quic/core/quic_framer.cc +++ b/quiche/quic/core/quic_framer.cc
@@ -628,10 +628,10 @@ const QuicAckFrequencyFrame& frame) { return QuicDataWriter::GetVarInt62Len(IETF_ACK_FREQUENCY) + QuicDataWriter::GetVarInt62Len(frame.sequence_number) + - QuicDataWriter::GetVarInt62Len(frame.packet_tolerance) + - QuicDataWriter::GetVarInt62Len(frame.max_ack_delay.ToMicroseconds()) + - // One byte for encoding boolean - 1; + QuicDataWriter::GetVarInt62Len(frame.ack_eliciting_threshold) + + QuicDataWriter::GetVarInt62Len( + frame.requested_max_ack_delay.ToMicroseconds()) + + QuicDataWriter::GetVarInt62Len(frame.reordering_threshold); } // static @@ -3391,14 +3391,10 @@ return false; } - if (!reader->ReadVarInt62(&frame->packet_tolerance)) { + if (!reader->ReadVarInt62(&frame->ack_eliciting_threshold)) { set_detailed_error("Unable to read packet tolerance."); return false; } - if (frame->packet_tolerance == 0) { - set_detailed_error("Invalid packet tolerance."); - return false; - } uint64_t max_ack_delay_us; if (!reader->ReadVarInt62(&max_ack_delay_us)) { set_detailed_error("Unable to read max_ack_delay_us."); @@ -3409,19 +3405,13 @@ set_detailed_error("Invalid max_ack_delay_us."); return false; } - frame->max_ack_delay = QuicTime::Delta::FromMicroseconds(max_ack_delay_us); + frame->requested_max_ack_delay = + QuicTime::Delta::FromMicroseconds(max_ack_delay_us); - uint8_t ignore_order; - if (!reader->ReadUInt8(&ignore_order)) { - set_detailed_error("Unable to read ignore_order."); + if (!reader->ReadVarInt62(&frame->reordering_threshold)) { + set_detailed_error("Unable to read reordering_threshold."); return false; } - if (ignore_order > 1) { - set_detailed_error("Invalid ignore_order."); - return false; - } - frame->ignore_order = ignore_order; - return true; } @@ -5270,17 +5260,17 @@ set_detailed_error("Writing sequence number failed."); return false; } - if (!writer->WriteVarInt62(frame.packet_tolerance)) { + if (!writer->WriteVarInt62(frame.ack_eliciting_threshold)) { set_detailed_error("Writing packet tolerance failed."); return false; } - if (!writer->WriteVarInt62( - static_cast<uint64_t>(frame.max_ack_delay.ToMicroseconds()))) { + if (!writer->WriteVarInt62(static_cast<uint64_t>( + frame.requested_max_ack_delay.ToMicroseconds()))) { set_detailed_error("Writing max_ack_delay_us failed."); return false; } - if (!writer->WriteUInt8(static_cast<uint8_t>(frame.ignore_order))) { - set_detailed_error("Writing ignore_order failed."); + if (!writer->WriteVarInt62(frame.reordering_threshold)) { + set_detailed_error("Writing reordering_threshold failed."); return false; }
diff --git a/quiche/quic/core/quic_framer_test.cc b/quiche/quic/core/quic_framer_test.cc index cced010..c8a79b3 100644 --- a/quiche/quic/core/quic_framer_test.cc +++ b/quiche/quic/core/quic_framer_test.cc
@@ -4847,12 +4847,12 @@ 0x40, 0xAF, // sequence_number 0x11, - // packet_tolerance + // ack_eliciting_threshold 0x02, // max_ack_delay_us = 2'5000 us 0x80, 0x00, 0x61, 0xA8, - // ignore_order - 0x01 + // reordering_threshold = 0 (ignore order) + 0x00 }; // clang-format on @@ -4872,9 +4872,9 @@ ASSERT_EQ(1u, visitor_.ack_frequency_frames_.size()); const auto& frame = visitor_.ack_frequency_frames_.front(); EXPECT_EQ(17u, frame->sequence_number); - EXPECT_EQ(2u, frame->packet_tolerance); - EXPECT_EQ(2'5000u, frame->max_ack_delay.ToMicroseconds()); - EXPECT_EQ(true, frame->ignore_order); + EXPECT_EQ(2u, frame->ack_eliciting_threshold); + EXPECT_EQ(2'5000u, frame->requested_max_ack_delay.ToMicroseconds()); + EXPECT_EQ(0u, frame->reordering_threshold); } TEST_P(QuicFramerTest, ParseImmediateAckFrame) { @@ -8253,9 +8253,10 @@ QuicAckFrequencyFrame ack_frequency_frame; ack_frequency_frame.sequence_number = 3; - ack_frequency_frame.packet_tolerance = 5; - ack_frequency_frame.max_ack_delay = QuicTime::Delta::FromMicroseconds(0x3fff); - ack_frequency_frame.ignore_order = false; + ack_frequency_frame.ack_eliciting_threshold = 5; + ack_frequency_frame.requested_max_ack_delay = + QuicTime::Delta::FromMicroseconds(0x3fff); + ack_frequency_frame.reordering_threshold = 3; QuicFrames frames = {QuicFrame(&ack_frequency_frame)}; // clang-format off @@ -8271,12 +8272,12 @@ 0x40, 0xaf, // sequence number 0x03, - // packet tolerance + // ack-eliciting threshold 0x05, // max_ack_delay_us 0x7f, 0xff, - // ignore_oder - 0x00 + // reordering threshold + 0x03 }; // clang-format on if (!VersionHasIetfQuicFrames(framer_.transport_version())) {
diff --git a/quiche/quic/core/quic_received_packet_manager.cc b/quiche/quic/core/quic_received_packet_manager.cc index 72a1425..5e18cab 100644 --- a/quiche/quic/core/quic_received_packet_manager.cc +++ b/quiche/quic/core/quic_received_packet_manager.cc
@@ -12,6 +12,7 @@ #include "quiche/quic/core/crypto/crypto_protocol.h" #include "quiche/quic/core/quic_config.h" #include "quiche/quic/core/quic_connection_stats.h" +#include "quiche/quic/core/quic_time.h" #include "quiche/quic/core/quic_types.h" #include "quiche/quic/platform/api/quic_bug_tracker.h" #include "quiche/quic/platform/api/quic_flag_utils.h" @@ -45,17 +46,14 @@ stats_(stats), num_retransmittable_packets_received_since_last_ack_sent_(0), min_received_before_ack_decimation_(kMinReceivedBeforeAckDecimation), - ack_frequency_(kDefaultRetransmittablePacketsBeforeAck), ack_decimation_delay_(GetQuicFlag(quic_ack_decimation_delay)), unlimited_ack_decimation_(false), one_immediate_ack_(false), - ignore_order_(false), local_max_ack_delay_( QuicTime::Delta::FromMilliseconds(GetDefaultDelayedAckTimeMs())), ack_timeout_(QuicTime::Zero()), time_of_previous_received_packet_(QuicTime::Zero()), - was_last_packet_missing_(false), - last_ack_frequency_frame_sequence_number_(-1) {} + was_last_packet_missing_(false) {} QuicReceivedPacketManager::~QuicReceivedPacketManager() {} @@ -294,11 +292,12 @@ return; } - if (!ignore_order_ && was_last_packet_missing_ && - last_sent_largest_acked_.IsInitialized() && + // TODO(martinduke): If a missing packet is received, do we send it when + // reordering_threshold_ == 0? + if (was_last_packet_missing_ && last_sent_largest_acked_.IsInitialized() && last_received_packet_number < last_sent_largest_acked_) { - // Only ack immediately if an ACK frame was sent with a larger largest acked - // than the newly received packet number. + // Ack immediately if an ACK frame was sent with a larger largest acked than + // the newly received packet number. ack_timeout_ = now; return; } @@ -322,7 +321,7 @@ return; } - if (!ignore_order_ && HasNewMissingPackets()) { + if (reordering_threshold_ == 1 && HasNewMissingPackets()) { // Fast path. ack_timeout_ = now; return; } @@ -384,15 +383,14 @@ void QuicReceivedPacketManager::OnAckFrequencyFrame( const QuicAckFrequencyFrame& frame) { - int64_t new_sequence_number = frame.sequence_number; - if (new_sequence_number <= last_ack_frequency_frame_sequence_number_) { + if (frame.sequence_number < next_ack_frequency_frame_sequence_number_) { // Ignore old ACK_FREQUENCY frames. return; } - last_ack_frequency_frame_sequence_number_ = new_sequence_number; - ack_frequency_ = frame.packet_tolerance; - local_max_ack_delay_ = frame.max_ack_delay; - ignore_order_ = frame.ignore_order; + next_ack_frequency_frame_sequence_number_ = frame.sequence_number + 1; + ack_frequency_ = frame.ack_eliciting_threshold + 1; + local_max_ack_delay_ = frame.requested_max_ack_delay; + reordering_threshold_ = frame.reordering_threshold; } } // namespace quic
diff --git a/quiche/quic/core/quic_received_packet_manager.h b/quiche/quic/core/quic_received_packet_manager.h index d1feccc..404d46d 100644 --- a/quiche/quic/core/quic_received_packet_manager.h +++ b/quiche/quic/core/quic_received_packet_manager.h
@@ -6,13 +6,15 @@ #define QUICHE_QUIC_CORE_QUIC_RECEIVED_PACKET_MANAGER_H_ #include <cstddef> +#include <cstdint> #include "quiche/quic/core/frames/quic_ack_frequency_frame.h" #include "quiche/quic/core/quic_config.h" -#include "quiche/quic/core/quic_framer.h" +#include "quiche/quic/core/quic_constants.h" #include "quiche/quic/core/quic_packets.h" +#include "quiche/quic/core/quic_time.h" #include "quiche/quic/core/quic_types.h" -#include "quiche/quic/platform/api/quic_export.h" +#include "quiche/common/platform/api/quiche_export.h" namespace quic { @@ -146,7 +148,7 @@ const RttStats& rtt_stats) const; bool AckFrequencyFrameReceived() const { - return last_ack_frequency_frame_sequence_number_ >= 0; + return next_ack_frequency_frame_sequence_number_ > 0; } void MaybeTrimAckRanges(); @@ -186,8 +188,8 @@ QuicPacketCount num_retransmittable_packets_received_since_last_ack_sent_; // Ack decimation will start happening after this many packets are received. size_t min_received_before_ack_decimation_; - // Ack every n-th packet. - size_t ack_frequency_; + // Ack every nth packet. + size_t ack_frequency_ = kDefaultRetransmittablePacketsBeforeAck; // The max delay in fraction of min_rtt to use when sending decimated acks. float ack_decimation_delay_; // When true, removes ack decimation's max number of packets(10) before @@ -195,12 +197,16 @@ bool unlimited_ack_decimation_; // When true, only send 1 immediate ACK when reordering is detected. bool one_immediate_ack_; - // When true, do not ack immediately upon observation of packet reordering. - bool ignore_order_; + // If the largest unacked packet has a packet number that is + // reordering_threshold_ more than the lowest missing packet number that is + // greater than largest_acked, then an immediate ACK will be sent. The default + // value is 1. A value of 0 means to ignore reordering. + uint64_t reordering_threshold_ = 1; // The local node's maximum ack delay time. This is the maximum amount of // time to wait before sending an acknowledgement. QuicTime::Delta local_max_ack_delay_; + // Time that an ACK needs to be sent. 0 means no ACK is pending. Used when // decide_when_to_send_acks_ is true. QuicTime ack_timeout_; @@ -222,9 +228,8 @@ // Last sent largest acked, which gets updated when ACK was successfully sent. QuicPacketNumber last_sent_largest_acked_; - // The sequence number of the last received AckFrequencyFrame. Negative if - // none received. - int64_t last_ack_frequency_frame_sequence_number_; + // The expected sequence number of the next received AckFrequencyFrame. + uint64_t next_ack_frequency_frame_sequence_number_ = 0; }; } // namespace quic
diff --git a/quiche/quic/core/quic_received_packet_manager_test.cc b/quiche/quic/core/quic_received_packet_manager_test.cc index 1f4aaee..97f00c5 100644 --- a/quiche/quic/core/quic_received_packet_manager_test.cc +++ b/quiche/quic/core/quic_received_packet_manager_test.cc
@@ -543,17 +543,17 @@ EXPECT_FALSE(HasPendingAck()); QuicAckFrequencyFrame frame; - frame.max_ack_delay = QuicTime::Delta::FromMilliseconds(10); - frame.packet_tolerance = 5; + frame.requested_max_ack_delay = QuicTime::Delta::FromMilliseconds(10); + frame.ack_eliciting_threshold = 4; received_manager_.OnAckFrequencyFrame(frame); for (int i = 1; i <= 50; ++i) { RecordPacketReceipt(i, clock_.ApproximateNow()); MaybeUpdateAckTimeout(kInstigateAck, i); - if (i % frame.packet_tolerance == 0) { + if (i % (frame.ack_eliciting_threshold + 1) == 0) { CheckAckTimeout(clock_.ApproximateNow()); } else { - CheckAckTimeout(clock_.ApproximateNow() + frame.max_ack_delay); + CheckAckTimeout(clock_.ApproximateNow() + frame.requested_max_ack_delay); } } } @@ -563,9 +563,9 @@ EXPECT_FALSE(HasPendingAck()); QuicAckFrequencyFrame frame; - frame.max_ack_delay = kDelayedAckTime; - frame.packet_tolerance = 2; - frame.ignore_order = true; + frame.requested_max_ack_delay = kDelayedAckTime; + frame.ack_eliciting_threshold = 1; + frame.reordering_threshold = 0; received_manager_.OnAckFrequencyFrame(frame); RecordPacketReceipt(4, clock_.ApproximateNow()); @@ -578,27 +578,27 @@ RecordPacketReceipt(3, clock_.ApproximateNow()); MaybeUpdateAckTimeout(kInstigateAck, 3); - // Don't ack as ignore_order is set by AckFrequencyFrame. - CheckAckTimeout(clock_.ApproximateNow() + kDelayedAckTime); - - RecordPacketReceipt(2, clock_.ApproximateNow()); - MaybeUpdateAckTimeout(kInstigateAck, 2); - // Immediate ack is sent as this is the 2nd packet of every two packets. + // Ack, since missing packets are always acknowledged. CheckAckTimeout(clock_.ApproximateNow()); - RecordPacketReceipt(1, clock_.ApproximateNow()); - MaybeUpdateAckTimeout(kInstigateAck, 1); - // Don't ack as ignore_order is set by AckFrequencyFrame. + RecordPacketReceipt(7, clock_.ApproximateNow()); + MaybeUpdateAckTimeout(kInstigateAck, 10); + // No ack, as the frame says to ignore ordering. CheckAckTimeout(clock_.ApproximateNow() + kDelayedAckTime); + + RecordPacketReceipt(1, clock_.ApproximateNow()); + MaybeUpdateAckTimeout(kInstigateAck, 11); + // It's the second packet in a row, ack it. + CheckAckTimeout(clock_.ApproximateNow()); } TEST_F(QuicReceivedPacketManagerTest, - DisableMissingPaketsAckByIgnoreOrderFromAckFrequencyFrame) { + DisableMissingPacketsAckByIgnoreOrderFromAckFrequencyFrame) { EXPECT_FALSE(HasPendingAck()); QuicAckFrequencyFrame frame; - frame.max_ack_delay = kDelayedAckTime; - frame.packet_tolerance = 2; - frame.ignore_order = true; + frame.requested_max_ack_delay = kDelayedAckTime; + frame.ack_eliciting_threshold = 1; + frame.reordering_threshold = 0; received_manager_.OnAckFrequencyFrame(frame); RecordPacketReceipt(1, clock_.ApproximateNow()); @@ -622,8 +622,7 @@ RecordPacketReceipt(7, clock_.ApproximateNow()); MaybeUpdateAckTimeout(kInstigateAck, 7); - // Don't ack even if packet 6 is newly missing as ignore_order is set by - // AckFrequencyFrame. + // Don't ack as AckFrequencyFrame says to ignore ordering. CheckAckTimeout(clock_.ApproximateNow() + kDelayedAckTime); } @@ -632,9 +631,9 @@ EXPECT_FALSE(HasPendingAck()); QuicAckFrequencyFrame frame; - frame.max_ack_delay = kDelayedAckTime; - frame.packet_tolerance = 3; - frame.ignore_order = true; + frame.requested_max_ack_delay = kDelayedAckTime; + frame.ack_eliciting_threshold = 2; + frame.reordering_threshold = 0; received_manager_.OnAckFrequencyFrame(frame); // Process all the packets in order so there aren't missing packets.
diff --git a/quiche/quic/core/quic_sent_packet_manager.cc b/quiche/quic/core/quic_sent_packet_manager.cc index 2e10bbc..ac1cc36 100644 --- a/quiche/quic/core/quic_sent_packet_manager.cc +++ b/quiche/quic/core/quic_sent_packet_manager.cc
@@ -637,6 +637,7 @@ return !peer_min_ack_delay_.IsInfinite() && handshake_finished_; } +// TODO(martinduke): Update to include reordering threshold. QuicAckFrequencyFrame QuicSentPacketManager::GetUpdatedAckFrequencyFrame() const { QuicAckFrequencyFrame frame; @@ -647,15 +648,16 @@ } QUIC_RELOADABLE_FLAG_COUNT_N(quic_can_send_ack_frequency, 1, 3); - frame.packet_tolerance = kMaxRetransmittablePacketsBeforeAck; + frame.ack_eliciting_threshold = kMaxRetransmittablePacketsBeforeAck; auto rtt = use_smoothed_rtt_in_ack_delay_ ? rtt_stats_.SmoothedOrInitialRtt() : rtt_stats_.MinOrInitialRtt(); - frame.max_ack_delay = rtt * kPeerAckDecimationDelay; - frame.max_ack_delay = std::max(frame.max_ack_delay, peer_min_ack_delay_); + frame.requested_max_ack_delay = rtt * kPeerAckDecimationDelay; + frame.requested_max_ack_delay = + std::max(frame.requested_max_ack_delay, peer_min_ack_delay_); // TODO(haoyuewang) Remove this once kDefaultMinAckDelayTimeMs is updated to // 5 ms on the client side. - frame.max_ack_delay = - std::max(frame.max_ack_delay, + frame.requested_max_ack_delay = + std::max(frame.requested_max_ack_delay, QuicTime::Delta::FromMilliseconds(kDefaultMinAckDelayTimeMs)); return frame; } @@ -1611,10 +1613,11 @@ void QuicSentPacketManager::OnAckFrequencyFrameSent( const QuicAckFrequencyFrame& ack_frequency_frame) { - in_use_sent_ack_delays_.emplace_back(ack_frequency_frame.max_ack_delay, - ack_frequency_frame.sequence_number); - if (ack_frequency_frame.max_ack_delay > peer_max_ack_delay_) { - peer_max_ack_delay_ = ack_frequency_frame.max_ack_delay; + in_use_sent_ack_delays_.emplace_back( + ack_frequency_frame.requested_max_ack_delay, + ack_frequency_frame.sequence_number); + if (ack_frequency_frame.requested_max_ack_delay > peer_max_ack_delay_) { + peer_max_ack_delay_ = ack_frequency_frame.requested_max_ack_delay; } }
diff --git a/quiche/quic/core/quic_sent_packet_manager_test.cc b/quiche/quic/core/quic_sent_packet_manager_test.cc index 5e92c19..f325d58 100644 --- a/quiche/quic/core/quic_sent_packet_manager_test.cc +++ b/quiche/quic/core/quic_sent_packet_manager_test.cc
@@ -2845,7 +2845,7 @@ int packet_number, int ack_frequency_sequence_number, QuicTime::Delta max_ack_delay) { auto* ack_frequency_frame = new QuicAckFrequencyFrame(); - ack_frequency_frame->max_ack_delay = max_ack_delay; + ack_frequency_frame->requested_max_ack_delay = max_ack_delay; ack_frequency_frame->sequence_number = ack_frequency_sequence_number; SerializedPacket packet(QuicPacketNumber(packet_number), PACKET_4BYTE_PACKET_NUMBER, nullptr, kDefaultLength, @@ -3074,10 +3074,10 @@ /*now=*/QuicTime::Zero() + QuicTime::Delta::FromMilliseconds(24)); auto frame = manager_.GetUpdatedAckFrequencyFrame(); - EXPECT_EQ(frame.max_ack_delay, + EXPECT_EQ(frame.requested_max_ack_delay, std::max(rtt_stats->min_rtt() * 0.25, QuicTime::Delta::FromMilliseconds(1u))); - EXPECT_EQ(frame.packet_tolerance, 10u); + EXPECT_EQ(frame.ack_eliciting_threshold, 10u); } TEST_F(QuicSentPacketManagerTest, SmoothedRttIgnoreAckDelay) { @@ -3216,7 +3216,7 @@ /*now=*/QuicTime::Zero() + QuicTime::Delta::FromMilliseconds(24)); auto frame = manager_.GetUpdatedAckFrequencyFrame(); - EXPECT_EQ(frame.max_ack_delay, + EXPECT_EQ(frame.requested_max_ack_delay, std::max(rtt_stats->SmoothedOrInitialRtt() * 0.25, QuicTime::Delta::FromMilliseconds(1u))); }
diff --git a/quiche/quic/test_tools/simple_session_notifier.cc b/quiche/quic/test_tools/simple_session_notifier.cc index d72b431..6e06b8c 100644 --- a/quiche/quic/test_tools/simple_session_notifier.cc +++ b/quiche/quic/test_tools/simple_session_notifier.cc
@@ -4,13 +4,13 @@ #include "quiche/quic/test_tools/simple_session_notifier.h" +#include "quiche/quic/core/frames/quic_ack_frequency_frame.h" #include "quiche/quic/core/frames/quic_frame.h" #include "quiche/quic/core/frames/quic_reset_stream_at_frame.h" #include "quiche/quic/core/quic_error_codes.h" #include "quiche/quic/core/quic_types.h" #include "quiche/quic/core/quic_utils.h" #include "quiche/quic/platform/api/quic_logging.h" -#include "quiche/quic/test_tools/quic_test_utils.h" namespace quic { @@ -170,11 +170,12 @@ const bool had_buffered_data = HasBufferedStreamData() || HasBufferedControlFrames(); QuicControlFrameId control_frame_id = ++last_control_frame_id_; - control_frames_.emplace_back(( - QuicFrame(new QuicAckFrequencyFrame(control_frame_id, - /*sequence_number=*/control_frame_id, - ack_frequency_frame.packet_tolerance, - ack_frequency_frame.max_ack_delay)))); + control_frames_.emplace_back((QuicFrame( + new QuicAckFrequencyFrame(control_frame_id, + /*sequence_number=*/control_frame_id, + ack_frequency_frame.ack_eliciting_threshold, + ack_frequency_frame.requested_max_ack_delay, + ack_frequency_frame.reordering_threshold)))); if (had_buffered_data) { QUIC_DLOG(WARNING) << "Connection is write blocked"; return;
diff --git a/quiche/quic/test_tools/simple_session_notifier.h b/quiche/quic/test_tools/simple_session_notifier.h index dc3bcb2..eec1b6a 100644 --- a/quiche/quic/test_tools/simple_session_notifier.h +++ b/quiche/quic/test_tools/simple_session_notifier.h
@@ -6,11 +6,11 @@ #define QUICHE_QUIC_TEST_TOOLS_SIMPLE_SESSION_NOTIFIER_H_ #include "absl/container/flat_hash_map.h" +#include "quiche/quic/core/quic_connection.h" #include "quiche/quic/core/quic_error_codes.h" #include "quiche/quic/core/quic_interval_set.h" #include "quiche/quic/core/quic_types.h" #include "quiche/quic/core/session_notifier_interface.h" -#include "quiche/quic/platform/api/quic_test.h" #include "quiche/common/quiche_circular_deque.h" #include "quiche/common/quiche_linked_hash_map.h"