diff --git a/bench/src/lib.rs b/bench/src/lib.rs index 0e5b5030e..8321bb2db 100644 --- a/bench/src/lib.rs +++ b/bench/src/lib.rs @@ -145,7 +145,7 @@ pub fn transport_config(opt: &Opt) -> quinn::TransportConfig { #[cfg(any(windows, os = "linux"))] config.mtu_discovery_config(Some(quinn::MtuDiscoveryConfig::default())); config.max_concurrent_uni_streams(opt.max_streams.try_into().unwrap()); - config.initial_max_udp_payload_size(opt.initial_mtu); + config.initial_mtu(opt.initial_mtu); config } diff --git a/perf/src/bin/perf_client.rs b/perf/src/bin/perf_client.rs index a924d7e61..6519d097a 100644 --- a/perf/src/bin/perf_client.rs +++ b/perf/src/bin/perf_client.rs @@ -65,7 +65,7 @@ struct Opt { keylog: bool, /// UDP payload size that the network must be capable of carrying #[clap(long, default_value = "1200")] - initial_max_udp_payload_size: u16, + initial_mtu: u16, /// Disable packet encryption/decryption (for debugging purpose) #[clap(long = "no-protection")] no_protection: bool, @@ -131,7 +131,7 @@ async fn run(opt: Opt) -> Result<()> { let mut transport = quinn::TransportConfig::default(); #[cfg(any(windows, os = "linux"))] transport.mtu_discovery_config(Some(quinn::MtuDiscoveryConfig::default())); - transport.initial_max_udp_payload_size(opt.initial_max_udp_payload_size); + transport.initial_mtu(opt.initial_mtu); let mut cfg = if opt.no_protection { quinn::ClientConfig::new(Arc::new(NoProtectionClientConfig::new(Arc::new(crypto)))) diff --git a/perf/src/bin/perf_server.rs b/perf/src/bin/perf_server.rs index 04bb22244..204487a48 100644 --- a/perf/src/bin/perf_server.rs +++ b/perf/src/bin/perf_server.rs @@ -34,7 +34,7 @@ struct Opt { keylog: bool, /// UDP payload size that the network must be capable of carrying #[clap(long, default_value = "1200")] - initial_max_udp_payload_size: u16, + initial_mtu: u16, /// Disable packet encryption/decryption (for debugging purpose) #[clap(long = "no-protection")] no_protection: bool, @@ -90,7 +90,7 @@ async fn run(opt: Opt) -> Result<()> { let mut transport = quinn::TransportConfig::default(); #[cfg(any(windows, os = "linux"))] transport.mtu_discovery_config(Some(quinn::MtuDiscoveryConfig::default())); - transport.initial_max_udp_payload_size(opt.initial_max_udp_payload_size); + transport.initial_mtu(opt.initial_mtu); let mut server_config = if opt.no_protection { quinn::ServerConfig::with_crypto(Arc::new(NoProtectionServerConfig::new(Arc::new(crypto)))) diff --git a/quinn-proto/src/config.rs b/quinn-proto/src/config.rs index 26037d20d..9d9c42310 100644 --- a/quinn-proto/src/config.rs +++ b/quinn-proto/src/config.rs @@ -1,4 +1,4 @@ -use std::{convert::TryInto, fmt, num::TryFromIntError, sync::Arc, time::Duration}; +use std::{fmt, num::TryFromIntError, sync::Arc, time::Duration}; use thiserror::Error; @@ -9,8 +9,7 @@ use crate::{ cid_generator::{ConnectionIdGenerator, RandomConnectionIdGenerator}, congestion, crypto::{self, HandshakeTokenKey, HmacKey}, - VarInt, VarIntBoundsExceeded, DEFAULT_SUPPORTED_VERSIONS, INITIAL_MAX_UDP_PAYLOAD_SIZE, - MAX_UDP_PAYLOAD, + VarInt, VarIntBoundsExceeded, DEFAULT_SUPPORTED_VERSIONS, INITIAL_MTU, MAX_UDP_PAYLOAD, }; /// Parameters governing the core QUIC state machine @@ -37,7 +36,8 @@ pub struct TransportConfig { pub(crate) packet_threshold: u32, pub(crate) time_threshold: f32, pub(crate) initial_rtt: Duration, - pub(crate) initial_max_udp_payload_size: u16, + pub(crate) initial_mtu: u16, + pub(crate) min_guaranteed_mtu: u16, pub(crate) mtu_discovery_config: Option, pub(crate) persistent_congestion_threshold: u32, @@ -160,16 +160,34 @@ impl TransportConfig { /// (see [`TransportConfig::mtu_discovery_config`]). /// /// Must be at least 1200, which is the default, and known to be safe for typical internet - /// applications. Larger values are more efficient, but increase the risk of unpredictable - /// catastrophic packet loss due to exceeding the network path's IP MTU. If the provided value - /// is higher than what the network path actually supports, packet loss will eventually trigger - /// black hole detection and bring it down to 1200. + /// applications. Larger values are more efficient, but increase the risk of packet loss due to + /// exceeding the network path's IP MTU. If the provided value is higher than what the network + /// path actually supports, packet loss will eventually trigger black hole detection and bring + /// it down to [`TransportConfig::min_guaranteed_mtu`]. + pub fn initial_mtu(&mut self, value: u16) -> &mut Self { + self.initial_mtu = value.max(INITIAL_MTU); + self + } + + pub(crate) fn get_initial_mtu(&self) -> u16 { + self.initial_mtu.max(self.min_guaranteed_mtu) + } + + /// The maximum UDP payload size guaranteed to be supported by the network. + /// + /// Must be at least 1200, which is the default, and lower than or equal to + /// [`TransportConfig::initial_mtu`]. /// /// Real-world MTUs can vary according to ISP, VPN, and properties of intermediate network links - /// outside of either endpoint's control. Caution should be used when raising this value for - /// connections outside of private networks where these factors are fully controlled. - pub fn initial_max_udp_payload_size(&mut self, value: u16) -> &mut Self { - self.initial_max_udp_payload_size = value.max(INITIAL_MAX_UDP_PAYLOAD_SIZE); + /// outside of either endpoint's control. Extreme care should be used when raising this value + /// outside of private networks where these factors are fully controlled. If the provided value + /// is higher than what the network path actually supports, the result will be unpredictable and + /// catastrophic packet loss, without a possibility of repair. Prefer + /// [`TransportConfig::initial_mtu`] together with + /// [`TransportConfig::mtu_discovery_config`] to set a maximum UDP payload size that robustly + /// adapts to the network. + pub fn min_guaranteed_mtu(&mut self, value: u16) -> &mut Self { + self.min_guaranteed_mtu = value.max(INITIAL_MTU); self } @@ -295,7 +313,8 @@ impl Default for TransportConfig { packet_threshold: 3, time_threshold: 9.0 / 8.0, initial_rtt: Duration::from_millis(333), // per spec, intentionally distinct from EXPECTED_RTT - initial_max_udp_payload_size: INITIAL_MAX_UDP_PAYLOAD_SIZE, + initial_mtu: INITIAL_MTU, + min_guaranteed_mtu: INITIAL_MTU, mtu_discovery_config: None, persistent_congestion_threshold: 3, @@ -385,7 +404,7 @@ impl fmt::Debug for TransportConfig { /// /// Since the search space for MTUs is quite big (the smallest possible MTU is 1200, and the highest /// is 65527), Quinn performs a binary search to keep the number of probes as low as possible. The -/// lower bound of the search is equal to [`TransportConfig::initial_max_udp_payload_size`] in the +/// lower bound of the search is equal to [`TransportConfig::initial_mtu`] in the /// initial MTU discovery run, and is equal to the currently discovered MTU in subsequent runs. The /// upper bound is determined by the minimum of [`MtuDiscoveryConfig::upper_bound`] and the /// `max_udp_payload_size` transport parameter received from the peer during the handshake. @@ -476,7 +495,7 @@ impl EndpointConfig { || Box::::default(); Self { reset_key, - max_udp_payload_size: 1480u32.into(), // Typical internet MTU minus IPv4 and UDP overhead, rounded up to a multiple of 8 + max_udp_payload_size: MAX_UDP_PAYLOAD.into(), // See RFC 9000 (https://www.rfc-editor.org/rfc/rfc9000.html#section-18.2-4.10.1) connection_id_generator_factory: Arc::new(cid_factory), supported_versions: DEFAULT_SUPPORTED_VERSIONS.to_vec(), grease_quic_bit: true, @@ -508,12 +527,17 @@ impl EndpointConfig { self } - /// Maximum UDP payload size accepted from peers. Excludes UDP and IP overhead. + /// Maximum UDP payload size accepted from peers (excluding UDP and IP overhead). + /// + /// Must be greater or equal than 1200. /// - /// The default is suitable for typical internet applications. Applications which expect to run - /// on networks supporting Ethernet jumbo frames or similar should set this appropriately. - pub fn max_udp_payload_size(&mut self, value: u64) -> Result<&mut Self, ConfigError> { - self.max_udp_payload_size = value.try_into()?; + /// Defaults to 65527, which is the maximum permitted UDP payload. + pub fn max_udp_payload_size(&mut self, value: u16) -> Result<&mut Self, ConfigError> { + if !(1200..=65_527).contains(&value) { + return Err(ConfigError::OutOfBounds); + } + + self.max_udp_payload_size = value.into(); Ok(self) } diff --git a/quinn-proto/src/congestion.rs b/quinn-proto/src/congestion.rs index 39a004975..41d24566d 100644 --- a/quinn-proto/src/congestion.rs +++ b/quinn-proto/src/congestion.rs @@ -58,6 +58,9 @@ pub trait Controller: Send { lost_bytes: u64, ); + /// The known MTU for the current network path has been updated + fn on_mtu_update(&mut self, new_mtu: u16); + /// Number of ack-eliciting bytes that may be in flight fn window(&self) -> u64; @@ -74,5 +77,7 @@ pub trait Controller: Send { /// Constructs controllers on demand pub trait ControllerFactory { /// Construct a fresh `Controller` - fn build(&self, now: Instant) -> Box; + fn build(&self, now: Instant, current_mtu: u16) -> Box; } + +const BASE_DATAGRAM_SIZE: u64 = 1200; diff --git a/quinn-proto/src/congestion/bbr/mod.rs b/quinn-proto/src/congestion/bbr/mod.rs index 5009ae2f2..07dde798d 100644 --- a/quinn-proto/src/congestion/bbr/mod.rs +++ b/quinn-proto/src/congestion/bbr/mod.rs @@ -9,7 +9,7 @@ use crate::congestion::bbr::bw_estimation::BandwidthEstimation; use crate::congestion::bbr::min_max::MinMax; use crate::connection::RttEstimator; -use super::{Controller, ControllerFactory}; +use super::{Controller, ControllerFactory, BASE_DATAGRAM_SIZE}; mod bw_estimation; mod min_max; @@ -23,6 +23,7 @@ mod min_max; #[derive(Debug, Clone)] pub struct Bbr { config: Arc, + current_mtu: u64, max_bandwidth: BandwidthEstimation, acked_bytes: u64, mode: Mode, @@ -59,11 +60,11 @@ pub struct Bbr { impl Bbr { /// Construct a state using the given `config` and current time `now` - pub fn new(config: Arc) -> Self { + pub fn new(config: Arc, current_mtu: u16) -> Self { let initial_window = config.initial_window; - let min_window = config.minimum_window; Self { config, + current_mtu: current_mtu as u64, max_bandwidth: BandwidthEstimation::default(), acked_bytes: 0, mode: Mode::Startup, @@ -79,7 +80,7 @@ impl Bbr { last_cycle_start: None, current_cycle_offset: 0, init_cwnd: initial_window, - min_cwnd: min_window, + min_cwnd: calculate_min_window(current_mtu as u64), prev_in_flight_count: 0, exit_probe_rtt_at: None, probe_rtt_last_started_at: None, @@ -238,7 +239,7 @@ impl Bbr { // ProbeRtt. The CWND during ProbeRtt is // kMinimumCongestionWindow, but we allow an extra packet since QUIC // checks CWND before sending a packet. - if bytes_in_flight < self.get_probe_rtt_cwnd() + MAX_DATAGRAM_SIZE { + if bytes_in_flight < self.get_probe_rtt_cwnd() + self.current_mtu { const K_PROBE_RTT_TIME: Duration = Duration::from_millis(200); self.exit_probe_rtt_at = Some(now + K_PROBE_RTT_TIME); } @@ -344,8 +345,8 @@ impl Bbr { if self.recovery_window >= bytes_lost { self.recovery_window -= bytes_lost; } else { - const K_MAX_SEGMENT_SIZE: u64 = MAX_DATAGRAM_SIZE; - self.recovery_window = K_MAX_SEGMENT_SIZE; + // k_max_segment_size = current_mtu + self.recovery_window = self.current_mtu; } // In CONSERVATION mode, just subtracting losses is sufficient. In GROWTH, // release additional |bytes_acked| to achieve a slow-start-like behavior. @@ -468,6 +469,13 @@ impl Controller for Bbr { self.loss_state.lost_bytes += lost_bytes; } + fn on_mtu_update(&mut self, new_mtu: u16) { + self.current_mtu = new_mtu as u64; + self.min_cwnd = calculate_min_window(self.current_mtu); + self.init_cwnd = self.config.initial_window.max(self.min_cwnd); + self.cwnd = self.cwnd.max(self.min_cwnd); + } + fn window(&self) -> u64 { if self.mode == Mode::ProbeRtt { return self.get_probe_rtt_cwnd(); @@ -493,20 +501,10 @@ impl Controller for Bbr { /// Configuration for the [`Bbr`] congestion controller #[derive(Debug, Clone)] pub struct BbrConfig { - max_datagram_size: u64, initial_window: u64, - minimum_window: u64, } impl BbrConfig { - /// The sender’s maximum UDP payload size. Does not include UDP or IP overhead. - /// - /// Used for calculating initial and minimum congestion windows. - pub fn max_datagram_size(&mut self, value: u64) -> &mut Self { - self.max_datagram_size = value; - self - } - /// Default limit on the amount of outstanding data in bytes. /// /// Recommended value: `min(10 * max_datagram_size, max(2 * max_datagram_size, 14720))` @@ -514,29 +512,19 @@ impl BbrConfig { self.initial_window = value; self } - - /// Default minimum congestion window. - /// - /// Recommended value: `2 * max_datagram_size`. - pub fn minimum_window(&mut self, value: u64) -> &mut Self { - self.minimum_window = value; - self - } } impl Default for BbrConfig { fn default() -> Self { Self { - max_datagram_size: MAX_DATAGRAM_SIZE, - initial_window: K_MAX_INITIAL_CONGESTION_WINDOW * MAX_DATAGRAM_SIZE, - minimum_window: 4 * MAX_DATAGRAM_SIZE, + initial_window: K_MAX_INITIAL_CONGESTION_WINDOW * BASE_DATAGRAM_SIZE, } } } impl ControllerFactory for Arc { - fn build(&self, _now: Instant) -> Box { - Box::new(Bbr::new(self.clone())) + fn build(&self, _now: Instant, current_mtu: u16) -> Box { + Box::new(Bbr::new(self.clone(), current_mtu)) } } @@ -628,7 +616,9 @@ impl LossState { } } -const MAX_DATAGRAM_SIZE: u64 = 1232; +fn calculate_min_window(current_mtu: u64) -> u64 { + 4 * current_mtu +} // The gain used for the STARTUP, equal to 2/ln(2). const K_DEFAULT_HIGH_GAIN: f32 = 2.885; diff --git a/quinn-proto/src/congestion/cubic.rs b/quinn-proto/src/congestion/cubic.rs index c694abbf5..7b6d41d6b 100644 --- a/quinn-proto/src/congestion/cubic.rs +++ b/quinn-proto/src/congestion/cubic.rs @@ -2,7 +2,7 @@ use std::any::Any; use std::sync::Arc; use std::time::{Duration, Instant}; -use super::{Controller, ControllerFactory}; +use super::{Controller, ControllerFactory, BASE_DATAGRAM_SIZE}; use crate::connection::RttEstimator; use std::cmp; @@ -69,19 +69,25 @@ pub struct Cubic { /// after this time is acknowledged, QUIC exits recovery. recovery_start_time: Option, cubic_state: State, + current_mtu: u64, } impl Cubic { /// Construct a state using the given `config` and current time `now` - pub fn new(config: Arc, _now: Instant) -> Self { + pub fn new(config: Arc, _now: Instant, current_mtu: u16) -> Self { Self { window: config.initial_window, ssthresh: u64::MAX, recovery_start_time: None, config, cubic_state: Default::default(), + current_mtu: current_mtu as u64, } } + + fn minimum_window(&self) -> u64 { + 2 * self.current_mtu + } } impl Controller for Cubic { @@ -125,14 +131,10 @@ impl Controller for Cubic { let t = now - ca_start_time; // w_cubic(t + rtt) - let w_cubic = self - .cubic_state - .w_cubic(t + rtt.get(), self.config.max_datagram_size); + let w_cubic = self.cubic_state.w_cubic(t + rtt.get(), self.current_mtu); // w_est(t) - let w_est = self - .cubic_state - .w_est(t, rtt.get(), self.config.max_datagram_size); + let w_est = self.cubic_state.w_est(t, rtt.get(), self.current_mtu); let mut cubic_cwnd = self.window; @@ -141,8 +143,8 @@ impl Controller for Cubic { cubic_cwnd = cmp::max(cubic_cwnd, w_est as u64); } else if cubic_cwnd < w_cubic as u64 { // Concave region or convex region use same increment. - let cubic_inc = (w_cubic - cubic_cwnd as f64) / cubic_cwnd as f64 - * self.config.max_datagram_size as f64; + let cubic_inc = + (w_cubic - cubic_cwnd as f64) / cubic_cwnd as f64 * self.current_mtu as f64; cubic_cwnd += cubic_inc as u64; } @@ -153,8 +155,8 @@ impl Controller for Cubic { // cwnd_inc can be more than 1 MSS in the late stage of max probing. // however RFC9002 §7.3.3 (Congestion Avoidance) limits // the increase of cwnd to 1 max_datagram_size per cwnd acknowledged. - if self.cubic_state.cwnd_inc >= self.config.max_datagram_size { - self.window += self.config.max_datagram_size; + if self.cubic_state.cwnd_inc >= self.current_mtu { + self.window += self.current_mtu; self.cubic_state.cwnd_inc = 0; } } @@ -188,10 +190,10 @@ impl Controller for Cubic { self.ssthresh = cmp::max( (self.cubic_state.w_max * BETA_CUBIC) as u64, - self.config.minimum_window, + self.minimum_window(), ); self.window = self.ssthresh; - self.cubic_state.k = self.cubic_state.cubic_k(self.config.max_datagram_size); + self.cubic_state.k = self.cubic_state.cubic_k(self.current_mtu); self.cubic_state.cwnd_inc = (self.cubic_state.cwnd_inc as f64 * BETA_CUBIC) as u64; @@ -202,15 +204,20 @@ impl Controller for Cubic { // 4.7 Timeout - reduce ssthresh based on BETA_CUBIC self.ssthresh = cmp::max( (self.window as f64 * BETA_CUBIC) as u64, - self.config.minimum_window, + self.minimum_window(), ); self.cubic_state.cwnd_inc = 0; - self.window = self.config.minimum_window; + self.window = self.minimum_window(); } } + fn on_mtu_update(&mut self, new_mtu: u16) { + self.current_mtu = new_mtu as u64; + self.window = self.window.max(self.minimum_window()); + } + fn window(&self) -> u64 { self.window } @@ -231,20 +238,10 @@ impl Controller for Cubic { /// Configuration for the `Cubic` congestion controller #[derive(Debug, Clone)] pub struct CubicConfig { - max_datagram_size: u64, initial_window: u64, - minimum_window: u64, } impl CubicConfig { - /// The sender’s maximum UDP payload size. Does not include UDP or IP overhead. - /// - /// Used for calculating initial and minimum congestion windows. - pub fn max_datagram_size(&mut self, value: u64) -> &mut Self { - self.max_datagram_size = value; - self - } - /// Default limit on the amount of outstanding data in bytes. /// /// Recommended value: `min(10 * max_datagram_size, max(2 * max_datagram_size, 14720))` @@ -252,29 +249,18 @@ impl CubicConfig { self.initial_window = value; self } - - /// Default minimum congestion window. - /// - /// Recommended value: `2 * max_datagram_size`. - pub fn minimum_window(&mut self, value: u64) -> &mut Self { - self.minimum_window = value; - self - } } impl Default for CubicConfig { fn default() -> Self { - const MAX_DATAGRAM_SIZE: u64 = 1232; Self { - max_datagram_size: MAX_DATAGRAM_SIZE, - initial_window: 14720.clamp(2 * MAX_DATAGRAM_SIZE, 10 * MAX_DATAGRAM_SIZE), - minimum_window: 2 * MAX_DATAGRAM_SIZE, + initial_window: 14720.clamp(2 * BASE_DATAGRAM_SIZE, 10 * BASE_DATAGRAM_SIZE), } } } impl ControllerFactory for Arc { - fn build(&self, now: Instant) -> Box { - Box::new(Cubic::new(self.clone(), now)) + fn build(&self, now: Instant, current_mtu: u16) -> Box { + Box::new(Cubic::new(self.clone(), now, current_mtu)) } } diff --git a/quinn-proto/src/congestion/new_reno.rs b/quinn-proto/src/congestion/new_reno.rs index 6fd08ba09..0b085c377 100644 --- a/quinn-proto/src/congestion/new_reno.rs +++ b/quinn-proto/src/congestion/new_reno.rs @@ -2,13 +2,14 @@ use std::any::Any; use std::sync::Arc; use std::time::Instant; -use super::{Controller, ControllerFactory}; +use super::{Controller, ControllerFactory, BASE_DATAGRAM_SIZE}; use crate::connection::RttEstimator; /// A simple, standard congestion controller #[derive(Debug, Clone)] pub struct NewReno { config: Arc, + current_mtu: u64, /// Maximum number of bytes in flight that may be sent. window: u64, /// Slow start threshold in bytes. When the congestion window is below ssthresh, the mode is @@ -23,15 +24,20 @@ pub struct NewReno { impl NewReno { /// Construct a state using the given `config` and current time `now` - pub fn new(config: Arc, now: Instant) -> Self { + pub fn new(config: Arc, now: Instant, current_mtu: u16) -> Self { Self { window: config.initial_window, ssthresh: u64::max_value(), recovery_start_time: now, + current_mtu: current_mtu as u64, config, bytes_acked: 0, } } + + fn minimum_window(&self) -> u64 { + 2 * self.current_mtu + } } impl Controller for NewReno { @@ -71,7 +77,7 @@ impl Controller for NewReno { if self.bytes_acked >= self.window { self.bytes_acked -= self.window; - self.window += self.config.max_datagram_size; + self.window += self.current_mtu; } } } @@ -89,14 +95,19 @@ impl Controller for NewReno { self.recovery_start_time = now; self.window = (self.window as f32 * self.config.loss_reduction_factor) as u64; - self.window = self.window.max(self.config.minimum_window); + self.window = self.window.max(self.minimum_window()); self.ssthresh = self.window; if is_persistent_congestion { - self.window = self.config.minimum_window; + self.window = self.minimum_window(); } } + fn on_mtu_update(&mut self, new_mtu: u16) { + self.current_mtu = new_mtu as u64; + self.window = self.window.max(self.minimum_window()); + } + fn window(&self) -> u64 { self.window } @@ -117,21 +128,11 @@ impl Controller for NewReno { /// Configuration for the `NewReno` congestion controller #[derive(Debug, Clone)] pub struct NewRenoConfig { - max_datagram_size: u64, initial_window: u64, - minimum_window: u64, loss_reduction_factor: f32, } impl NewRenoConfig { - /// The sender’s maximum UDP payload size. Does not include UDP or IP overhead. - /// - /// Used for calculating initial and minimum congestion windows. - pub fn max_datagram_size(&mut self, value: u64) -> &mut Self { - self.max_datagram_size = value; - self - } - /// Default limit on the amount of outstanding data in bytes. /// /// Recommended value: `min(10 * max_datagram_size, max(2 * max_datagram_size, 14720))` @@ -140,14 +141,6 @@ impl NewRenoConfig { self } - /// Default minimum congestion window. - /// - /// Recommended value: `2 * max_datagram_size`. - pub fn minimum_window(&mut self, value: u64) -> &mut Self { - self.minimum_window = value; - self - } - /// Reduction in congestion window when a new loss event is detected. pub fn loss_reduction_factor(&mut self, value: f32) -> &mut Self { self.loss_reduction_factor = value; @@ -157,18 +150,15 @@ impl NewRenoConfig { impl Default for NewRenoConfig { fn default() -> Self { - const MAX_DATAGRAM_SIZE: u64 = 1232; Self { - max_datagram_size: MAX_DATAGRAM_SIZE, - initial_window: 14720.clamp(2 * MAX_DATAGRAM_SIZE, 10 * MAX_DATAGRAM_SIZE), - minimum_window: 2 * MAX_DATAGRAM_SIZE, + initial_window: 14720.clamp(2 * BASE_DATAGRAM_SIZE, 10 * BASE_DATAGRAM_SIZE), loss_reduction_factor: 0.5, } } } impl ControllerFactory for Arc { - fn build(&self, now: Instant) -> Box { - Box::new(NewReno::new(self.clone(), now)) + fn build(&self, now: Instant, current_mtu: u16) -> Box { + Box::new(NewReno::new(self.clone(), now, current_mtu)) } } diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index 40e00f278..3069af8db 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -263,8 +263,11 @@ impl Connection { path: PathData::new( remote, config.initial_rtt, - config.congestion_controller_factory.build(now), - config.initial_max_udp_payload_size, + config + .congestion_controller_factory + .build(now, config.get_initial_mtu()), + config.get_initial_mtu(), + config.min_guaranteed_mtu, None, config.mtu_discovery_config.clone(), now, @@ -1220,7 +1223,13 @@ impl Connection { ack_eliciting_acked |= info.ack_eliciting; // Notify MTU discovery that a packet was acked, because it might be an MTU probe - self.path.mtud.on_acked(space, packet, info.size); + let mtu_updated = self.path.mtud.on_acked(space, packet, info.size); + if mtu_updated { + self.path + .congestion + .on_mtu_update(self.path.mtud.current_mtu()); + } + self.on_packet_acked(now, space, info); } } @@ -1470,6 +1479,7 @@ impl Connection { lost_packets, size_of_lost_packets ); + for packet in &lost_packets { let info = self.spaces[pn_space].sent_packets.remove(packet).unwrap(); // safe: lost_packets is populated just above self.remove_in_flight(pn_space, &info); @@ -1477,8 +1487,13 @@ impl Connection { self.streams.retransmit(frame); } self.spaces[pn_space].pending |= info.retransmits; - self.path.mtud.on_non_probe_lost(now, *packet, info.size); + self.path.mtud.on_non_probe_lost(*packet, info.size); } + + if self.path.mtud.black_hole_detected(now) { + self.stats.path.black_holes_detected += 1; + } + // Don't apply congestion penalty for lost ack-only packets let lost_ack_eliciting = old_bytes_in_flight != self.in_flight.bytes; @@ -2722,8 +2737,11 @@ impl Connection { PathData::new( remote, self.config.initial_rtt, - self.config.congestion_controller_factory.build(now), - self.config.initial_max_udp_payload_size, + self.config + .congestion_controller_factory + .build(now, self.config.get_initial_mtu()), + self.config.get_initial_mtu(), + self.config.min_guaranteed_mtu, Some(peer_max_udp_payload_size), self.config.mtu_discovery_config.clone(), now, diff --git a/quinn-proto/src/connection/mtud.rs b/quinn-proto/src/connection/mtud.rs index 284f4d377..1bad832e3 100644 --- a/quinn-proto/src/connection/mtud.rs +++ b/quinn-proto/src/connection/mtud.rs @@ -11,25 +11,27 @@ pub(crate) struct MtuDiscovery { current_mtu: u16, /// The state of the MTU discovery, if enabled state: Option, - /// A count of the number of packets with a size > BASE_UDP_PAYLOAD_SIZE lost since - /// the last time a packet with size equal to the current MTU was acknowledged - black_hole_counter: u8, - /// The largest acked packet of size `current_mtu` - largest_acked_mtu_sized_packet: Option, + /// The state of the black hole detector + black_hole_detector: BlackHoleDetector, } impl MtuDiscovery { pub(crate) fn new( initial_plpmtu: u16, + min_guaranteed_mtu: u16, peer_max_udp_payload_size: Option, config: MtuDiscoveryConfig, ) -> Self { debug_assert!( - initial_plpmtu >= BASE_UDP_PAYLOAD_SIZE, - "initial_max_udp_payload_size must be at least {BASE_UDP_PAYLOAD_SIZE}" + initial_plpmtu >= min_guaranteed_mtu, + "initial_max_udp_payload_size must be at least {min_guaranteed_mtu}" ); - let mut mtud = Self::with_state(initial_plpmtu, Some(EnabledMtuDiscovery::new(config))); + let mut mtud = Self::with_state( + initial_plpmtu, + min_guaranteed_mtu, + Some(EnabledMtuDiscovery::new(config)), + ); // We might be migrating an existing connection to a new path, in which case the transport // parameters have already been transmitted, and we already know the value of @@ -42,16 +44,19 @@ impl MtuDiscovery { } /// MTU discovery will be disabled and the current MTU will be fixed to the provided value - pub(crate) fn disabled(plpmtu: u16) -> Self { - Self::with_state(plpmtu, None) + pub(crate) fn disabled(plpmtu: u16, min_guaranteed_mtu: u16) -> Self { + Self::with_state(plpmtu, min_guaranteed_mtu, None) } - fn with_state(current_mtu: u16, state: Option) -> Self { + fn with_state( + current_mtu: u16, + min_guaranteed_mtu: u16, + state: Option, + ) -> Self { Self { current_mtu, state, - black_hole_counter: 0, - largest_acked_mtu_sized_packet: None, + black_hole_detector: BlackHoleDetector::new(min_guaranteed_mtu), } } @@ -81,10 +86,17 @@ impl MtuDiscovery { } /// Notifies the [`MtuDiscovery`] that a packet has been ACKed - pub(crate) fn on_acked(&mut self, space: SpaceId, packet_number: u64, packet_bytes: u16) { + /// + /// Returns true if the packet was an MTU probe + pub(crate) fn on_acked( + &mut self, + space: SpaceId, + packet_number: u64, + packet_bytes: u16, + ) -> bool { // MTU probes are only sent in application data space if space != SpaceId::Data { - return; + return false; } // Update the state of the MTU search @@ -94,21 +106,17 @@ impl MtuDiscovery { .and_then(|state| state.on_probe_acked(packet_number)) { self.current_mtu = new_mtu; - trace!(max_udp_payload_size = self.current_mtu, "new MTU detected"); - - // We know for sure the path supports the current MTU - self.black_hole_counter = 0; - } + trace!(current_mtu = self.current_mtu, "new MTU detected"); - // Reset the black hole counter if a packet the size of the current MTU or larger - // has been acknowledged - if packet_bytes >= self.current_mtu - && self - .largest_acked_mtu_sized_packet - .map_or(true, |pn| packet_number > pn) - { - self.black_hole_counter = 0; - self.largest_acked_mtu_sized_packet = Some(packet_number); + self.black_hole_detector.on_probe_acked(); + true + } else { + self.black_hole_detector.on_non_probe_acked( + self.current_mtu, + packet_number, + packet_bytes, + ); + false } } @@ -131,31 +139,31 @@ impl MtuDiscovery { } /// Notifies the [`MtuDiscovery`] that a non-probe packet was lost - pub(crate) fn on_non_probe_lost(&mut self, now: Instant, packet_number: u64, packet_size: u16) { - // Ignore packets smaller or equal than the base size - if packet_size <= BASE_UDP_PAYLOAD_SIZE { - return; - } + /// + /// When done notifying of lost packets, [`MtuDiscovery::black_hole_detected`] must be called, to + /// ensure the last loss burst is properly processed and to trigger black hole recovery logic if + /// necessary. + pub(crate) fn on_non_probe_lost(&mut self, packet_number: u64, packet_bytes: u16) { + self.black_hole_detector + .on_non_probe_lost(packet_number, packet_bytes); + } - // Ignore lost packets if we have received an ACK for a more recent MTU-sized packet - if self - .largest_acked_mtu_sized_packet - .map_or(false, |pn| packet_number < pn) - { - return; + /// Returns true if a black hole was detected + /// + /// Calling this function will close the previous loss burst. If a black hole is detected, the + /// current MTU will be reset to `min_guaranteed_mtu`. + pub(crate) fn black_hole_detected(&mut self, now: Instant) -> bool { + if !self.black_hole_detector.black_hole_detected() { + return false; } - // The packet counts towards black hole detection - self.black_hole_counter += 1; - if self.black_hole_counter > BLACK_HOLE_THRESHOLD { - self.black_hole_counter = 0; - self.largest_acked_mtu_sized_packet = None; - self.current_mtu = BASE_UDP_PAYLOAD_SIZE; + self.current_mtu = self.black_hole_detector.min_guaranteed_mtu; - if let Some(state) = &mut self.state { - state.on_black_hole_detected(now); - } + if let Some(state) = &mut self.state { + state.on_black_hole_detected(now); } + + true } } @@ -212,7 +220,7 @@ impl EnabledMtuDiscovery { // Retransmit lost probes, if any if 0 < state.lost_probe_count && state.lost_probe_count < MAX_PROBE_RETRANSMITS { state.in_flight_probe = Some(next_packet_number); - return Some(state.last_probed_udp_payload_size); + return Some(state.last_probed_mtu); } let last_probe_succeeded = state.lost_probe_count == 0; @@ -225,7 +233,7 @@ impl EnabledMtuDiscovery { if let Some(probe_udp_payload_size) = state.next_mtu_to_probe(last_probe_succeeded) { state.in_flight_probe = Some(next_packet_number); - state.last_probed_udp_payload_size = probe_udp_payload_size; + state.last_probed_mtu = probe_udp_payload_size; return Some(probe_udp_payload_size); } else { let next_mtud_activation = now + self.config.interval; @@ -245,7 +253,7 @@ impl EnabledMtuDiscovery { Phase::Searching(state) if state.in_flight_probe == Some(packet_number) => { state.in_flight_probe = None; state.lost_probe_count = 0; - Some(state.last_probed_udp_payload_size) + Some(state.last_probed_mtu) } _ => None, } @@ -285,7 +293,7 @@ struct SearchState { /// The upper bound for the current binary search upper_bound: u16, /// The UDP payload size we last sent a probe for - last_probed_udp_payload_size: u16, + last_probed_mtu: u16, /// Packet number of an in-flight probe (if any) in_flight_probe: Option, /// Lost probes at the current probe size @@ -312,7 +320,7 @@ impl SearchState { upper_bound, // During initialization, we consider the lower bound to have already been // successfully probed - last_probed_udp_payload_size: lower_bound, + last_probed_mtu: lower_bound, } } @@ -321,23 +329,20 @@ impl SearchState { debug_assert_eq!(self.in_flight_probe, None); if last_probe_succeeded { - self.lower_bound = self.last_probed_udp_payload_size; + self.lower_bound = self.last_probed_mtu; } else { - self.upper_bound = self.last_probed_udp_payload_size - 1; + self.upper_bound = self.last_probed_mtu - 1; } let next_mtu = (self.lower_bound as i32 + self.upper_bound as i32) / 2; // Binary search stopping condition - if ((next_mtu - self.last_probed_udp_payload_size as i32).unsigned_abs() as u16) + if ((next_mtu - self.last_probed_mtu as i32).unsigned_abs() as u16) < BINARY_SEARCH_MINIMUM_CHANGE { // Special case: if the upper bound is far enough, we want to probe it as a last // step (otherwise we will never achieve the upper bound) - if self - .upper_bound - .saturating_sub(self.last_probed_udp_payload_size) - >= BINARY_SEARCH_MINIMUM_CHANGE + if self.upper_bound.saturating_sub(self.last_probed_mtu) >= BINARY_SEARCH_MINIMUM_CHANGE { return Some(self.upper_bound); } @@ -349,10 +354,133 @@ impl SearchState { } } +#[derive(Clone)] +struct BlackHoleDetector { + /// Counts suspicious packet loss bursts since a packet with size equal to the current MTU was + /// acknowledged (or since a black hole was detected) + /// + /// A packet loss burst is a group of contiguous packets that are deemed lost at the same time + /// (see usages of [`MtuDiscovery::on_non_probe_lost`] for details on how loss detection is + /// implemented) + /// + /// A packet loss burst is considered suspicious when it contains only suspicious packets and no + /// MTU-sized packet has been acknowledged since the group's packets were sent + suspicious_loss_bursts: u8, + /// Indicates whether the current loss burst has any non-suspicious packets + /// + /// Non-suspicious packets are non-probe packets of size <= `min_guaranteed_mtu` + loss_burst_has_non_suspicious_packets: bool, + /// The largest suspicious packet that was lost in the current burst + /// + /// Suspicious packets are non-probe packets of size > `min_guaranteed_mtu` + largest_suspicious_packet_lost: Option, + /// The largest non-probe packet that was lost (used to keep track of loss bursts) + largest_non_probe_lost: Option, + /// The largest acked packet of size `current_mtu` + largest_acked_mtu_sized_packet: Option, + /// The UDP payload size guaranteed to be supported by the network + min_guaranteed_mtu: u16, +} + +impl BlackHoleDetector { + fn new(min_guaranteed_mtu: u16) -> Self { + Self { + suspicious_loss_bursts: 0, + largest_non_probe_lost: None, + loss_burst_has_non_suspicious_packets: false, + largest_suspicious_packet_lost: None, + largest_acked_mtu_sized_packet: None, + min_guaranteed_mtu, + } + } + + fn on_probe_acked(&mut self) { + // We know for sure the path supports the current MTU + self.suspicious_loss_bursts = 0; + } + + fn on_non_probe_acked(&mut self, current_mtu: u16, packet_number: u64, packet_bytes: u16) { + // Reset the black hole counter if a packet the size of the current MTU or larger + // has been acknowledged + if packet_bytes >= current_mtu + && self + .largest_acked_mtu_sized_packet + .map_or(true, |pn| packet_number > pn) + { + self.suspicious_loss_bursts = 0; + self.largest_acked_mtu_sized_packet = Some(packet_number); + } + } + + fn on_non_probe_lost(&mut self, packet_number: u64, packet_bytes: u16) { + // A loss burst is a group of consecutive packets that are declared lost, so a distance + // greater than 1 indicates a new burst + let new_loss_burst = self + .largest_non_probe_lost + .map_or(true, |prev| packet_number - prev != 1); + + if new_loss_burst { + self.finish_loss_burst(); + } + + if packet_bytes <= self.min_guaranteed_mtu { + self.loss_burst_has_non_suspicious_packets = true; + } else { + self.largest_suspicious_packet_lost = Some(packet_number); + } + + self.largest_non_probe_lost = Some(packet_number); + } + + fn black_hole_detected(&mut self) -> bool { + self.finish_loss_burst(); + + if self.suspicious_loss_bursts <= BLACK_HOLE_THRESHOLD { + return false; + } + + self.suspicious_loss_bursts = 0; + self.largest_acked_mtu_sized_packet = None; + + true + } + + /// Marks the end of the current loss burst, checking whether it was suspicious + fn finish_loss_burst(&mut self) { + if self.last_burst_was_suspicious() { + self.suspicious_loss_bursts = self.suspicious_loss_bursts.saturating_add(1); + } + + self.loss_burst_has_non_suspicious_packets = false; + self.largest_suspicious_packet_lost = None; + self.largest_non_probe_lost = None; + } + + /// Returns true if the burst was suspicious and should count towards black hole detection + fn last_burst_was_suspicious(&self) -> bool { + // Ignore burst if it contains any non-suspicious packets, because in that case packet loss + // was likely caused by congestion (instead of a sudden decrease in the path's MTU) + if self.loss_burst_has_non_suspicious_packets { + return false; + } + + // Ignore burst if we have received an ACK for a more recent MTU-sized packet, because that + // proves the network still supports the current MTU + let largest_acked = self.largest_acked_mtu_sized_packet.unwrap_or(0); + if self + .largest_suspicious_packet_lost + .map_or(true, |largest_lost| largest_lost < largest_acked) + { + return false; + } + + true + } +} + // Corresponds to the RFC's `MAX_PROBES` constant (see // https://www.rfc-editor.org/rfc/rfc8899#section-5.1.2) const MAX_PROBE_RETRANSMITS: usize = 3; -const BASE_UDP_PAYLOAD_SIZE: u16 = 1_200; const BLACK_HOLE_THRESHOLD: u8 = 3; const BINARY_SEARCH_MINIMUM_CHANGE: u16 = 20; @@ -366,7 +494,7 @@ mod tests { fn default_mtud() -> MtuDiscovery { let config = MtuDiscoveryConfig::default(); - MtuDiscovery::new(1_200, None, config) + MtuDiscovery::new(1_200, 1_200, None, config) } fn completed(mtud: &MtuDiscovery) -> bool { @@ -401,35 +529,96 @@ mod tests { probed_sizes } + #[test] + fn black_hole_detector_ignores_burst_containing_non_suspicious_packet() { + let mut mtud = default_mtud(); + mtud.on_non_probe_lost(2, 1300); + mtud.on_non_probe_lost(3, 1300); + assert_eq!( + mtud.black_hole_detector.largest_suspicious_packet_lost, + Some(3) + ); + assert_eq!(mtud.black_hole_detector.suspicious_loss_bursts, 0); + + mtud.on_non_probe_lost(4, 800); + assert!(!mtud.black_hole_detected(Instant::now())); + assert_eq!( + mtud.black_hole_detector.largest_suspicious_packet_lost, + None + ); + assert_eq!(mtud.black_hole_detector.suspicious_loss_bursts, 0); + } + + #[test] + fn black_hole_detector_counts_burst_containing_only_suspicious_packets() { + let mut mtud = default_mtud(); + mtud.on_non_probe_lost(2, 1300); + mtud.on_non_probe_lost(3, 1300); + assert_eq!( + mtud.black_hole_detector.largest_suspicious_packet_lost, + Some(3) + ); + assert_eq!(mtud.black_hole_detector.suspicious_loss_bursts, 0); + + assert!(!mtud.black_hole_detected(Instant::now())); + assert_eq!( + mtud.black_hole_detector.largest_suspicious_packet_lost, + None + ); + assert_eq!(mtud.black_hole_detector.suspicious_loss_bursts, 1); + } + + #[test] + fn black_hole_detector_ignores_empty_burst() { + let mut mtud = default_mtud(); + assert!(!mtud.black_hole_detected(Instant::now())); + assert_eq!(mtud.black_hole_detector.suspicious_loss_bursts, 0); + } + #[test] fn mtu_discovery_disabled_does_nothing() { - let mut mtud = MtuDiscovery::disabled(1_200); + let mut mtud = MtuDiscovery::disabled(1_200, 1_200); let probe_size = mtud.poll_transmit(Instant::now(), 0); assert_eq!(probe_size, None); } #[test] - fn mtu_discovery_disabled_lost_four_packets_triggers_black_hole_detection() { - let mut mtud = MtuDiscovery::disabled(1_400); + fn mtu_discovery_disabled_lost_four_packet_bursts_triggers_black_hole_detection() { + let mut mtud = MtuDiscovery::disabled(1_400, 1_250); let now = Instant::now(); - for _ in 0..4 { - mtud.on_non_probe_lost(now, 0, 1300); + for i in 0..4 { + // The packets are never contiguous, so each one has its own burst + mtud.on_non_probe_lost(i * 2, 1300); } - assert_eq!(mtud.current_mtu, 1200); + assert!(mtud.black_hole_detected(now)); + assert_eq!(mtud.current_mtu, 1250); assert_matches!(mtud.state, None); } #[test] - fn mtu_discovery_lost_four_packets_triggers_black_hole_detection_and_resets_timer() { + fn mtu_discovery_lost_two_packet_bursts_does_not_trigger_black_hole_detection() { + let mut mtud = default_mtud(); + let now = Instant::now(); + + for i in 0..2 { + mtud.on_non_probe_lost(i, 1300); + assert!(!mtud.black_hole_detected(now)); + } + } + + #[test] + fn mtu_discovery_lost_four_packet_bursts_triggers_black_hole_detection_and_resets_timer() { let mut mtud = default_mtud(); let now = Instant::now(); - for _ in 0..4 { - mtud.on_non_probe_lost(now, 0, 1300); + for i in 0..4 { + // The packets are never contiguous, so each one has its own burst + mtud.on_non_probe_lost(i * 2, 1300); } + assert!(mtud.black_hole_detected(now)); assert_eq!(mtud.current_mtu, 1200); if let Phase::Complete(next_mtud_activation) = mtud.state.unwrap().phase { assert_eq!(next_mtud_activation, now + Duration::from_secs(60)); @@ -442,7 +631,7 @@ mod tests { fn mtu_discovery_after_complete_reactivates_when_interval_elapsed() { let mut config = MtuDiscoveryConfig::default(); config.upper_bound(9_000); - let mut mtud = MtuDiscovery::new(1_200, None, config); + let mut mtud = MtuDiscovery::new(1_200, 1_200, None, config); let now = Instant::now(); drive_to_completion(&mut mtud, now, 1_500); @@ -491,7 +680,7 @@ mod tests { assert!(fourth_probe_size < first_probe_size); assert_eq!( fourth_probe_size, - first_probe_size - (first_probe_size - BASE_UDP_PAYLOAD_SIZE) / 2 - 1 + first_probe_size - (first_probe_size - 1_200) / 2 - 1 ); } @@ -511,7 +700,7 @@ mod tests { #[test] fn mtu_discovery_with_previous_peer_max_udp_payload_size_clamps_upper_bound() { - let mut mtud = MtuDiscovery::new(1500, Some(1400), MtuDiscoveryConfig::default()); + let mut mtud = MtuDiscovery::new(1500, 1_200, Some(1400), MtuDiscoveryConfig::default()); assert_eq!(mtud.current_mtu, 1400); assert_eq!(mtud.state.as_ref().unwrap().peer_max_udp_payload_size, 1400); @@ -547,7 +736,7 @@ mod tests { fn mtu_discovery_with_1500_limit_and_10000_upper_bound() { let mut config = MtuDiscoveryConfig::default(); config.upper_bound(10_000); - let mut mtud = MtuDiscovery::new(1_200, None, config); + let mut mtud = MtuDiscovery::new(1_200, 1_200, None, config); let probed_sizes = drive_to_completion(&mut mtud, Instant::now(), 1500); @@ -564,7 +753,7 @@ mod tests { fn mtu_discovery_no_lost_probes_finds_maximum_udp_payload() { let mut config = MtuDiscoveryConfig::default(); config.upper_bound(MAX_UDP_PAYLOAD); - let mut mtud = MtuDiscovery::new(1200, None, config); + let mut mtud = MtuDiscovery::new(1200, 1200, None, config); drive_to_completion(&mut mtud, Instant::now(), u16::MAX); @@ -576,7 +765,7 @@ mod tests { fn mtu_discovery_lost_half_of_probes_finds_maximum_udp_payload() { let mut config = MtuDiscoveryConfig::default(); config.upper_bound(MAX_UDP_PAYLOAD); - let mut mtud = MtuDiscovery::new(1200, None, config); + let mut mtud = MtuDiscovery::new(1200, 1200, None, config); let now = Instant::now(); let mut iterations = 0; diff --git a/quinn-proto/src/connection/pacing.rs b/quinn-proto/src/connection/pacing.rs index 7b8c875f7..f77889faf 100644 --- a/quinn-proto/src/connection/pacing.rs +++ b/quinn-proto/src/connection/pacing.rs @@ -22,17 +22,12 @@ pub(super) struct Pacer { impl Pacer { /// Obtains a new [`Pacer`]. - pub(super) fn new( - smoothed_rtt: Duration, - window: u64, - max_udp_payload_size: u16, - now: Instant, - ) -> Self { - let capacity = optimal_capacity(smoothed_rtt, window, max_udp_payload_size); + pub(super) fn new(smoothed_rtt: Duration, window: u64, mtu: u16, now: Instant) -> Self { + let capacity = optimal_capacity(smoothed_rtt, window, mtu); Self { capacity, last_window: window, - last_mtu: max_udp_payload_size, + last_mtu: mtu, tokens: capacity, prev: now, } @@ -131,17 +126,14 @@ impl Pacer { /// tokens for the extra-elapsed time can be stored. /// /// Too long burst intervals make pacing less effective. -fn optimal_capacity(smoothed_rtt: Duration, window: u64, max_udp_payload_size: u16) -> u64 { +fn optimal_capacity(smoothed_rtt: Duration, window: u64, mtu: u16) -> u64 { let rtt = smoothed_rtt.as_nanos().max(1); let capacity = ((window as u128 * BURST_INTERVAL_NANOS) / rtt) as u64; // Small bursts are less efficient (no GSO), could increase latency and don't effectively // use the channel's buffer capacity. Large bursts might block the connection on sending. - capacity.clamp( - MIN_BURST_SIZE * max_udp_payload_size as u64, - MAX_BURST_SIZE * max_udp_payload_size as u64, - ) + capacity.clamp(MIN_BURST_SIZE * mtu as u64, MAX_BURST_SIZE * mtu as u64) } /// The burst interval diff --git a/quinn-proto/src/connection/paths.rs b/quinn-proto/src/connection/paths.rs index 9a690c459..a945b5387 100644 --- a/quinn-proto/src/connection/paths.rs +++ b/quinn-proto/src/connection/paths.rs @@ -37,7 +37,8 @@ impl PathData { remote: SocketAddr, initial_rtt: Duration, congestion: Box, - initial_max_udp_payload_size: u16, + initial_mtu: u16, + min_guaranteed_mtu: u16, peer_max_udp_payload_size: Option, mtud_config: Option, now: Instant, @@ -47,12 +48,7 @@ impl PathData { remote, rtt: RttEstimator::new(initial_rtt), sending_ecn: true, - pacing: Pacer::new( - initial_rtt, - congestion.initial_window(), - initial_max_udp_payload_size, - now, - ), + pacing: Pacer::new(initial_rtt, congestion.initial_window(), initial_mtu, now), congestion, challenge: None, challenge_pending: false, @@ -60,10 +56,11 @@ impl PathData { total_sent: 0, total_recvd: 0, mtud: mtud_config.map_or( - MtuDiscovery::disabled(initial_max_udp_payload_size), + MtuDiscovery::disabled(initial_mtu, min_guaranteed_mtu), |config| { MtuDiscovery::new( - initial_max_udp_payload_size, + initial_mtu, + min_guaranteed_mtu, peer_max_udp_payload_size, config, ) diff --git a/quinn-proto/src/connection/stats.rs b/quinn-proto/src/connection/stats.rs index 222438379..9b7a2a468 100644 --- a/quinn-proto/src/connection/stats.rs +++ b/quinn-proto/src/connection/stats.rs @@ -137,6 +137,8 @@ pub struct PathStats { /// The amount of PLPMTUD probe packets lost on this path (ignored by `lost_packets` and /// `lost_bytes`) pub lost_plpmtud_probes: u64, + /// The number of times a black hole was detected in the path + pub black_holes_detected: u64, } /// Connection statistics diff --git a/quinn-proto/src/endpoint.rs b/quinn-proto/src/endpoint.rs index 9f9e34bf0..ba9a9fd5a 100644 --- a/quinn-proto/src/endpoint.rs +++ b/quinn-proto/src/endpoint.rs @@ -28,8 +28,8 @@ use crate::{ EndpointEventInner, IssuedCid, }, transport_parameters::TransportParameters, - ResetToken, RetryToken, Side, Transmit, TransportConfig, TransportError, - INITIAL_MAX_UDP_PAYLOAD_SIZE, MAX_CID_SIZE, MIN_INITIAL_SIZE, RESET_TOKEN_SIZE, + ResetToken, RetryToken, Side, Transmit, TransportConfig, TransportError, INITIAL_MTU, + MAX_CID_SIZE, MIN_INITIAL_SIZE, RESET_TOKEN_SIZE, }; /// The main entry point to the library @@ -65,8 +65,6 @@ pub struct Endpoint { impl Endpoint { /// Create a new endpoint - /// - /// Returns `Err` if the configuration is invalid. pub fn new(config: Arc, server_config: Option>) -> Self { Self { rng: StdRng::from_entropy(), @@ -681,9 +679,8 @@ impl Endpoint { let mut buf = Vec::::new(); let partial_encode = header.encode(&mut buf); - let max_len = INITIAL_MAX_UDP_PAYLOAD_SIZE as usize - - partial_encode.header_len - - crypto.packet.local.tag_len(); + let max_len = + INITIAL_MTU as usize - partial_encode.header_len - crypto.packet.local.tag_len(); frame::Close::from(reason).encode(&mut buf, max_len); buf.resize(buf.len() + crypto.packet.local.tag_len(), 0); partial_encode.finish( diff --git a/quinn-proto/src/lib.rs b/quinn-proto/src/lib.rs index ba233d96c..2d1d14374 100644 --- a/quinn-proto/src/lib.rs +++ b/quinn-proto/src/lib.rs @@ -294,7 +294,7 @@ const RESET_TOKEN_SIZE: usize = 16; const MAX_CID_SIZE: usize = 20; const MIN_INITIAL_SIZE: u16 = 1200; /// -const INITIAL_MAX_UDP_PAYLOAD_SIZE: u16 = 1200; +const INITIAL_MTU: u16 = 1200; const MAX_UDP_PAYLOAD: u16 = 65527; const TIMER_GRANULARITY: Duration = Duration::from_millis(1); /// Maximum number of streams that can be uniquely identified by a stream ID diff --git a/quinn-proto/src/tests/mod.rs b/quinn-proto/src/tests/mod.rs index 0cdc6ed45..4cb8754cb 100644 --- a/quinn-proto/src/tests/mod.rs +++ b/quinn-proto/src/tests/mod.rs @@ -1920,7 +1920,7 @@ fn connect_too_low_mtu() { let mut pair = Pair::default(); // The maximum payload size is lower than 1200, so no packages will get through! - pair.max_udp_payload_size = 1000; + pair.mtu = 1000; pair.begin_connect(client_config()); pair.drive(); @@ -1957,7 +1957,7 @@ fn connect_detects_mtu() { for &(pair_max_udp, expected_mtu) in max_udp_payload_and_expected_mtu { println!("Trying {pair_max_udp}"); let mut pair = Pair::default(); - pair.max_udp_payload_size = pair_max_udp; + pair.mtu = pair_max_udp; let (client_ch, server_ch) = pair.connect(); pair.drive(); @@ -1984,7 +1984,7 @@ fn migrate_detects_new_mtu_and_respects_original_peer_max_udp_payload_size() { }; let client = Endpoint::new(Arc::new(client_endpoint_config), None); let mut pair = Pair::new_from_endpoint(client, server); - pair.max_udp_payload_size = 1300; + pair.mtu = 1300; // Connect let (client_ch, server_ch) = pair.connect(); @@ -1996,7 +1996,7 @@ fn migrate_detects_new_mtu_and_respects_original_peer_max_udp_payload_size() { assert_eq!(pair.server_conn_mut(server_ch).path_mtu(), 1300); // Migrate client to a different port (and simulate a higher path MTU) - pair.max_udp_payload_size = 1500; + pair.mtu = 1500; pair.client.addr = SocketAddr::new( Ipv4Addr::new(127, 0, 0, 1).into(), CLIENT_PORTS.lock().unwrap().next().unwrap(), @@ -2037,7 +2037,7 @@ fn connect_runs_mtud_again_after_600_seconds() { .max_idle_timeout(None); let mut pair = Pair::new(Default::default(), server_config); - pair.max_udp_payload_size = 1400; + pair.mtu = 1400; let (client_ch, server_ch) = pair.connect_with(client_config); pair.drive(); @@ -2053,7 +2053,7 @@ fn connect_runs_mtud_again_after_600_seconds() { // Sanity check: the mtu does not change after the fact, even though the link now supports a // higher udp payload size - pair.max_udp_payload_size = 1500; + pair.mtu = 1500; pair.drive(); assert_eq!(pair.client_conn_mut(client_ch).path_mtu(), 1389); assert_eq!(pair.server_conn_mut(server_ch).path_mtu(), 1389); @@ -2079,7 +2079,7 @@ fn packet_loss_and_retry_too_low_mtu() { pair.drive(); // Nothing will get past this mtu - pair.max_udp_payload_size = 10; + pair.mtu = 10; pair.client_send(client_ch, s).write(b" world").unwrap(); pair.drive_client(); @@ -2088,7 +2088,7 @@ fn packet_loss_and_retry_too_low_mtu() { assert!(pair.server.inbound.is_empty()); // Restore the default mtu, so future packets are properly transmitted - pair.max_udp_payload_size = DEFAULT_MAX_UDP_PAYLOAD_SIZE; + pair.mtu = DEFAULT_MTU; // The lost packet is resent pair.drive(); @@ -2104,7 +2104,7 @@ fn packet_loss_and_retry_too_low_mtu() { fn blackhole_after_mtu_change_repairs_itself() { let _guard = subscribe(); let mut pair = Pair::default(); - pair.max_udp_payload_size = 1500; + pair.mtu = 1500; let (client_ch, server_ch) = pair.connect(); pair.drive(); @@ -2113,7 +2113,7 @@ fn blackhole_after_mtu_change_repairs_itself() { assert_eq!(pair.server_conn_mut(server_ch).path_mtu(), 1452); // Back to the base MTU - pair.max_udp_payload_size = 1200; + pair.mtu = 1200; // The payload will be sent in a single packet, because the detected MTU was 1444, but it will // be dropped because the link no longer supports that packet size! @@ -2136,6 +2136,7 @@ fn blackhole_after_mtu_change_repairs_itself() { let client_stats = pair.client_conn_mut(client_ch).stats(); assert!(client_stats.path.lost_packets >= 3); assert!(client_stats.path.congestion_events >= 3); + assert_eq!(client_stats.path.black_holes_detected, 1); } #[test] @@ -2165,7 +2166,7 @@ fn packet_splitting_not_necessary_after_higher_mtu_discovered() { let payload = vec![42; 1300]; let mut pair = Pair::default(); - pair.max_udp_payload_size = 1500; + pair.mtu = 1500; let (client_ch, _) = pair.connect(); pair.drive(); diff --git a/quinn-proto/src/tests/util.rs b/quinn-proto/src/tests/util.rs index 529ebb1f8..9bed347a8 100644 --- a/quinn-proto/src/tests/util.rs +++ b/quinn-proto/src/tests/util.rs @@ -18,14 +18,14 @@ use tracing::{info_span, trace}; use super::*; -pub(super) const DEFAULT_MAX_UDP_PAYLOAD_SIZE: usize = 1200; +pub(super) const DEFAULT_MTU: usize = 1200; pub(super) struct Pair { pub(super) server: TestEndpoint, pub(super) client: TestEndpoint, pub(super) time: Instant, /// Simulates the maximum size allowed for UDP payloads by the link (packets exceeding this size will be dropped) - pub(super) max_udp_payload_size: usize, + pub(super) mtu: usize, // One-way pub(super) latency: Duration, /// Number of spin bit flips @@ -54,7 +54,7 @@ impl Pair { server: TestEndpoint::new(server, server_addr), client: TestEndpoint::new(client, client_addr), time: Instant::now(), - max_udp_payload_size: DEFAULT_MAX_UDP_PAYLOAD_SIZE, + mtu: DEFAULT_MTU, latency: Duration::new(0, 0), spins: 0, last_spin: false, @@ -115,7 +115,7 @@ impl Pair { let _guard = span.enter(); self.client.drive(self.time, self.server.addr); for x in self.client.outbound.drain(..) { - if packet_size(&x) > self.max_udp_payload_size { + if packet_size(&x) > self.mtu { info!( packet_size = packet_size(&x), "dropping packet (max size exceeded)" @@ -143,7 +143,7 @@ impl Pair { let _guard = span.enter(); self.server.drive(self.time, self.client.addr); for x in self.server.outbound.drain(..) { - if packet_size(&x) > self.max_udp_payload_size { + if packet_size(&x) > self.mtu { info!( packet_size = packet_size(&x), "dropping packet (max size exceeded)"