diff --git a/neqo-bin/src/lib.rs b/neqo-bin/src/lib.rs index 72df8fd306..98aee900a5 100644 --- a/neqo-bin/src/lib.rs +++ b/neqo-bin/src/lib.rs @@ -110,6 +110,10 @@ impl SharedArgs { } #[derive(Clone, Debug, Parser)] +#[expect( + clippy::struct_excessive_bools, + reason = "A config struct is not a good use of that lint." +)] pub struct QuicParameters { #[arg( short = 'Q', @@ -165,6 +169,10 @@ pub struct QuicParameters { /// Whether to slice the SNI. pub no_sni_slicing: bool, + #[arg(long)] + /// Whether to disable Alternative Backoff with ECN (RFC 8511). + pub no_abe: bool, + #[arg(name = "preferred-address-v4", long)] /// An IPv4 address for the server preferred address. pub preferred_address_v4: Option, @@ -191,6 +199,7 @@ impl Default for QuicParameters { preferred_address_v4: None, preferred_address_v6: None, no_sni_slicing: false, + no_abe: false, } } } @@ -266,7 +275,8 @@ impl QuicParameters { .slow_start(self.slow_start) .pacing(!self.no_pacing) .pmtud(!self.no_pmtud) - .sni_slicing(!self.no_sni_slicing); + .sni_slicing(!self.no_sni_slicing) + .abe(!self.no_abe); params = if let Some(pa) = self.preferred_address() { params.preferred_address(pa) } else { diff --git a/neqo-transport/src/cc/cubic.rs b/neqo-transport/src/cc/cubic.rs index ba43f3a594..d7be6b9fce 100644 --- a/neqo-transport/src/cc/cubic.rs +++ b/neqo-transport/src/cc/cubic.rs @@ -99,13 +99,26 @@ impl Display for State { } } -#[derive(Debug, Default)] +#[derive(Debug)] pub struct Cubic { /// Current CUBIC parameters. current: State, /// CUBIC parameters that have been stored on a congestion event to restore later in case it /// turns out to have been spurious. stored: Option, + /// Whether to apply the alternative backoff factor [`Self::BETA_USIZE_DIVIDEND_ECN`] on ECN-CE + /// congestion events (RFC 8511). + abe: bool, +} + +impl Default for Cubic { + fn default() -> Self { + Self { + current: State::default(), + stored: None, + abe: true, + } + } } impl Display for Cubic { @@ -186,6 +199,13 @@ impl Cubic { /// construct the beta value from `85/100`. pub const BETA_USIZE_DIVIDEND_ECN: usize = 85; + /// Enable or disable Alternative Backoff with ECN (RFC 8511). Enabled by default. + #[must_use] + pub const fn abe(mut self, abe: bool) -> Self { + self.abe = abe; + self + } + /// This is the factor that is used by fast convergence to further reduce the next `W_max` when /// a congestion event occurs while `cwnd < W_max`. This speeds up the bandwidth release for /// when a new flow joins the network. @@ -458,7 +478,7 @@ impl WindowAdjustment for Cubic { // Reducing the congestion window and resetting time self.current.t_epoch = None; - let beta_dividend = if congestion_trigger == Ecn { + let beta_dividend = if self.abe && congestion_trigger == Ecn { Self::BETA_USIZE_DIVIDEND_ECN } else { Self::BETA_USIZE_DIVIDEND diff --git a/neqo-transport/src/cc/tests/cubic.rs b/neqo-transport/src/cc/tests/cubic.rs index 5e0ff445dc..4fcffacaac 100644 --- a/neqo-transport/src/cc/tests/cubic.rs +++ b/neqo-transport/src/cc/tests/cubic.rs @@ -18,7 +18,7 @@ use std::{ use neqo_common::to_u64; use test_fixture::now; -use super::{RTT, make_cc_cubic}; +use super::{RTT, make_cc_cubic, make_cc_cubic_with}; use crate::{ cc::{ ClassicSlowStart, CongestionController as _, @@ -48,7 +48,18 @@ fn setup_congestion_avoidance( ClassicCongestionController, CongestionControlStats, ) { - let mut cc = make_cc_cubic(); + setup_congestion_avoidance_with(Cubic::default(), fast_convergence) +} + +/// Like [`setup_congestion_avoidance`], but with the given Cubic instance. +fn setup_congestion_avoidance_with( + cubic: Cubic, + fast_convergence: bool, +) -> ( + ClassicCongestionController, + CongestionControlStats, +) { + let mut cc = make_cc_cubic_with(cubic); let mut cc_stats = CongestionControlStats::default(); // Enter congestion avoidance phase. cc.set_ssthresh(1); @@ -335,8 +346,9 @@ fn congestion_event_congestion_avoidance() { assert_eq!(cc_stats.congestion_events.loss, 1); } -/// Verify that `acked_bytes` is correctly reduced on a congestion event. -fn acked_bytes_reduced_on_congestion_event( +/// Verify that `cwnd` and `acked_bytes` are correctly reduced on a congestion event. +fn cwnd_and_acked_bytes_reduced_on_congestion_event( + cubic: Cubic, trigger: impl FnOnce( &mut ClassicCongestionController, Instant, @@ -344,7 +356,7 @@ fn acked_bytes_reduced_on_congestion_event( ), beta: usize, ) { - let (mut cubic, mut cc_stats) = setup_congestion_avoidance(false); + let (mut cubic, mut cc_stats) = setup_congestion_avoidance_with(cubic, false); // The helper acked packet 0. Ack one more to accumulate acked_bytes. let now = now() + RTT / 10; @@ -360,27 +372,44 @@ fn acked_bytes_reduced_on_congestion_event( // Trigger the congestion event. trigger(&mut cubic, now, &mut cc_stats); - // Verify acked_bytes was reduced by the correct factor. - let expected = acked_bytes_before * beta / Cubic::BETA_USIZE_DIVISOR; - assert_eq!(cubic.acked_bytes(), expected); + // Verify cwnd and acked_bytes were reduced by the correct factor. + assert_eq!( + cubic.cwnd(), + cubic.cwnd_initial() * beta / Cubic::BETA_USIZE_DIVISOR + ); + assert_eq!( + cubic.acked_bytes(), + acked_bytes_before * beta / Cubic::BETA_USIZE_DIVISOR + ); } #[test] -fn acked_bytes_reduced_on_loss() { - acked_bytes_reduced_on_congestion_event( +fn cwnd_and_acked_bytes_reduced_on_loss() { + cwnd_and_acked_bytes_reduced_on_congestion_event( + Cubic::default(), |cc, _, stats| packet_lost(cc, 2, stats), Cubic::BETA_USIZE_DIVIDEND, ); } #[test] -fn acked_bytes_reduced_on_ecn_ce() { - acked_bytes_reduced_on_congestion_event( +fn cwnd_and_acked_bytes_reduced_on_ecn_ce() { + cwnd_and_acked_bytes_reduced_on_congestion_event( + Cubic::default(), |cc, now, stats| ecn_ce(cc, 2, now, stats), Cubic::BETA_USIZE_DIVIDEND_ECN, ); } +#[test] +fn cwnd_and_acked_bytes_reduced_on_ecn_ce_without_abe() { + cwnd_and_acked_bytes_reduced_on_congestion_event( + Cubic::default().abe(false), + |cc, now, stats| ecn_ce(cc, 2, now, stats), + Cubic::BETA_USIZE_DIVIDEND, + ); +} + #[test] fn congestion_event_congestion_avoidance_fast_convergence() { let (mut cubic, mut cc_stats) = setup_congestion_avoidance(true); diff --git a/neqo-transport/src/cc/tests/mod.rs b/neqo-transport/src/cc/tests/mod.rs index 0926bc8904..2c5dfec12b 100644 --- a/neqo-transport/src/cc/tests/mod.rs +++ b/neqo-transport/src/cc/tests/mod.rs @@ -39,9 +39,14 @@ pub fn make_cc_newreno() -> ClassicCongestionController ClassicCongestionController { + make_cc_cubic_with(Cubic::default()) +} + +/// Helper to create `ClassicCongestionController` with the given Cubic instance for tests. +pub fn make_cc_cubic_with(cubic: Cubic) -> ClassicCongestionController { ClassicCongestionController::new( ClassicSlowStart::default(), - Cubic::default(), + cubic, Pmtud::new(IP_ADDR, MTU), true, ) diff --git a/neqo-transport/src/connection/params.rs b/neqo-transport/src/connection/params.rs index 5a215fca18..06b79bd90c 100644 --- a/neqo-transport/src/connection/params.rs +++ b/neqo-transport/src/connection/params.rs @@ -178,6 +178,9 @@ pub struct ConnectionParameters { /// Whether to recover from spurious congestion events by restoring prior Congestion Controller /// state. Detection and metrics are always active regardless of this setting. spurious_recovery: bool, + /// Whether to use Alternative Backoff with ECN (RFC 8511), i.e. a smaller multiplicative + /// decrease on ECN-CE congestion events than on loss. Only affects CUBIC. + abe: bool, } impl Default for ConnectionParameters { @@ -209,6 +212,7 @@ impl Default for ConnectionParameters { scone: false, reliable_stream_reset: true, spurious_recovery: true, + abe: true, } } } @@ -541,6 +545,17 @@ impl ConnectionParameters { self } + #[must_use] + pub const fn abe_enabled(&self) -> bool { + self.abe + } + + #[must_use] + pub const fn abe(mut self, abe: bool) -> Self { + self.abe = abe; + self + } + /// # Errors /// When a connection ID cannot be obtained. /// # Panics @@ -669,6 +684,13 @@ mod tests { ); } + #[test] + fn abe_enabled() { + // Default is true; verify builder can toggle it. + assert!(ConnectionParameters::default().abe_enabled()); + assert!(!ConnectionParameters::default().abe(false).abe_enabled()); + } + #[test] fn max_stream_data_transport_parameters() { use std::{cell::RefCell, rc::Rc}; diff --git a/neqo-transport/src/sender.rs b/neqo-transport/src/sender.rs index 79f5843a16..28d14e0335 100644 --- a/neqo-transport/src/sender.rs +++ b/neqo-transport/src/sender.rs @@ -40,6 +40,7 @@ impl PacketSender { pub fn new(conn_params: &ConnectionParameters, pmtud: Pmtud, now: Instant) -> Self { let mtu = pmtud.plpmtu(); let spurious_recovery = conn_params.spurious_recovery_enabled(); + let abe = conn_params.abe_enabled(); Self { cc: match ( conn_params.get_congestion_control(), @@ -81,7 +82,7 @@ impl PacketSender { (CongestionControl::Cubic, SlowStart::Classic) => { CongestionControlImplementation::ClassicCubic(ClassicCongestionController::new( ClassicSlowStart::default(), - Cubic::default(), + Cubic::default().abe(abe), pmtud, spurious_recovery, )) @@ -92,7 +93,7 @@ impl PacketSender { conn_params.pacing_enabled(), conn_params.get_hystart_css_baseline(), ), - Cubic::default(), + Cubic::default().abe(abe), pmtud, spurious_recovery, )) @@ -100,7 +101,7 @@ impl PacketSender { (CongestionControl::Cubic, SlowStart::Search) => { CongestionControlImplementation::SearchCubic(ClassicCongestionController::new( Search::new(), - Cubic::default(), + Cubic::default().abe(abe), pmtud, spurious_recovery, ))