From 95898ef8b700a289fd71a19fed51692efce414ae Mon Sep 17 00:00:00 2001 From: Crystal Jin Date: Wed, 13 Nov 2024 16:57:21 -0800 Subject: [PATCH] Fall back to evb timer if no pacer timer set Reviewed By: kvtsoy Differential Revision: D65848549 fbshipit-source-id: 38c1e9b8c820dbe2848cb132d7e49079c45eb67d --- quic/api/QuicTransportBaseLite.cpp | 32 ++++++++++++----------------- quic/api/test/QuicTransportTest.cpp | 4 ++-- quic/common/FunctionLooper.cpp | 30 +++++++++++++-------------- quic/common/FunctionLooper.h | 2 -- 4 files changed, 29 insertions(+), 39 deletions(-) diff --git a/quic/api/QuicTransportBaseLite.cpp b/quic/api/QuicTransportBaseLite.cpp index 984e1ab81..8f5bc9ffb 100644 --- a/quic/api/QuicTransportBaseLite.cpp +++ b/quic/api/QuicTransportBaseLite.cpp @@ -2751,23 +2751,18 @@ void QuicTransportBaseLite::setTransportSettings( } return 0us; }); - if (writeLooper_->hasPacingTimer()) { - bool usingBbr = - (conn_->transportSettings.defaultCongestionController == - CongestionControlType::BBR || - conn_->transportSettings.defaultCongestionController == - CongestionControlType::BBRTesting || - conn_->transportSettings.defaultCongestionController == - CongestionControlType::BBR2); - auto minCwnd = usingBbr ? kMinCwndInMssForBbr - : conn_->transportSettings.minCwndInMss; - conn_->pacer = std::make_unique(*conn_, minCwnd); - conn_->pacer->setExperimental(conn_->transportSettings.experimentalPacer); - conn_->canBePaced = conn_->transportSettings.pacingEnabledFirstFlight; - } else { - LOG(ERROR) << "Pacing cannot be enabled without a timer"; - conn_->transportSettings.pacingEnabled = false; - } + bool usingBbr = + (conn_->transportSettings.defaultCongestionController == + CongestionControlType::BBR || + conn_->transportSettings.defaultCongestionController == + CongestionControlType::BBRTesting || + conn_->transportSettings.defaultCongestionController == + CongestionControlType::BBR2); + auto minCwnd = + usingBbr ? kMinCwndInMssForBbr : conn_->transportSettings.minCwndInMss; + conn_->pacer = std::make_unique(*conn_, minCwnd); + conn_->pacer->setExperimental(conn_->transportSettings.experimentalPacer); + conn_->canBePaced = conn_->transportSettings.pacingEnabledFirstFlight; } setCongestionControl(conn_->transportSettings.defaultCongestionController); if (conn_->transportSettings.datagramConfig.enabled) { @@ -2840,8 +2835,7 @@ void QuicTransportBaseLite::validateCongestionAndPacing( if ((type == CongestionControlType::BBR || type == CongestionControlType::BBRTesting || type == CongestionControlType::BBR2) && - (!conn_->transportSettings.pacingEnabled || - !writeLooper_->hasPacingTimer())) { + !conn_->transportSettings.pacingEnabled) { LOG(ERROR) << "Unpaced BBR isn't supported"; type = CongestionControlType::Cubic; } diff --git a/quic/api/test/QuicTransportTest.cpp b/quic/api/test/QuicTransportTest.cpp index 76df89faa..1c3cabebc 100644 --- a/quic/api/test/QuicTransportTest.cpp +++ b/quic/api/test/QuicTransportTest.cpp @@ -3916,12 +3916,12 @@ TEST_F(QuicTransportTest, NotifyPendingWriteConnBufferFreeUpSpace) { NetworkData(ReceivedUdpPacket(IOBuf::copyBuffer("fake data")))); } -TEST_F(QuicTransportTest, NoPacingTimerNoPacing) { +TEST_F(QuicTransportTest, NoPacingTimerStillPaced) { TransportSettings transportSettings; transportSettings.pacingEnabled = true; transport_->setTransportSettings(transportSettings); transport_->getConnectionState().canBePaced = true; - EXPECT_FALSE(isConnectionPaced(transport_->getConnectionState())); + EXPECT_TRUE(isConnectionPaced(transport_->getConnectionState())); } TEST_F(QuicTransportTest, SetPacingTimerThenEnablesPacing) { diff --git a/quic/common/FunctionLooper.cpp b/quic/common/FunctionLooper.cpp index f7a448fdd..d468bcedd 100644 --- a/quic/common/FunctionLooper.cpp +++ b/quic/common/FunctionLooper.cpp @@ -29,10 +29,6 @@ void FunctionLooper::setPacingTimer(QuicTimer::SharedPtr pacingTimer) noexcept { pacingTimer_ = std::move(pacingTimer); } -bool FunctionLooper::hasPacingTimer() const noexcept { - return pacingTimer_ != nullptr; -} - void FunctionLooper::setPacingFunction( folly::Function&& pacingFunc) { CHECK(pacingFunc); @@ -59,12 +55,18 @@ void FunctionLooper::commonLoopBody() noexcept { } bool FunctionLooper::schedulePacingTimeout() noexcept { - if (pacingFunc_ && pacingTimer_ && !isTimerCallbackScheduled()) { + if (pacingFunc_ && (pacingTimer_ || evb_) && !isTimerCallbackScheduled()) { auto timeUntilWrite = pacingFunc_(); if (timeUntilWrite != 0us) { nextPacingTime_ = Clock::now() + timeUntilWrite; - pacingTimer_->scheduleTimeout(this, timeUntilWrite); - return true; + if (pacingTimer_) { + pacingTimer_->scheduleTimeout(this, timeUntilWrite); + return true; + } + if (evb_) { + evb_->scheduleTimeoutHighRes(this, timeUntilWrite); + return true; + } } } return false; @@ -93,13 +95,13 @@ void FunctionLooper::run(bool thisIteration, bool runInline) noexcept { return; } if (isLoopCallbackScheduled() || - (!fireLoopEarly_ && pacingTimer_ && isTimerCallbackScheduled())) { + (!fireLoopEarly_ && pacingFunc_ && isTimerCallbackScheduled())) { VLOG(10) << __func__ << ": " << type_ << " already scheduled"; return; } // If we are pacing, we're about to write again, if it's close, just write // now. - if (pacingTimer_ && isTimerCallbackScheduled()) { + if (pacingFunc_ && isTimerCallbackScheduled()) { auto n = Clock::now(); auto timeUntilWrite = nextPacingTime_ < n ? 0us @@ -122,9 +124,7 @@ void FunctionLooper::stop() noexcept { if (evb_) { cancelLoopCallback(); } - if (pacingTimer_) { - cancelTimerCallback(); - } + cancelTimerCallback(); } bool FunctionLooper::isRunning() const { @@ -132,7 +132,7 @@ bool FunctionLooper::isRunning() const { } bool FunctionLooper::isPacingScheduled() { - return pacingTimer_ && isTimerCallbackScheduled(); + return pacingFunc_ && isTimerCallbackScheduled(); } bool FunctionLooper::isLoopCallbackScheduled() { @@ -150,9 +150,7 @@ void FunctionLooper::detachEventBase() { VLOG(10) << __func__ << ": " << type_; DCHECK(evb_ && evb_->isInEventBaseThread()); stop(); - if (pacingTimer_) { - cancelTimerCallback(); - } + cancelTimerCallback(); evb_ = nullptr; } diff --git a/quic/common/FunctionLooper.h b/quic/common/FunctionLooper.h index ec09c2ace..a57e9a540 100644 --- a/quic/common/FunctionLooper.h +++ b/quic/common/FunctionLooper.h @@ -43,8 +43,6 @@ class FunctionLooper : public QuicEventBaseLoopCallback, void setPacingTimer(QuicTimer::SharedPtr pacingTimer) noexcept; - bool hasPacingTimer() const noexcept; - void runLoopCallback() noexcept override; /**