From b89882a77272b55bf4a025122daf6bd18271bd65 Mon Sep 17 00:00:00 2001 From: Matt Joras Date: Thu, 18 May 2023 13:51:53 -0700 Subject: [PATCH] Add TransportSetting and knob for default priority. Summary: Allows the default stream priority (which also means scheduling policy) to be set via TransportSettings. Reviewed By: jbeshay, hanidamlaj Differential Revision: D45881729 fbshipit-source-id: fcb72022cd6eac2f1dafc55173d5a46a72a95dbc --- quic/QuicConstants.h | 3 +- quic/api/test/QuicPacketSchedulerTest.cpp | 67 ++++++++++++++++++++ quic/server/QuicServerTransport.cpp | 25 ++++++++ quic/server/test/QuicServerTransportTest.cpp | 13 +++- quic/state/StateData.cpp | 1 + quic/state/TransportSettings.h | 4 ++ 6 files changed, 111 insertions(+), 2 deletions(-) diff --git a/quic/QuicConstants.h b/quic/QuicConstants.h index d46b6cddb..06d515b7f 100644 --- a/quic/QuicConstants.h +++ b/quic/QuicConstants.h @@ -167,7 +167,8 @@ BETTER_ENUM( // Controls whether to fire write loops early when pacing FIRE_LOOP_EARLY = 0x10001, // Controls the timer tick used for pacing - PACING_TIMER_TICK = 0x10002) + PACING_TIMER_TICK = 0x10002, + DEFAULT_STREAM_PRIORITY = 0x10003) enum class FrameType : uint64_t { PADDING = 0x00, diff --git a/quic/api/test/QuicPacketSchedulerTest.cpp b/quic/api/test/QuicPacketSchedulerTest.cpp index 0ac7e1013..5324f3f26 100644 --- a/quic/api/test/QuicPacketSchedulerTest.cpp +++ b/quic/api/test/QuicPacketSchedulerTest.cpp @@ -1430,6 +1430,73 @@ TEST_F(QuicPacketSchedulerTest, StreamFrameSchedulerSequential) { EXPECT_EQ(*frames[2].asWriteStreamFrame(), f3); } +TEST_F(QuicPacketSchedulerTest, StreamFrameSchedulerSequentialDefault) { + QuicClientConnectionState conn( + FizzClientQuicHandshakeContext::Builder().build()); + conn.streamManager->setMaxLocalBidirectionalStreams(10); + conn.flowControlState.peerAdvertisedMaxOffset = 100000; + conn.flowControlState.peerAdvertisedInitialMaxStreamOffsetBidiRemote = 100000; + conn.transportSettings.defaultPriority = Priority(0, false); + auto connId = getTestConnectionId(); + StreamFrameScheduler scheduler(conn); + ShortHeader shortHeader1( + ProtectionType::KeyPhaseZero, + connId, + getNextPacketNum(conn, PacketNumberSpace::AppData)); + RegularQuicPacketBuilder builder1( + conn.udpSendPacketLen, + std::move(shortHeader1), + conn.ackStates.appDataAckState.largestAckedByPeer.value_or(0)); + auto stream1 = + conn.streamManager->createNextBidirectionalStream().value()->id; + auto stream2 = + conn.streamManager->createNextBidirectionalStream().value()->id; + auto stream3 = + conn.streamManager->createNextBidirectionalStream().value()->id; + auto largeBuf = folly::IOBuf::createChain(conn.udpSendPacketLen * 2, 4096); + auto curBuf = largeBuf.get(); + do { + curBuf->append(curBuf->capacity()); + curBuf = curBuf->next(); + } while (curBuf != largeBuf.get()); + auto chainLen = largeBuf->computeChainDataLength(); + writeDataToQuicStream( + *conn.streamManager->findStream(stream1), std::move(largeBuf), false); + writeDataToQuicStream( + *conn.streamManager->findStream(stream2), + folly::IOBuf::copyBuffer("some data"), + false); + writeDataToQuicStream( + *conn.streamManager->findStream(stream3), + folly::IOBuf::copyBuffer("some data"), + false); + // The default is to wraparound initially. + scheduler.writeStreams(builder1); + EXPECT_EQ( + conn.streamManager->writableStreams().getNextScheduledStream( + Priority(0, false)), + stream1); + + // Should write frames for stream1, stream2, stream3, in that order. + NiceMock builder2; + EXPECT_CALL(builder2, remainingSpaceInPkt()).WillRepeatedly(Return(4096)); + EXPECT_CALL(builder2, appendFrame(_)).WillRepeatedly(Invoke([&](auto f) { + builder2.frames_.push_back(f); + })); + scheduler.writeStreams(builder2); + auto& frames = builder2.frames_; + ASSERT_EQ(frames.size(), 3); + WriteStreamFrame f1(stream1, 0, chainLen, false); + WriteStreamFrame f2(stream2, 0, 9, false); + WriteStreamFrame f3(stream3, 0, 9, false); + ASSERT_TRUE(frames[0].asWriteStreamFrame()); + EXPECT_EQ(*frames[0].asWriteStreamFrame(), f1); + ASSERT_TRUE(frames[1].asWriteStreamFrame()); + EXPECT_EQ(*frames[1].asWriteStreamFrame(), f2); + ASSERT_TRUE(frames[2].asWriteStreamFrame()); + EXPECT_EQ(*frames[2].asWriteStreamFrame(), f3); +} + TEST_F(QuicPacketSchedulerTest, StreamFrameSchedulerRoundRobinControl) { QuicClientConnectionState conn( FizzClientQuicHandshakeContext::Builder().build()); diff --git a/quic/server/QuicServerTransport.cpp b/quic/server/QuicServerTransport.cpp index 470cff6b8..1778f51c0 100644 --- a/quic/server/QuicServerTransport.cpp +++ b/quic/server/QuicServerTransport.cpp @@ -1046,6 +1046,31 @@ void QuicServerTransport::registerAllTransportKnobParamHandlers() { std::chrono::microseconds(val); VLOG(3) << "PACING_TIMER_TICK KnobParam received: " << val; }); + registerTransportKnobParamHandler( + static_cast(TransportKnobParamId::DEFAULT_STREAM_PRIORITY), + [](QuicServerTransport* serverTransport, TransportKnobParam::Val value) { + CHECK(serverTransport); + auto val = std::get(value); + auto serverConn = serverTransport->serverConn_; + uint8_t level; + bool incremental; + bool parseSuccess = false; + try { + parseSuccess = folly::split(",", val, level, incremental); + } catch (std::exception&) { + parseSuccess = false; + } + if (!parseSuccess) { + auto errMsg = fmt::format( + "Received invalid KnobParam for DEFAULT_STREAM_PRIORITY: {}", + val); + VLOG(3) << errMsg; + throw std::runtime_error(errMsg); + } + serverConn->transportSettings.defaultPriority = + Priority(level, incremental); + VLOG(3) << "DEFAULT_STREAM_PRIORITY KnobParam received: " << val; + }); } QuicConnectionStats QuicServerTransport::getConnectionsStats() const { diff --git a/quic/server/test/QuicServerTransportTest.cpp b/quic/server/test/QuicServerTransportTest.cpp index 51487c555..f013e84d9 100644 --- a/quic/server/test/QuicServerTransportTest.cpp +++ b/quic/server/test/QuicServerTransportTest.cpp @@ -4825,7 +4825,18 @@ TEST_F(QuicServerTransportTest, TestAckFrequencyPolicyKnobHandler) { server->handleKnobParams( {{static_cast(TransportKnobParamId::ACK_FREQUENCY_POLICY), "1,3,1,1"}}); - EXPECT_FALSE(server->getTransportSettings().bbrConfig.ackFrequencyConfig); + server->handleKnobParams( + {{static_cast(TransportKnobParamId::DEFAULT_STREAM_PRIORITY), + "1,1"}}); + EXPECT_EQ(server->getTransportSettings().defaultPriority, Priority(1, true)); + server->handleKnobParams( + {{static_cast(TransportKnobParamId::DEFAULT_STREAM_PRIORITY), + "4,0"}}); + EXPECT_EQ(server->getTransportSettings().defaultPriority, Priority(4, false)); + server->handleKnobParams( + {{static_cast(TransportKnobParamId::DEFAULT_STREAM_PRIORITY), + "4,0,10"}}); + EXPECT_EQ(server->getTransportSettings().defaultPriority, Priority(4, false)); } TEST_F(QuicServerTransportTest, TestSetMaxPacingRateLifecycle) { diff --git a/quic/state/StateData.cpp b/quic/state/StateData.cpp index 5e84ed591..9992c6089 100644 --- a/quic/state/StateData.cpp +++ b/quic/state/StateData.cpp @@ -38,6 +38,7 @@ QuicStreamState::QuicStreamState(StreamId idIn, QuicConnectionStateBase& connIn) sendState = StreamSendState::Invalid; } } + priority = connIn.transportSettings.defaultPriority; } QuicStreamState::QuicStreamState( diff --git a/quic/state/TransportSettings.h b/quic/state/TransportSettings.h index 3ae3417d7..f8abd63be 100644 --- a/quic/state/TransportSettings.h +++ b/quic/state/TransportSettings.h @@ -9,6 +9,7 @@ #include #include +#include #include #include @@ -267,6 +268,9 @@ struct TransportSettings { bool advertisedKnobFrameSupport{true}; bool removeStreamAfterEomCallbackUnset{false}; + // The default priority to instantiate streams with. + Priority defaultPriority{kDefaultPriority}; + // Local configuration for ACK receive timestamps. // // Determines the ACK receive timestamp configuration sent to peer,