From 6f8823bdc1e5e57e982329cb3d5758a3ff0ea448 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Thu, 29 Aug 2024 17:34:12 +0200 Subject: [PATCH] feat(transport): add ecn metrics to Stats (#2072) * feat(transport): add ecn metrics to Stats * clippy * Document NotEct 0 * clippy * fix doc links --- neqo-transport/src/connection/mod.rs | 9 +++- neqo-transport/src/connection/tests/ecn.rs | 35 ++++++++++++++- neqo-transport/src/ecn.rs | 51 +++++++++++++++++----- neqo-transport/src/path.rs | 8 ++-- neqo-transport/src/stats.rs | 28 +++++++++++- 5 files changed, 110 insertions(+), 21 deletions(-) diff --git a/neqo-transport/src/connection/mod.rs b/neqo-transport/src/connection/mod.rs index 2930af3997..4d25f38630 100644 --- a/neqo-transport/src/connection/mod.rs +++ b/neqo-transport/src/connection/mod.rs @@ -1460,7 +1460,9 @@ impl Connection { ) { let space = PacketNumberSpace::from(packet.packet_type()); if let Some(space) = self.acks.get_mut(space) { - *space.ecn_marks() += d.tos().into(); + let space_ecn_marks = space.ecn_marks(); + *space_ecn_marks += d.tos().into(); + self.stats.borrow_mut().ecn_rx = *space_ecn_marks; } else { qtrace!("Not tracking ECN for dropped packet number space"); } @@ -2427,7 +2429,10 @@ impl Connection { self.loss_recovery.on_packet_sent(path, initial); } path.borrow_mut().add_sent(packets.len()); - Ok(SendOption::Yes(path.borrow_mut().datagram(packets))) + Ok(SendOption::Yes( + path.borrow_mut() + .datagram(packets, &mut self.stats.borrow_mut()), + )) } } diff --git a/neqo-transport/src/connection/tests/ecn.rs b/neqo-transport/src/connection/tests/ecn.rs index 4399ab09ff..1ac9ad6973 100644 --- a/neqo-transport/src/connection/tests/ecn.rs +++ b/neqo-transport/src/connection/tests/ecn.rs @@ -12,11 +12,11 @@ use test_fixture::{ fixture_init, now, DEFAULT_ADDR_V4, }; -use super::{send_something_with_modifier, send_with_modifier_and_receive, DEFAULT_RTT}; use crate::{ connection::tests::{ connect_force_idle, connect_force_idle_with_modifier, default_client, default_server, - handshake_with_modifier, migration::get_cid, new_client, new_server, send_something, + handshake_with_modifier, migration::get_cid, new_client, new_server, send_and_receive, + send_something, send_something_with_modifier, send_with_modifier_and_receive, DEFAULT_RTT, }, ecn::ECN_TEST_COUNT, ConnectionId, ConnectionParameters, StreamType, @@ -91,6 +91,37 @@ fn handshake_delay_with_ecn_blackhole() { ); } +#[test] +fn stats() { + let now = now(); + let mut client = default_client(); + let mut server = default_server(); + connect_force_idle(&mut client, &mut server); + + for _ in 0..ECN_TEST_COUNT { + let ack = send_and_receive(&mut client, &mut server, now); + client.process_input(&ack.unwrap(), now); + } + + for _ in 0..ECN_TEST_COUNT { + let ack = send_and_receive(&mut server, &mut client, now); + server.process_input(&ack.unwrap(), now); + } + + for stats in [client.stats(), server.stats()] { + assert_eq!(stats.ecn_paths_capable, 1); + assert_eq!(stats.ecn_paths_not_capable, 0); + + for codepoint in [IpTosEcn::Ect1, IpTosEcn::Ce] { + assert_eq!(stats.ecn_tx[codepoint], 0); + assert_eq!(stats.ecn_rx[codepoint], 0); + } + } + + assert!(client.stats().ecn_tx[IpTosEcn::Ect0] <= server.stats().ecn_rx[IpTosEcn::Ect0]); + assert!(server.stats().ecn_tx[IpTosEcn::Ect0] <= client.stats().ecn_rx[IpTosEcn::Ect0]); +} + #[test] fn disables_on_loss() { let now = now(); diff --git a/neqo-transport/src/ecn.rs b/neqo-transport/src/ecn.rs index 3329b37070..422ae5eb1e 100644 --- a/neqo-transport/src/ecn.rs +++ b/neqo-transport/src/ecn.rs @@ -12,6 +12,7 @@ use neqo_common::{qdebug, qinfo, qwarn, IpTosEcn}; use crate::{ packet::{PacketNumber, PacketType}, recovery::SentPacket, + Stats, }; /// The number of packets to use for testing a path for ECN capability. @@ -25,7 +26,7 @@ const ECN_TEST_COUNT_INITIAL_PHASE: usize = 3; /// The state information related to testing a path for ECN capability. /// See RFC9000, Appendix A.4. -#[derive(Debug, PartialEq, Clone)] +#[derive(Debug, PartialEq, Clone, Copy)] enum EcnValidationState { /// The path is currently being tested for ECN capability, with the number of probes sent so /// far on the path during the ECN validation. @@ -50,7 +51,32 @@ impl Default for EcnValidationState { } } +impl EcnValidationState { + fn set(&mut self, new: Self, stats: &mut Stats) { + let old = std::mem::replace(self, new); + + match old { + Self::Testing { .. } | Self::Unknown => {} + Self::Failed => debug_assert!(false, "Failed is a terminal state"), + Self::Capable => stats.ecn_paths_capable -= 1, + } + match new { + Self::Testing { .. } | Self::Unknown => {} + Self::Failed => stats.ecn_paths_not_capable += 1, + Self::Capable => stats.ecn_paths_capable += 1, + } + } +} + /// The counts for different ECN marks. +/// +/// Note: [`EcnCount`] is used both for outgoing UDP datagrams, returned by +/// remote through QUIC ACKs and for incoming UDP datagrams, read from IP TOS +/// header. In the former case, given that QUIC ACKs only carry +/// [`IpTosEcn::Ect0`], [`IpTosEcn::Ect1`] and [`IpTosEcn::Ce`], but never +/// [`IpTosEcn::NotEct`], the [`IpTosEcn::NotEct`] value will always be 0. +/// +/// See also . #[derive(PartialEq, Eq, Debug, Clone, Copy, Default)] pub struct EcnCount(EnumMap); @@ -126,13 +152,13 @@ impl EcnInfo { /// Exit ECN validation if the number of packets sent exceeds `ECN_TEST_COUNT`. /// We do not implement the part of the RFC that says to exit ECN validation if the time since /// the start of ECN validation exceeds 3 * PTO, since this seems to happen much too quickly. - pub fn on_packet_sent(&mut self) { + pub fn on_packet_sent(&mut self, stats: &mut Stats) { if let EcnValidationState::Testing { probes_sent, .. } = &mut self.state { *probes_sent += 1; qdebug!("ECN probing: sent {} probes", probes_sent); if *probes_sent == ECN_TEST_COUNT { qdebug!("ECN probing concluded with {} probes sent", probes_sent); - self.state = EcnValidationState::Unknown; + self.state.set(EcnValidationState::Unknown, stats); } } } @@ -144,16 +170,17 @@ impl EcnInfo { &mut self, acked_packets: &[SentPacket], ack_ecn: Option, + stats: &mut Stats, ) -> bool { let prev_baseline = self.baseline; - self.validate_ack_ecn_and_update(acked_packets, ack_ecn); + self.validate_ack_ecn_and_update(acked_packets, ack_ecn, stats); matches!(self.state, EcnValidationState::Capable) && (self.baseline - prev_baseline)[IpTosEcn::Ce] > 0 } - pub fn on_packets_lost(&mut self, lost_packets: &[SentPacket]) { + pub fn on_packets_lost(&mut self, lost_packets: &[SentPacket], stats: &mut Stats) { if let EcnValidationState::Testing { probes_sent, initial_probes_lost: probes_lost, @@ -170,7 +197,7 @@ impl EcnInfo { "ECN validation failed, all {} initial marked packets were lost", probes_lost ); - self.state = EcnValidationState::Failed; + self.state.set(EcnValidationState::Failed, stats); } } } @@ -180,6 +207,7 @@ impl EcnInfo { &mut self, acked_packets: &[SentPacket], ack_ecn: Option, + stats: &mut Stats, ) { // RFC 9000, Appendix A.4: // @@ -212,7 +240,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.state = EcnValidationState::Failed; + self.state.set(EcnValidationState::Failed, stats); return; }; @@ -229,7 +257,7 @@ impl EcnInfo { .unwrap(); if newly_acked_sent_with_ect0 == 0 { qwarn!("ECN validation failed, no ECT(0) packets were newly acked"); - self.state = EcnValidationState::Failed; + self.state.set(EcnValidationState::Failed, stats); return; } let ecn_diff = ack_ecn - self.baseline; @@ -240,15 +268,16 @@ impl EcnInfo { sum_inc, newly_acked_sent_with_ect0 ); - self.state = EcnValidationState::Failed; + self.state.set(EcnValidationState::Failed, stats); } else if ecn_diff[IpTosEcn::Ect1] > 0 { qwarn!("ECN validation failed, ACK counted ECT(1) marks that were never sent"); - self.state = EcnValidationState::Failed; + self.state.set(EcnValidationState::Failed, stats); } else if self.state != EcnValidationState::Capable { qinfo!("ECN validation succeeded, path is capable"); - self.state = EcnValidationState::Capable; + self.state.set(EcnValidationState::Capable, stats); } self.baseline = ack_ecn; + stats.ecn_tx = ack_ecn; self.largest_acked = largest_acked; } diff --git a/neqo-transport/src/path.rs b/neqo-transport/src/path.rs index 22b677a239..6ea1cbedbb 100644 --- a/neqo-transport/src/path.rs +++ b/neqo-transport/src/path.rs @@ -698,12 +698,12 @@ impl Path { } /// Make a datagram. - pub fn datagram>>(&mut self, payload: V) -> Datagram { + pub fn datagram>>(&mut self, payload: V, stats: &mut Stats) -> Datagram { // Make sure to use the TOS value from before calling EcnInfo::on_packet_sent, which may // update the ECN state and can hence change it - this packet should still be sent // with the current value. let tos = self.tos(); - self.ecn_info.on_packet_sent(); + self.ecn_info.on_packet_sent(stats); Datagram::new(self.local, self.remote, tos, payload) } @@ -980,7 +980,7 @@ impl Path { ) { debug_assert!(self.is_primary()); - let ecn_ce_received = self.ecn_info.on_packets_acked(acked_pkts, ack_ecn); + let ecn_ce_received = self.ecn_info.on_packets_acked(acked_pkts, ack_ecn, stats); if ecn_ce_received { let cwnd_reduced = self .sender @@ -1004,7 +1004,7 @@ impl Path { now: Instant, ) { debug_assert!(self.is_primary()); - self.ecn_info.on_packets_lost(lost_packets); + self.ecn_info.on_packets_lost(lost_packets, stats); let cwnd_reduced = self.sender.on_packets_lost( self.rtt.first_sample_time(), prev_largest_acked_sent, diff --git a/neqo-transport/src/stats.rs b/neqo-transport/src/stats.rs index e0c96b505d..ebd9c463ed 100644 --- a/neqo-transport/src/stats.rs +++ b/neqo-transport/src/stats.rs @@ -16,7 +16,7 @@ use std::{ use neqo_common::qwarn; -use crate::packet::PacketNumber; +use crate::{ecn::EcnCount, packet::PacketNumber}; pub const MAX_PTO_COUNTS: usize = 16; @@ -166,6 +166,25 @@ pub struct Stats { pub incoming_datagram_dropped: usize, 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 counts for outgoing UDP datagrams, returned by remote through QUIC ACKs. + /// + /// Note: Given that QUIC ACKs only carry [`Ect0`], [`Ect1`] and [`Ce`], but + /// never [`NotEct`], the [`NotEct`] value will always be 0. + /// + /// See also . + /// + /// [`Ect0`]: neqo_common::tos::IpTosEcn::Ect0 + /// [`Ect1`]: neqo_common::tos::IpTosEcn::Ect1 + /// [`Ce`]: neqo_common::tos::IpTosEcn::Ce + /// [`NotEct`]: neqo_common::tos::IpTosEcn::NotEct + pub ecn_tx: EcnCount, + /// ECN counts for incoming UDP datagrams, read from IP TOS header. + pub ecn_rx: EcnCount, } impl Stats { @@ -222,7 +241,12 @@ impl Debug for Stats { writeln!(f, " frames rx:")?; self.frame_rx.fmt(f)?; writeln!(f, " frames tx:")?; - self.frame_tx.fmt(f) + 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 + ) } }