From a4d235934c0c761de22a996e5f62a66d8725693a Mon Sep 17 00:00:00 2001 From: Yang Chi Date: Mon, 19 Apr 2021 11:07:16 -0700 Subject: [PATCH] Do not clone if only D6D packets are outstanding Summary: The cloning scheduler will later bypass all D6D probes anyway. So the hasData() API should answer False if only thing outstanding is D6D packets. Reading the code, it's actually not very clear to me if the counter is correct though. :/ So i left some TODO warning comments to the d6d enabling flag (Note: this ignores all push blocking failures!) Reviewed By: mjoras Differential Revision: D27480434 fbshipit-source-id: 12ddded1b496b92ff5c03125c5283b8c7f3fcdb3 --- quic/api/QuicPacketScheduler.cpp | 5 +- quic/api/test/QuicPacketSchedulerTest.cpp | 96 ++++++++++++++++------- quic/state/TransportSettings.h | 3 + 3 files changed, 76 insertions(+), 28 deletions(-) diff --git a/quic/api/QuicPacketScheduler.cpp b/quic/api/QuicPacketScheduler.cpp index e7cb82435..325413d46 100644 --- a/quic/api/QuicPacketScheduler.cpp +++ b/quic/api/QuicPacketScheduler.cpp @@ -686,7 +686,10 @@ CloningScheduler::CloningScheduler( cipherOverhead_(cipherOverhead) {} bool CloningScheduler::hasData() const { - return frameScheduler_.hasData() || conn_.outstandings.numOutstanding() > 0; + // TODO: I'm not completely convinced d6d.outstandingProbes has been updated + // correctly. + return frameScheduler_.hasData() || + conn_.outstandings.numOutstanding() > conn_.d6d.outstandingProbes; } SchedulingResult CloningScheduler::scheduleFramesForPacket( diff --git a/quic/api/test/QuicPacketSchedulerTest.cpp b/quic/api/test/QuicPacketSchedulerTest.cpp index 25b85c2d3..ca28bc413 100644 --- a/quic/api/test/QuicPacketSchedulerTest.cpp +++ b/quic/api/test/QuicPacketSchedulerTest.cpp @@ -28,6 +28,45 @@ enum PacketBuilderType { Regular, Inplace }; namespace { +SchedulingResult sendD6DProbe( + QuicConnectionStateBase& conn, + uint64_t cipherOverhead, + uint32_t probeSize, + PacketBuilderType builderType) { + auto connId = quic::test::getTestConnectionId(); + D6DProbeScheduler d6dProbeScheduler( + conn, "d6d probe", cipherOverhead, probeSize); + EXPECT_TRUE(d6dProbeScheduler.hasData()); + + ShortHeader shortHeader( + ProtectionType::KeyPhaseZero, + connId, + getNextPacketNum(conn, PacketNumberSpace::AppData)); + if (builderType == PacketBuilderType::Regular) { + RegularQuicPacketBuilder builder( + conn.udpSendPacketLen, + std::move(shortHeader), + conn.ackStates.appDataAckState.largestAckedByPeer.value_or(0)); + auto result = d6dProbeScheduler.scheduleFramesForPacket( + std::move(builder), kDefaultUDPSendPacketLen); + EXPECT_FALSE(d6dProbeScheduler.hasData()); + return result; + } else { + // Just enough to build the probe + auto simpleBufAccessor = std::make_unique(probeSize); + InplaceQuicPacketBuilder builder( + *simpleBufAccessor, + conn.udpSendPacketLen, + std::move(shortHeader), + conn.ackStates.appDataAckState.largestAckedByPeer.value_or(0)); + auto result = d6dProbeScheduler.scheduleFramesForPacket( + std::move(builder), kDefaultUDPSendPacketLen); + EXPECT_FALSE(d6dProbeScheduler.hasData()); + return result; + } + folly::assume_unreachable(); +} + PacketNum addInitialOutstandingPacket(QuicConnectionStateBase& conn) { PacketNum nextPacketNum = getNextPacketNum(conn, PacketNumberSpace::Initial); std::vector zeroConnIdData(quic::kDefaultConnectionIdSize, 0); @@ -482,36 +521,39 @@ TEST_P(QuicPacketSchedulerTest, D6DProbeSchedulerTest) { connId, getNextPacketNum(conn, PacketNumberSpace::AppData)); auto param = GetParam(); - size_t packetSize = 0; - if (param == PacketBuilderType::Regular) { - RegularQuicPacketBuilder builder( - conn.udpSendPacketLen, - std::move(shortHeader), - conn.ackStates.appDataAckState.largestAckedByPeer.value_or(0)); - auto result = d6dProbeScheduler.scheduleFramesForPacket( - std::move(builder), kDefaultUDPSendPacketLen); - ASSERT_TRUE(result.packet.has_value()); - packetSize = result.packet->header->computeChainDataLength() + - result.packet->body->computeChainDataLength() + cipherOverhead; - } else { - // Just enough to build the probe - auto simpleBufAccessor = std::make_unique(probeSize); - InplaceQuicPacketBuilder builder( - *simpleBufAccessor, - conn.udpSendPacketLen, - std::move(shortHeader), - conn.ackStates.appDataAckState.largestAckedByPeer.value_or(0)); - auto result = d6dProbeScheduler.scheduleFramesForPacket( - std::move(builder), kDefaultUDPSendPacketLen); - ASSERT_TRUE(result.packet.has_value()); - packetSize = result.packet->header->computeChainDataLength() + - result.packet->body->computeChainDataLength() + cipherOverhead; - } - - EXPECT_FALSE(d6dProbeScheduler.hasData()); + auto result = sendD6DProbe(conn, cipherOverhead, probeSize, param); + ASSERT_TRUE(result.packet.has_value()); + auto packetSize = result.packet->header->computeChainDataLength() + + result.packet->body->computeChainDataLength() + cipherOverhead; EXPECT_EQ(packetSize, probeSize); } +TEST_P(QuicPacketSchedulerTest, NoCloningWithOnlyD6DProbes) { + QuicClientConnectionState conn( + FizzClientQuicHandshakeContext::Builder().build()); + uint64_t cipherOverhead = 2; + uint32_t probeSize = 1450; + auto param = GetParam(); + auto result = sendD6DProbe(conn, cipherOverhead, probeSize, param); + ASSERT_TRUE(result.packet.has_value()); + auto packetSize = result.packet->header->computeChainDataLength() + + result.packet->body->computeChainDataLength() + cipherOverhead; + updateConnection( + conn, + folly::none, + result.packet->packet, + Clock::now(), + packetSize, + result.packet->body->computeChainDataLength(), + false /* isDSRPacket */); + ASSERT_EQ(1, conn.outstandings.packets.size()); + EXPECT_TRUE(conn.outstandings.packets.back().metadata.isD6DProbe); + FrameScheduler noopScheduler("noopScheduler"); + CloningScheduler cloningScheduler( + noopScheduler, conn, "MarShall Mathers", cipherOverhead); + EXPECT_FALSE(cloningScheduler.hasData()); +} + TEST_F(QuicPacketSchedulerTest, WriteOnlyOutstandingPacketsTest) { QuicClientConnectionState conn( FizzClientQuicHandshakeContext::Builder().build()); diff --git a/quic/state/TransportSettings.h b/quic/state/TransportSettings.h index 927da1d10..227c5513b 100644 --- a/quic/state/TransportSettings.h +++ b/quic/state/TransportSettings.h @@ -53,6 +53,9 @@ struct D6DConfig { * whether it will send the base PMTU transport parameter during handshake. * As a result, d6d is activated for a connection only when *both* client and * server enables d6d. + * + * TODO: Please make sure QuicConnectionStateBase::D6DState::outstandingProbes + * are mutated correctly throughout Mvfst. */ bool enabled{false};