diff --git a/quic/QuicConstants.h b/quic/QuicConstants.h index 89bc6f739..8f6a1cad1 100644 --- a/quic/QuicConstants.h +++ b/quic/QuicConstants.h @@ -180,6 +180,8 @@ BETTER_ENUM( PACER_EXPERIMENTAL = 0x5557, // Set short header padding modulo size SHORT_HEADER_PADDING_KNOB = 0x6666, + // Set fixed short header padding size + FIXED_SHORT_HEADER_PADDING_KNOB = 0x6667, // Keepalive timer enabled KEEPALIVE_ENABLED = 0x7777, // Remove from loss buffer on spurious loss diff --git a/quic/api/QuicPacketScheduler.cpp b/quic/api/QuicPacketScheduler.cpp index 182cbaa52..d5f57248c 100644 --- a/quic/api/QuicPacketScheduler.cpp +++ b/quic/api/QuicPacketScheduler.cpp @@ -241,7 +241,19 @@ SchedulingResult FrameScheduler::scheduleFramesForPacket( PacketBuilderInterface&& builder, uint32_t writableBytes) { size_t shortHeaderPadding = 0; + const ShortHeader* shortHeader = builder.getPacketHeader().asShort(); + const LongHeader* longHeader = builder.getPacketHeader().asLong(); + bool initialPacket = + longHeader && longHeader->getHeaderType() == LongHeader::Types::Initial; builder.encodePacketHeader(); + // Add fixed padding at start of short header packets if configured + if (shortHeader && conn_.transportSettings.fixedShortHeaderPadding > 0) { + for (size_t i = 0; i < conn_.transportSettings.fixedShortHeaderPadding; + i++) { + writeFrame(PaddingFrame(), builder); + } + shortHeaderPadding = conn_.transportSettings.fixedShortHeaderPadding; + } // We need to keep track of writable bytes after writing header. writableBytes = writableBytes > builder.getHeaderBytes() ? writableBytes - builder.getHeaderBytes() @@ -307,16 +319,12 @@ SchedulingResult FrameScheduler::scheduleFramesForPacket( } if (builder.hasFramesPending()) { - const LongHeader* longHeader = builder.getPacketHeader().asLong(); - bool initialPacket = - longHeader && longHeader->getHeaderType() == LongHeader::Types::Initial; if (initialPacket) { // This is the initial packet, we need to fill er up. while (builder.remainingSpaceInPkt() > 0) { writeFrame(PaddingFrame(), builder); } } - const ShortHeader* shortHeader = builder.getPacketHeader().asShort(); if (shortHeader) { size_t paddingModulo = conn_.transportSettings.paddingModulo; if (paddingModulo > 0) { @@ -324,7 +332,7 @@ SchedulingResult FrameScheduler::scheduleFramesForPacket( for (size_t i = 0; i < paddingIncrement; i++) { writeFrame(PaddingFrame(), builder); } - shortHeaderPadding = paddingIncrement; + shortHeaderPadding += paddingIncrement; } } } @@ -879,6 +887,9 @@ bool CloningScheduler::hasData() const { SchedulingResult CloningScheduler::scheduleFramesForPacket( PacketBuilderInterface&& builder, uint32_t writableBytes) { + // Store header type information before any moves + auto builderPnSpace = builder.getPacketHeader().getPacketNumberSpace(); + auto header = builder.getPacketHeader(); // The writableBytes in this function shouldn't be limited by cwnd, since // we only use CloningScheduler for the cases that we want to bypass cwnd for // now. @@ -896,7 +907,6 @@ SchedulingResult CloningScheduler::scheduleFramesForPacket( } // TODO: We can avoid the copy & rebuild of the header by creating an // independent header builder. - auto header = builder.getPacketHeader(); std::move(builder).releaseOutputBuffer(); // Look for an outstanding packet that's no larger than the writableBytes for (auto& outstandingPacket : conn_.outstandings.packets) { @@ -909,7 +919,6 @@ SchedulingResult CloningScheduler::scheduleFramesForPacket( // clone packet. So re-create a RegularQuicPacketBuilder every time. // TODO: We can avoid the copy & rebuild of the header by creating an // independent header builder. - auto builderPnSpace = builder.getPacketHeader().getPacketNumberSpace(); if (opPnSpace != builderPnSpace) { continue; } diff --git a/quic/api/test/QuicPacketSchedulerTest.cpp b/quic/api/test/QuicPacketSchedulerTest.cpp index 7e6b00534..884bd8c1f 100644 --- a/quic/api/test/QuicPacketSchedulerTest.cpp +++ b/quic/api/test/QuicPacketSchedulerTest.cpp @@ -444,7 +444,7 @@ TEST_F(QuicPacketSchedulerTest, CryptoPaddingRetransmissionClientInitial) { ChainedByteRangeHead clientHelloData(helloBuf); conn.cryptoState->initialStream.lossBuffer.push_back( WriteStreamBuffer{std::move(clientHelloData), 0, false}); - auto result = scheduler.scheduleFramesForPacket( + auto result = std::move(scheduler).scheduleFramesForPacket( std::move(builder), conn.udpSendPacketLen); auto packetLength = result.packet->header.computeChainDataLength() + result.packet->body.computeChainDataLength(); @@ -2334,6 +2334,53 @@ TEST_F(QuicPacketSchedulerTest, ShortHeaderPaddingWithSpaceForPadding) { EXPECT_EQ(packetLength1, packetLength2); } +TEST_F(QuicPacketSchedulerTest, ShortHeaderFixedPaddingAtStart) { + QuicServerConnectionState conn( + FizzServerQuicHandshakeContext::Builder().build()); + conn.transportSettings.fixedShortHeaderPadding = 2; + conn.transportSettings.paddingModulo = 16; + conn.flowControlState.peerAdvertisedMaxOffset = 1000000; + conn.flowControlState.peerAdvertisedInitialMaxStreamOffsetBidiRemote = + 1000000; + + // Create stream and write data + conn.streamManager->setMaxLocalBidirectionalStreams(10); + auto stream = conn.streamManager->createNextBidirectionalStream().value(); + auto data = buildRandomInputData(50); // Small enough to fit in one packet + writeDataToQuicStream(*stream, std::move(data), false); + + // Set up scheduler and builder + FrameScheduler scheduler = std::move(FrameScheduler::Builder( + conn, + EncryptionLevel::AppData, + PacketNumberSpace::AppData, + "streamScheduler") + .streamFrames()) + .build(); + + ShortHeader header( + ProtectionType::KeyPhaseOne, + conn.clientConnectionId.value_or(getTestConnectionId()), + getNextPacketNum(conn, PacketNumberSpace::AppData)); + + RegularQuicPacketBuilder builder( + conn.udpSendPacketLen, + std::move(header), + conn.ackStates.appDataAckState.largestAckedByPeer.value_or(0)); + + // Schedule frames + auto result = scheduler.scheduleFramesForPacket( + std::move(builder), conn.udpSendPacketLen); + + // Verify padding frames were added at start + EXPECT_TRUE(result.packet.hasValue()); + const auto& frames = result.packet->packet.frames; + ASSERT_EQ(frames.size(), 3); + EXPECT_TRUE(frames[0].asPaddingFrame()); + EXPECT_TRUE(frames[1].asWriteStreamFrame()); + EXPECT_TRUE(frames[2].asPaddingFrame()); +} + TEST_F(QuicPacketSchedulerTest, ShortHeaderPaddingNearMaxPacketLength) { QuicServerConnectionState conn( FizzServerQuicHandshakeContext::Builder().build()); @@ -2464,8 +2511,9 @@ TEST_F(QuicPacketSchedulerTest, ImmediateAckFrameSchedulerOnRequest) { .immediateAckFrames()) .build(); - auto result = immediateAckOnlyScheduler.scheduleFramesForPacket( - std::move(builder), conn.udpSendPacketLen); + auto result = + std::move(immediateAckOnlyScheduler) + .scheduleFramesForPacket(std::move(builder), conn.udpSendPacketLen); auto packetLength = result.packet->header.computeChainDataLength() + result.packet->body.computeChainDataLength(); EXPECT_EQ(conn.udpSendPacketLen, packetLength); @@ -2500,12 +2548,13 @@ TEST_F(QuicPacketSchedulerTest, ImmediateAckFrameSchedulerNotRequested) { .immediateAckFrames()) .build(); - auto result = immediateAckOnlyScheduler.scheduleFramesForPacket( - std::move(builder), conn.udpSendPacketLen); + auto result = + std::move(immediateAckOnlyScheduler) + .scheduleFramesForPacket(std::move(builder), conn.udpSendPacketLen); auto packetLength = result.packet->header.computeChainDataLength() + result.packet->body.computeChainDataLength(); - // The immediate ACK scheduler was not triggered. This packet has no frames - // and it shouldn't get padded. + // The immediate ACK scheduler was not triggered. This packet has no + // frames and it shouldn't get padded. EXPECT_LT(packetLength, conn.udpSendPacketLen); } @@ -2620,4 +2669,52 @@ TEST_F(QuicPacketSchedulerTest, PausedPriorityInitial) { ASSERT_FALSE(conn.streamManager->hasWritable()); } +TEST_F(QuicPacketSchedulerTest, FixedShortHeaderPadding) { + QuicServerConnectionState conn( + FizzServerQuicHandshakeContext::Builder().build()); + conn.transportSettings.fixedShortHeaderPadding = 2; + conn.transportSettings.paddingModulo = 0; + conn.flowControlState.peerAdvertisedMaxOffset = 1000000; + conn.flowControlState.peerAdvertisedInitialMaxStreamOffsetBidiRemote = + 1000000; + + // Create stream and write data + conn.streamManager->setMaxLocalBidirectionalStreams(10); + auto stream = conn.streamManager->createNextBidirectionalStream().value(); + auto data = buildRandomInputData(50); // Small enough to fit in one packet + writeDataToQuicStream(*stream, std::move(data), false); + conn.streamManager->updateWritableStreams(*stream); + + // Set up scheduler and builder + FrameScheduler scheduler = std::move(FrameScheduler::Builder( + conn, + EncryptionLevel::AppData, + PacketNumberSpace::AppData, + "streamScheduler") + .streamFrames()) + .build(); + + ShortHeader header( + ProtectionType::KeyPhaseOne, + conn.clientConnectionId.value_or(getTestConnectionId()), + getNextPacketNum(conn, PacketNumberSpace::AppData)); + + RegularQuicPacketBuilder builder( + conn.udpSendPacketLen, + std::move(header), + conn.ackStates.appDataAckState.largestAckedByPeer.value_or(0)); + + // Schedule frames + auto result = scheduler.scheduleFramesForPacket( + std::move(builder), conn.udpSendPacketLen); + + // Verify padding frames were added + // at start + EXPECT_TRUE(result.packet.hasValue()); + const auto& frames = result.packet->packet.frames; + ASSERT_EQ(frames.size(), 2); + EXPECT_TRUE(frames[0].asPaddingFrame()); + EXPECT_TRUE(frames[1].asWriteStreamFrame()); +} + } // namespace quic::test diff --git a/quic/server/QuicServerTransport.cpp b/quic/server/QuicServerTransport.cpp index 1a15a98d3..3501dc828 100644 --- a/quic/server/QuicServerTransport.cpp +++ b/quic/server/QuicServerTransport.cpp @@ -938,6 +938,18 @@ void QuicServerTransport::registerAllTransportKnobParamHandlers() { "SHORT_HEADER_PADDING_KNOB KnobParam received, setting paddingModulo={}", val); }); + registerTransportKnobParamHandler( + static_cast( + TransportKnobParamId::FIXED_SHORT_HEADER_PADDING_KNOB), + [](QuicServerTransport* serverTransport, TransportKnobParam::Val value) { + CHECK(serverTransport); + auto val = std::get(value); + serverTransport->serverConn_->transportSettings + .fixedShortHeaderPadding = val; + VLOG(3) << fmt::format( + "FIXED_SHORT_HEADER_PADDING_KNOB KnobParam received, setting fixedShortHeaderPadding={}", + val); + }); registerTransportKnobParamHandler( static_cast(TransportKnobParamId::ADAPTIVE_LOSS_DETECTION), diff --git a/quic/server/test/QuicServerTransportTest.cpp b/quic/server/test/QuicServerTransportTest.cpp index ec4c55fc7..d081b7847 100644 --- a/quic/server/test/QuicServerTransportTest.cpp +++ b/quic/server/test/QuicServerTransportTest.cpp @@ -5580,6 +5580,17 @@ TEST_F( EXPECT_EQ(server->getConn().udpSendPacketLen, 1452); } +TEST_F( + QuicServerTransportTest, + TestHandleTransportKnobParamFixedShortHeaderPadding) { + EXPECT_EQ(server->getConn().transportSettings.fixedShortHeaderPadding, 0); + server->handleKnobParams( + {{static_cast( + TransportKnobParamId::FIXED_SHORT_HEADER_PADDING_KNOB), + uint64_t{42}}}); + EXPECT_EQ(server->getConn().transportSettings.fixedShortHeaderPadding, 42); +} + TEST_F(QuicServerTransportTest, WriteDSR) { EXPECT_EQ(server->getConn().dsrPacketCount, 0); // Make sure we are post-handshake diff --git a/quic/state/TransportSettings.h b/quic/state/TransportSettings.h index 84f1bc57e..e28261563 100644 --- a/quic/state/TransportSettings.h +++ b/quic/state/TransportSettings.h @@ -291,6 +291,9 @@ struct TransportSettings { // packet size from packet analysis. // Padding Modulo of 0 turns off padding for short header packets. size_t paddingModulo{kShortHeaderPaddingModulo}; + // Number of padding frames to add at the start of short header packets. + // A value of 0 means no fixed padding is added. + size_t fixedShortHeaderPadding{0}; // Whether to use adaptive loss thresholds for reodering and timeout bool useAdaptiveLossReorderingThresholds{false}; bool useAdaptiveLossTimeThresholds{false};