From 8d8c9898c333db56f13079af640a6b317ea2290e Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Wed, 14 Feb 2024 16:22:06 +0200 Subject: [PATCH] feat: Send and process ACK-ECN Most of the remaining bits from #1495 Depends on #1604 --- neqo-common/src/datagram.rs | 3 +- neqo-transport/Cargo.toml | 2 +- neqo-transport/src/connection/mod.rs | 16 ++++++++-- neqo-transport/src/frame.rs | 47 +++++++++++++++++++++------- neqo-transport/src/path.rs | 30 +++++++++++++++--- neqo-transport/src/qlog.rs | 9 +++--- neqo-transport/src/tracking.rs | 31 ++++++++++++++++-- neqo-transport/tests/connection.rs | 4 +-- test-fixture/src/lib.rs | 4 +-- 9 files changed, 116 insertions(+), 30 deletions(-) diff --git a/neqo-common/src/datagram.rs b/neqo-common/src/datagram.rs index d6ed43bde1..e4dbac0d8d 100644 --- a/neqo-common/src/datagram.rs +++ b/neqo-common/src/datagram.rs @@ -90,7 +90,6 @@ fn fmt_datagram() { let d = datagram([0; 1].to_vec()); assert_eq!( format!("{d:?}"), - "Datagram IpTos(Cs0, NotEct) TTL Some(128) [fe80::1]:443->[fe80::1]:443: [1]: 00" - .to_string() + "Datagram IpTos(Cs0, Ect0) TTL Some(128) [fe80::1]:443->[fe80::1]:443: [1]: 00".to_string() ); } diff --git a/neqo-transport/Cargo.toml b/neqo-transport/Cargo.toml index 14141a5ab8..712297e162 100644 --- a/neqo-transport/Cargo.toml +++ b/neqo-transport/Cargo.toml @@ -10,6 +10,7 @@ license.workspace = true [dependencies] # Sync with https://searchfox.org/mozilla-central/source/Cargo.lock 2024-02-08 +enum-map = "2.7" indexmap = "1.9" log = { version = "0.4", default-features = false } neqo-common = { path = "../neqo-common" } @@ -19,7 +20,6 @@ smallvec = "1.11" [dev-dependencies] criterion = { version = "0.5", features = ["html_reports"] } -enum-map = "2.7" test-fixture = { path = "../test-fixture" } [features] diff --git a/neqo-transport/src/connection/mod.rs b/neqo-transport/src/connection/mod.rs index 749cf315d3..26c34e0daf 100644 --- a/neqo-transport/src/connection/mod.rs +++ b/neqo-transport/src/connection/mod.rs @@ -462,7 +462,7 @@ impl Connection { } /// # Errors - /// When the operation fails. + /// When the operation fails. pub fn client_enable_ech(&mut self, ech_config_list: impl AsRef<[u8]>) -> Res<()> { self.crypto.client_enable_ech(ech_config_list) } @@ -1502,7 +1502,16 @@ impl Connection { self.stats.borrow_mut().dups_rx += 1; } else { match self.process_packet(path, &payload, now) { - Ok(migrate) => self.postprocess_packet(path, d, &packet, migrate, now), + Ok(migrate) => { + // Since we processed frames from this IP packet now, + // update the ECN counts (RFC9000, Section 13.4.1). + if let Some(space) = self.acks.get_mut(space) { + space.inc_ecn_count(d.tos().into(), 1); + } else { + qdebug!("Not tracking ECN for dropped packet number space"); + } + self.postprocess_packet(path, d, &packet, migrate, now); + } Err(e) => { self.ensure_error_path(path, &packet, now); return Err(e); @@ -2706,10 +2715,13 @@ impl Connection { ack_delay, first_ack_range, ack_ranges, + ecn_count, } => { let ranges = Frame::decode_ack_frame(largest_acknowledged, first_ack_range, &ack_ranges)?; self.handle_ack(space, largest_acknowledged, ranges, ack_delay, now); + // TODO: Handle incoming ECN info. + qdebug!("input_frame {:?}", ecn_count); } Frame::Crypto { offset, data } => { qtrace!( diff --git a/neqo-transport/src/frame.rs b/neqo-transport/src/frame.rs index a3af801925..925b566d9a 100644 --- a/neqo-transport/src/frame.rs +++ b/neqo-transport/src/frame.rs @@ -8,12 +8,14 @@ use std::{convert::TryFrom, ops::RangeInclusive}; -use neqo_common::{qtrace, Decoder}; +use enum_map::{enum_map, EnumMap}; +use neqo_common::{qtrace, Decoder, IpTosEcn}; use crate::{ cid::MAX_CONNECTION_ID_LEN, packet::PacketType, stream_id::{StreamId, StreamType}, + tracking::EcnCount, AppError, ConnectionError, Error, Res, TransportError, }; @@ -23,7 +25,7 @@ pub type FrameType = u64; const FRAME_TYPE_PADDING: FrameType = 0x0; pub const FRAME_TYPE_PING: FrameType = 0x1; pub const FRAME_TYPE_ACK: FrameType = 0x2; -const FRAME_TYPE_ACK_ECN: FrameType = 0x3; +pub const FRAME_TYPE_ACK_ECN: FrameType = 0x3; pub const FRAME_TYPE_RESET_STREAM: FrameType = 0x4; pub const FRAME_TYPE_STOP_SENDING: FrameType = 0x5; pub const FRAME_TYPE_CRYPTO: FrameType = 0x6; @@ -110,6 +112,7 @@ pub enum Frame<'a> { ack_delay: u64, first_ack_range: u64, ack_ranges: Vec, + ecn_count: EcnCount, }, ResetStream { stream_id: StreamId, @@ -217,7 +220,7 @@ impl<'a> Frame<'a> { match self { Self::Padding => FRAME_TYPE_PADDING, Self::Ping => FRAME_TYPE_PING, - Self::Ack { .. } => FRAME_TYPE_ACK, // We don't do ACK ECN. + Self::Ack { .. } => FRAME_TYPE_ACK, Self::ResetStream { .. } => FRAME_TYPE_RESET_STREAM, Self::StopSending { .. } => FRAME_TYPE_STOP_SENDING, Self::Crypto { .. } => FRAME_TYPE_CRYPTO, @@ -442,17 +445,25 @@ impl<'a> Frame<'a> { } // Now check for the values for ACK_ECN. - if t == FRAME_TYPE_ACK_ECN { - dv(dec)?; - dv(dec)?; - dv(dec)?; - } + let ecn_count: EcnCount = match t { + FRAME_TYPE_ACK_ECN => { + let (ect0, ect1, ce) = (dv(dec)?, dv(dec)?, dv(dec)?); + enum_map! { + IpTosEcn::NotEct => 0, + IpTosEcn::Ect0 => ect0, + IpTosEcn::Ect1 => ect1, + IpTosEcn::Ce => ce, + } + } + _ => EnumMap::default(), + }; Ok(Self::Ack { largest_acknowledged: la, ack_delay: ad, first_ack_range: fa, ack_ranges: arr, + ecn_count, }) } FRAME_TYPE_STOP_SENDING => Ok(Self::StopSending { @@ -645,7 +656,8 @@ mod tests { largest_acknowledged: 0x1234, ack_delay: 0x1235, first_ack_range: 0x1236, - ack_ranges: ar, + ack_ranges: ar.clone(), + ecn_count: EnumMap::default(), }; just_dec(&f, "025234523502523601020304"); @@ -655,10 +667,23 @@ mod tests { let mut dec = enc.as_decoder(); assert_eq!(Frame::decode(&mut dec).unwrap_err(), Error::NoMoreData); - // Try to parse ACK_ECN without ECN values + // Try to parse ACK_ECN with ECN values + let ecn_count = enum_map! { + IpTosEcn::NotEct => 0, + IpTosEcn::Ect0 => 1, + IpTosEcn::Ect1 => 2, + IpTosEcn::Ce => 3, + }; + let fe = Frame::Ack { + largest_acknowledged: 0x1234, + ack_delay: 0x1235, + first_ack_range: 0x1236, + ack_ranges: ar, + ecn_count, + }; let enc = Encoder::from_hex("035234523502523601020304010203"); let mut dec = enc.as_decoder(); - assert_eq!(Frame::decode(&mut dec).unwrap(), f); + assert_eq!(Frame::decode(&mut dec).unwrap(), fe); } #[test] diff --git a/neqo-transport/src/path.rs b/neqo-transport/src/path.rs index 897763d7de..f866f36e8c 100644 --- a/neqo-transport/src/path.rs +++ b/neqo-transport/src/path.rs @@ -17,12 +17,12 @@ use std::{ time::{Duration, Instant}, }; -use neqo_common::{hex, qdebug, qinfo, qlog::NeqoQlog, qtrace, Datagram, Encoder, IpTos}; +use neqo_common::{hex, qdebug, qinfo, qlog::NeqoQlog, qtrace, Datagram, Encoder, IpTos, IpTosEcn}; use neqo_crypto::random; use crate::{ ackrate::{AckRate, PeerAckDelay}, - cc::CongestionControlAlgorithm, + cc::{CongestionControlAlgorithm, MAX_DATAGRAM_SIZE}, cid::{ConnectionId, ConnectionIdRef, ConnectionIdStore, RemoteConnectionIdEntry}, frame::{FRAME_TYPE_PATH_CHALLENGE, FRAME_TYPE_PATH_RESPONSE, FRAME_TYPE_RETIRE_CONNECTION_ID}, packet::PacketBuilder, @@ -545,6 +545,8 @@ pub struct Path { received_bytes: usize, /// The number of bytes sent on this path. sent_bytes: usize, + /// The number of ECN-marked bytes sent on this path that were declared lost. + lost_ecn_bytes: usize, /// For logging of events. qlog: NeqoQlog, @@ -574,10 +576,11 @@ impl Path { challenge: None, rtt: RttEstimate::default(), sender, - tos: IpTos::default(), // TODO: Default to Ect0 when ECN is supported. - ttl: 64, // This is the default TTL on many OSes. + tos: IpTosEcn::Ect0.into(), + ttl: 64, // This is the default TTL on many OSes. received_bytes: 0, sent_bytes: 0, + lost_ecn_bytes: 0, qlog, } } @@ -980,6 +983,25 @@ impl Path { self.rtt.pto(space), // Important: the base PTO, not adjusted. lost_packets, ); + + if self.tos == IpTosEcn::Ect0.into() { + // If the path is currently marking outgoing packets as ECT(0), + // update the count of lost ECN-marked bytes. + self.lost_ecn_bytes += lost_packets.iter().map(|p| p.size).sum::(); + + // If we lost more than 3 MTUs worth of ECN-marked bytes, then + // disable ECN on this path. See RFC 9000, Section 13.4.2. + // This doesn't quite implement the algorithm given in RFC 9000, + // Appendix A.4, but it should be OK. (It might be worthwhile caching + // destination IP addresses for paths on which we had to disable ECN, + // in order to not persitently delay connection establishment to + // those destinations.) + if self.lost_ecn_bytes > MAX_DATAGRAM_SIZE * 3 { + qinfo!([self], "Disabling ECN on path due to excessive loss"); + self.tos = IpTosEcn::NotEct.into(); + } + } + if cwnd_reduced { self.rtt.update_ack_delay(self.sender.cwnd(), self.mtu()); } diff --git a/neqo-transport/src/qlog.rs b/neqo-transport/src/qlog.rs index f6d3f4e1e2..3f287721ac 100644 --- a/neqo-transport/src/qlog.rs +++ b/neqo-transport/src/qlog.rs @@ -13,7 +13,7 @@ use std::{ time::Duration, }; -use neqo_common::{hex, qinfo, qlog::NeqoQlog, Decoder}; +use neqo_common::{hex, qinfo, qlog::NeqoQlog, Decoder, IpTosEcn}; use qlog::events::{ connectivity::{ConnectionStarted, ConnectionState, ConnectionStateUpdated}, quic::{ @@ -404,6 +404,7 @@ fn frame_to_qlogframe(frame: &Frame) -> QuicFrame { ack_delay, first_ack_range, ack_ranges, + ecn_count: ecn_counts, } => { let ranges = Frame::decode_ack_frame(*largest_acknowledged, *first_ack_range, ack_ranges).ok(); @@ -419,9 +420,9 @@ fn frame_to_qlogframe(frame: &Frame) -> QuicFrame { QuicFrame::Ack { ack_delay: Some(*ack_delay as f32 / 1000.0), acked_ranges, - ect1: None, - ect0: None, - ce: None, + ect1: Some(ecn_counts[IpTosEcn::Ect1]), + ect0: Some(ecn_counts[IpTosEcn::Ect0]), + ce: Some(ecn_counts[IpTosEcn::Ce]), } } Frame::ResetStream { diff --git a/neqo-transport/src/tracking.rs b/neqo-transport/src/tracking.rs index 012c895a18..3d6e466ad9 100644 --- a/neqo-transport/src/tracking.rs +++ b/neqo-transport/src/tracking.rs @@ -16,11 +16,13 @@ use std::{ time::{Duration, Instant}, }; -use neqo_common::{qdebug, qinfo, qtrace, qwarn}; +use enum_map::EnumMap; +use neqo_common::{qdebug, qinfo, qtrace, qwarn, IpTosEcn}; use neqo_crypto::{Epoch, TLS_EPOCH_HANDSHAKE, TLS_EPOCH_INITIAL}; use smallvec::{smallvec, SmallVec}; use crate::{ + frame::{FRAME_TYPE_ACK, FRAME_TYPE_ACK_ECN}, packet::{PacketBuilder, PacketNumber, PacketType}, recovery::RecoveryToken, stats::FrameStats, @@ -352,6 +354,9 @@ pub struct AckToken { ranges: Vec, } +/// The counts for different ECN marks. +pub type EcnCount = EnumMap; + /// A structure that tracks what packets have been received, /// and what needs acknowledgement for a packet number space. #[derive(Debug)] @@ -380,6 +385,8 @@ pub struct RecvdPackets { /// Whether we are ignoring packets that arrive out of order /// for the purposes of generating immediate acknowledgment. ignore_order: bool, + /// The counts of different ECN marks that have been received. + ecn_count: EcnCount, } impl RecvdPackets { @@ -397,9 +404,15 @@ impl RecvdPackets { unacknowledged_count: 0, unacknowledged_tolerance: DEFAULT_ACK_PACKET_TOLERANCE, ignore_order: false, + ecn_count: EcnCount::default(), } } + /// Increase the ECN count for the mark given by `ecn` by `n`. + pub fn inc_ecn_count(&mut self, ecn: IpTosEcn, n: u64) { + self.ecn_count[ecn] += n; + } + /// Get the time at which the next ACK should be sent. pub fn ack_time(&self) -> Option { self.ack_time @@ -596,7 +609,15 @@ impl RecvdPackets { .cloned() .collect::>(); - builder.encode_varint(crate::frame::FRAME_TYPE_ACK); + let have_ecn_counts = self.ecn_count[IpTosEcn::Ect0] > 0 + || self.ecn_count[IpTosEcn::Ect1] > 0 + || self.ecn_count[IpTosEcn::Ce] > 0; + + builder.encode_varint(if have_ecn_counts { + FRAME_TYPE_ACK_ECN + } else { + FRAME_TYPE_ACK + }); let mut iter = ranges.iter(); let Some(first) = iter.next() else { return }; builder.encode_varint(first.largest); @@ -620,6 +641,12 @@ impl RecvdPackets { last = r.smallest; } + if have_ecn_counts { + builder.encode_varint(self.ecn_count[IpTosEcn::Ect0]); + builder.encode_varint(self.ecn_count[IpTosEcn::Ect1]); + builder.encode_varint(self.ecn_count[IpTosEcn::Ce]); + } + // We've sent an ACK, reset the timer. self.ack_time = None; self.last_ack_time = Some(now); diff --git a/neqo-transport/tests/connection.rs b/neqo-transport/tests/connection.rs index 6f8aa393af..17eaa63c37 100644 --- a/neqo-transport/tests/connection.rs +++ b/neqo-transport/tests/connection.rs @@ -62,8 +62,8 @@ fn truncate_long_packet() { /// Test that reordering parts of the server Initial doesn't change things. #[test] fn reorder_server_initial() { - // A simple ACK frame for a single packet with packet number 0. - const ACK_FRAME: &[u8] = &[0x02, 0x00, 0x00, 0x00, 0x00]; + // A simple ACK_ECN frame for a single packet with packet number 0 with a single ECT(0) mark. + const ACK_FRAME: &[u8] = &[0x03, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00]; let mut client = new_client( ConnectionParameters::default().versions(Version::Version1, vec![Version::Version1]), diff --git a/test-fixture/src/lib.rs b/test-fixture/src/lib.rs index aa0b3ea371..f7542c2e51 100644 --- a/test-fixture/src/lib.rs +++ b/test-fixture/src/lib.rs @@ -24,7 +24,7 @@ use neqo_common::{ event::Provider, hex, qlog::{new_trace, NeqoQlog}, - qtrace, Datagram, Decoder, IpTos, Role, + qtrace, Datagram, Decoder, IpTosEcn, Role, }; use neqo_crypto::{init_db, random, AllowZeroRtt, AntiReplay, AuthenticationStatus}; use neqo_http3::{Http3Client, Http3Parameters, Http3Server}; @@ -96,7 +96,7 @@ pub fn datagram(data: Vec) -> Datagram { Datagram::new( DEFAULT_ADDR, DEFAULT_ADDR, - IpTos::default(), + IpTosEcn::Ect0.into(), Some(128), data, )