Tie up PLPMTUD's loose ends

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
 }
 

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))))

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))))

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<MtuDiscoveryConfig>,
 
     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::<RandomConnectionIdGenerator>::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)
     }
 

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<dyn Controller>;
+    fn build(&self, now: Instant, current_mtu: u16) -> Box<dyn Controller>;
 }
+
+const BASE_DATAGRAM_SIZE: u64 = 1200;

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<BbrConfig>,
+    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<BbrConfig>) -> Self {
+    pub fn new(config: Arc<BbrConfig>, 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,50 +501,30 @@ 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))`
     pub fn initial_window(&mut self, value: u64) -> &mut Self {
         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<BbrConfig> {
-    fn build(&self, _now: Instant) -> Box<dyn Controller> {
-        Box::new(Bbr::new(self.clone()))
+    fn build(&self, _now: Instant, current_mtu: u16) -> Box<dyn Controller> {
+        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;