From 11db770061119de2cad10f330ac59b87c6d7aa56 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Sat, 20 Jul 2024 10:34:24 -0700 Subject: [PATCH 1/4] add initial RTT configurability --- neqo-bin/src/lib.rs | 5 +++++ neqo-transport/src/connection/params.rs | 15 ++++++++++++++- neqo-transport/src/connection/tests/mod.rs | 4 ++-- neqo-transport/src/rtt.rs | 10 +++++----- 4 files changed, 26 insertions(+), 8 deletions(-) diff --git a/neqo-bin/src/lib.rs b/neqo-bin/src/lib.rs index f151f2642e..d53a5039eb 100644 --- a/neqo-bin/src/lib.rs +++ b/neqo-bin/src/lib.rs @@ -117,6 +117,10 @@ pub struct QuicParameters { /// The idle timeout for connections, in seconds. pub idle_timeout: u64, + #[arg(long = "init_rtt", default_value = "100")] + /// The initial round-trip time. + pub initial_rtt_ms: u64, + #[arg(long = "cc", default_value = "newreno")] /// The congestion controller to use. pub congestion_control: CongestionControlAlgorithm, @@ -222,6 +226,7 @@ impl QuicParameters { .max_streams(StreamType::BiDi, self.max_streams_bidi) .max_streams(StreamType::UniDi, self.max_streams_uni) .idle_timeout(Duration::from_secs(self.idle_timeout)) + .initial_rtt(Duration::from_millis(self.initial_rtt_ms)) .cc_algorithm(self.congestion_control) .pacing(!self.no_pacing) .pmtud(!self.no_pmtud); diff --git a/neqo-transport/src/connection/params.rs b/neqo-transport/src/connection/params.rs index fb176d904e..d73da35e0d 100644 --- a/neqo-transport/src/connection/params.rs +++ b/neqo-transport/src/connection/params.rs @@ -10,7 +10,7 @@ pub use crate::recovery::FAST_PTO_SCALE; use crate::{ connection::{ConnectionIdManager, Role, LOCAL_ACTIVE_CID_LIMIT}, recv_stream::RECV_BUFFER_SIZE, - rtt::GRANULARITY, + rtt::{DEFAULT_INITIAL_RTT, GRANULARITY}, stream_id::StreamType, tparams::{self, PreferredAddress, TransportParameter, TransportParametersHandler}, tracking::DEFAULT_ACK_DELAY, @@ -71,6 +71,7 @@ pub struct ConnectionParameters { /// acknowledgments every round trip, set the value to `5 * ACK_RATIO_SCALE`. /// Values less than `ACK_RATIO_SCALE` are clamped to `ACK_RATIO_SCALE`. ack_ratio: u8, + initial_rtt: Duration, /// The duration of the idle timeout for the connection. idle_timeout: Duration, preferred_address: PreferredAddressConfig, @@ -98,6 +99,7 @@ impl Default for ConnectionParameters { max_streams_uni: LOCAL_STREAM_LIMIT_UNI, ack_ratio: DEFAULT_ACK_RATIO, idle_timeout: DEFAULT_IDLE_TIMEOUT, + initial_rtt: DEFAULT_INITIAL_RTT, preferred_address: PreferredAddressConfig::Default, datagram_size: 0, outgoing_datagram_queue: MAX_QUEUED_DATAGRAMS_DEFAULT, @@ -271,6 +273,17 @@ impl ConnectionParameters { self.datagram_size } + #[must_use] + pub const fn get_initial_rtt(&self) -> Duration { + self.initial_rtt + } + + #[must_use] + pub const fn initial_rtt(mut self, init_rtt: Duration) -> Self { + self.initial_rtt = init_rtt; + self + } + #[must_use] pub const fn datagram_size(mut self, v: u64) -> Self { self.datagram_size = v; diff --git a/neqo-transport/src/connection/tests/mod.rs b/neqo-transport/src/connection/tests/mod.rs index 3528f7d488..25349bad74 100644 --- a/neqo-transport/src/connection/tests/mod.rs +++ b/neqo-transport/src/connection/tests/mod.rs @@ -697,8 +697,8 @@ fn create_client() { assert!(matches!(client.state(), State::Init)); let stats = client.stats(); assert_default_stats(&stats); - assert_eq!(stats.rtt, crate::rtt::INITIAL_RTT); - assert_eq!(stats.rttvar, crate::rtt::INITIAL_RTT / 2); + assert_eq!(stats.rtt, crate::rtt::DEFAULT_INITIAL_RTT); + assert_eq!(stats.rttvar, crate::rtt::DEFAULT_INITIAL_RTT / 2); } #[test] diff --git a/neqo-transport/src/rtt.rs b/neqo-transport/src/rtt.rs index ba623a1a1c..37128ac92e 100644 --- a/neqo-transport/src/rtt.rs +++ b/neqo-transport/src/rtt.rs @@ -25,7 +25,7 @@ use crate::{ /// `select()`, or similar) can reliably deliver; see `neqo_common::hrtime`. pub const GRANULARITY: Duration = Duration::from_millis(1); // Defined in -recovery 6.2 as 333ms but using lower value. -pub const INITIAL_RTT: Duration = Duration::from_millis(100); +pub const DEFAULT_INITIAL_RTT: Duration = Duration::from_millis(100); /// The source of the RTT measurement. #[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Clone, Copy)] @@ -219,10 +219,10 @@ impl Default for RttEstimate { fn default() -> Self { Self { first_sample_time: None, - latest_rtt: INITIAL_RTT, - smoothed_rtt: INITIAL_RTT, - rttvar: INITIAL_RTT / 2, - min_rtt: INITIAL_RTT, + latest_rtt: DEFAULT_INITIAL_RTT, + smoothed_rtt: DEFAULT_INITIAL_RTT, + rttvar: DEFAULT_INITIAL_RTT / 2, + min_rtt: DEFAULT_INITIAL_RTT, ack_delay: PeerAckDelay::default(), best_source: RttSource::Guesstimate, } From 8a1fe081b88418247566125865110d891c4491f8 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Thu, 29 Aug 2024 18:40:39 +0200 Subject: [PATCH 2/4] adaptions for CLI arguments --- neqo-bin/src/lib.rs | 14 +++++++++----- neqo-transport/src/connection/params.rs | 4 ++-- neqo-transport/src/connection/tests/mod.rs | 4 ++-- neqo-transport/src/lib.rs | 2 ++ neqo-transport/src/rtt.rs | 10 +++++----- 5 files changed, 20 insertions(+), 14 deletions(-) diff --git a/neqo-bin/src/lib.rs b/neqo-bin/src/lib.rs index d53a5039eb..c7d7ca883e 100644 --- a/neqo-bin/src/lib.rs +++ b/neqo-bin/src/lib.rs @@ -17,7 +17,7 @@ use std::{ use clap::Parser; use neqo_transport::{ tparams::PreferredAddress, CongestionControlAlgorithm, ConnectionParameters, StreamType, - Version, + Version, INITIAL_RTT, }; pub mod client; @@ -117,9 +117,9 @@ pub struct QuicParameters { /// The idle timeout for connections, in seconds. pub idle_timeout: u64, - #[arg(long = "init_rtt", default_value = "100")] - /// The initial round-trip time. - pub initial_rtt_ms: u64, + #[arg(long = "init_rtt")] + /// The initial round-trip time. Defaults to [``INITIAL_RTT``] if not specified. + pub initial_rtt_ms: Option, #[arg(long = "cc", default_value = "newreno")] /// The congestion controller to use. @@ -150,6 +150,7 @@ impl Default for QuicParameters { max_streams_bidi: 16, max_streams_uni: 16, idle_timeout: 30, + initial_rtt_ms: None, congestion_control: CongestionControlAlgorithm::NewReno, no_pacing: false, no_pmtud: false, @@ -226,7 +227,10 @@ impl QuicParameters { .max_streams(StreamType::BiDi, self.max_streams_bidi) .max_streams(StreamType::UniDi, self.max_streams_uni) .idle_timeout(Duration::from_secs(self.idle_timeout)) - .initial_rtt(Duration::from_millis(self.initial_rtt_ms)) + .initial_rtt( + self.initial_rtt_ms + .map_or(INITIAL_RTT, Duration::from_millis), + ) .cc_algorithm(self.congestion_control) .pacing(!self.no_pacing) .pmtud(!self.no_pmtud); diff --git a/neqo-transport/src/connection/params.rs b/neqo-transport/src/connection/params.rs index d73da35e0d..c4bc38d2c3 100644 --- a/neqo-transport/src/connection/params.rs +++ b/neqo-transport/src/connection/params.rs @@ -10,7 +10,7 @@ pub use crate::recovery::FAST_PTO_SCALE; use crate::{ connection::{ConnectionIdManager, Role, LOCAL_ACTIVE_CID_LIMIT}, recv_stream::RECV_BUFFER_SIZE, - rtt::{DEFAULT_INITIAL_RTT, GRANULARITY}, + rtt::{GRANULARITY, INITIAL_RTT}, stream_id::StreamType, tparams::{self, PreferredAddress, TransportParameter, TransportParametersHandler}, tracking::DEFAULT_ACK_DELAY, @@ -99,7 +99,7 @@ impl Default for ConnectionParameters { max_streams_uni: LOCAL_STREAM_LIMIT_UNI, ack_ratio: DEFAULT_ACK_RATIO, idle_timeout: DEFAULT_IDLE_TIMEOUT, - initial_rtt: DEFAULT_INITIAL_RTT, + initial_rtt: INITIAL_RTT, preferred_address: PreferredAddressConfig::Default, datagram_size: 0, outgoing_datagram_queue: MAX_QUEUED_DATAGRAMS_DEFAULT, diff --git a/neqo-transport/src/connection/tests/mod.rs b/neqo-transport/src/connection/tests/mod.rs index 25349bad74..3528f7d488 100644 --- a/neqo-transport/src/connection/tests/mod.rs +++ b/neqo-transport/src/connection/tests/mod.rs @@ -697,8 +697,8 @@ fn create_client() { assert!(matches!(client.state(), State::Init)); let stats = client.stats(); assert_default_stats(&stats); - assert_eq!(stats.rtt, crate::rtt::DEFAULT_INITIAL_RTT); - assert_eq!(stats.rttvar, crate::rtt::DEFAULT_INITIAL_RTT / 2); + assert_eq!(stats.rtt, crate::rtt::INITIAL_RTT); + assert_eq!(stats.rttvar, crate::rtt::INITIAL_RTT / 2); } #[test] diff --git a/neqo-transport/src/lib.rs b/neqo-transport/src/lib.rs index 7b650b7f2e..4e93c143d6 100644 --- a/neqo-transport/src/lib.rs +++ b/neqo-transport/src/lib.rs @@ -50,6 +50,8 @@ pub mod tparams; mod tracking; pub mod version; +pub use rtt::INITIAL_RTT; + pub use self::{ cc::CongestionControlAlgorithm, cid::{ diff --git a/neqo-transport/src/rtt.rs b/neqo-transport/src/rtt.rs index 37128ac92e..ba623a1a1c 100644 --- a/neqo-transport/src/rtt.rs +++ b/neqo-transport/src/rtt.rs @@ -25,7 +25,7 @@ use crate::{ /// `select()`, or similar) can reliably deliver; see `neqo_common::hrtime`. pub const GRANULARITY: Duration = Duration::from_millis(1); // Defined in -recovery 6.2 as 333ms but using lower value. -pub const DEFAULT_INITIAL_RTT: Duration = Duration::from_millis(100); +pub const INITIAL_RTT: Duration = Duration::from_millis(100); /// The source of the RTT measurement. #[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Clone, Copy)] @@ -219,10 +219,10 @@ impl Default for RttEstimate { fn default() -> Self { Self { first_sample_time: None, - latest_rtt: DEFAULT_INITIAL_RTT, - smoothed_rtt: DEFAULT_INITIAL_RTT, - rttvar: DEFAULT_INITIAL_RTT / 2, - min_rtt: DEFAULT_INITIAL_RTT, + latest_rtt: INITIAL_RTT, + smoothed_rtt: INITIAL_RTT, + rttvar: INITIAL_RTT / 2, + min_rtt: INITIAL_RTT, ack_delay: PeerAckDelay::default(), best_source: RttSource::Guesstimate, } From 1bc2aa69968ca844f3d1666f0a682efcd07dbd93 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Mon, 11 Nov 2024 14:26:49 +0100 Subject: [PATCH 3/4] very basic test for initial rtt configurability --- neqo-transport/src/connection/tests/idle.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/neqo-transport/src/connection/tests/idle.rs b/neqo-transport/src/connection/tests/idle.rs index deb4312dca..afe81eb9c5 100644 --- a/neqo-transport/src/connection/tests/idle.rs +++ b/neqo-transport/src/connection/tests/idle.rs @@ -53,6 +53,13 @@ fn test_idle_timeout(client: &mut Connection, server: &mut Connection, timeout: assert!(matches!(client.state(), State::Closed(_))); } +#[test] +fn init_rtt_configuration() { + const CUSTOM_INIT_RTT: Duration = Duration::from_millis(200); + let client = new_client(ConnectionParameters::default().initial_rtt(CUSTOM_INIT_RTT)); + assert_eq!(client.conn_params.get_initial_rtt(), CUSTOM_INIT_RTT); +} + #[test] fn idle_timeout() { let mut client = default_client(); From 7b6fe8c4d813a9e546dba837379731be45efb6a0 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Mon, 11 Nov 2024 16:49:52 +0100 Subject: [PATCH 4/4] Use new initial_rtt field in Connection --- neqo-transport/src/connection/mod.rs | 6 +++--- neqo-transport/src/connection/tests/idle.rs | 7 ++++--- neqo-transport/src/rtt.rs | 23 +++++++++++++-------- 3 files changed, 21 insertions(+), 15 deletions(-) diff --git a/neqo-transport/src/connection/mod.rs b/neqo-transport/src/connection/mod.rs index b6b6b15762..2d6f6dfb6d 100644 --- a/neqo-transport/src/connection/mod.rs +++ b/neqo-transport/src/connection/mod.rs @@ -48,7 +48,7 @@ use crate::{ quic_datagrams::{DatagramTracking, QuicDatagrams}, recovery::{LossRecovery, RecoveryToken, SendProfile, SentPacket}, recv_stream::RecvStreamStats, - rtt::{RttEstimate, GRANULARITY, INITIAL_RTT}, + rtt::{RttEstimate, GRANULARITY}, send_stream::SendStream, stats::{Stats, StatsCell}, stream_id::StreamType, @@ -604,7 +604,7 @@ impl Connection { // When we have no actual RTT sample, do not encode a guestimated RTT larger // than the default initial RTT. (The guess can be very large under lossy // conditions.) - if rtt < INITIAL_RTT { + if rtt < self.conn_params.get_initial_rtt() { rtt } else { Duration::from_millis(0) @@ -638,7 +638,7 @@ impl Connection { /// only use it where a more precise value is not important. fn pto(&self) -> Duration { self.paths.primary().map_or_else( - || RttEstimate::default().pto(self.confirmed()), + || RttEstimate::new(self.conn_params.get_initial_rtt()).pto(self.confirmed()), |p| p.borrow().rtt().pto(self.confirmed()), ) } diff --git a/neqo-transport/src/connection/tests/idle.rs b/neqo-transport/src/connection/tests/idle.rs index afe81eb9c5..b17fe8d92b 100644 --- a/neqo-transport/src/connection/tests/idle.rs +++ b/neqo-transport/src/connection/tests/idle.rs @@ -24,6 +24,7 @@ use crate::{ stream_id::{StreamId, StreamType}, tparams::{self, TransportParameter}, tracking::PacketNumberSpace, + INITIAL_RTT, }; fn default_timeout() -> Duration { @@ -55,9 +56,9 @@ fn test_idle_timeout(client: &mut Connection, server: &mut Connection, timeout: #[test] fn init_rtt_configuration() { - const CUSTOM_INIT_RTT: Duration = Duration::from_millis(200); - let client = new_client(ConnectionParameters::default().initial_rtt(CUSTOM_INIT_RTT)); - assert_eq!(client.conn_params.get_initial_rtt(), CUSTOM_INIT_RTT); + let custom_init_rtt = INITIAL_RTT * 2; + let client = new_client(ConnectionParameters::default().initial_rtt(custom_init_rtt)); + assert_eq!(client.conn_params.get_initial_rtt(), custom_init_rtt); } #[test] diff --git a/neqo-transport/src/rtt.rs b/neqo-transport/src/rtt.rs index ba623a1a1c..451a9ea18a 100644 --- a/neqo-transport/src/rtt.rs +++ b/neqo-transport/src/rtt.rs @@ -51,6 +51,19 @@ pub struct RttEstimate { } impl RttEstimate { + pub fn new(rtt: Duration) -> Self { + // Only allow this when there are no samples. + Self { + first_sample_time: None, + latest_rtt: rtt, + smoothed_rtt: rtt, + rttvar: rtt / 2, + min_rtt: rtt, + ack_delay: PeerAckDelay::default(), + best_source: RttSource::Guesstimate, + } + } + fn init(&mut self, rtt: Duration) { // Only allow this when there are no samples. debug_assert!(self.first_sample_time.is_none()); @@ -217,14 +230,6 @@ impl RttEstimate { impl Default for RttEstimate { fn default() -> Self { - Self { - first_sample_time: None, - latest_rtt: INITIAL_RTT, - smoothed_rtt: INITIAL_RTT, - rttvar: INITIAL_RTT / 2, - min_rtt: INITIAL_RTT, - ack_delay: PeerAckDelay::default(), - best_source: RttSource::Guesstimate, - } + Self::new(INITIAL_RTT) } }