From 25a646f96a6ff24285cb9a4726e3ea8054dcfd6a Mon Sep 17 00:00:00 2001 From: Yang Chi Date: Thu, 18 Jun 2020 15:28:59 -0700 Subject: [PATCH] No more Pending mode in Quic ack writing Summary: The AckScheduler right now has two modes: Immediate mode which always write acks into the current packet, pending mode which only write if needsToSendAckImmediately is true. The FrameScheduler choose the Immdiate mode if there are other data to write as well. Otherwise, it chooses the Pending mode. But if there is no other data to write and needsToSendAckImmediately is false, the FrameScheduler will end up writing nothing. This isn't a problem today because to be on the write path, the shouldWriteData function would make sure we either have non-ack data to write, or needsToSendAckImmediately is true for a packet number space. But once we allow packets in Initial and Handshake space to be cloned, we would be on the write path when there are probe quota. The FrameScheduler's hasData function doesn't check needsToSendAckImmediately. It will think it has data to write as long as AckState has changed, but can ends up writing nothing with the Pending ack mode. I think given the write looper won't be schedule to loop when there is no non-ack data to write and needsToSendAckImmediately is true, it's safe to remove Pending ack mode from AckScheduler. Reviewed By: mjoras Differential Revision: D22044741 fbshipit-source-id: 26fcaabdd5c45c1cae12d459ee5924a30936e209 --- quic/api/QuicPacketScheduler-inl.h | 24 -------------------- quic/api/QuicPacketScheduler.cpp | 6 ++--- quic/api/QuicPacketScheduler.h | 17 +------------- quic/api/test/QuicTransportFunctionsTest.cpp | 5 ++-- quic/api/test/QuicTransportTest.cpp | 20 ---------------- 5 files changed, 7 insertions(+), 65 deletions(-) diff --git a/quic/api/QuicPacketScheduler-inl.h b/quic/api/QuicPacketScheduler-inl.h index 48ddcdc22..8ccf0d49e 100644 --- a/quic/api/QuicPacketScheduler-inl.h +++ b/quic/api/QuicPacketScheduler-inl.h @@ -15,30 +15,6 @@ namespace quic { template inline folly::Optional AckScheduler::writeNextAcks( - PacketBuilderInterface& builder, - AckMode mode) { - switch (mode) { - case AckMode::Immediate: { - return writeAcksImpl(builder); - } - case AckMode::Pending: { - return writeAcksIfPending(builder); - } - } - folly::assume_unreachable(); -} - -template -inline folly::Optional AckScheduler::writeAcksIfPending( - PacketBuilderInterface& builder) { - if (ackState_.needsToSendAckImmediately) { - return writeAcksImpl(builder); - } - return folly::none; -} - -template -folly::Optional AckScheduler::writeAcksImpl( PacketBuilderInterface& builder) { // Use default ack delay for long headers. Usually long headers are sent // before crypto negotiation, so the peer might not know about the ack delay diff --git a/quic/api/QuicPacketScheduler.cpp b/quic/api/QuicPacketScheduler.cpp index 01c8f6a76..a725ed4ab 100644 --- a/quic/api/QuicPacketScheduler.cpp +++ b/quic/api/QuicPacketScheduler.cpp @@ -125,7 +125,6 @@ SchedulingResult FrameScheduler::scheduleFramesForPacket( // We cannot return early if the writablyBytes dropps to 0 here, since pure // acks can skip writableBytes entirely. PacketBuilderWrapper wrapper(builder, writableBytes); - auto ackMode = hasImmediateData() ? AckMode::Immediate : AckMode::Pending; bool cryptoDataWritten = false; bool rstWritten = false; if (cryptoStreamScheduler_ && cryptoStreamScheduler_->hasData()) { @@ -134,18 +133,19 @@ SchedulingResult FrameScheduler::scheduleFramesForPacket( if (rstScheduler_ && rstScheduler_->hasPendingRsts()) { rstWritten = rstScheduler_->writeRsts(wrapper); } + // TODO: Long time ago we decided RST has higher priority than Acks. Why tho? if (ackScheduler_ && ackScheduler_->hasPendingAcks()) { if (cryptoDataWritten || rstWritten) { // If packet has non ack data, it is subject to congestion control. We // need to use the wrapper/ - ackScheduler_->writeNextAcks(wrapper, ackMode); + ackScheduler_->writeNextAcks(wrapper); } else { // If we start with writing acks, we will let the ack scheduler write // up to the full packet space. If the ack bytes exceeds the writable // bytes, this will be a pure ack packet and it will skip congestion // controller. Otherwise, we will give other schedulers an opportunity to // write up to writable bytes. - ackScheduler_->writeNextAcks(builder, ackMode); + ackScheduler_->writeNextAcks(builder); } } if (windowUpdateScheduler_ && diff --git a/quic/api/QuicPacketScheduler.h b/quic/api/QuicPacketScheduler.h index cab78a73b..37f883dd5 100644 --- a/quic/api/QuicPacketScheduler.h +++ b/quic/api/QuicPacketScheduler.h @@ -60,9 +60,6 @@ class QuicPacketScheduler { virtual std::string name() const = 0; }; -// A tag to denote how we should schedule ack in this packet. -enum class AckMode { Pending, Immediate }; - class RetransmissionScheduler { public: explicit RetransmissionScheduler(const QuicConnectionStateBase& conn); @@ -198,23 +195,11 @@ class AckScheduler { template folly::Optional writeNextAcks( - PacketBuilderInterface& builder, - AckMode mode); + PacketBuilderInterface& builder); bool hasPendingAcks() const; private: - /* Write out pending acks if needsToSendAckImmeidately in the connection's - * pendingEvent is true. - */ - template - folly::Optional writeAcksIfPending( - PacketBuilderInterface& builder); - - // Write out pending acks - template - folly::Optional writeAcksImpl(PacketBuilderInterface& builder); - const QuicConnectionStateBase& conn_; const AckState& ackState_; }; diff --git a/quic/api/test/QuicTransportFunctionsTest.cpp b/quic/api/test/QuicTransportFunctionsTest.cpp index 10fd27eca..7486d6b37 100644 --- a/quic/api/test/QuicTransportFunctionsTest.cpp +++ b/quic/api/test/QuicTransportFunctionsTest.cpp @@ -1856,10 +1856,11 @@ TEST_F(QuicTransportFunctionsTest, HasAckDataToWriteCipherAndAckStateMatch) { TEST_F(QuicTransportFunctionsTest, HasAckDataToWriteNoImmediateAcks) { auto conn = createConn(); conn->oneRttWriteCipher = test::createNoOpAead(); - conn->ackStates.initialAckState.needsToSendAckImmediately = false; - conn->ackStates.handshakeAckState.needsToSendAckImmediately = false; + conn->ackStates.appDataAckState.acks.insert(0, 100); conn->ackStates.appDataAckState.needsToSendAckImmediately = false; EXPECT_FALSE(hasAckDataToWrite(*conn)); + conn->ackStates.appDataAckState.needsToSendAckImmediately = true; + EXPECT_TRUE(hasAckDataToWrite(*conn)); } TEST_F(QuicTransportFunctionsTest, HasAckDataToWriteNoAcksScheduled) { diff --git a/quic/api/test/QuicTransportTest.cpp b/quic/api/test/QuicTransportTest.cpp index b2685bd6e..e6f10281a 100644 --- a/quic/api/test/QuicTransportTest.cpp +++ b/quic/api/test/QuicTransportTest.cpp @@ -800,26 +800,6 @@ TEST_F(QuicTransportTest, WriteImmediateAcks) { EXPECT_EQ(WriteDataReason::NO_WRITE, shouldWriteData(conn)); } -TEST_F(QuicTransportTest, NotWriteAcksIfNoData) { - auto& conn = transport_->getConnectionState(); - - addAckStatesWithCurrentTimestamps(conn.ackStates.appDataAckState, 0, 100); - conn.ackStates.appDataAckState.needsToSendAckImmediately = false; - conn.ackStates.appDataAckState.numNonRxPacketsRecvd = 3; - // Should not write ack blocks if there is only ack to write - EXPECT_EQ( - 0, - writeQuicDataToSocket( - *socket_, - conn, - *conn.clientConnectionId, - *conn.serverConnectionId, - *aead_, - *headerCipher_, - transport_->getVersion(), - conn.transportSettings.writeConnectionDataPacketsLimit)); -} - TEST_F(QuicTransportTest, WritePendingAckIfHavingData) { auto& conn = transport_->getConnectionState(); auto streamId = transport_->createBidirectionalStream().value();