In quicconnection, revert to a previously validated mtu after a delay equivalent to the duration of three ptos. protected by --gfe2_reloadable_flag_quic_revert_mtu_after_two_ptos. PiperOrigin-RevId: 323411817 Change-Id: I1972673c2e75677fcffad3a45d9e0087e8fc6fc2
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc index df6c5c2..9a74a33 100644 --- a/quic/core/quic_connection.cc +++ b/quic/core/quic_connection.cc
@@ -2721,7 +2721,7 @@ << ", previous_validated_mtu_:" << previous_validated_mtu_ << ", max_packet_length():" << max_packet_length() << ", is_mtu_discovery:" << is_mtu_discovery; - if (ShouldIgnoreWriteError()) { + if (MaybeRevertToPreviousMtu()) { return true; } @@ -2752,7 +2752,8 @@ // because either both detections are inactive when sending last packet // or this connection just gets out of quiescence. blackhole_detector_.RestartDetection(GetPathDegradingDeadline(), - GetNetworkBlackholeDeadline()); + GetNetworkBlackholeDeadline(), + GetPathMtuReductionDeadline()); } idle_network_detector_.OnPacketSent(packet_send_time); } @@ -2824,7 +2825,7 @@ return; } - if (IsWriteError(result.status) && !ShouldIgnoreWriteError()) { + if (IsWriteError(result.status) && !MaybeRevertToPreviousMtu()) { OnWriteError(result.error_code); } } @@ -2854,7 +2855,22 @@ return false; } -bool QuicConnection::ShouldIgnoreWriteError() { +QuicTime QuicConnection::GetPathMtuReductionDeadline() const { + if (!blackhole_detector_.revert_mtu_after_two_ptos()) { + return QuicTime::Zero(); + } + if (previous_validated_mtu_ == 0) { + return QuicTime::Zero(); + } + QuicTime::Delta delay = sent_packet_manager_.GetMtuReductionDelay( + num_rtos_for_blackhole_detection_); + if (delay.IsZero()) { + return QuicTime::Zero(); + } + return clock_->ApproximateNow() + delay; +} + +bool QuicConnection::MaybeRevertToPreviousMtu() { if (previous_validated_mtu_ == 0) { return false; } @@ -4489,7 +4505,8 @@ if (sent_packet_manager_.HasInFlightPackets()) { // Restart detections if forward progress has been made. blackhole_detector_.RestartDetection(GetPathDegradingDeadline(), - GetNetworkBlackholeDeadline()); + GetNetworkBlackholeDeadline(), + GetPathMtuReductionDeadline()); } else { // Stop detections in quiecense. blackhole_detector_.StopDetection(); @@ -4639,6 +4656,15 @@ ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET); } +void QuicConnection::OnPathMtuReductionDetected() { + DCHECK(blackhole_detector_.revert_mtu_after_two_ptos()); + if (MaybeRevertToPreviousMtu()) { + QUIC_RELOADABLE_FLAG_COUNT_N(quic_revert_mtu_after_two_ptos, 1, 2); + } else { + QUIC_RELOADABLE_FLAG_COUNT_N(quic_revert_mtu_after_two_ptos, 2, 2); + } +} + void QuicConnection::OnHandshakeTimeout() { const QuicTime::Delta duration = clock_->ApproximateNow() - stats_.connection_creation_time;
diff --git a/quic/core/quic_connection.h b/quic/core/quic_connection.h index 36ef402..9439adf 100644 --- a/quic/core/quic_connection.h +++ b/quic/core/quic_connection.h
@@ -612,6 +612,7 @@ // QuicNetworkBlackholeDetector::Delegate void OnPathDegradingDetected() override; void OnBlackholeDetected() override; + void OnPathMtuReductionDetected() override; // QuicIdleNetworkDetector::Delegate void OnHandshakeTimeout() override; @@ -1292,10 +1293,12 @@ // |supports_release_time_| is false. QuicTime CalculatePacketSentTime(); - // We've got a packet write error, should we ignore it? - // NOTE: This is not a const function - if return true, the max packet size is - // reverted to a previous(smaller) value to avoid write errors in the future. - bool ShouldIgnoreWriteError(); + // If we have a previously validate MTU value, e.g. due to a write error, + // revert to it and disable MTU discovery. + // Return true iff we reverted to a previously validate MTU. + bool MaybeRevertToPreviousMtu(); + + QuicTime GetPathMtuReductionDeadline() const; // Returns path degrading deadline. QuicTime::Zero() means no path degrading // detection is needed.
diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc index 59f12bb..ac301c2 100644 --- a/quic/core/quic_connection_test.cc +++ b/quic/core/quic_connection_test.cc
@@ -932,6 +932,11 @@ .IsInitialized(); } + bool PathMtuReductionDetectionInProgress() { + return QuicConnectionPeer::GetPathMtuReductionDetectionDeadline(this) + .IsInitialized(); + } + void SetMaxTailLossProbes(size_t max_tail_loss_probes) { QuicSentPacketManagerPeer::SetMaxTailLossProbes( QuicConnectionPeer::GetSentPacketManager(this), max_tail_loss_probes); @@ -5434,6 +5439,32 @@ ConstructAckFrame(creator_->packet_number(), second_probe_packet_number); ProcessAckPacket(&third_ack); EXPECT_EQ(third_probe_size, connection_.max_packet_length()); + + SendStreamDataToPeer(3, "$", stream_offset++, NO_FIN, nullptr); + if (!GetQuicReloadableFlag(quic_revert_mtu_after_two_ptos)) { + EXPECT_FALSE(connection_.PathMtuReductionDetectionInProgress()); + return; + } + + EXPECT_TRUE(connection_.PathMtuReductionDetectionInProgress()); + + if (connection_.PathDegradingDetectionInProgress() && + QuicConnectionPeer::GetPathDegradingDeadline(&connection_) < + QuicConnectionPeer::GetPathMtuReductionDetectionDeadline( + &connection_)) { + // Fire path degrading alarm first. + connection_.PathDegradingTimeout(); + } + + // Verify the max packet size has not reduced. + EXPECT_EQ(third_probe_size, connection_.max_packet_length()); + + // Fire alarm to get path mtu reduction callback called. + EXPECT_TRUE(connection_.PathMtuReductionDetectionInProgress()); + connection_.GetBlackholeDetectorAlarm()->Fire(); + + // Verify the max packet size has reduced to the previous value. + EXPECT_EQ(probe_size, connection_.max_packet_length()); } // Tests whether MTU discovery works when the writer has a limit on how large a
diff --git a/quic/core/quic_network_blackhole_detector.cc b/quic/core/quic_network_blackhole_detector.cc index 9206ae4..02e3308 100644 --- a/quic/core/quic_network_blackhole_detector.cc +++ b/quic/core/quic_network_blackhole_detector.cc
@@ -30,45 +30,128 @@ QuicConnectionArena* arena, QuicAlarmFactory* alarm_factory) : delegate_(delegate), - path_degrading_deadline_(QuicTime::Zero()), - blackhole_deadline_(QuicTime::Zero()), alarm_( alarm_factory->CreateAlarm(arena->New<AlarmDelegate>(this), arena)) {} void QuicNetworkBlackholeDetector::OnAlarm() { - if (path_degrading_deadline_.IsInitialized()) { - path_degrading_deadline_ = QuicTime::Zero(); - delegate_->OnPathDegradingDetected(); - // Switch to blackhole detection mode. - alarm_->Update(blackhole_deadline_, kAlarmGranularity); + if (!revert_mtu_after_two_ptos_) { + if (path_degrading_deadline_.IsInitialized()) { + path_degrading_deadline_ = QuicTime::Zero(); + delegate_->OnPathDegradingDetected(); + // Switch to blackhole detection mode. + alarm_->Update(blackhole_deadline_, kAlarmGranularity); + return; + } + if (blackhole_deadline_.IsInitialized()) { + blackhole_deadline_ = QuicTime::Zero(); + delegate_->OnBlackholeDetected(); + } return; } - if (blackhole_deadline_.IsInitialized()) { + + QuicTime next_deadline = GetEarliestDeadline(); + if (!next_deadline.IsInitialized()) { + QUIC_BUG << "BlackholeDetector alarm fired unexpectedly"; + return; + } + + QUIC_DLOG(INFO) << "BlackholeDetector alarm firing. next_deadline:" + << next_deadline + << ", path_degrading_deadline_:" << path_degrading_deadline_ + << ", path_mtu_reduction_deadline_:" + << path_mtu_reduction_deadline_ + << ", blackhole_deadline_:" << blackhole_deadline_; + if (path_degrading_deadline_ == next_deadline) { + path_degrading_deadline_ = QuicTime::Zero(); + delegate_->OnPathDegradingDetected(); + } + + if (path_mtu_reduction_deadline_ == next_deadline) { + path_mtu_reduction_deadline_ = QuicTime::Zero(); + delegate_->OnPathMtuReductionDetected(); + } + + if (blackhole_deadline_ == next_deadline) { blackhole_deadline_ = QuicTime::Zero(); delegate_->OnBlackholeDetected(); } + + UpdateAlarm(); } void QuicNetworkBlackholeDetector::StopDetection() { alarm_->Cancel(); path_degrading_deadline_ = QuicTime::Zero(); blackhole_deadline_ = QuicTime::Zero(); + path_mtu_reduction_deadline_ = QuicTime::Zero(); } void QuicNetworkBlackholeDetector::RestartDetection( QuicTime path_degrading_deadline, - QuicTime blackhole_deadline) { + QuicTime blackhole_deadline, + QuicTime path_mtu_reduction_deadline) { path_degrading_deadline_ = path_degrading_deadline; blackhole_deadline_ = blackhole_deadline; - QUIC_BUG_IF(path_degrading_deadline_.IsInitialized() && - blackhole_deadline_.IsInitialized() && - path_degrading_deadline_ > blackhole_deadline_) - << "Path degrading timeout is later than blackhole detection timeout"; - alarm_->Update(path_degrading_deadline_, kAlarmGranularity); - if (alarm_->IsSet()) { - return; + path_mtu_reduction_deadline_ = path_mtu_reduction_deadline; + + if (!revert_mtu_after_two_ptos_) { + QUIC_BUG_IF(path_degrading_deadline_.IsInitialized() && + blackhole_deadline_.IsInitialized() && + path_degrading_deadline_ > blackhole_deadline_) + << "Path degrading timeout is later than blackhole detection timeout"; + } else { + QUIC_BUG_IF(blackhole_deadline_.IsInitialized() && + blackhole_deadline_ != GetLastDeadline()) + << "Blackhole detection deadline should be the last deadline."; } - alarm_->Update(blackhole_deadline_, kAlarmGranularity); + + if (!revert_mtu_after_two_ptos_) { + alarm_->Update(path_degrading_deadline_, kAlarmGranularity); + if (alarm_->IsSet()) { + return; + } + alarm_->Update(blackhole_deadline_, kAlarmGranularity); + } else { + UpdateAlarm(); + } +} + +QuicTime QuicNetworkBlackholeDetector::GetEarliestDeadline() const { + DCHECK(revert_mtu_after_two_ptos_); + + QuicTime result = QuicTime::Zero(); + for (QuicTime t : {path_degrading_deadline_, blackhole_deadline_, + path_mtu_reduction_deadline_}) { + if (!t.IsInitialized()) { + continue; + } + + if (!result.IsInitialized() || t < result) { + result = t; + } + } + + return result; +} + +QuicTime QuicNetworkBlackholeDetector::GetLastDeadline() const { + DCHECK(revert_mtu_after_two_ptos_); + return std::max({path_degrading_deadline_, blackhole_deadline_, + path_mtu_reduction_deadline_}); +} + +void QuicNetworkBlackholeDetector::UpdateAlarm() const { + DCHECK(revert_mtu_after_two_ptos_); + + QuicTime next_deadline = GetEarliestDeadline(); + + QUIC_DLOG(INFO) << "Updating alarm. next_deadline:" << next_deadline + << ", path_degrading_deadline_:" << path_degrading_deadline_ + << ", path_mtu_reduction_deadline_:" + << path_mtu_reduction_deadline_ + << ", blackhole_deadline_:" << blackhole_deadline_; + + alarm_->Update(next_deadline, kAlarmGranularity); } bool QuicNetworkBlackholeDetector::IsDetectionInProgress() const {
diff --git a/quic/core/quic_network_blackhole_detector.h b/quic/core/quic_network_blackhole_detector.h index b839d42..77c5c59 100644 --- a/quic/core/quic_network_blackhole_detector.h +++ b/quic/core/quic_network_blackhole_detector.h
@@ -10,6 +10,7 @@ #include "net/third_party/quiche/src/quic/core/quic_one_block_arena.h" #include "net/third_party/quiche/src/quic/core/quic_time.h" #include "net/third_party/quiche/src/quic/platform/api/quic_export.h" +#include "net/third_party/quiche/src/quic/platform/api/quic_flags.h" namespace quic { @@ -34,6 +35,9 @@ // Called when the path blackhole alarm fires. virtual void OnBlackholeDetected() = 0; + + // Called when the path mtu reduction alarm fires. + virtual void OnPathMtuReductionDetected() = 0; }; QuicNetworkBlackholeDetector(Delegate* delegate, @@ -43,11 +47,12 @@ // Called to stop all detections. void StopDetection(); - // Called to restart path degrading or/and blackhole detections. Please note, - // if both deadlines are set, |blackhole_deadline| must be later than - // |path_degrading_deadline|. + // Called to restart path degrading, path mtu reduction and blackhole + // detections. Please note, if |blackhole_deadline| is set, it must be the + // furthest in the future of all deadlines. void RestartDetection(QuicTime path_degrading_deadline, - QuicTime blackhole_deadline); + QuicTime blackhole_deadline, + QuicTime path_mtu_reduction_deadline); // Called when |alarm_| fires. void OnAlarm(); @@ -55,18 +60,32 @@ // Returns true if |alarm_| is set. bool IsDetectionInProgress() const; + bool revert_mtu_after_two_ptos() const { return revert_mtu_after_two_ptos_; } + private: friend class test::QuicConnectionPeer; friend class test::QuicNetworkBlackholeDetectorPeer; + QuicTime GetEarliestDeadline() const; + QuicTime GetLastDeadline() const; + + // Update alarm to the next deadline. + void UpdateAlarm() const; + Delegate* delegate_; // Not owned. + const bool revert_mtu_after_two_ptos_ = + GetQuicReloadableFlag(quic_revert_mtu_after_two_ptos); + // Time that Delegate::OnPathDegrading will be called. 0 means no path // degrading detection is in progress. - QuicTime path_degrading_deadline_; + QuicTime path_degrading_deadline_ = QuicTime::Zero(); // Time that Delegate::OnBlackholeDetected will be called. 0 means no // blackhole detection is in progress. - QuicTime blackhole_deadline_; + QuicTime blackhole_deadline_ = QuicTime::Zero(); + // Time that Delegate::OnPathMtuReductionDetected will be called. 0 means no + // path mtu reduction detection is in progress. + QuicTime path_mtu_reduction_deadline_ = QuicTime::Zero(); QuicArenaScopedPtr<QuicAlarm> alarm_; };
diff --git a/quic/core/quic_network_blackhole_detector_test.cc b/quic/core/quic_network_blackhole_detector_test.cc index 0178887..3f6f747 100644 --- a/quic/core/quic_network_blackhole_detector_test.cc +++ b/quic/core/quic_network_blackhole_detector_test.cc
@@ -23,9 +23,11 @@ public: MOCK_METHOD(void, OnPathDegradingDetected, (), (override)); MOCK_METHOD(void, OnBlackholeDetected, (), (override)); + MOCK_METHOD(void, OnPathMtuReductionDetected, (), (override)); }; const size_t kPathDegradingDelayInSeconds = 5; +const size_t kPathMtuReductionDelayInSeconds = 7; const size_t kBlackholeDelayInSeconds = 10; class QuicNetworkBlackholeDetectorTest : public QuicTest { @@ -36,12 +38,20 @@ QuicNetworkBlackholeDetectorPeer::GetAlarm(&detector_))), path_degrading_delay_( QuicTime::Delta::FromSeconds(kPathDegradingDelayInSeconds)), + path_mtu_reduction_delay_( + QuicTime::Delta::FromSeconds(kPathMtuReductionDelayInSeconds)), blackhole_delay_( QuicTime::Delta::FromSeconds(kBlackholeDelayInSeconds)) { clock_.AdvanceTime(QuicTime::Delta::FromSeconds(1)); } protected: + void RestartDetection() { + detector_.RestartDetection(clock_.Now() + path_degrading_delay_, + clock_.Now() + blackhole_delay_, + clock_.Now() + path_mtu_reduction_delay_); + } + testing::StrictMock<MockDelegate> delegate_; QuicConnectionArena arena_; MockAlarmFactory alarm_factory_; @@ -51,14 +61,14 @@ MockAlarmFactory::TestAlarm* alarm_; MockClock clock_; const QuicTime::Delta path_degrading_delay_; + const QuicTime::Delta path_mtu_reduction_delay_; const QuicTime::Delta blackhole_delay_; }; TEST_F(QuicNetworkBlackholeDetectorTest, StartAndFire) { EXPECT_FALSE(detector_.IsDetectionInProgress()); - detector_.RestartDetection(clock_.Now() + path_degrading_delay_, - clock_.Now() + blackhole_delay_); + RestartDetection(); EXPECT_TRUE(detector_.IsDetectionInProgress()); EXPECT_EQ(clock_.Now() + path_degrading_delay_, alarm_->deadline()); @@ -66,25 +76,48 @@ clock_.AdvanceTime(path_degrading_delay_); EXPECT_CALL(delegate_, OnPathDegradingDetected()); alarm_->Fire(); + + if (!detector_.revert_mtu_after_two_ptos()) { + // Verify blackhole detection is still in progress. + EXPECT_TRUE(detector_.IsDetectionInProgress()); + EXPECT_EQ(clock_.Now() + blackhole_delay_ - path_degrading_delay_, + alarm_->deadline()); + + // Fire blackhole detection alarm. + clock_.AdvanceTime(blackhole_delay_ - path_degrading_delay_); + EXPECT_CALL(delegate_, OnBlackholeDetected()); + alarm_->Fire(); + EXPECT_FALSE(detector_.IsDetectionInProgress()); + return; + } + + // Verify path mtu reduction detection is still in progress. + EXPECT_TRUE(detector_.IsDetectionInProgress()); + EXPECT_EQ(clock_.Now() + path_mtu_reduction_delay_ - path_degrading_delay_, + alarm_->deadline()); + + // Fire path mtu reduction detection alarm. + clock_.AdvanceTime(path_mtu_reduction_delay_ - path_degrading_delay_); + EXPECT_CALL(delegate_, OnPathMtuReductionDetected()); + alarm_->Fire(); + // Verify blackhole detection is still in progress. EXPECT_TRUE(detector_.IsDetectionInProgress()); - EXPECT_EQ(clock_.Now() + blackhole_delay_ - path_degrading_delay_, + EXPECT_EQ(clock_.Now() + blackhole_delay_ - path_mtu_reduction_delay_, alarm_->deadline()); // Fire blackhole detection alarm. - clock_.AdvanceTime(blackhole_delay_ - path_degrading_delay_); + clock_.AdvanceTime(blackhole_delay_ - path_mtu_reduction_delay_); EXPECT_CALL(delegate_, OnBlackholeDetected()); alarm_->Fire(); EXPECT_FALSE(detector_.IsDetectionInProgress()); } TEST_F(QuicNetworkBlackholeDetectorTest, RestartAndStop) { - detector_.RestartDetection(clock_.Now() + path_degrading_delay_, - clock_.Now() + blackhole_delay_); + RestartDetection(); clock_.AdvanceTime(QuicTime::Delta::FromSeconds(1)); - detector_.RestartDetection(clock_.Now() + path_degrading_delay_, - clock_.Now() + blackhole_delay_); + RestartDetection(); EXPECT_EQ(clock_.Now() + path_degrading_delay_, alarm_->deadline()); detector_.StopDetection(); @@ -93,8 +126,7 @@ TEST_F(QuicNetworkBlackholeDetectorTest, PathDegradingFiresAndRestart) { EXPECT_FALSE(detector_.IsDetectionInProgress()); - detector_.RestartDetection(clock_.Now() + path_degrading_delay_, - clock_.Now() + blackhole_delay_); + RestartDetection(); EXPECT_TRUE(detector_.IsDetectionInProgress()); EXPECT_EQ(clock_.Now() + path_degrading_delay_, alarm_->deadline()); @@ -102,15 +134,22 @@ clock_.AdvanceTime(path_degrading_delay_); EXPECT_CALL(delegate_, OnPathDegradingDetected()); alarm_->Fire(); - // Verify blackhole detection is still in progress. - EXPECT_TRUE(detector_.IsDetectionInProgress()); - EXPECT_EQ(clock_.Now() + blackhole_delay_ - path_degrading_delay_, - alarm_->deadline()); + + if (!detector_.revert_mtu_after_two_ptos()) { + // Verify blackhole detection is still in progress. + EXPECT_TRUE(detector_.IsDetectionInProgress()); + EXPECT_EQ(clock_.Now() + blackhole_delay_ - path_degrading_delay_, + alarm_->deadline()); + } else { + // Verify path mtu reduction detection is still in progress. + EXPECT_TRUE(detector_.IsDetectionInProgress()); + EXPECT_EQ(clock_.Now() + path_mtu_reduction_delay_ - path_degrading_delay_, + alarm_->deadline()); + } // After 100ms, restart detections on forward progress. clock_.AdvanceTime(QuicTime::Delta::FromMilliseconds(100)); - detector_.RestartDetection(clock_.Now() + path_degrading_delay_, - clock_.Now() + blackhole_delay_); + RestartDetection(); // Verify alarm is armed based on path degrading deadline. EXPECT_EQ(clock_.Now() + path_degrading_delay_, alarm_->deadline()); }
diff --git a/quic/core/quic_sent_packet_manager.cc b/quic/core/quic_sent_packet_manager.cc index 2848137..ededb87 100644 --- a/quic/core/quic_sent_packet_manager.cc +++ b/quic/core/quic_sent_packet_manager.cc
@@ -1167,6 +1167,11 @@ max_tail_loss_probes_ + num_rtos_for_blackhole_detection); } +QuicTime::Delta QuicSentPacketManager::GetMtuReductionDelay( + int8_t num_rtos_for_blackhole_detection) const { + return GetNetworkBlackholeDelay(num_rtos_for_blackhole_detection / 2); +} + const QuicTime::Delta QuicSentPacketManager::GetCryptoRetransmissionDelay() const { // This is equivalent to the TailLossProbeDelay, but slightly more aggressive
diff --git a/quic/core/quic_sent_packet_manager.h b/quic/core/quic_sent_packet_manager.h index 310cd1a..4e6cab1 100644 --- a/quic/core/quic_sent_packet_manager.h +++ b/quic/core/quic_sent_packet_manager.h
@@ -215,6 +215,11 @@ const QuicTime::Delta GetNetworkBlackholeDelay( int8_t num_rtos_for_blackhole_detection) const; + // Returns the delay before reducing max packet size. This delay is guranteed + // to be smaller than the network blackhole delay. + QuicTime::Delta GetMtuReductionDelay( + int8_t num_rtos_for_blackhole_detection) const; + const RttStats* GetRttStats() const { return &rtt_stats_; } // Returns the estimated bandwidth calculated by the congestion algorithm.
diff --git a/quic/test_tools/quic_connection_peer.cc b/quic/test_tools/quic_connection_peer.cc index 7192797..5fb31a5 100644 --- a/quic/test_tools/quic_connection_peer.cc +++ b/quic/test_tools/quic_connection_peer.cc
@@ -356,6 +356,12 @@ } // static +QuicTime QuicConnectionPeer::GetPathMtuReductionDetectionDeadline( + QuicConnection* connection) { + return connection->blackhole_detector_.path_mtu_reduction_deadline_; +} + +// static QuicAlarm* QuicConnectionPeer::GetIdleNetworkDetectorAlarm( QuicConnection* connection) { return connection->idle_network_detector_.alarm_.get();
diff --git a/quic/test_tools/quic_connection_peer.h b/quic/test_tools/quic_connection_peer.h index 258d953..8f3a78c 100644 --- a/quic/test_tools/quic_connection_peer.h +++ b/quic/test_tools/quic_connection_peer.h
@@ -141,6 +141,9 @@ static QuicTime GetBlackholeDetectionDeadline(QuicConnection* connection); + static QuicTime GetPathMtuReductionDetectionDeadline( + QuicConnection* connection); + static QuicAlarm* GetIdleNetworkDetectorAlarm(QuicConnection* connection); static void SetServerConnectionId(