Add fixed short header padding

Summary: This adds a knob which puts a fixed amount of padding at the start of short header packets. This is useful to test the consequences of e.g. a larger CID or the impacts of a smaller packet size.

Reviewed By: jbeshay

Differential Revision: D69603113

fbshipit-source-id: b92ba78682eed21b7d75e38c9584a93481e2eb2f

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

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;
     }

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(
+  auto result =
-      std::move(builder), conn.udpSendPacketLen);
+      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(
+  auto result =
-      std::move(builder), conn.udpSendPacketLen);
+      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
+  // The immediate ACK scheduler was not triggered. This packet has no
-  // and it shouldn't get padded.
+  // 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

quic/server/QuicServerTransport.cpp

@@ -938,6 +938,18 @@ void QuicServerTransport::registerAllTransportKnobParamHandlers() {
             "SHORT_HEADER_PADDING_KNOB KnobParam received, setting paddingModulo={}",
             val);
       });
+  registerTransportKnobParamHandler(
+      static_cast<uint64_t>(
+          TransportKnobParamId::FIXED_SHORT_HEADER_PADDING_KNOB),
+      [](QuicServerTransport* serverTransport, TransportKnobParam::Val value) {
+        CHECK(serverTransport);
+        auto val = std::get<uint64_t>(value);
+        serverTransport->serverConn_->transportSettings
+            .fixedShortHeaderPadding = val;
+        VLOG(3) << fmt::format(
+            "FIXED_SHORT_HEADER_PADDING_KNOB KnobParam received, setting fixedShortHeaderPadding={}",
+            val);
+      });
 
   registerTransportKnobParamHandler(
       static_cast<uint64_t>(TransportKnobParamId::ADAPTIVE_LOSS_DETECTION),

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<uint64_t>(
+            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

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};