Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add initial RTT configurability #2041

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion neqo-bin/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use std::{
use clap::Parser;
use neqo_transport::{
tparams::PreferredAddress, CongestionControlAlgorithm, ConnectionParameters, StreamType,
Version,
Version, INITIAL_RTT,
};

pub mod client;
Expand Down Expand Up @@ -117,6 +117,10 @@ pub struct QuicParameters {
/// The idle timeout for connections, in seconds.
pub idle_timeout: u64,

#[arg(long = "init_rtt")]
/// The initial round-trip time. Defaults to [``INITIAL_RTT``] if not specified.
pub initial_rtt_ms: Option<u64>,

#[arg(long = "cc", default_value = "newreno")]
/// The congestion controller to use.
pub congestion_control: CongestionControlAlgorithm,
Expand Down Expand Up @@ -146,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,
Expand Down Expand Up @@ -222,6 +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(
self.initial_rtt_ms
.map_or(INITIAL_RTT, Duration::from_millis),
)
.cc_algorithm(self.congestion_control)
.pacing(!self.no_pacing)
.pmtud(!self.no_pmtud);
Expand Down
15 changes: 14 additions & 1 deletion neqo-transport/src/connection/params.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::{GRANULARITY, INITIAL_RTT},
stream_id::StreamType,
tparams::{self, PreferredAddress, TransportParameter, TransportParametersHandler},
tracking::DEFAULT_ACK_DELAY,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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: INITIAL_RTT,
preferred_address: PreferredAddressConfig::Default,
datagram_size: 0,
outgoing_datagram_queue: MAX_QUEUED_DATAGRAMS_DEFAULT,
Expand Down Expand Up @@ -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;
Expand Down
7 changes: 7 additions & 0 deletions neqo-transport/src/connection/tests/idle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest to use a multiple of INITIAL_RTT instead of hardcoding a value. (We may change INITIAL_RTT at some point.)

let client = new_client(ConnectionParameters::default().initial_rtt(CUSTOM_INIT_RTT));
assert_eq!(client.conn_params.get_initial_rtt(), CUSTOM_INIT_RTT);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to test whether the different initial RTT actually causes the desired behavior to happen?

Copy link
Author

@robamu robamu Nov 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, for that I guess I have to get more familiar with the main consumer of the configuration, Connection?

I had a look at how this parameter is used inside the Connection structure and I found some usages where I made adaptions. The changes are inside 7b6fe8c . The next step would be to find a way to verify that a different value actually leads to different behavior (on faulty behaviour?). I might have to get a little bit more familiar with QUIC in general before tackling this.

}

#[test]
fn idle_timeout() {
let mut client = default_client();
Expand Down
2 changes: 2 additions & 0 deletions neqo-transport/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ pub mod tparams;
mod tracking;
pub mod version;

pub use rtt::INITIAL_RTT;

pub use self::{
cc::CongestionControlAlgorithm,
cid::{
Expand Down
Loading