From 599d410b01eb82222059d28a2a47c3303cc698fe Mon Sep 17 00:00:00 2001 From: Jolene Tan Date: Tue, 15 Oct 2024 18:04:35 -0700 Subject: [PATCH] Only skip cloned CRYPTO packet if same as most recent outstanding packet Summary: In CloningScheduler, in the loop for selecting a candidate packet for cloning under the "cloneAllPacketsWithCryptoFrame" mode of operation, skip a packet containing a CRYPTO frame only if both it and the most recent outstanding packet are clones of the same packet. Otherwise, it would clone the CRYPTO packets no more than once. Reviewed By: mjoras Differential Revision: D64411438 fbshipit-source-id: eac9e3dbb4c6d2536b1259af08f3c9647ef06ad8 --- quic/api/QuicPacketScheduler.cpp | 19 +++-- quic/api/test/QuicPacketSchedulerTest.cpp | 85 ++++++++++++++++------- 2 files changed, 72 insertions(+), 32 deletions(-) diff --git a/quic/api/QuicPacketScheduler.cpp b/quic/api/QuicPacketScheduler.cpp index 697b231c3..5394cc7c1 100644 --- a/quic/api/QuicPacketScheduler.cpp +++ b/quic/api/QuicPacketScheduler.cpp @@ -920,14 +920,19 @@ SchedulingResult CloningScheduler::scheduleFramesForPacket( } // The packet is already a clone if (outstandingPacket.maybeClonedPacketIdentifier) { - // If packet has CRYPTO frame, don't clone it again even if not processed - // yet, move on to give next packet a chance to be cloned const auto& frames = outstandingPacket.packet.frames; - if (conn_.transportSettings.cloneAllPacketsWithCryptoFrame && - std::find_if(frames.begin(), frames.end(), [](const auto& frame) { - return frame.type() == QuicWriteFrame::Type::WriteCryptoFrame; - }) != frames.end()) { - continue; + if (conn_.transportSettings.cloneAllPacketsWithCryptoFrame) { + // Has CRYPTO frame + if (std::find_if(frames.begin(), frames.end(), [](const auto& frame) { + return frame.type() == QuicWriteFrame::Type::WriteCryptoFrame; + }) != frames.end()) { + auto mostRecentOutstandingPacketIdentifier = + conn_.outstandings.packets.back().maybeClonedPacketIdentifier; + if (mostRecentOutstandingPacketIdentifier == + outstandingPacket.maybeClonedPacketIdentifier) { + continue; + } + } } // Otherwise, clone until it is processed if (conn_.outstandings.clonedPacketIdentifiers.count( diff --git a/quic/api/test/QuicPacketSchedulerTest.cpp b/quic/api/test/QuicPacketSchedulerTest.cpp index b7934b1b0..14a6f7d91 100644 --- a/quic/api/test/QuicPacketSchedulerTest.cpp +++ b/quic/api/test/QuicPacketSchedulerTest.cpp @@ -642,29 +642,60 @@ TEST_P( FrameScheduler noopScheduler("frame", conn); CloningScheduler cloningScheduler(noopScheduler, conn, "cryptoClone", 0); - // First packet has a crypto frame - PacketNum firstPacketNum = addOutstandingPacket(conn); - conn.outstandings.packets.back().packet.frames.push_back( - WriteCryptoFrame(0, 1)); - ClonedPacketIdentifier clonedPacketIdentifier( - PacketNumberSpace::AppData, firstPacketNum); - conn.outstandings.packets.back().maybeClonedPacketIdentifier = - clonedPacketIdentifier; - // It is not processed yet - conn.outstandings.clonedPacketIdentifiers.insert(clonedPacketIdentifier); - // There needs to have retransmittable frame for the rebuilder to work - conn.outstandings.packets.back().packet.frames.push_back( - MaxDataFrame(conn.flowControlState.advertisedMaxOffset)); + PacketNum firstPacketNum = addInitialOutstandingPacket(conn); + { + conn.outstandings.packets.back().packet.frames.push_back( + WriteCryptoFrame(0, 1)); + ClonedPacketIdentifier clonedPacketIdentifier( + PacketNumberSpace::Initial, firstPacketNum); + conn.outstandings.packets.back().maybeClonedPacketIdentifier = + clonedPacketIdentifier; + // It is not processed yet + conn.outstandings.clonedPacketIdentifiers.insert(clonedPacketIdentifier); + // There needs to have retransmittable frame for the rebuilder to work + conn.outstandings.packets.back().packet.frames.push_back( + MaxDataFrame(conn.flowControlState.advertisedMaxOffset)); + } - PacketNum secondPacketNum = addOutstandingPacket(conn); - // There needs to have retransmittable frame for the rebuilder to work - conn.outstandings.packets.back().packet.frames.push_back( - MaxDataFrame(conn.flowControlState.advertisedMaxOffset)); + PacketNum secondPacketNum = addInitialOutstandingPacket(conn); + { + conn.outstandings.packets.back().packet.frames.push_back( + WriteCryptoFrame(0, 1)); + ClonedPacketIdentifier clonedPacketIdentifier( + PacketNumberSpace::Initial, secondPacketNum); + conn.outstandings.packets.back().maybeClonedPacketIdentifier = + clonedPacketIdentifier; + // It is not processed yet + conn.outstandings.clonedPacketIdentifiers.insert(clonedPacketIdentifier); + // There needs to have retransmittable frame for the rebuilder to work + conn.outstandings.packets.back().packet.frames.push_back( + MaxDataFrame(conn.flowControlState.advertisedMaxOffset)); + } - ShortHeader header( - ProtectionType::KeyPhaseOne, + // Add a third outstanding packet, which is a clone of the first packet + { + addInitialOutstandingPacket(conn); + conn.outstandings.packets.back().packet.frames.push_back( + WriteCryptoFrame(0, 1)); + ClonedPacketIdentifier clonedPacketIdentifier( + PacketNumberSpace::Initial, firstPacketNum); + conn.outstandings.packets.back().maybeClonedPacketIdentifier = + clonedPacketIdentifier; + // It is not processed yet + conn.outstandings.clonedPacketIdentifiers.insert(clonedPacketIdentifier); + // There needs to have retransmittable frame for the rebuilder to work + conn.outstandings.packets.back().packet.frames.push_back( + MaxDataFrame(conn.flowControlState.advertisedMaxOffset)); + } + + std::vector zeroConnIdData(quic::kDefaultConnectionIdSize, 0); + ConnectionId srcConnId(zeroConnIdData); + LongHeader header( + LongHeader::Types::Initial, + srcConnId, conn.clientConnectionId.value_or(getTestConnectionId()), - getNextPacketNum(conn, PacketNumberSpace::AppData)); + getNextPacketNum(conn, PacketNumberSpace::Initial), + QuicVersion::MVFST); RegularQuicPacketBuilder builder( conn.udpSendPacketLen, std::move(header), @@ -692,22 +723,26 @@ TEST_F(QuicPacketSchedulerTest, DoNotSkipUnclonedCryptoPacket) { CloningScheduler cloningScheduler(noopScheduler, conn, "cryptoClone", 0); // First packet has a crypto frame - PacketNum firstPacketNum = addOutstandingPacket(conn); + PacketNum firstPacketNum = addInitialOutstandingPacket(conn); conn.outstandings.packets.back().packet.frames.push_back( WriteCryptoFrame(0, 1)); // There needs to have retransmittable frame for the rebuilder to work conn.outstandings.packets.back().packet.frames.push_back( MaxDataFrame(conn.flowControlState.advertisedMaxOffset)); - addOutstandingPacket(conn); + addInitialOutstandingPacket(conn); // There needs to have retransmittable frame for the rebuilder to work conn.outstandings.packets.back().packet.frames.push_back( MaxDataFrame(conn.flowControlState.advertisedMaxOffset)); - ShortHeader header( - ProtectionType::KeyPhaseOne, + std::vector zeroConnIdData(quic::kDefaultConnectionIdSize, 0); + ConnectionId srcConnId(zeroConnIdData); + LongHeader header( + LongHeader::Types::Initial, + srcConnId, conn.clientConnectionId.value_or(getTestConnectionId()), - getNextPacketNum(conn, PacketNumberSpace::AppData)); + getNextPacketNum(conn, PacketNumberSpace::Initial), + QuicVersion::MVFST); RegularQuicPacketBuilder builder( conn.udpSendPacketLen, std::move(header),