Move the increments of retransmittable-on-wire counters to when the alarm fires in retransmittable-on-wire mode. Protected by FLAGS_quic_reloadable_flag_quic_use_ping_manager2. PiperOrigin-RevId: 447876160
diff --git a/quiche/quic/core/quic_connection.cc b/quiche/quic/core/quic_connection.cc index 3913635..1e458c6 100644 --- a/quiche/quic/core/quic_connection.cc +++ b/quiche/quic/core/quic_connection.cc
@@ -128,7 +128,7 @@ void OnAlarm() override { QUICHE_DCHECK(connection_->connected()); - QUICHE_DCHECK(!GetQuicReloadableFlag(quic_use_ping_manager)); + QUICHE_DCHECK(!GetQuicReloadableFlag(quic_use_ping_manager2)); connection_->OnPingTimeout(); } }; @@ -4702,7 +4702,7 @@ return; } if (use_ping_manager_) { - QUIC_RELOADABLE_FLAG_COUNT(quic_use_ping_manager); + QUIC_RELOADABLE_FLAG_COUNT(quic_use_ping_manager2); ping_manager_.SetAlarm(clock_->ApproximateNow(), visitor_->ShouldKeepConnectionAlive(), sent_packet_manager_.HasInFlightPackets());
diff --git a/quiche/quic/core/quic_connection.h b/quiche/quic/core/quic_connection.h index b214361..09cef3d 100644 --- a/quiche/quic/core/quic_connection.h +++ b/quiche/quic/core/quic_connection.h
@@ -1995,7 +1995,7 @@ bool defer_send_in_response_to_packets_; // TODO(fayang): remove PING related fields below when deprecating - // quic_use_ping_manager. + // quic_use_ping_manager2. // The timeout for keep-alive PING. QuicTime::Delta keep_alive_ping_timeout_; @@ -2019,7 +2019,7 @@ // An alarm that is scheduled when the SentPacketManager requires a delay // before sending packets and fires when the packet may be sent. QuicArenaScopedPtr<QuicAlarm> send_alarm_; - // TODO(fayang): remove ping_alarm_ when deprecating quic_use_ping_manager. + // TODO(fayang): remove ping_alarm_ when deprecating quic_use_ping_manager2. // An alarm that fires when a ping should be sent. QuicArenaScopedPtr<QuicAlarm> ping_alarm_; // An alarm that fires when an MTU probe should be sent. @@ -2256,7 +2256,7 @@ // If true, send connection close packet on INVALID_VERSION. bool send_connection_close_for_invalid_version_ = false; - const bool use_ping_manager_ = GetQuicReloadableFlag(quic_use_ping_manager); + const bool use_ping_manager_ = GetQuicReloadableFlag(quic_use_ping_manager2); QuicPingManager ping_manager_;
diff --git a/quiche/quic/core/quic_connection_test.cc b/quiche/quic/core/quic_connection_test.cc index 0da0673..692e3ef 100644 --- a/quiche/quic/core/quic_connection_test.cc +++ b/quiche/quic/core/quic_connection_test.cc
@@ -7185,7 +7185,7 @@ } TEST_P(QuicConnectionTest, RetransmittableOnWireSendFirstPacket) { - if (!GetQuicReloadableFlag(quic_use_ping_manager) || + if (!GetQuicReloadableFlag(quic_use_ping_manager2) || !VersionHasIetfQuicFrames(connection_.version().transport_version)) { return; } @@ -7231,7 +7231,7 @@ } TEST_P(QuicConnectionTest, RetransmittableOnWireSendRandomBytes) { - if (!GetQuicReloadableFlag(quic_use_ping_manager) || + if (!GetQuicReloadableFlag(quic_use_ping_manager2) || !VersionHasIetfQuicFrames(connection_.version().transport_version)) { return; } @@ -7280,7 +7280,7 @@ TEST_P(QuicConnectionTest, RetransmittableOnWireSendRandomBytesWithWriterBlocked) { - if (!GetQuicReloadableFlag(quic_use_ping_manager) || + if (!GetQuicReloadableFlag(quic_use_ping_manager2) || !VersionHasIetfQuicFrames(connection_.version().transport_version)) { return; } @@ -8250,6 +8250,8 @@ peer_creator_.packet_number() + 1); EXPECT_EQ(initial_retransmittable_on_wire_timeout, connection_.GetPingAlarm()->deadline() - clock_.ApproximateNow()); + clock_.AdvanceTime(initial_retransmittable_on_wire_timeout); + connection_.GetPingAlarm()->Fire(); // Verify the count of consecutive aggressive pings is reset. for (int i = 0; i < max_aggressive_retransmittable_on_wire_ping_count; i++) {
diff --git a/quiche/quic/core/quic_flags_list.h b/quiche/quic/core/quic_flags_list.h index 75c3687..73b41a3 100644 --- a/quiche/quic/core/quic_flags_list.h +++ b/quiche/quic/core/quic_flags_list.h
@@ -86,7 +86,7 @@ // If true, use BBRv2 as the default congestion controller. Takes precedence over --quic_default_to_bbr. QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_default_to_bbr_v2, false) // If true, use PING manager to manage the PING alarm. -QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_use_ping_manager, true) +QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_use_ping_manager2, true) // If true, use new connection ID in connection migration. QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_connection_migration_use_new_cid_v2, true) // If true, uses conservative cwnd gain and pacing gain when cwnd gets bootstrapped.
diff --git a/quiche/quic/core/quic_ping_manager.cc b/quiche/quic/core/quic_ping_manager.cc index e3947e5..e2ef9c2 100644 --- a/quiche/quic/core/quic_ping_manager.cc +++ b/quiche/quic/core/quic_ping_manager.cc
@@ -8,6 +8,11 @@ namespace { +// Maximum shift used to calculate retransmittable on wire timeout. For 200ms +// initial retransmittable on wire delay, this would get a maximum of 200ms * (1 +// << 10) = 204.8s +const int kMaxRetransmittableOnWireDelayShift = 10; + class AlarmDelegate : public QuicAlarm::DelegateWithContext { public: explicit AlarmDelegate(QuicPingManager* manager, @@ -47,11 +52,6 @@ return; } alarm_->Update(earliest_deadline, kAlarmGranularity); - if (GetQuicFlag( - FLAGS_quic_max_aggressive_retransmittable_on_wire_ping_count) != 0) { - ++consecutive_retransmittable_on_wire_count_; - } - ++retransmittable_on_wire_count_; } void QuicPingManager::OnAlarm() { @@ -65,6 +65,12 @@ // to SetAlarm later. if (earliest_deadline == retransmittable_on_wire_deadline_) { retransmittable_on_wire_deadline_ = QuicTime::Zero(); + if (GetQuicFlag( + FLAGS_quic_max_aggressive_retransmittable_on_wire_ping_count) != + 0) { + ++consecutive_retransmittable_on_wire_count_; + } + ++retransmittable_on_wire_count_; delegate_->OnRetransmittableOnWireTimeout(); return; } @@ -125,8 +131,9 @@ max_aggressive_retransmittable_on_wire_count) { // Exponentially back off the timeout if the number of consecutive // retransmittable on wire pings has exceeds the allowance. - int shift = consecutive_retransmittable_on_wire_count_ - - max_aggressive_retransmittable_on_wire_count; + int shift = std::min(consecutive_retransmittable_on_wire_count_ - + max_aggressive_retransmittable_on_wire_count, + kMaxRetransmittableOnWireDelayShift); retransmittable_on_wire_timeout = initial_retransmittable_on_wire_timeout_ * (1 << shift); }
diff --git a/quiche/quic/core/quic_ping_manager_test.cc b/quiche/quic/core/quic_ping_manager_test.cc index e7b4b78..509022b 100644 --- a/quiche/quic/core/quic_ping_manager_test.cc +++ b/quiche/quic/core/quic_ping_manager_test.cc
@@ -16,6 +16,11 @@ static QuicAlarm* GetAlarm(QuicPingManager* manager) { return manager->alarm_.get(); } + + static void SetPerspective(QuicPingManager* manager, + Perspective perspective) { + manager->perspective_ = perspective; + } }; namespace { @@ -292,6 +297,9 @@ !kHasInflightPackets); EXPECT_EQ(initial_retransmittable_on_wire_timeout, alarm_->deadline() - clock_.ApproximateNow()); + EXPECT_CALL(delegate_, OnRetransmittableOnWireTimeout()); + clock_.AdvanceTime(initial_retransmittable_on_wire_timeout); + alarm_->Fire(); for (int i = 0; i < kMaxAggressiveRetransmittableOnWireCount; i++) { manager_.SetAlarm(clock_.ApproximateNow(), kShouldKeepAlive, @@ -375,6 +383,46 @@ EXPECT_FALSE(alarm_->IsSet()); } +TEST_F(QuicPingManagerTest, MaxRetransmittableOnWireDelayShift) { + QuicPingManagerPeer::SetPerspective(&manager_, Perspective::IS_SERVER); + const int kMaxAggressiveRetransmittableOnWireCount = 3; + SetQuicFlag(FLAGS_quic_max_aggressive_retransmittable_on_wire_ping_count, + kMaxAggressiveRetransmittableOnWireCount); + const QuicTime::Delta initial_retransmittable_on_wire_timeout = + QuicTime::Delta::FromMilliseconds(200); + manager_.set_initial_retransmittable_on_wire_timeout( + initial_retransmittable_on_wire_timeout); + + for (int i = 0; i <= kMaxAggressiveRetransmittableOnWireCount; i++) { + manager_.SetAlarm(clock_.ApproximateNow(), kShouldKeepAlive, + !kHasInflightPackets); + EXPECT_TRUE(alarm_->IsSet()); + EXPECT_EQ(initial_retransmittable_on_wire_timeout, + alarm_->deadline() - clock_.ApproximateNow()); + clock_.AdvanceTime(initial_retransmittable_on_wire_timeout); + EXPECT_CALL(delegate_, OnRetransmittableOnWireTimeout()); + alarm_->Fire(); + manager_.SetAlarm(clock_.ApproximateNow(), kShouldKeepAlive, + kHasInflightPackets); + } + for (int i = 1; i <= 20; ++i) { + manager_.SetAlarm(clock_.ApproximateNow(), kShouldKeepAlive, + !kHasInflightPackets); + EXPECT_TRUE(alarm_->IsSet()); + if (i <= 10) { + EXPECT_EQ(initial_retransmittable_on_wire_timeout * (1 << i), + alarm_->deadline() - clock_.ApproximateNow()); + } else { + // Verify shift is capped. + EXPECT_EQ(initial_retransmittable_on_wire_timeout * (1 << 10), + alarm_->deadline() - clock_.ApproximateNow()); + } + clock_.AdvanceTime(alarm_->deadline() - clock_.ApproximateNow()); + EXPECT_CALL(delegate_, OnRetransmittableOnWireTimeout()); + alarm_->Fire(); + } +} + } // namespace } // namespace test
diff --git a/quiche/quic/test_tools/quic_connection_peer.cc b/quiche/quic/test_tools/quic_connection_peer.cc index 686d0c8..e38bfc3 100644 --- a/quiche/quic/test_tools/quic_connection_peer.cc +++ b/quiche/quic/test_tools/quic_connection_peer.cc
@@ -146,7 +146,7 @@ // static QuicAlarm* QuicConnectionPeer::GetPingAlarm(QuicConnection* connection) { - if (GetQuicReloadableFlag(quic_use_ping_manager)) { + if (GetQuicReloadableFlag(quic_use_ping_manager2)) { return connection->ping_manager_.alarm_.get(); } return connection->ping_alarm_.get();