diff --git a/neqo-transport/src/connection/tests/ecn.rs b/neqo-transport/src/connection/tests/ecn.rs index 3b5bd5bc19..0b947c4d38 100644 --- a/neqo-transport/src/connection/tests/ecn.rs +++ b/neqo-transport/src/connection/tests/ecn.rs @@ -19,7 +19,7 @@ use crate::{ new_server, send_and_receive, send_something, send_something_with_modifier, send_with_modifier_and_receive, DEFAULT_RTT, }, - ecn::ECN_TEST_COUNT, + ecn::{EcnValidationOutcome, ECN_TEST_COUNT}, path::MAX_PATH_PROBES, ConnectionId, ConnectionParameters, StreamType, }; @@ -154,8 +154,12 @@ fn stats() { } for stats in [client.stats(), server.stats()] { - assert_eq!(stats.ecn_paths_capable, 1); - assert_eq!(stats.ecn_paths_not_capable, 0); + for (outcome, count) in stats.ecn_path_validation.iter() { + match outcome { + EcnValidationOutcome::Capable => assert_eq!(*count, 1), + EcnValidationOutcome::NotCapable(_) => assert_eq!(*count, 0), + } + } for codepoint in [IpTosEcn::Ect1, IpTosEcn::Ce] { assert_eq!(stats.ecn_tx[codepoint], 0); diff --git a/neqo-transport/src/ecn.rs b/neqo-transport/src/ecn.rs index 97d68fb6c1..944c63ac8a 100644 --- a/neqo-transport/src/ecn.rs +++ b/neqo-transport/src/ecn.rs @@ -6,7 +6,7 @@ use std::ops::{AddAssign, Deref, DerefMut, Sub}; -use enum_map::EnumMap; +use enum_map::{Enum, EnumMap}; use neqo_common::{qdebug, qinfo, qwarn, IpTosEcn}; use crate::{ @@ -37,7 +37,7 @@ enum EcnValidationState { /// The validation test has concluded but the path's ECN capability is not yet known. Unknown, /// The path is known to **not** be ECN capable. - Failed, + Failed(EcnValidationError), /// The path is known to be ECN capable. Capable, } @@ -57,13 +57,15 @@ impl EcnValidationState { match old { Self::Testing { .. } | Self::Unknown => {} - Self::Failed => debug_assert!(false, "Failed is a terminal state"), - Self::Capable => stats.ecn_paths_capable -= 1, + Self::Failed(_) => debug_assert!(false, "Failed is a terminal state"), + Self::Capable => stats.ecn_path_validation[EcnValidationOutcome::Capable] -= 1, } match new { Self::Testing { .. } | Self::Unknown => {} - Self::Failed => stats.ecn_paths_not_capable += 1, - Self::Capable => stats.ecn_paths_capable += 1, + Self::Failed(error) => { + stats.ecn_path_validation[EcnValidationOutcome::NotCapable(error)] += 1; + } + Self::Capable => stats.ecn_path_validation[EcnValidationOutcome::Capable] += 1, } } } @@ -125,6 +127,36 @@ impl AddAssign for EcnCount { } } +#[derive(PartialEq, Eq, Debug, Clone, Copy, Default)] +pub struct EcnValidationCount(EnumMap); + +impl Deref for EcnValidationCount { + type Target = EnumMap; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl DerefMut for EcnValidationCount { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.0 + } +} + +#[derive(Debug, Clone, Copy, Enum, PartialEq, Eq)] +pub enum EcnValidationError { + BlackHole, + Bleaching, + ReceivedUnsentECT1, +} + +#[derive(Debug, Clone, Copy, Enum, PartialEq, Eq)] +pub enum EcnValidationOutcome { + Capable, + NotCapable(EcnValidationError), +} + #[derive(Debug, Default)] pub struct EcnInfo { /// The current state of ECN validation on this path. @@ -164,8 +196,8 @@ impl EcnInfo { } /// Disable ECN. - pub fn disable_ecn(&mut self, stats: &mut Stats) { - self.state.set(EcnValidationState::Failed, stats); + pub fn disable_ecn(&mut self, stats: &mut Stats, reason: EcnValidationError) { + self.state.set(EcnValidationState::Failed(reason), stats); } /// Process ECN counts from an ACK frame. @@ -202,7 +234,7 @@ impl EcnInfo { "ECN validation failed, all {} initial marked packets were lost", probes_lost ); - self.disable_ecn(stats); + self.disable_ecn(stats, EcnValidationError::BlackHole); } } } @@ -220,7 +252,7 @@ impl EcnInfo { // > (see Section 13.4.2.1) causes the ECN state for the path to become "capable", unless // > no marked packet has been acknowledged. match self.state { - EcnValidationState::Testing { .. } | EcnValidationState::Failed => return, + EcnValidationState::Testing { .. } | EcnValidationState::Failed(_) => return, EcnValidationState::Unknown | EcnValidationState::Capable => {} } @@ -245,7 +277,7 @@ impl EcnInfo { // > corresponding ECN counts are not present in the ACK frame. let Some(ack_ecn) = ack_ecn else { qwarn!("ECN validation failed, no ECN counts in ACK frame"); - self.disable_ecn(stats); + self.disable_ecn(stats, EcnValidationError::Bleaching); return; }; @@ -262,7 +294,7 @@ impl EcnInfo { .unwrap(); if newly_acked_sent_with_ect0 == 0 { qwarn!("ECN validation failed, no ECT(0) packets were newly acked"); - self.disable_ecn(stats); + self.disable_ecn(stats, EcnValidationError::Bleaching); return; } let ecn_diff = ack_ecn - self.baseline; @@ -273,10 +305,10 @@ impl EcnInfo { sum_inc, newly_acked_sent_with_ect0 ); - self.disable_ecn(stats); + self.disable_ecn(stats, EcnValidationError::Bleaching); } else if ecn_diff[IpTosEcn::Ect1] > 0 { qwarn!("ECN validation failed, ACK counted ECT(1) marks that were never sent"); - self.disable_ecn(stats); + self.disable_ecn(stats, EcnValidationError::ReceivedUnsentECT1); } else if self.state != EcnValidationState::Capable { qinfo!("ECN validation succeeded, path is capable"); self.state.set(EcnValidationState::Capable, stats); @@ -290,7 +322,7 @@ impl EcnInfo { pub const fn ecn_mark(&self) -> IpTosEcn { match self.state { EcnValidationState::Testing { .. } | EcnValidationState::Capable => IpTosEcn::Ect0, - EcnValidationState::Failed | EcnValidationState::Unknown => IpTosEcn::NotEct, + EcnValidationState::Failed(_) | EcnValidationState::Unknown => IpTosEcn::NotEct, } } } diff --git a/neqo-transport/src/path.rs b/neqo-transport/src/path.rs index 7d0aaf62a6..5513ee651a 100644 --- a/neqo-transport/src/path.rs +++ b/neqo-transport/src/path.rs @@ -733,7 +733,8 @@ impl Path { [self], "Possible ECN blackhole, disabling ECN and re-probing path" ); - self.ecn_info.disable_ecn(stats); + self.ecn_info + .disable_ecn(stats, crate::ecn::EcnValidationError::BlackHole); ProbeState::ProbeNeeded { probe_count: 0 } } else { qinfo!([self], "Probing failed"); diff --git a/neqo-transport/src/stats.rs b/neqo-transport/src/stats.rs index a8c35c1c66..af8ae69a70 100644 --- a/neqo-transport/src/stats.rs +++ b/neqo-transport/src/stats.rs @@ -16,7 +16,10 @@ use std::{ use neqo_common::qwarn; -use crate::{ecn::EcnCount, packet::PacketNumber}; +use crate::{ + ecn::{EcnCount, EcnValidationCount}, + packet::PacketNumber, +}; pub const MAX_PTO_COUNTS: usize = 16; @@ -194,10 +197,8 @@ pub struct Stats { pub datagram_tx: DatagramStats, - /// Number of paths known to be ECN capable. - pub ecn_paths_capable: usize, - /// Number of paths known to be ECN incapable. - pub ecn_paths_not_capable: usize, + /// ECN path validation count, indexed by validation outcome. + pub ecn_path_validation: EcnValidationCount, /// ECN counts for outgoing UDP datagrams, returned by remote through QUIC ACKs. /// /// Note: Given that QUIC ACKs only carry [`Ect0`], [`Ect1`] and [`Ce`], but @@ -271,8 +272,8 @@ impl Debug for Stats { self.frame_tx.fmt(f)?; writeln!( f, - " ecn: {:?} for tx {:?} for rx {} capable paths {} not capable paths", - self.ecn_tx, self.ecn_rx, self.ecn_paths_capable, self.ecn_paths_not_capable + " ecn: {:?} for tx {:?} for rx {:?} path validation outcomes", + self.ecn_tx, self.ecn_rx, self.ecn_path_validation, ) } }