From 71e0934e13c2c8ec1b8599c3094d1fbd8e2c6db0 Mon Sep 17 00:00:00 2001 From: Joseph Beshay Date: Wed, 25 Oct 2023 09:45:07 -0700 Subject: [PATCH] Allow including a cwnd_hint in the 0-rtt app token Summary: This enables the server to include a cwnd hint in the 0-rtt ticket it sends to the client. Reviewed By: mjoras Differential Revision: D43131826 fbshipit-source-id: 742e4e531027ec6618a1b761c450b507368e5a2f --- quic/QuicConstants.h | 4 ++ quic/handshake/TransportParameters.h | 3 +- quic/server/QuicServerTransport.cpp | 44 +++++++++++++++++-- quic/server/QuicServerTransport.h | 4 +- quic/server/handshake/AppToken.cpp | 7 ++- quic/server/handshake/AppToken.h | 3 +- .../handshake/DefaultAppTokenValidator.cpp | 7 ++- quic/server/handshake/test/AppTokenTest.cpp | 28 ++++++++++++ quic/server/state/ServerStateMachine.cpp | 10 ++++- quic/server/state/ServerStateMachine.h | 5 ++- quic/state/TransportSettings.h | 2 + 11 files changed, 107 insertions(+), 10 deletions(-) diff --git a/quic/QuicConstants.h b/quic/QuicConstants.h index 78ae7bf7d..0cd603fe2 100644 --- a/quic/QuicConstants.h +++ b/quic/QuicConstants.h @@ -614,6 +614,10 @@ constexpr uint16_t kMaxDatagramPacketOverhead = 25 + 16; // The Maximum number of datagrams (in/out) to buffer constexpr uint32_t kDefaultMaxDatagramsBuffered = 75; +// Minimum interval between new session tickets sent by the server in +// milliseconds +constexpr std::chrono::milliseconds kMinIntervalBetweenSessionTickets = 100ms; + enum class ZeroRttSourceTokenMatchingPolicy : uint8_t { REJECT_IF_NO_EXACT_MATCH = 0, LIMIT_IF_NO_EXACT_MATCH = 1, diff --git a/quic/handshake/TransportParameters.h b/quic/handshake/TransportParameters.h index 28c808de9..96b273275 100644 --- a/quic/handshake/TransportParameters.h +++ b/quic/handshake/TransportParameters.h @@ -37,7 +37,8 @@ enum class TransportParameterId : uint64_t { max_receive_timestamps_per_ack = 0xff0a002, receive_timestamps_exponent = 0xff0a003, stream_groups_enabled = 0x0000ff99, - knob_frames_supported = 0x00005178 + knob_frames_supported = 0x00005178, + cwnd_hint_bytes = 0x00007492 }; struct TransportParameter { diff --git a/quic/server/QuicServerTransport.cpp b/quic/server/QuicServerTransport.cpp index 9e5c21ffe..0a3899685 100644 --- a/quic/server/QuicServerTransport.cpp +++ b/quic/server/QuicServerTransport.cpp @@ -534,13 +534,50 @@ void QuicServerTransport::processPendingData(bool async) { } } +bool QuicServerTransport::shouldWriteNewSessionTicket() { + if (!newSessionTicketWrittenTimestamp_) { + // No session ticket has been written yet, we should write one. + return true; + } + // Conditions for writing more session tickets after the first one: + // 1. includeCwndHintsInSessionTicket transport setting is set + // 2. The current congestion window is either smaller than or more than twice + // the last one we sent in a session ticket + // 3. We haven't sent any session ticket in the last + // kMinIntervalBetweenSessionTickets + + if (serverConn_->congestionController && + conn_->transportSettings.includeCwndHintsInSessionTicket && + Clock::now() - newSessionTicketWrittenTimestamp_.value() > + kMinIntervalBetweenSessionTickets) { + bool cwndChangedSinceLastHint = + !newSessionTicketWrittenCwndHint_.has_value() || + conn_->congestionController->getCongestionWindow() / 2 > + *newSessionTicketWrittenCwndHint_ || + conn_->congestionController->getCongestionWindow() < + *newSessionTicketWrittenCwndHint_; + if (cwndChangedSinceLastHint) { + return true; + } + } + return false; +} + void QuicServerTransport::maybeWriteNewSessionTicket() { - if (!newSessionTicketWritten_ && !ctx_->getSendNewSessionTicket() && + if (shouldWriteNewSessionTicket() && !ctx_->getSendNewSessionTicket() && serverConn_->serverHandshakeLayer->isHandshakeDone()) { + VLOG(7) << "Writing a new session ticket with cwnd=" + << conn_->congestionController->getCongestionWindow(); if (conn_->qLogger) { conn_->qLogger->addTransportStateUpdate(kWriteNst); } - newSessionTicketWritten_ = true; + newSessionTicketWrittenTimestamp_ = Clock::now(); + folly::Optional cwndHint = folly::none; + if (conn_->transportSettings.includeCwndHintsInSessionTicket && + conn_->congestionController) { + cwndHint = conn_->congestionController->getCongestionWindow(); + newSessionTicketWrittenCwndHint_ = cwndHint; + } AppToken appToken; appToken.transportParams = createTicketTransportParameters( conn_->transportSettings.idleTimeout.count(), @@ -552,7 +589,8 @@ void QuicServerTransport::maybeWriteNewSessionTicket() { .advertisedInitialBidiRemoteStreamFlowControlWindow, conn_->transportSettings.advertisedInitialUniStreamFlowControlWindow, conn_->transportSettings.advertisedInitialMaxStreamsBidi, - conn_->transportSettings.advertisedInitialMaxStreamsUni); + conn_->transportSettings.advertisedInitialMaxStreamsUni, + cwndHint); appToken.sourceAddresses = serverConn_->tokenSourceAddresses; appToken.version = conn_->version.value(); // If a client connects to server for the first time and doesn't attempt diff --git a/quic/server/QuicServerTransport.h b/quic/server/QuicServerTransport.h index be632a459..ac03a5f90 100644 --- a/quic/server/QuicServerTransport.h +++ b/quic/server/QuicServerTransport.h @@ -190,6 +190,7 @@ class QuicServerTransport void maybeNotifyHandshakeFinished(); bool hasReadCipher() const; void registerAllTransportKnobParamHandlers(); + bool shouldWriteNewSessionTicket(); private: RoutingCallback* routingCb_{nullptr}; @@ -197,7 +198,8 @@ class QuicServerTransport std::shared_ptr ctx_; bool notifiedRouting_{false}; bool notifiedConnIdBound_{false}; - bool newSessionTicketWritten_{false}; + folly::Optional newSessionTicketWrittenTimestamp_; + folly::Optional newSessionTicketWrittenCwndHint_; QuicServerConnectionState* serverConn_; std::unordered_map< uint64_t, diff --git a/quic/server/handshake/AppToken.cpp b/quic/server/handshake/AppToken.cpp index d59567517..7d810250b 100644 --- a/quic/server/handshake/AppToken.cpp +++ b/quic/server/handshake/AppToken.cpp @@ -17,7 +17,8 @@ TicketTransportParameters createTicketTransportParameters( uint64_t initialMaxStreamDataBidiRemote, uint64_t initialMaxStreamDataUni, uint64_t initialMaxStreamsBidi, - uint64_t initialMaxStreamsUni) { + uint64_t initialMaxStreamsUni, + folly::Optional cwndHintBytes) { TicketTransportParameters params; params.parameters.push_back( encodeIntegerParameter(TransportParameterId::idle_timeout, idleTimeout)); @@ -38,6 +39,10 @@ TicketTransportParameters createTicketTransportParameters( TransportParameterId::initial_max_streams_bidi, initialMaxStreamsBidi)); params.parameters.push_back(encodeIntegerParameter( TransportParameterId::initial_max_streams_uni, initialMaxStreamsUni)); + if (cwndHintBytes) { + params.parameters.push_back((encodeIntegerParameter( + TransportParameterId::cwnd_hint_bytes, *cwndHintBytes))); + } return params; } diff --git a/quic/server/handshake/AppToken.h b/quic/server/handshake/AppToken.h index 0ccc49c3b..ba966f794 100644 --- a/quic/server/handshake/AppToken.h +++ b/quic/server/handshake/AppToken.h @@ -35,6 +35,7 @@ TicketTransportParameters createTicketTransportParameters( uint64_t initialMaxStreamDataBidiRemote, uint64_t initialMaxStreamDataUni, uint64_t initialMaxStreamsBidi, - uint64_t initialMaxStreamsUni); + uint64_t initialMaxStreamsUni, + folly::Optional cwndHintBytes = folly::none); } // namespace quic diff --git a/quic/server/handshake/DefaultAppTokenValidator.cpp b/quic/server/handshake/DefaultAppTokenValidator.cpp index 00b22cdc5..ebc6d0aa8 100644 --- a/quic/server/handshake/DefaultAppTokenValidator.cpp +++ b/quic/server/handshake/DefaultAppTokenValidator.cpp @@ -130,6 +130,10 @@ bool DefaultAppTokenValidator::validate( return validated = false; } + // This is an optional parameter in the ticket + auto cwndHintBytes = + getIntegerParameter(TransportParameterId::cwnd_hint_bytes, params); + // TODO max ack delay, is this really necessary? // spec says disable_migration should also be in the ticket. It shouldn't. @@ -159,7 +163,8 @@ bool DefaultAppTokenValidator::validate( *ticketMaxStreamDataBidiRemote, *ticketMaxStreamDataUni, *ticketMaxStreamsBidi, - *ticketMaxStreamsUni); + *ticketMaxStreamsUni, + cwndHintBytes); return validated; } diff --git a/quic/server/handshake/test/AppTokenTest.cpp b/quic/server/handshake/test/AppTokenTest.cpp index ca846b68a..6ddcfa4f1 100644 --- a/quic/server/handshake/test/AppTokenTest.cpp +++ b/quic/server/handshake/test/AppTokenTest.cpp @@ -72,6 +72,14 @@ void expectAppTokenEqual( appToken.transportParams.parameters); EXPECT_EQ(ackDelayExponent, expectedAckDelayExponent); + auto cwndHintBytes = getIntegerParameter( + TransportParameterId::cwnd_hint_bytes, + decodedAppToken->transportParams.parameters); + auto expectedCwndHintBytes = getIntegerParameter( + TransportParameterId::cwnd_hint_bytes, + appToken.transportParams.parameters); + EXPECT_EQ(cwndHintBytes, expectedCwndHintBytes); + EXPECT_EQ( decodedAppToken->sourceAddresses.size(), appToken.sourceAddresses.size()); for (size_t ii = 0; ii < appToken.sourceAddresses.size(); ++ii) { @@ -245,5 +253,25 @@ TEST(AppTokenTest, TestEncodeAndDecodeIPv6AndIPv4AddressesWithAppToken) { expectAppTokenEqual(decodeAppToken(*buf), appToken); } +TEST(AppTokenTest, TestEncodeAndDecodeCwndHint) { + AppToken appToken; + appToken.transportParams = createTicketTransportParameters( + kDefaultIdleTimeout.count(), + kDefaultUDPReadBufferSize, + kDefaultConnectionFlowControlWindow, + kDefaultStreamFlowControlWindow, + kDefaultStreamFlowControlWindow, + kDefaultStreamFlowControlWindow, + std::numeric_limits::max() - 8, + std::numeric_limits::max() - 9, + std::numeric_limits::max() - 10); + appToken.sourceAddresses = { + folly::IPAddress("2401:db00:2111:7283:face::46:2")}; + appToken.version = QuicVersion::MVFST; + Buf buf = encodeAppToken(appToken); + + expectAppTokenEqual(decodeAppToken(*buf), appToken); +} + } // namespace test } // namespace quic diff --git a/quic/server/state/ServerStateMachine.cpp b/quic/server/state/ServerStateMachine.cpp index c7d3a84e1..1718eec54 100644 --- a/quic/server/state/ServerStateMachine.cpp +++ b/quic/server/state/ServerStateMachine.cpp @@ -545,7 +545,8 @@ void updateTransportParamsFromTicket( uint64_t initialMaxStreamDataBidiRemote, uint64_t initialMaxStreamDataUni, uint64_t initialMaxStreamsBidi, - uint64_t initialMaxStreamsUni) { + uint64_t initialMaxStreamsUni, + folly::Optional maybeCwndHintBytes) { conn.transportSettings.idleTimeout = std::chrono::milliseconds(idleTimeout); conn.transportSettings.maxRecvPacketSize = maxRecvPacketSize; @@ -563,6 +564,13 @@ void updateTransportParamsFromTicket( conn.transportSettings.advertisedInitialMaxStreamsBidi = initialMaxStreamsBidi; conn.transportSettings.advertisedInitialMaxStreamsUni = initialMaxStreamsUni; + + conn.maybeCwndHintBytes = maybeCwndHintBytes; + + if (maybeCwndHintBytes) { + VLOG(7) << fmt::format( + "Got a cwnd hint in a 0-rtt ticket. = {}", *maybeCwndHintBytes); + } } void onConnectionMigration( diff --git a/quic/server/state/ServerStateMachine.h b/quic/server/state/ServerStateMachine.h index 701d15575..1338d49a4 100644 --- a/quic/server/state/ServerStateMachine.h +++ b/quic/server/state/ServerStateMachine.h @@ -122,6 +122,8 @@ struct QuicServerConnectionState : public QuicConnectionStateBase { // A false value indicates 0-rtt is rejected. folly::Optional transportParamsMatching; + folly::Optional maybeCwndHintBytes; + // Whether source address token matches client ip. // A false value indicates either 0-rtt is rejected or inflight bytes are // limited until CFIN depending on matching policy. @@ -227,7 +229,8 @@ void updateTransportParamsFromTicket( uint64_t initialMaxStreamDataBidiRemote, uint64_t initialMaxStreamDataUni, uint64_t initialMaxStreamsBidi, - uint64_t initialMaxStreamsUni); + uint64_t initialMaxStreamsUni, + folly::Optional maybeCwndHintBytes); void onConnectionMigration( QuicServerConnectionState& conn, diff --git a/quic/state/TransportSettings.h b/quic/state/TransportSettings.h index 8d77828b4..16cb84a08 100644 --- a/quic/state/TransportSettings.h +++ b/quic/state/TransportSettings.h @@ -296,6 +296,8 @@ struct TransportSettings { bool dropIngressOnStopSending{false}; bool advertisedKnobFrameSupport{true}; bool removeStreamAfterEomCallbackUnset{false}; + // Whether to include cwnd hint in new session tickets for 0-rtt + bool includeCwndHintsInSessionTicket{false}; // The default priority to instantiate streams with. Priority defaultPriority{kDefaultPriority};