diff --git a/quic/api/QuicTransportFunctions.cpp b/quic/api/QuicTransportFunctions.cpp index 8bfb5f02c..b41866192 100644 --- a/quic/api/QuicTransportFunctions.cpp +++ b/quic/api/QuicTransportFunctions.cpp @@ -534,7 +534,14 @@ uint64_t congestionControlWritableBytes(const QuicConnectionStateBase& conn) { writableBytes, conn.congestionController->getWritableBytes()); } - return writableBytes; + if (writableBytes == std::numeric_limits::max()) { + return writableBytes; + } + + // For real-CC/PathChallenge cases, round the result up to the nearest + // multiple of udpSendPacketLen. + return (writableBytes + conn.udpSendPacketLen - 1) / conn.udpSendPacketLen * + conn.udpSendPacketLen; } uint64_t unlimitedWritableBytes(const QuicConnectionStateBase&) { @@ -1066,11 +1073,8 @@ WriteDataReason shouldWriteData(const QuicConnectionStateBase& conn) { << conn; return WriteDataReason::ACK; } - const size_t minimumDataSize = std::max( - kLongHeaderHeaderSize + kCipherOverheadHeuristic, sizeof(Sample)); - uint64_t availableWriteWindow = congestionControlWritableBytes(conn); - if (availableWriteWindow <= minimumDataSize) { + if (!congestionControlWritableBytes(conn)) { QUIC_STATS(conn.infoCallback, onCwndBlocked); return WriteDataReason::NO_WRITE; } diff --git a/quic/api/test/QuicTransportFunctionsTest.cpp b/quic/api/test/QuicTransportFunctionsTest.cpp index 42521010b..63d060c2e 100644 --- a/quic/api/test/QuicTransportFunctionsTest.cpp +++ b/quic/api/test/QuicTransportFunctionsTest.cpp @@ -971,6 +971,7 @@ TEST_F(QuicTransportFunctionsTest, TestUpdateConnectionConnWindowUpdate) { TEST_F(QuicTransportFunctionsTest, WriteQuicDataToSocketWithCC) { auto conn = createConn(); + conn->udpSendPacketLen = 30; auto mockCongestionController = std::make_unique>(); auto rawCongestionController = mockCongestionController.get(); @@ -1199,7 +1200,7 @@ TEST_F(QuicTransportFunctionsTest, WriteQuicDataToSocketWithNoBytesForHeader) { writeDataToQuicStream(*stream1, buf->clone(), true); EXPECT_CALL(*rawCongestionController, getWritableBytes()) - .WillRepeatedly(Return(10)); + .WillRepeatedly(Return(0)); EXPECT_CALL(*rawCongestionController, onPacketSent(_)).Times(0); writeQuicDataToSocket( *rawSocket, @@ -1719,42 +1720,30 @@ TEST_F(QuicTransportFunctionsTest, ShouldWriteDataTestDuringPathValidation) { auto buf = IOBuf::copyBuffer("0123456789"); writeDataToQuicStream(*stream1, buf->clone(), false); - // shouldWriteData checks this first - const size_t minimumDataSize = std::max( - kLongHeaderHeaderSize + kCipherOverheadHeuristic, sizeof(Sample)); - // Only case that we allow the write; both CC / PathLimiter have writablebytes - EXPECT_CALL(*rawCongestionController, getWritableBytes()) - .WillOnce(Return(minimumDataSize + 1)); - EXPECT_CALL(*rawLimiter, currentCredit(_, _)) - .WillOnce(Return(minimumDataSize + 1)); + EXPECT_CALL(*rawCongestionController, getWritableBytes()).WillOnce(Return(1)); + EXPECT_CALL(*rawLimiter, currentCredit(_, _)).WillOnce(Return(1)); EXPECT_CALL(*transportInfoCb_, onCwndBlocked()).Times(0); EXPECT_NE(WriteDataReason::NO_WRITE, shouldWriteData(*conn)); // CC has writableBytes, but PathLimiter doesn't. - EXPECT_CALL(*rawCongestionController, getWritableBytes()) - .WillOnce(Return(minimumDataSize + 1)); - EXPECT_CALL(*rawLimiter, currentCredit(_, _)) - .WillOnce(Return(minimumDataSize - 2)); + EXPECT_CALL(*rawCongestionController, getWritableBytes()).WillOnce(Return(1)); + EXPECT_CALL(*rawLimiter, currentCredit(_, _)).WillOnce(Return(0)); EXPECT_CALL(*transportInfoCb_, onCwndBlocked()); EXPECT_EQ(WriteDataReason::NO_WRITE, shouldWriteData(*conn)); // PathLimiter has writableBytes, CC doesn't - EXPECT_CALL(*rawCongestionController, getWritableBytes()) - .WillOnce(Return(minimumDataSize - 1)); - EXPECT_CALL(*rawLimiter, currentCredit(_, _)) - .WillOnce(Return(minimumDataSize + 1)); + EXPECT_CALL(*rawCongestionController, getWritableBytes()).WillOnce(Return(0)); + EXPECT_CALL(*rawLimiter, currentCredit(_, _)).WillOnce(Return(1)); EXPECT_CALL(*transportInfoCb_, onCwndBlocked()); EXPECT_EQ(WriteDataReason::NO_WRITE, shouldWriteData(*conn)); // Neither PathLimiter or CC have writablebytes - EXPECT_CALL(*rawCongestionController, getWritableBytes()) - .WillOnce(Return(minimumDataSize - 1)); - EXPECT_CALL(*rawLimiter, currentCredit(_, _)) - .WillOnce(Return(minimumDataSize - 1)); + EXPECT_CALL(*rawCongestionController, getWritableBytes()).WillOnce(Return(0)); + EXPECT_CALL(*rawLimiter, currentCredit(_, _)).WillOnce(Return(0)); EXPECT_CALL(*transportInfoCb_, onCwndBlocked()); EXPECT_EQ(WriteDataReason::NO_WRITE, shouldWriteData(*conn)); @@ -2051,5 +2040,32 @@ TEST_F(QuicTransportFunctionsTest, WriteLimitBytRttFraction) { 500 /* packetLimit */)); } +TEST_F(QuicTransportFunctionsTest, CongestionControlWritableBytesRoundUp) { + auto conn = createConn(); + conn->udpSendPacketLen = 2000; + auto mockCongestionController = + std::make_unique>(); + auto rawCongestionController = mockCongestionController.get(); + conn->congestionController = std::move(mockCongestionController); + + EXPECT_CALL(*rawCongestionController, getWritableBytes()).WillOnce(Return(1)); + EXPECT_EQ(conn->udpSendPacketLen, congestionControlWritableBytes(*conn)); + + EXPECT_CALL(*rawCongestionController, getWritableBytes()) + .WillOnce(Return(1000)); + EXPECT_EQ(conn->udpSendPacketLen, congestionControlWritableBytes(*conn)); + + EXPECT_CALL(*rawCongestionController, getWritableBytes()).WillOnce(Return(0)); + EXPECT_EQ(0, congestionControlWritableBytes(*conn)); + + EXPECT_CALL(*rawCongestionController, getWritableBytes()) + .WillOnce(Return(2000)); + EXPECT_EQ(conn->udpSendPacketLen, congestionControlWritableBytes(*conn)); + + EXPECT_CALL(*rawCongestionController, getWritableBytes()) + .WillOnce(Return(2001)); + EXPECT_EQ(conn->udpSendPacketLen * 2, congestionControlWritableBytes(*conn)); +} + } // namespace test } // namespace quic diff --git a/quic/api/test/QuicTransportTest.cpp b/quic/api/test/QuicTransportTest.cpp index c62f66a86..c7fede89f 100644 --- a/quic/api/test/QuicTransportTest.cpp +++ b/quic/api/test/QuicTransportTest.cpp @@ -2505,6 +2505,7 @@ TEST_F(QuicTransportTest, PacingWillBurstFirst) { TEST_F(QuicTransportTest, AlreadyScheduledPacingNoWrite) { transport_->setPacingTimer(TimerHighRes::newTimer(&evb_, 1ms)); auto& conn = transport_->getConnectionState(); + conn.udpSendPacketLen = 100; auto mockCongestionController = std::make_unique>(); auto rawCongestionController = mockCongestionController.get(); diff --git a/quic/codec/QuicPacketBuilder.h b/quic/codec/QuicPacketBuilder.h index c1b923e53..a305a6943 100644 --- a/quic/codec/QuicPacketBuilder.h +++ b/quic/codec/QuicPacketBuilder.h @@ -34,11 +34,6 @@ constexpr auto kLongHeaderHeaderSize = sizeof(uint8_t) /* Type bytes */ + kReservedPacketLenSize /* minimal size of length */ + kReservedPacketNumSize /* packet number */; -// A possible cipher overhead. The real overhead depends on the AEAD we will -// use. But we need a ball-park value when deciding if we should schedule a -// write. -constexpr auto kCipherOverheadHeuristic = 16; - // TODO: i'm sure this isn't the optimal value: // Appender growth byte size for in PacketBuilder: constexpr size_t kAppenderGrowthSize = 100;