From bdd1115c9720f1a452f8caf7db5c573c6c1a6e9b Mon Sep 17 00:00:00 2001 From: Sterling Deng Date: Thu, 19 Sep 2024 10:52:57 -0700 Subject: [PATCH 01/30] Implement TransportParameters --- quinn-proto/Cargo.toml | 1 + quinn-proto/src/transport_parameters.rs | 73 +++++++++++++++++++++++++ 2 files changed, 74 insertions(+) diff --git a/quinn-proto/Cargo.toml b/quinn-proto/Cargo.toml index 191569e24..e64e8461d 100644 --- a/quinn-proto/Cargo.toml +++ b/quinn-proto/Cargo.toml @@ -22,6 +22,7 @@ ring = ["dep:ring"] platform-verifier = ["dep:rustls-platform-verifier"] # Configure `tracing` to log events via `log` if no `tracing` subscriber exists. log = ["tracing/log"] +acktimestamps = [] [dependencies] arbitrary = { workspace = true, optional = true } diff --git a/quinn-proto/src/transport_parameters.rs b/quinn-proto/src/transport_parameters.rs index c09c41f97..2cf645614 100644 --- a/quinn-proto/src/transport_parameters.rs +++ b/quinn-proto/src/transport_parameters.rs @@ -99,6 +99,12 @@ macro_rules! make_struct { pub(crate) stateless_reset_token: Option, /// The server's preferred address for communication after handshake completion pub(crate) preferred_address: Option, + + + #[cfg(feature = "acktimestamps")] + pub(crate) receive_timestamps_exponent: Option, + #[cfg(feature = "acktimestamps")] + pub(crate) max_recv_timestamps_per_ack: Option, } // We deliberately don't implement the `Default` trait, since that would be public, and @@ -120,6 +126,10 @@ macro_rules! make_struct { retry_src_cid: None, stateless_reset_token: None, preferred_address: None, + #[cfg(feature = "acktimestamps")] + receive_timestamps_exponent: None, + #[cfg(feature = "acktimestamps")] + max_recv_timestamps_per_ack: None, } } } @@ -160,6 +170,19 @@ impl TransportParameters { min_ack_delay: Some( VarInt::from_u64(u64::try_from(TIMER_GRANULARITY.as_micros()).unwrap()).unwrap(), ), + + #[cfg(feature = "acktimestamps")] + receive_timestamps_exponent: config + .ack_timestamp_config + .as_ref() + .map_or(None, |cfg| Some(cfg.exponent)), + + #[cfg(feature = "acktimestamps")] + max_recv_timestamps_per_ack: config + .ack_timestamp_config + .as_ref() + .map_or(None, |cfg| Some(cfg.max_timestamps_per_ack)), + ..Self::default() } } @@ -349,6 +372,25 @@ impl TransportParameters { w.write_var(x.size() as u64); w.write(x); } + + // The below 2 fields are for the implementation of + // https://www.ietf.org/archive/id/draft-smith-quic-receive-ts-00.html#name-extension-negotiation + // The transport parameter values selected are placeholders, using the first 2 reserved values specified + // in https://www.rfc-editor.org/rfc/rfc9000#section-22.3 + + #[cfg(feature = "acktimestamps")] + if let Some(x) = self.max_recv_timestamps_per_ack { + w.write_var(0x00f0); + w.write_var(x.size() as u64); + w.write(x); + } + + #[cfg(feature = "acktimestamps")] + if let Some(x) = self.receive_timestamps_exponent { + w.write_var(0x00f1); + w.write_var(x.size() as u64); + w.write(x); + } } /// Decode `TransportParameters` from buffer @@ -413,6 +455,22 @@ impl TransportParameters { _ => return Err(Error::Malformed), }, 0xff04de1b => params.min_ack_delay = Some(r.get().unwrap()), + + #[cfg(feature = "acktimestamps")] + 0x00f0 => { + if len > 8 || params.max_recv_timestamps_per_ack.is_some() { + return Err(Error::Malformed); + } + params.max_recv_timestamps_per_ack = Some(r.get().unwrap()); + } + #[cfg(feature = "acktimestamps")] + 0x00f1 => { + if len > 8 || params.receive_timestamps_exponent.is_some() { + return Err(Error::Malformed); + } + params.receive_timestamps_exponent = Some(r.get().unwrap()); + } + _ => { macro_rules! parse { {$($(#[$doc:meta])* $name:ident ($code:expr) = $default:expr,)*} => { @@ -464,6 +522,15 @@ impl TransportParameters { return Err(Error::IllegalValue); } + // https://www.ietf.org/archive/id/draft-smith-quic-receive-ts-00.html#name-extension-negotiation + #[cfg(feature = "acktimestamps")] + if params + .receive_timestamps_exponent + .map_or(false, |x| x.0 > 20) + { + return Err(Error::IllegalValue); + } + Ok(params) } } @@ -499,6 +566,12 @@ mod test { }), grease_quic_bit: true, min_ack_delay: Some(2_000u32.into()), + + #[cfg(feature = "acktimestamps")] + receive_timestamps_exponent: Some(3u32.into()), + #[cfg(feature = "acktimestamps")] + max_recv_timestamps_per_ack: Some(5u32.into()), + ..TransportParameters::default() }; params.write(&mut buf); From cb6d5c4fd12d7e0f161d45f48b4dda4be8d40c5c Mon Sep 17 00:00:00 2001 From: Sterling Deng Date: Thu, 19 Sep 2024 11:08:52 -0700 Subject: [PATCH 02/30] Implement AckTimestampsConfig Surfaces the TransportParameters to the application --- quinn-proto/src/config.rs | 74 ++++++++++++++++++++++++++++++++++++--- quinn-proto/src/lib.rs | 2 ++ 2 files changed, 72 insertions(+), 4 deletions(-) diff --git a/quinn-proto/src/config.rs b/quinn-proto/src/config.rs index d4c8e6385..a25384d40 100644 --- a/quinn-proto/src/config.rs +++ b/quinn-proto/src/config.rs @@ -53,6 +53,9 @@ pub struct TransportConfig { pub(crate) mtu_discovery_config: Option, pub(crate) ack_frequency_config: Option, + #[cfg(feature = "acktimestamps")] + pub(crate) ack_timestamp_config: Option, + pub(crate) persistent_congestion_threshold: u32, pub(crate) keep_alive_interval: Option, pub(crate) crypto_buffer_size: usize, @@ -223,6 +226,16 @@ impl TransportConfig { self } + /// Specifies the ACK timestamp config. + /// Defaults to `None`, which disables receiving acknowledgement timestamps from the sender. + /// If `Some`, TransportParameters are sent to the peer to enable acknowledgement timestamps + /// if supported. + #[cfg(feature = "acktimestamps")] + pub fn ack_timestamp_config(&mut self, value: Option) -> &mut Self { + self.ack_timestamp_config = value; + self + } + /// Number of consecutive PTOs after which network is considered to be experiencing persistent congestion. pub fn persistent_congestion_threshold(&mut self, value: u32) -> &mut Self { self.persistent_congestion_threshold = value; @@ -360,6 +373,9 @@ impl Default for TransportConfig { congestion_controller_factory: Arc::new(congestion::CubicConfig::default()), enable_segmentation_offload: true, + + #[cfg(feature = "acktimestamps")] + ack_timestamp_config: None, } } } @@ -390,9 +406,12 @@ impl fmt::Debug for TransportConfig { deterministic_packet_numbers: _, congestion_controller_factory: _, enable_segmentation_offload, + #[cfg(feature = "acktimestamps")] + ack_timestamp_config, } = self; - fmt.debug_struct("TransportConfig") - .field("max_concurrent_bidi_streams", max_concurrent_bidi_streams) + let mut s = fmt.debug_struct("TransportConfig"); + + s.field("max_concurrent_bidi_streams", max_concurrent_bidi_streams) .field("max_concurrent_uni_streams", max_concurrent_uni_streams) .field("max_idle_timeout", max_idle_timeout) .field("stream_receive_window", stream_receive_window) @@ -415,8 +434,55 @@ impl fmt::Debug for TransportConfig { .field("datagram_receive_buffer_size", datagram_receive_buffer_size) .field("datagram_send_buffer_size", datagram_send_buffer_size) .field("congestion_controller_factory", &"[ opaque ]") - .field("enable_segmentation_offload", enable_segmentation_offload) - .finish() + .field("enable_segmentation_offload", enable_segmentation_offload); + + #[cfg(feature = "acktimestamps")] + s.field("ack_timestamp_config", ack_timestamp_config); + + s.finish() + } +} + +/// Parameters for controlling the peer's acknowledgements with receiver timestamps. +#[cfg(feature = "acktimestamps")] +#[derive(Clone, Debug)] +pub struct AckTimestampsConfig { + pub(crate) max_timestamps_per_ack: VarInt, + pub(crate) exponent: VarInt, + pub(crate) basis: std::time::Instant, +} + +#[cfg(feature = "acktimestamps")] +impl AckTimestampsConfig { + /// Sets the maximum number of timestamp entries per ACK frame. + pub fn max_timestamps_per_ack(&mut self, value: VarInt) -> &mut Self { + self.max_timestamps_per_ack = value; + self + } + + /// Timestamp values are divided by the exponent value provided. This reduces the size of the + /// VARINT for loss in precision. A exponent of 0 represents microsecond precision. + pub fn exponent(&mut self, value: VarInt) -> &mut Self { + self.exponent = value; + self + } + + /// Sets the time base for which all timestamps are anchored on. + /// Defaults to Instant::now of when the default struct was created. + pub fn basis(&mut self, instant: std::time::Instant) -> &mut Self { + self.basis = instant; + self + } +} + +#[cfg(feature = "acktimestamps")] +impl Default for AckTimestampsConfig { + fn default() -> Self { + Self { + max_timestamps_per_ack: 10u32.into(), + exponent: 0u32.into(), + basis: std::time::Instant::now(), + } } } diff --git a/quinn-proto/src/lib.rs b/quinn-proto/src/lib.rs index bb39101b7..a3443a986 100644 --- a/quinn-proto/src/lib.rs +++ b/quinn-proto/src/lib.rs @@ -48,6 +48,8 @@ pub use crate::connection::{ }; mod config; +#[cfg(feature = "acktimestamps")] +pub use config::AckTimestampsConfig; pub use config::{ AckFrequencyConfig, ClientConfig, ConfigError, EndpointConfig, IdleTimeout, MtuDiscoveryConfig, ServerConfig, TransportConfig, From 8dc45793cdf8e030b27e6beec1648a1f6b5b48c8 Mon Sep 17 00:00:00 2001 From: Sterling Deng Date: Thu, 19 Sep 2024 11:15:21 -0700 Subject: [PATCH 03/30] Implement Ack Timestamps Frame --- quinn-proto/src/ack_timestamp_frame.rs | 436 +++++++++++++++++++++++++ quinn-proto/src/connection/stats.rs | 2 + quinn-proto/src/frame.rs | 178 +++++++++- quinn-proto/src/lib.rs | 3 + 4 files changed, 611 insertions(+), 8 deletions(-) create mode 100644 quinn-proto/src/ack_timestamp_frame.rs diff --git a/quinn-proto/src/ack_timestamp_frame.rs b/quinn-proto/src/ack_timestamp_frame.rs new file mode 100644 index 000000000..24fadbeb8 --- /dev/null +++ b/quinn-proto/src/ack_timestamp_frame.rs @@ -0,0 +1,436 @@ +use std::{ + fmt::{self, Write}, + ops::RangeInclusive, + time::{Duration, Instant}, +}; + +use crate::{ + coding::{BufExt, BufMutExt}, + connection::{PacketTimestamp, ReceiverTimestamps}, + frame::{AckIter, Type}, + range_set::ArrayRangeSet, +}; + +use bytes::{Buf, BufMut, Bytes}; + +#[derive(Clone, Eq, PartialEq)] +pub(crate) struct AckTimestampFrame { + pub largest: u64, + pub delay: u64, + pub ranges: Bytes, + pub timestamps: Bytes, +} + +impl AckTimestampFrame { + pub(crate) fn encode( + delay: u64, + ranges: &ArrayRangeSet, + timestamps: &ReceiverTimestamps, + timestamp_basis: Instant, + timestamp_exponent: u64, + max_timestamps: u64, + buf: &mut W, + ) { + let mut rest = ranges.iter().rev(); + let first = rest.next().unwrap(); + let largest = first.end - 1; + let first_size = first.end - first.start; + buf.write(Type::ACK_RECEIVE_TIMESTAMPS); + buf.write_var(largest); + buf.write_var(delay); + buf.write_var(ranges.len() as u64 - 1); + buf.write_var(first_size - 1); + let mut prev = first.start; + for block in rest { + let size = block.end - block.start; + buf.write_var(prev - block.end - 1); + buf.write_var(size - 1); + prev = block.start; + } + + Self::encode_timestamps( + timestamps, + largest, + buf, + timestamp_basis, + timestamp_exponent, + max_timestamps, + ); + } + + // https://www.ietf.org/archive/id/draft-smith-quic-receive-ts-00.html#ts-ranges + fn encode_timestamps( + timestamps: &ReceiverTimestamps, + mut largest: u64, + buf: &mut W, + mut basis: Instant, + exponent: u64, + max_timestamps: u64, + ) { + if timestamps.len() == 0 { + buf.write_var(0); + return; + } + let mut prev: Option = None; + + // segment_idx tracks the positions in `timestamps` in which a gap occurs. + let mut segment_idxs = Vec::::new(); + // iterates from largest number to smallest + for (i, pkt) in timestamps.iter().rev().enumerate() { + if let Some(prev) = prev { + if pkt.packet_number + 1 != prev { + segment_idxs.push(timestamps.len() - i); + } + } + prev = Some(pkt.packet_number); + } + segment_idxs.push(0); + // Timestamp Range Count + buf.write_var(segment_idxs.len() as u64); + + let mut right = timestamps.len(); + let mut first = true; + let mut counter = 0; + + for segment_idx in segment_idxs { + let Some(elt) = timestamps.inner().get(right - 1) else { + debug_assert!( + false, + "an invalid indexing occurred on the ReceiverTimestamps vector" + ); + break; + }; + // *Gap + // For the first Timestamp Range: Gap is the difference between (a) the Largest Acknowledged packet number + // in the frame and (b) the largest packet in the current (first) Timestamp Range. + let gap = if first { + debug_assert!( + elt.packet_number <= largest, + "largest packet number is less than what was found in timestamp vec" + ); + largest - elt.packet_number + } else { + // For subsequent Timestamp Ranges: Gap is the difference between (a) the packet number two lower + // than the smallest packet number in the previous Timestamp Range + // and (b) the largest packet in the current Timestamp Range. + largest - 2 - elt.packet_number + }; + buf.write_var(gap); + // *Timestamp Delta Count + buf.write_var((right - segment_idx) as u64); + // *Timestamp Deltas + for pkt in timestamps.inner().range(segment_idx..right).rev() { + let delta: u64 = if first { + first = false; + // For the first Timestamp Delta of the first Timestamp Range in the frame: the value + // is the difference between (a) the receive timestamp of the largest packet in the + // Timestamp Range (indicated by Gap) and (b) the session receive_timestamp_basis + pkt.timestamp.duration_since(basis).as_micros() as u64 + } else { + // For all other Timestamp Deltas: the value is the difference between + // (a) the receive timestamp specified by the previous Timestamp Delta and + // (b) the receive timestamp of the current packet in the Timestamp Range, decoded as described below. + basis.duration_since(pkt.timestamp).as_micros() as u64 + }; + buf.write_var(delta >> exponent); + basis = pkt.timestamp; + largest = pkt.packet_number; + counter += 1; + } + + right = segment_idx; + } + + debug_assert!( + counter <= max_timestamps, + "the number of timestamps in an ack frame exceeded the max allowed" + ); + } + + /// timestamp_iter returns an iterator that reads the timestamp records from newest to oldest + /// (or highest packet number to smallest). + pub(crate) fn timestamp_iter(&self, basis: Instant, exponent: u64) -> AckTimestampDecoder { + AckTimestampDecoder::new(self.largest, basis, exponent, &self.timestamps[..]) + } + + pub(crate) fn iter(&self) -> AckIter<'_> { + self.into_iter() + } +} + +impl<'a> IntoIterator for &'a AckTimestampFrame { + type Item = RangeInclusive; + type IntoIter = AckIter<'a>; + + fn into_iter(self) -> AckIter<'a> { + AckIter::new(self.largest, &self.ranges[..]) + } +} + +impl fmt::Debug for AckTimestampFrame { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let mut ranges = "[".to_string(); + let mut first = true; + for range in self.iter() { + if !first { + ranges.push(','); + } + write!(ranges, "{range:?}").unwrap(); + first = false; + } + ranges.push(']'); + + let timestamp_count = self.timestamp_iter(Instant::now(), 0).count(); + + f.debug_struct("AckTimestamp") + .field("largest", &self.largest) + .field("delay", &self.delay) + .field("ranges", &ranges) + .field("timestamps_count", ×tamp_count) + .finish() + } +} + +pub(crate) struct AckTimestampDecoder<'a> { + timestamp_basis: u64, + timestamp_exponent: u64, + timestamp_instant_basis: Instant, + data: &'a [u8], + + deltas_remaining: usize, + first: bool, + next_pn: u64, +} + +impl<'a> AckTimestampDecoder<'a> { + fn new(largest: u64, basis_instant: Instant, exponent: u64, mut data: &'a [u8]) -> Self { + // We read and throw away the Timestamp Range Count value because + // it was already used to properly slice the data. + let _ = data.get_var().unwrap(); + AckTimestampDecoder { + timestamp_basis: 0, + timestamp_exponent: exponent, + timestamp_instant_basis: basis_instant, + data, + deltas_remaining: 0, + first: true, + next_pn: largest, + } + } +} + +impl<'a> Iterator for AckTimestampDecoder<'a> { + type Item = PacketTimestamp; + fn next(&mut self) -> Option { + if !self.data.has_remaining() { + debug_assert!( + self.deltas_remaining == 0, + "timestamp delta remaining should be 0" + ); + return None; + } + if self.deltas_remaining == 0 { + let gap = self.data.get_var().unwrap(); + self.deltas_remaining = self.data.get_var().unwrap() as usize; + if self.first { + self.next_pn -= gap; + } else { + self.next_pn -= gap + 2; + } + } else { + self.next_pn -= 1; + } + + let delta = self.data.get_var().unwrap(); + self.deltas_remaining -= 1; + + if self.first { + self.timestamp_basis = delta << self.timestamp_exponent; + self.first = false; + } else { + self.timestamp_basis -= delta << self.timestamp_exponent; + } + + Some(PacketTimestamp { + packet_number: self.next_pn, + timestamp: self.timestamp_instant_basis + Duration::from_micros(self.timestamp_basis), + }) + } +} + +#[cfg(test)] +mod test { + use super::*; + + #[test] + fn timestamp_iter() { + let mut timestamps = ReceiverTimestamps::new(100); + let second = Duration::from_secs(1); + let t0 = Instant::now(); + timestamps.add(1, t0 + second); + timestamps.add(2, t0 + second * 2); + timestamps.add(3, t0 + second * 3); + let mut buf = bytes::BytesMut::new(); + + AckTimestampFrame::encode_timestamps(×tamps, 12, &mut buf, t0, 0, 10); + + // Manually decode and assert the values in the buffer. + assert_eq!(1, buf.get_var().unwrap()); // timestamp_range_count + assert_eq!(9, buf.get_var().unwrap()); // gap: 12-3 + assert_eq!(3, buf.get_var().unwrap()); // timestamp delta count + assert_eq!(3_000_000, buf.get_var().unwrap()); // timestamp delta: 3_000_000 μs = 3 seconds = diff between largest timestamp and basis + assert_eq!(1_000_000, buf.get_var().unwrap()); // timestamp delta: 1 second diff + assert_eq!(1_000_000, buf.get_var().unwrap()); // timestamp delta: 1 second diff + assert!(buf.get_var().is_err()); + } + + #[test] + fn timestamp_iter_with_gaps() { + let mut timestamps = ReceiverTimestamps::new(100); + let one_second = Duration::from_secs(1); + let t0 = Instant::now(); + vec![(1..=3), (5..=5), (10..=12)] + .into_iter() + .flatten() + .for_each(|i| timestamps.add(i, t0 + one_second * i as u32)); + + let mut buf = bytes::BytesMut::new(); + + AckTimestampFrame::encode_timestamps(×tamps, 12, &mut buf, t0, 0, 10); + // Manually decode and assert the values in the buffer. + assert_eq!(3, buf.get_var().unwrap()); // timestamp_range_count + // + assert_eq!(0, buf.get_var().unwrap()); // gap: 12 - 12 = 0 + assert_eq!(3, buf.get_var().unwrap()); // timestamp_delta_count + assert_eq!(12_000_000, buf.get_var().unwrap()); // delta: 3_000_000 μs = 3 seconds = diff between largest timestamp and basis + assert_eq!(1_000_000, buf.get_var().unwrap()); // delta: 1 second diff + assert_eq!(1_000_000, buf.get_var().unwrap()); // delta: 1 second diff + // + assert_eq!(3, buf.get_var().unwrap()); // gap: 10 - 2 - 5 = 3 + assert_eq!(1, buf.get_var().unwrap()); // timestamp_delta_count + assert_eq!(5_000_000, buf.get_var().unwrap()); // delta: 1 second diff + + assert_eq!(0, buf.get_var().unwrap()); // gap + assert_eq!(3, buf.get_var().unwrap()); // timestamp_delta_count + assert_eq!(2_000_000, buf.get_var().unwrap()); // delta: 2 second diff + assert_eq!(1_000_000, buf.get_var().unwrap()); // delta: 1 second diff + assert_eq!(1_000_000, buf.get_var().unwrap()); // delta: 1 second diff + + // end + assert!(buf.get_var().is_err()); + } + + #[test] + fn timestamp_iter_with_exponent() { + let mut timestamps = ReceiverTimestamps::new(100); + let millisecond = Duration::from_millis(1); + let t0 = Instant::now(); + timestamps.add(1, t0 + millisecond * 200); + timestamps.add(2, t0 + millisecond * 300); + let mut buf = bytes::BytesMut::new(); + + let exponent = 2; + AckTimestampFrame::encode_timestamps(×tamps, 12, &mut buf, t0, exponent, 10); + + // values below are tested in another unit test + buf.get_var().unwrap(); // timestamp_range_count + buf.get_var().unwrap(); // gap + buf.get_var().unwrap(); // timestamp_delta_count + assert_eq!(300_000 >> exponent, buf.get_var().unwrap()); // 300ms diff + assert_eq!(100_000 >> exponent, buf.get_var().unwrap()); // 100ms diff + assert!(buf.get_var().is_err()); + } + + #[test] + fn timestamp_encode_decode() { + let mut timestamps = ReceiverTimestamps::new(100); + let one_second = Duration::from_secs(1); + let t0 = Instant::now(); + timestamps.add(1, t0 + one_second); + timestamps.add(2, t0 + one_second * 2); + timestamps.add(3, t0 + one_second * 3); + + let mut buf = bytes::BytesMut::new(); + + AckTimestampFrame::encode_timestamps(×tamps, 12, &mut buf, t0, 0, 10); + + let decoder = AckTimestampDecoder::new(12, t0, 0, &buf); + + let got: Vec<_> = decoder.collect(); + // [(3, _), (2, _), (1, _)] + assert_eq!(3, got.len()); + assert_eq!(3, got[0].packet_number); + assert_eq!(t0 + (3 * one_second), got[0].timestamp,); + + assert_eq!(2, got[1].packet_number); + assert_eq!(t0 + (2 * one_second), got[1].timestamp,); + + assert_eq!(1, got[2].packet_number); + assert_eq!(t0 + (1 * one_second), got[2].timestamp); + } + + #[test] + fn timestamp_encode_decode_with_gaps() { + let mut timestamps = ReceiverTimestamps::new(100); + let one_second = Duration::from_secs(1); + let t0 = Instant::now(); + let expect: Vec<_> = vec![(1..=3), (5..=5), (10..=12)] + .into_iter() + .flatten() + .collect::>() + .into_iter() + .map(|i| { + let t = t0 + one_second * i as u32; + timestamps.add(i, t); + PacketTimestamp { + packet_number: i, + timestamp: t, + } + }) + .collect(); + + let mut buf = bytes::BytesMut::new(); + + AckTimestampFrame::encode_timestamps(×tamps, 12, &mut buf, t0, 0, 10); + + let decoder = AckTimestampDecoder::new(12, t0, 0, &buf); + let got: Vec<_> = decoder.collect(); + + assert_eq!(7, got.len()); + assert_eq!(expect, got.into_iter().rev().collect::>()); + } + + #[test] + fn timestamp_encode_max_ack() { + // fix this + let mut timestamps = ReceiverTimestamps::new(2); + let one_second = Duration::from_secs(1); + let t0 = Instant::now(); + let expect: Vec<_> = vec![(1..=3), (5..=5), (10..=12)] + .into_iter() + .flatten() + .collect::>() + .into_iter() + .map(|i| { + let t = t0 + one_second * i as u32; + timestamps.add(i, t); + PacketTimestamp { + packet_number: i, + timestamp: t, + } + }) + .collect(); + + let mut buf = bytes::BytesMut::new(); + + AckTimestampFrame::encode_timestamps(×tamps, 12, &mut buf, t0, 0, 2); + let decoder = AckTimestampDecoder::new(12, t0, 0, &buf); + let got: Vec<_> = decoder.collect(); + + assert_eq!(2, got.len()); + assert_eq!( + expect[expect.len() - 2..expect.len()], // the last 2 values + got.into_iter().rev().collect::>() + ); + } +} diff --git a/quinn-proto/src/connection/stats.rs b/quinn-proto/src/connection/stats.rs index fc19c6304..189ccf7f3 100644 --- a/quinn-proto/src/connection/stats.rs +++ b/quinn-proto/src/connection/stats.rs @@ -62,6 +62,8 @@ impl FrameStats { Frame::Padding => {} Frame::Ping => self.ping += 1, Frame::Ack(_) => self.acks += 1, + #[cfg(feature = "acktimestamps")] + Frame::AckTimestamps(_) => self.acks += 1, Frame::ResetStream(_) => self.reset_stream += 1, Frame::StopSending(_) => self.stop_sending += 1, Frame::Crypto(_) => self.crypto += 1, diff --git a/quinn-proto/src/frame.rs b/quinn-proto/src/frame.rs index d58cb36e5..32a2eef13 100644 --- a/quinn-proto/src/frame.rs +++ b/quinn-proto/src/frame.rs @@ -15,6 +15,9 @@ use crate::{ RESET_TOKEN_SIZE, }; +#[cfg(feature = "acktimestamps")] +use crate::ack_timestamp_frame; + #[cfg(feature = "arbitrary")] use arbitrary::Arbitrary; @@ -133,6 +136,9 @@ frame_types! { ACK_FREQUENCY = 0xaf, IMMEDIATE_ACK = 0x1f, // DATAGRAM + // Custom frame for https://www.ietf.org/archive/id/draft-smith-quic-receive-ts-00.html + // can't cfg here in macro #[cfg(feature = "acktimestamps")] + ACK_RECEIVE_TIMESTAMPS = 0x40, } const STREAM_TYS: RangeInclusive = RangeInclusive::new(0x08, 0x0f); @@ -143,19 +149,39 @@ pub(crate) enum Frame { Padding, Ping, Ack(Ack), + #[cfg(feature = "acktimestamps")] + AckTimestamps(ack_timestamp_frame::AckTimestampFrame), ResetStream(ResetStream), StopSending(StopSending), Crypto(Crypto), - NewToken { token: Bytes }, + NewToken { + token: Bytes, + }, Stream(Stream), MaxData(VarInt), - MaxStreamData { id: StreamId, offset: u64 }, - MaxStreams { dir: Dir, count: u64 }, - DataBlocked { offset: u64 }, - StreamDataBlocked { id: StreamId, offset: u64 }, - StreamsBlocked { dir: Dir, limit: u64 }, + MaxStreamData { + id: StreamId, + offset: u64, + }, + MaxStreams { + dir: Dir, + count: u64, + }, + DataBlocked { + offset: u64, + }, + StreamDataBlocked { + id: StreamId, + offset: u64, + }, + StreamsBlocked { + dir: Dir, + limit: u64, + }, NewConnectionId(NewConnectionId), - RetireConnectionId { sequence: u64 }, + RetireConnectionId { + sequence: u64, + }, PathChallenge(u64), PathResponse(u64), Close(Close), @@ -185,6 +211,8 @@ impl Frame { StopSending { .. } => Type::STOP_SENDING, RetireConnectionId { .. } => Type::RETIRE_CONNECTION_ID, Ack(_) => Type::ACK, + #[cfg(feature = "acktimestamps")] + AckTimestamps(_) => Type::ACK_RECEIVE_TIMESTAMPS, Stream(ref x) => { let mut ty = *STREAM_TYS.start(); if x.fin { @@ -641,6 +669,27 @@ impl Iter { }, }) } + #[cfg(feature = "acktimestamps")] + Type::ACK_RECEIVE_TIMESTAMPS => { + let largest = self.bytes.get_var()?; + let delay = self.bytes.get_var()?; + let range_count = self.bytes.get_var()? as usize; + let start = self.bytes.position() as usize; + scan_ack_blocks(&mut self.bytes, largest, range_count)?; + let end = self.bytes.position() as usize; + Frame::AckTimestamps(ack_timestamp_frame::AckTimestampFrame { + delay, + largest, + ranges: self.bytes.get_ref().slice(start..end), + timestamps: { + let ts_start = end; + let ts_range_count = self.bytes.get_var()?; + scan_ack_timestamp_blocks(&mut self.bytes, largest, ts_range_count)?; + let ts_end = self.bytes.position() as usize; + self.bytes.get_ref().slice(ts_start..ts_end) + }, + }) + } Type::PATH_CHALLENGE => Frame::PathChallenge(self.bytes.get()?), Type::PATH_RESPONSE => Frame::PathResponse(self.bytes.get()?), Type::NEW_CONNECTION_ID => { @@ -767,12 +816,46 @@ fn scan_ack_blocks(buf: &mut io::Cursor, largest: u64, n: usize) -> Resul Ok(()) } +#[cfg(feature = "acktimestamps")] +fn scan_ack_timestamp_blocks( + buf: &mut io::Cursor, + largest: u64, + range_count: u64, +) -> Result<(), IterErr> { + let mut next = largest; + let mut first = true; + for _ in 0..range_count { + let gap = buf.get_var()?; + next = if first { + first = false; + next.checked_sub(gap).ok_or(IterErr::Malformed)? + } else { + next.checked_sub(gap + 2).ok_or(IterErr::Malformed)? + }; + let timestamp_delta_count = buf.get_var()?; + next = next + .checked_sub(timestamp_delta_count - 1) + .ok_or(IterErr::Malformed)?; + for _ in 0..timestamp_delta_count { + buf.get_var()?; + } + } + Ok(()) +} + +#[derive(PartialEq, Eq)] enum IterErr { UnexpectedEnd, InvalidFrameId, Malformed, } +impl std::fmt::Debug for IterErr { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str(self.reason()) + } +} + impl IterErr { fn reason(&self) -> &'static str { use self::IterErr::*; @@ -797,7 +880,7 @@ pub struct AckIter<'a> { } impl<'a> AckIter<'a> { - fn new(largest: u64, payload: &'a [u8]) -> Self { + pub(crate) fn new(largest: u64, payload: &'a [u8]) -> Self { let data = io::Cursor::new(payload); Self { largest, data } } @@ -995,4 +1078,83 @@ mod test { assert_eq!(frames.len(), 1); assert_matches!(&frames[0], Frame::ImmediateAck); } + + #[cfg(test)] + #[cfg(feature = "acktimestamps")] + mod ack_timestamp_tests { + use super::*; + + #[test] + fn test_scan_ack_timestamp_block() { + let mut buf = bytes::BytesMut::new(); + buf.write_var(0); // gap + buf.write_var(3); // delta count + buf.write_var(1); // delta + buf.write_var(2); // delta + buf.write_var(3); // delta + + let buf_len = buf.len() as u64; + let mut c = io::Cursor::new(buf.freeze()); + scan_ack_timestamp_blocks(&mut c, 3, 1).unwrap(); + assert_eq!(buf_len, c.position()); + } + + #[test] + fn test_scan_ack_timestamp_block_with_gap() { + let mut buf = bytes::BytesMut::new(); + buf.write_var(1); // gap + buf.write_var(3); // delta count + buf.write_var(1); // pn 9 + buf.write_var(2); // pn 8 + buf.write_var(3); // pn 7 + + buf.write_var(1); // gap + buf.write_var(6); // delta count + buf.write_var(1); // pn 4 + buf.write_var(2); // pn 3 + buf.write_var(3); // pn 2 + buf.write_var(3); // pn 1 + buf.write_var(3); // pn 0 + + let buf_len = buf.len() as u64; + let mut c = io::Cursor::new(buf.freeze()); + scan_ack_timestamp_blocks(&mut c, 10, 2).unwrap(); + assert_eq!(buf_len, c.position()); + } + + #[test] + fn test_scan_ack_timestamp_block_with_gap_malforned() { + let mut buf = bytes::BytesMut::new(); + buf.write_var(1); // gap + buf.write_var(3); // delta count + buf.write_var(1); // pn 9 + buf.write_var(2); // pn 8 + buf.write_var(3); // pn 7 + + buf.write_var(2); // gap + buf.write_var(6); // delta count + buf.write_var(1); // pn 3 + buf.write_var(2); // pn 2 + buf.write_var(3); // pn 1 + buf.write_var(3); // pn 0 + buf.write_var(3); // pn -1 this will cause an error + + let mut c = io::Cursor::new(buf.freeze()); + assert!(scan_ack_timestamp_blocks(&mut c, 10, 2).is_err()); + } + + #[test] + fn test_scan_ack_timestamp_block_malformed() { + let mut buf = bytes::BytesMut::new(); + buf.write_var(5); // gap + buf.write_var(1); // delta count + buf.write_var(1); // packet number 0 + + let mut c = io::Cursor::new(buf.freeze()); + assert_eq!( + IterErr::Malformed, + scan_ack_timestamp_blocks(&mut c, 5, 2).unwrap_err(), + ); + } + } } diff --git a/quinn-proto/src/lib.rs b/quinn-proto/src/lib.rs index a3443a986..fd9cd0bfb 100644 --- a/quinn-proto/src/lib.rs +++ b/quinn-proto/src/lib.rs @@ -88,6 +88,9 @@ pub use crate::cid_generator::{ mod token; use token::{ResetToken, RetryToken}; +#[cfg(feature = "acktimestamps")] +mod ack_timestamp_frame; + #[cfg(feature = "arbitrary")] use arbitrary::Arbitrary; From d83b6848206f92da69fe10c0088528ce7cc4f649 Mon Sep 17 00:00:00 2001 From: Sterling Deng Date: Thu, 19 Sep 2024 11:17:50 -0700 Subject: [PATCH 04/30] Implement ReceiverTimestamp data structure --- .../src/connection/receiver_timestamps.rs | 135 ++++++++++++++++++ 1 file changed, 135 insertions(+) create mode 100644 quinn-proto/src/connection/receiver_timestamps.rs diff --git a/quinn-proto/src/connection/receiver_timestamps.rs b/quinn-proto/src/connection/receiver_timestamps.rs new file mode 100644 index 000000000..9991ea539 --- /dev/null +++ b/quinn-proto/src/connection/receiver_timestamps.rs @@ -0,0 +1,135 @@ +use std::collections::VecDeque; +use std::time::Instant; + +use tracing::warn; + +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct PacketTimestamp { + pub packet_number: u64, + pub timestamp: Instant, +} + +impl Default for PacketTimestamp { + fn default() -> Self { + PacketTimestamp { + packet_number: 0, + timestamp: Instant::now(), + } + } +} + +pub(crate) struct ReceiverTimestamps { + data: VecDeque, + max: usize, +} + +impl std::fmt::Debug for ReceiverTimestamps { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let mut l = f.debug_list(); + let mut last: Option<(u64, Instant)> = None; + for curr in self.data.iter() { + if let Some(last) = last.take() { + let s = format!( + "{}..{} diff_micros: {}", + last.0, + curr.packet_number, + curr.timestamp.duration_since(last.1).as_micros(), + ); + l.entry(&s); + } + let _ = last.insert((curr.packet_number, curr.timestamp)); + } + l.finish() + } +} + +impl ReceiverTimestamps { + pub(crate) fn new(max: usize) -> Self { + ReceiverTimestamps { + data: VecDeque::with_capacity(max), + max, + } + } + + pub(crate) fn add(&mut self, packet_number: u64, timestamp: Instant) { + if self.data.len() == self.max { + self.data.pop_front(); + } + if let Some(v) = self.data.back() { + if packet_number <= v.packet_number { + warn!("out of order packets are not supported"); + return; + } + } + self.data.push_back(PacketTimestamp { + packet_number, + timestamp, + }); + } + + fn clear(&mut self) { + self.data.clear() + } + + pub(crate) fn iter(&self) -> impl DoubleEndedIterator + '_ { + self.data.iter().cloned() + } + + pub(crate) fn len(&self) -> usize { + self.data.len() + } + + pub(crate) fn inner(&self) -> &VecDeque { + &self.data + } + + pub(crate) fn subtract_below(&mut self, packet_number: u64) { + if self.data.is_empty() { + return; + } + let idx = self + .data + .partition_point(|v| v.packet_number < packet_number); + if idx == self.data.len() { + self.clear(); + } else { + let _ = self.data.drain(0..=idx); + } + } +} + +#[cfg(test)] +mod receiver_timestamp_tests { + use super::*; + + #[test] + fn subtract_below() { + let mut ts = ReceiverTimestamps::new(10); + let _ = ts.add(1, Instant::now()); + let _ = ts.add(2, Instant::now()); + let _ = ts.add(3, Instant::now()); + let _ = ts.add(4, Instant::now()); + ts.subtract_below(3); + assert_eq!(1, ts.len()); + } + + #[test] + fn subtract_below_everything() { + let mut ts = ReceiverTimestamps::new(10); + let _ = ts.add(5, Instant::now()); + ts.subtract_below(10); + assert_eq!(0, ts.len()); + } + + #[test] + fn receiver_timestamp_max() { + let mut ts = ReceiverTimestamps::new(2); + let _ = ts.add(1, Instant::now()); + let _ = ts.add(2, Instant::now()); + let _ = ts.add(3, Instant::now()); + let _ = ts.add(4, Instant::now()); + assert_eq!(2, ts.len()); + assert_eq!(3, ts.data.front().unwrap().packet_number); + assert_eq!(4, ts.data.back().unwrap().packet_number); + } +} From 3366153fcd3f5d975bb7335c835af80025742f6f Mon Sep 17 00:00:00 2001 From: Sterling Deng Date: Thu, 19 Sep 2024 11:18:24 -0700 Subject: [PATCH 05/30] small cleanup --- quinn-proto/src/packet.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/quinn-proto/src/packet.rs b/quinn-proto/src/packet.rs index ee1babcc7..3c32e094e 100644 --- a/quinn-proto/src/packet.rs +++ b/quinn-proto/src/packet.rs @@ -969,7 +969,6 @@ mod tests { for byte in &buf { print!("{byte:02x}"); } - println!(); assert_eq!( buf[..], hex!( From 94cb098ca2506410af6b71904a5893ab63f6863a Mon Sep 17 00:00:00 2001 From: Sterling Deng Date: Thu, 19 Sep 2024 11:21:18 -0700 Subject: [PATCH 06/30] Process TransportParameters from peer if available --- quinn-proto/src/connection/mod.rs | 41 +++++++++++++++++++++++++------ 1 file changed, 33 insertions(+), 8 deletions(-) diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index 2c0f92b55..915a21156 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -1911,7 +1911,7 @@ impl Connection { Ok(()) } - fn init_0rtt(&mut self) { + fn init_0rtt(&mut self, now: Instant) { let (header, packet) = match self.crypto.early_crypto() { Some(x) => x, None => return, @@ -1933,7 +1933,7 @@ impl Connection { max_ack_delay: TransportParameters::default().max_ack_delay, ..params }; - self.set_peer_params(params); + self.set_peer_params(params, now); } Err(e) => { error!("session ticket has malformed transport parameters: {}", e); @@ -2452,7 +2452,7 @@ impl Connection { self.endpoint_events .push_back(EndpointEventInner::ResetToken(self.path.remote, token)); } - self.handle_peer_params(params)?; + self.handle_peer_params(params, now)?; self.issue_first_cids(now); } else { // Server-only @@ -2499,9 +2499,9 @@ impl Connection { frame: None, reason: "transport parameters missing".into(), })?; - self.handle_peer_params(params)?; + self.handle_peer_params(params, now)?; self.issue_first_cids(now); - self.init_0rtt(); + self.init_0rtt(now); } Ok(()) } @@ -3272,7 +3272,11 @@ impl Connection { } /// Handle transport parameters received from the peer - fn handle_peer_params(&mut self, params: TransportParameters) -> Result<(), TransportError> { + fn handle_peer_params( + &mut self, + params: TransportParameters, + now: Instant, + ) -> Result<(), TransportError> { if Some(self.orig_rem_cid) != params.initial_src_cid || (self.side.is_client() && (Some(self.initial_dst_cid) != params.original_dst_cid @@ -3283,12 +3287,12 @@ impl Connection { )); } - self.set_peer_params(params); + self.set_peer_params(params, now); Ok(()) } - fn set_peer_params(&mut self, params: TransportParameters) { + fn set_peer_params(&mut self, params: TransportParameters, now: Instant) { self.streams.set_params(¶ms); self.idle_timeout = match (self.config.max_idle_timeout, params.max_idle_timeout) { (None, VarInt(0)) => None, @@ -3309,6 +3313,27 @@ impl Connection { self.path.mtud.on_peer_max_udp_payload_size_received( u16::try_from(self.peer_params.max_udp_payload_size.into_inner()).unwrap_or(u16::MAX), ); + + #[cfg(feature = "acktimestamps")] + { + self.peer_ack_timestamp_cfg = if let (Some(max_timestamps_per_ack), Some(exponent)) = ( + params.max_recv_timestamps_per_ack, + params.receive_timestamps_exponent, + ) { + for space in self.spaces.iter_mut() { + space + .pending_acks + .set_receiver_timestamp(max_timestamps_per_ack.0 as usize); + } + Some(AckTimestampsConfig { + exponent, + max_timestamps_per_ack, + basis: now, + }) + } else { + None + }; + } } fn decrypt_packet( From 76ddf51523a214de9a15231e40580cb3ea098edc Mon Sep 17 00:00:00 2001 From: Sterling Deng Date: Thu, 19 Sep 2024 11:22:31 -0700 Subject: [PATCH 07/30] Update spaces to include ReceiverTimestamps Adds ReceiverTimestamps data structure to the space and expose methods. --- quinn-proto/src/connection/spaces.rs | 42 ++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/quinn-proto/src/connection/spaces.rs b/quinn-proto/src/connection/spaces.rs index 3f999c886..dc0b49132 100644 --- a/quinn-proto/src/connection/spaces.rs +++ b/quinn-proto/src/connection/spaces.rs @@ -16,6 +16,9 @@ use crate::{ shared::IssuedCid, Dir, StreamId, TransportError, VarInt, }; +#[cfg(feature = "acktimestamps")] +use crate::connection::receiver_timestamps::ReceiverTimestamps; + pub(super) struct PacketSpace { pub(super) crypto: Option, pub(super) dedup: Dedup, @@ -278,6 +281,12 @@ impl IndexMut for [PacketSpace; 3] { pub(super) struct SentPacket { /// The time the packet was sent. pub(super) time_sent: Instant, + + #[cfg(feature = "acktimestamps")] + /// The time the packet was received by the receiver. The time Instant on this field is + /// relative to a basis negotiated by the two connections. Time arithmetic done using the + /// time_received field is only useful when compared to other time_received. + pub(super) time_received: Option, /// The number of bytes sent in the packet, not including UDP or IP overhead, but including QUIC /// framing overhead. Zero if this packet is not counted towards congestion control, i.e. not an /// "in flight" packet. @@ -587,6 +596,9 @@ pub(super) struct PendingAcks { largest_ack_eliciting_packet: Option, /// The largest acknowledged packet number sent in an ACK frame largest_acked: Option, + + #[cfg(feature = "acktimestamps")] + receiver_timestamps: Option, } impl PendingAcks { @@ -602,9 +614,17 @@ impl PendingAcks { largest_packet: None, largest_ack_eliciting_packet: None, largest_acked: None, + + #[cfg(feature = "acktimestamps")] + receiver_timestamps: None, } } + #[cfg(feature = "acktimestamps")] + pub(super) fn set_receiver_timestamp(&mut self, max_timestamps: usize) { + self.receiver_timestamps = Some(ReceiverTimestamps::new(max_timestamps)); + } + pub(super) fn set_ack_frequency_params(&mut self, frame: &frame::AckFrequency) { self.ack_eliciting_threshold = frame.ack_eliciting_threshold.into_inner(); self.reordering_threshold = frame.reordering_threshold.into_inner(); @@ -644,6 +664,11 @@ impl PendingAcks { ack_eliciting: bool, dedup: &Dedup, ) -> bool { + #[cfg(feature = "acktimestamps")] + if let Some(ts) = self.receiver_timestamps_as_mut() { + ts.add(packet_number, now); + } + if !ack_eliciting { self.non_ack_eliciting_since_last_ack_sent += 1; return false; @@ -748,6 +773,11 @@ impl PendingAcks { /// Remove ACKs of packets numbered at or below `max` from the set of pending ACKs pub(super) fn subtract_below(&mut self, max: u64) { self.ranges.remove(0..(max + 1)); + + #[cfg(feature = "acktimestamps")] + self.receiver_timestamps.as_mut().map(|v| { + v.subtract_below(max); + }); } /// Returns the set of currently pending ACK ranges @@ -755,6 +785,18 @@ impl PendingAcks { &self.ranges } + + #[cfg(feature = "acktimestamps")] + pub(super) fn receiver_timestamps_as_mut(&mut self) -> Option<&mut ReceiverTimestamps> { + self.receiver_timestamps.as_mut() + } + + + #[cfg(feature = "acktimestamps")] + pub(super) fn receiver_timestamps_as_ref(&self) -> Option<&ReceiverTimestamps> { + self.receiver_timestamps.as_ref() + } + /// Queue an ACK if a significant number of non-ACK-eliciting packets have not yet been /// acknowledged /// From a5dae4a662fc0e06d328bc36211a77f97b2f1bbc Mon Sep 17 00:00:00 2001 From: Sterling Deng Date: Thu, 19 Sep 2024 11:30:49 -0700 Subject: [PATCH 08/30] Process ACK frames with timestamps This commit processes the inbound ACK frames that contain timestamps. --- quinn-proto/src/connection/mod.rs | 186 +++++++++++++++++++++++++++++- 1 file changed, 182 insertions(+), 4 deletions(-) diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index 915a21156..9282244b2 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -20,8 +20,7 @@ use crate::{ coding::BufMutExt, config::{ServerConfig, TransportConfig}, crypto::{self, KeyPair, Keys, PacketKey}, - frame, - frame::{Close, Datagram, FrameStruct}, + frame::{self, Close, Datagram, FrameStruct}, packet::{ FixedLengthConnectionIdParser, Header, InitialHeader, InitialPacket, LongType, Packet, PacketNumber, PartialDecode, SpaceId, @@ -37,6 +36,12 @@ use crate::{ VarInt, MAX_STREAM_COUNT, MIN_INITIAL_SIZE, TIMER_GRANULARITY, }; +#[cfg(feature = "acktimestamps")] +use crate::config::AckTimestampsConfig; + +#[cfg(feature = "acktimestamps")] +use crate::ack_timestamp_frame::AckTimestampFrame; + mod ack_frequency; use ack_frequency::AckFrequencyState; @@ -65,7 +70,12 @@ use paths::{PathData, PathResponses}; mod send_buffer; -mod spaces; +#[cfg(feature = "acktimestamps")] +mod receiver_timestamps; +#[cfg(feature = "acktimestamps")] +pub(crate) use receiver_timestamps::{PacketTimestamp, ReceiverTimestamps}; + +pub(crate) mod spaces; #[cfg(fuzzing)] pub use spaces::Retransmits; #[cfg(not(fuzzing))] @@ -227,6 +237,12 @@ pub struct Connection { /// no outgoing application data. app_limited: bool, + // + // Ack Receive Timestamps + // + #[cfg(feature = "acktimestamps")] + peer_ack_timestamp_cfg: Option, + streams: StreamsState, /// Surplus remote CIDs for future use on new paths rem_cids: CidQueue, @@ -337,6 +353,9 @@ impl Connection { &TransportParameters::default(), )), + #[cfg(feature = "acktimestamps")] + peer_ack_timestamp_cfg: None, + pto_count: 0, app_limited: false, @@ -361,7 +380,7 @@ impl Connection { if side.is_client() { // Kick off the connection this.write_crypto(); - this.init_0rtt(); + this.init_0rtt(now); } this } @@ -1334,6 +1353,142 @@ impl Connection { } } + #[cfg(feature = "acktimestamps")] + fn on_ack_with_timestamp_received( + &mut self, + now: Instant, + space: SpaceId, + ack: AckTimestampFrame, + ) -> Result<(), TransportError> { + if ack.largest >= self.spaces[space].next_packet_number { + return Err(TransportError::PROTOCOL_VIOLATION("unsent packet acked")); + } + + let Some(timestamp_config) = self.config.ack_timestamp_config.as_ref() else { + return Err(TransportError::PROTOCOL_VIOLATION( + "no timestamp config set", + )); + }; + + let new_largest = { + let space = &mut self.spaces[space]; + if space + .largest_acked_packet + .map_or(true, |pn| ack.largest > pn) + { + space.largest_acked_packet = Some(ack.largest); + if let Some(info) = space.sent_packets.get(&ack.largest) { + // This should always succeed, but a misbehaving peer might ACK a packet we + // haven't sent. At worst, that will result in us spuriously reducing the + // congestion window. + space.largest_acked_packet_sent = info.time_sent; + } + true + } else { + false + } + }; + + let decoder = ack.timestamp_iter(timestamp_config.basis, timestamp_config.exponent.0); + let mut timestamp_iter = { + let mut v: tinyvec::TinyVec<[PacketTimestamp; 10]> = tinyvec::TinyVec::new(); + decoder.for_each(|elt| v.push(elt)); + v.reverse(); + v.into_iter().peekable() + }; + + // Avoid DoS from unreasonably huge ack ranges by filtering out just the new acks. + let mut newly_acked = ArrayRangeSet::new(); + for range in ack.iter() { + self.packet_number_filter.check_ack(space, range.clone())?; + for (&pn, _) in self.spaces[space].sent_packets.range(range) { + newly_acked.insert_one(pn); + } + } + + if newly_acked.is_empty() { + return Ok(()); + } + + let mut ack_eliciting_acked = false; + for packet in newly_acked.elts() { + if let Some(mut info) = self.spaces[space].take(packet) { + if let Some(acked) = info.largest_acked { + // Assume ACKs for all packets below the largest acknowledged in `packet` have + // been received. This can cause the peer to spuriously retransmit if some of + // our earlier ACKs were lost, but allows for simpler state tracking. See + // discussion at + // https://www.rfc-editor.org/rfc/rfc9000.html#name-limiting-ranges-by-tracking + self.spaces[space].pending_acks.subtract_below(acked); + } + ack_eliciting_acked |= info.ack_eliciting; + + // Notify MTU discovery that a packet was acked, because it might be an MTU probe + let mtu_updated = self.path.mtud.on_acked(space, packet, info.size); + if mtu_updated { + self.path + .congestion + .on_mtu_update(self.path.mtud.current_mtu()); + } + + // Notify ack frequency that a packet was acked, because it might contain an ACK_FREQUENCY frame + self.ack_frequency.on_acked(packet); + + while let Some(v) = timestamp_iter.peek() { + match v.packet_number.cmp(&packet) { + cmp::Ordering::Less => { + let _ = timestamp_iter.next(); + } + cmp::Ordering::Equal => { + // Unwrap safety is guaranteed because a value was validated + // to exist using peek + let ts = timestamp_iter.next().unwrap(); + info.time_received = Some(ts.timestamp); + } + cmp::Ordering::Greater => { + break; + } + } + } + self.on_packet_acked(now, packet, info); + } + } + + self.path.congestion.on_end_acks( + now, + self.path.in_flight.bytes, + self.app_limited, + self.spaces[space].largest_acked_packet, + ); + + if new_largest && ack_eliciting_acked { + let ack_delay = if space != SpaceId::Data { + Duration::from_micros(0) + } else { + cmp::min( + self.ack_frequency.peer_max_ack_delay, + Duration::from_micros(ack.delay << self.peer_params.ack_delay_exponent.0), + ) + }; + let rtt = instant_saturating_sub(now, self.spaces[space].largest_acked_packet_sent); + self.path.rtt.update(ack_delay, rtt); + if self.path.first_packet_after_rtt_sample.is_none() { + self.path.first_packet_after_rtt_sample = + Some((space, self.spaces[space].next_packet_number)); + } + } + + // Must be called before crypto/pto_count are clobbered + self.detect_lost_packets(now, space, true); + + if self.peer_completed_address_validation() { + self.pto_count = 0; + } + + self.set_loss_detection_timer(now); + Ok(()) + } + fn on_ack_received( &mut self, now: Instant, @@ -1497,6 +1652,17 @@ impl Connection { self.app_limited, &self.path.rtt, ); + + #[cfg(feature = "acktimestamps")] + self.path.congestion.on_ack_packet( + pn, + now, + info.time_sent, + info.time_received, + info.size.into(), + self.app_limited, + &self.path.rtt, + ); } // Update state for confirmed delivery of frames @@ -2570,6 +2736,12 @@ impl Connection { self.state = State::Draining; return Ok(()); } + + #[cfg(feature = "acktimestamps")] + Frame::AckTimestamps(ack) => { + self.on_ack_with_timestamp_received(now, packet.header.space(), ack)?; + } + _ => { let mut err = TransportError::PROTOCOL_VIOLATION("illegal frame type in handshake"); @@ -2662,6 +2834,12 @@ impl Connection { Frame::Ack(ack) => { self.on_ack_received(now, SpaceId::Data, ack)?; } + + #[cfg(feature = "acktimestamps")] + Frame::AckTimestamps(ack) => { + self.on_ack_with_timestamp_received(now, SpaceId::Data, ack)?; + } + Frame::Padding | Frame::Ping => {} Frame::Close(reason) => { close = Some(reason); From ead292bb746e51d5a53e6df36b761cae3ea3daaa Mon Sep 17 00:00:00 2001 From: Sterling Deng Date: Thu, 19 Sep 2024 11:34:28 -0700 Subject: [PATCH 09/30] Send ACK frame with timestamps This commit handles sending frames that contain timestamps --- quinn-proto/src/connection/mod.rs | 34 ++++++++++++++++++++++++++++--- 1 file changed, 31 insertions(+), 3 deletions(-) diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index 9282244b2..2f9cd8fe5 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -836,6 +836,8 @@ impl Connection { &mut self.spaces[space_id], buf, &mut self.stats, + #[cfg(feature = "acktimestamps")] + None, ); } @@ -2350,8 +2352,10 @@ impl Connection { } Ok((packet, number)) => { let span = match number { - Some(pn) => trace_span!("recv", space = ?packet.header.space(), pn), - None => trace_span!("recv", space = ?packet.header.space()), + Some(pn) => { + trace_span!("recv", space = ?packet.header.space(), pn, side=?self.side) + } + None => trace_span!("recv", space = ?packet.header.space(), side=?self.side), }; let _guard = span.enter(); @@ -3225,6 +3229,10 @@ impl Connection { space, buf, &mut self.stats, + #[cfg(feature = "acktimestamps")] + self.peer_ack_timestamp_cfg + .as_ref() + .map_or(None, |v| Some(v.clone())), ); } @@ -3409,6 +3417,7 @@ impl Connection { space: &mut PacketSpace, buf: &mut Vec, stats: &mut ConnectionStats, + #[cfg(feature = "acktimestamps")] timestamp_config: Option, ) { debug_assert!(!space.pending_acks.ranges().is_empty()); @@ -3433,7 +3442,26 @@ impl Connection { delay_micros ); - frame::Ack::encode(delay as _, space.pending_acks.ranges(), ecn, buf); + #[cfg(feature = "acktimestamps")] + if timestamp_config.is_some() { + let timestamp_config = timestamp_config.unwrap(); + AckTimestampFrame::encode( + delay as _, + space.pending_acks.ranges(), + space.pending_acks.receiver_timestamps_as_ref().unwrap(), + timestamp_config.basis, + timestamp_config.exponent.0, + timestamp_config.max_timestamps_per_ack.0, + buf, + ); + } else { + frame::Ack::encode(delay as _, space.pending_acks.ranges(), ecn, buf); + } + + if !cfg!(feature = "acktimestamps") { + frame::Ack::encode(delay as _, space.pending_acks.ranges(), ecn, buf); + } + stats.frame_tx.acks += 1; } From c98b93fedd95b9933b9b0318b93fc17da8899906 Mon Sep 17 00:00:00 2001 From: Sterling Deng Date: Thu, 19 Sep 2024 11:36:03 -0700 Subject: [PATCH 10/30] Update congestion controller interface Surfaces timing information to the CC interface. --- quinn-proto/src/congestion.rs | 15 +++++++++++++++ quinn-proto/src/connection/packet_builder.rs | 3 +++ 2 files changed, 18 insertions(+) diff --git a/quinn-proto/src/congestion.rs b/quinn-proto/src/congestion.rs index fac6e80e5..1794f3e98 100644 --- a/quinn-proto/src/congestion.rs +++ b/quinn-proto/src/congestion.rs @@ -34,6 +34,21 @@ pub trait Controller: Send + Sync { ) { } + #[allow(unused_variables)] + #[cfg(feature = "acktimestamps")] + /// Packet deliveries were confirmed with timestamps information. + fn on_ack_packet( + &mut self, + pn: u64, + now: Instant, + sent: Instant, + received: Option, + bytes: u64, + app_limited: bool, + rtt: &RttEstimator, + ) { + } + /// Packets are acked in batches, all with the same `now` argument. This indicates one of those batches has completed. #[allow(unused_variables)] fn on_end_acks( diff --git a/quinn-proto/src/connection/packet_builder.rs b/quinn-proto/src/connection/packet_builder.rs index a29ff448e..85b038547 100644 --- a/quinn-proto/src/connection/packet_builder.rs +++ b/quinn-proto/src/connection/packet_builder.rs @@ -210,6 +210,9 @@ impl PacketBuilder { ack_eliciting, retransmits: sent.retransmits, stream_frames: sent.stream_frames, + + #[cfg(feature = "acktimestamps")] + time_received: None, }; conn.path From 3fd8f2e6fbc8ecf69fcea14754003ec224ecfdc4 Mon Sep 17 00:00:00 2001 From: Sterling Deng Date: Thu, 19 Sep 2024 11:50:19 -0700 Subject: [PATCH 11/30] Fix lint --- quinn-proto/src/connection/mod.rs | 44 ++++++++++++++++++++++------ quinn-proto/src/connection/spaces.rs | 2 -- 2 files changed, 35 insertions(+), 11 deletions(-) diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index 2f9cd8fe5..b77368f1a 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -380,7 +380,10 @@ impl Connection { if side.is_client() { // Kick off the connection this.write_crypto(); - this.init_0rtt(now); + this.init_0rtt( + #[cfg(feature = "acktimestamps")] + now, + ); } this } @@ -2079,7 +2082,7 @@ impl Connection { Ok(()) } - fn init_0rtt(&mut self, now: Instant) { + fn init_0rtt(&mut self, #[cfg(feature = "acktimestamps")] now: Instant) { let (header, packet) = match self.crypto.early_crypto() { Some(x) => x, None => return, @@ -2101,7 +2104,11 @@ impl Connection { max_ack_delay: TransportParameters::default().max_ack_delay, ..params }; - self.set_peer_params(params, now); + self.set_peer_params( + params, + #[cfg(feature = "acktimestamps")] + now, + ); } Err(e) => { error!("session ticket has malformed transport parameters: {}", e); @@ -2622,7 +2629,11 @@ impl Connection { self.endpoint_events .push_back(EndpointEventInner::ResetToken(self.path.remote, token)); } - self.handle_peer_params(params, now)?; + self.handle_peer_params( + params, + #[cfg(feature = "acktimestamps")] + now, + )?; self.issue_first_cids(now); } else { // Server-only @@ -2669,9 +2680,16 @@ impl Connection { frame: None, reason: "transport parameters missing".into(), })?; - self.handle_peer_params(params, now)?; + self.handle_peer_params( + params, + #[cfg(feature = "acktimestamps")] + now, + )?; self.issue_first_cids(now); - self.init_0rtt(now); + self.init_0rtt( + #[cfg(feature = "acktimestamps")] + now, + ); } Ok(()) } @@ -3481,7 +3499,7 @@ impl Connection { fn handle_peer_params( &mut self, params: TransportParameters, - now: Instant, + #[cfg(feature = "acktimestamps")] now: Instant, ) -> Result<(), TransportError> { if Some(self.orig_rem_cid) != params.initial_src_cid || (self.side.is_client() @@ -3493,12 +3511,20 @@ impl Connection { )); } - self.set_peer_params(params, now); + self.set_peer_params( + params, + #[cfg(feature = "acktimestamps")] + now, + ); Ok(()) } - fn set_peer_params(&mut self, params: TransportParameters, now: Instant) { + fn set_peer_params( + &mut self, + params: TransportParameters, + #[cfg(feature = "acktimestamps")] now: Instant, + ) { self.streams.set_params(¶ms); self.idle_timeout = match (self.config.max_idle_timeout, params.max_idle_timeout) { (None, VarInt(0)) => None, diff --git a/quinn-proto/src/connection/spaces.rs b/quinn-proto/src/connection/spaces.rs index dc0b49132..e450a1e5b 100644 --- a/quinn-proto/src/connection/spaces.rs +++ b/quinn-proto/src/connection/spaces.rs @@ -785,13 +785,11 @@ impl PendingAcks { &self.ranges } - #[cfg(feature = "acktimestamps")] pub(super) fn receiver_timestamps_as_mut(&mut self) -> Option<&mut ReceiverTimestamps> { self.receiver_timestamps.as_mut() } - #[cfg(feature = "acktimestamps")] pub(super) fn receiver_timestamps_as_ref(&self) -> Option<&ReceiverTimestamps> { self.receiver_timestamps.as_ref() From 3af9df4ef147be06c1d0a764c12109895586cf46 Mon Sep 17 00:00:00 2001 From: Sterling Deng Date: Tue, 24 Sep 2024 13:42:52 -0700 Subject: [PATCH 12/30] Remove feature flag --- quinn-proto/Cargo.toml | 1 - quinn-proto/src/config.rs | 16 +---- quinn-proto/src/congestion.rs | 1 - quinn-proto/src/connection/mod.rs | 64 +++----------------- quinn-proto/src/connection/packet_builder.rs | 1 - quinn-proto/src/connection/spaces.rs | 9 --- quinn-proto/src/connection/stats.rs | 1 - quinn-proto/src/frame.rs | 39 +++--------- quinn-proto/src/lib.rs | 2 - quinn-proto/src/transport_parameters.rs | 13 ---- 10 files changed, 20 insertions(+), 127 deletions(-) diff --git a/quinn-proto/Cargo.toml b/quinn-proto/Cargo.toml index e64e8461d..191569e24 100644 --- a/quinn-proto/Cargo.toml +++ b/quinn-proto/Cargo.toml @@ -22,7 +22,6 @@ ring = ["dep:ring"] platform-verifier = ["dep:rustls-platform-verifier"] # Configure `tracing` to log events via `log` if no `tracing` subscriber exists. log = ["tracing/log"] -acktimestamps = [] [dependencies] arbitrary = { workspace = true, optional = true } diff --git a/quinn-proto/src/config.rs b/quinn-proto/src/config.rs index a25384d40..d051651e6 100644 --- a/quinn-proto/src/config.rs +++ b/quinn-proto/src/config.rs @@ -53,7 +53,6 @@ pub struct TransportConfig { pub(crate) mtu_discovery_config: Option, pub(crate) ack_frequency_config: Option, - #[cfg(feature = "acktimestamps")] pub(crate) ack_timestamp_config: Option, pub(crate) persistent_congestion_threshold: u32, @@ -230,7 +229,6 @@ impl TransportConfig { /// Defaults to `None`, which disables receiving acknowledgement timestamps from the sender. /// If `Some`, TransportParameters are sent to the peer to enable acknowledgement timestamps /// if supported. - #[cfg(feature = "acktimestamps")] pub fn ack_timestamp_config(&mut self, value: Option) -> &mut Self { self.ack_timestamp_config = value; self @@ -374,7 +372,6 @@ impl Default for TransportConfig { enable_segmentation_offload: true, - #[cfg(feature = "acktimestamps")] ack_timestamp_config: None, } } @@ -406,7 +403,6 @@ impl fmt::Debug for TransportConfig { deterministic_packet_numbers: _, congestion_controller_factory: _, enable_segmentation_offload, - #[cfg(feature = "acktimestamps")] ack_timestamp_config, } = self; let mut s = fmt.debug_struct("TransportConfig"); @@ -434,17 +430,13 @@ impl fmt::Debug for TransportConfig { .field("datagram_receive_buffer_size", datagram_receive_buffer_size) .field("datagram_send_buffer_size", datagram_send_buffer_size) .field("congestion_controller_factory", &"[ opaque ]") - .field("enable_segmentation_offload", enable_segmentation_offload); - - #[cfg(feature = "acktimestamps")] - s.field("ack_timestamp_config", ack_timestamp_config); - - s.finish() + .field("enable_segmentation_offload", enable_segmentation_offload) + .field("ack_timestamp_config", ack_timestamp_config) + .finish() } } /// Parameters for controlling the peer's acknowledgements with receiver timestamps. -#[cfg(feature = "acktimestamps")] #[derive(Clone, Debug)] pub struct AckTimestampsConfig { pub(crate) max_timestamps_per_ack: VarInt, @@ -452,7 +444,6 @@ pub struct AckTimestampsConfig { pub(crate) basis: std::time::Instant, } -#[cfg(feature = "acktimestamps")] impl AckTimestampsConfig { /// Sets the maximum number of timestamp entries per ACK frame. pub fn max_timestamps_per_ack(&mut self, value: VarInt) -> &mut Self { @@ -475,7 +466,6 @@ impl AckTimestampsConfig { } } -#[cfg(feature = "acktimestamps")] impl Default for AckTimestampsConfig { fn default() -> Self { Self { diff --git a/quinn-proto/src/congestion.rs b/quinn-proto/src/congestion.rs index 1794f3e98..1ae3757a7 100644 --- a/quinn-proto/src/congestion.rs +++ b/quinn-proto/src/congestion.rs @@ -35,7 +35,6 @@ pub trait Controller: Send + Sync { } #[allow(unused_variables)] - #[cfg(feature = "acktimestamps")] /// Packet deliveries were confirmed with timestamps information. fn on_ack_packet( &mut self, diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index b77368f1a..d8e232e0a 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -36,10 +36,8 @@ use crate::{ VarInt, MAX_STREAM_COUNT, MIN_INITIAL_SIZE, TIMER_GRANULARITY, }; -#[cfg(feature = "acktimestamps")] use crate::config::AckTimestampsConfig; -#[cfg(feature = "acktimestamps")] use crate::ack_timestamp_frame::AckTimestampFrame; mod ack_frequency; @@ -70,9 +68,7 @@ use paths::{PathData, PathResponses}; mod send_buffer; -#[cfg(feature = "acktimestamps")] mod receiver_timestamps; -#[cfg(feature = "acktimestamps")] pub(crate) use receiver_timestamps::{PacketTimestamp, ReceiverTimestamps}; pub(crate) mod spaces; @@ -240,7 +236,6 @@ pub struct Connection { // // Ack Receive Timestamps // - #[cfg(feature = "acktimestamps")] peer_ack_timestamp_cfg: Option, streams: StreamsState, @@ -353,7 +348,6 @@ impl Connection { &TransportParameters::default(), )), - #[cfg(feature = "acktimestamps")] peer_ack_timestamp_cfg: None, pto_count: 0, @@ -380,10 +374,7 @@ impl Connection { if side.is_client() { // Kick off the connection this.write_crypto(); - this.init_0rtt( - #[cfg(feature = "acktimestamps")] - now, - ); + this.init_0rtt(now); } this } @@ -839,7 +830,6 @@ impl Connection { &mut self.spaces[space_id], buf, &mut self.stats, - #[cfg(feature = "acktimestamps")] None, ); } @@ -1358,7 +1348,6 @@ impl Connection { } } - #[cfg(feature = "acktimestamps")] fn on_ack_with_timestamp_received( &mut self, now: Instant, @@ -1658,7 +1647,6 @@ impl Connection { &self.path.rtt, ); - #[cfg(feature = "acktimestamps")] self.path.congestion.on_ack_packet( pn, now, @@ -2082,7 +2070,7 @@ impl Connection { Ok(()) } - fn init_0rtt(&mut self, #[cfg(feature = "acktimestamps")] now: Instant) { + fn init_0rtt(&mut self, now: Instant) { let (header, packet) = match self.crypto.early_crypto() { Some(x) => x, None => return, @@ -2104,11 +2092,7 @@ impl Connection { max_ack_delay: TransportParameters::default().max_ack_delay, ..params }; - self.set_peer_params( - params, - #[cfg(feature = "acktimestamps")] - now, - ); + self.set_peer_params(params, now); } Err(e) => { error!("session ticket has malformed transport parameters: {}", e); @@ -2629,11 +2613,7 @@ impl Connection { self.endpoint_events .push_back(EndpointEventInner::ResetToken(self.path.remote, token)); } - self.handle_peer_params( - params, - #[cfg(feature = "acktimestamps")] - now, - )?; + self.handle_peer_params(params, now)?; self.issue_first_cids(now); } else { // Server-only @@ -2680,16 +2660,9 @@ impl Connection { frame: None, reason: "transport parameters missing".into(), })?; - self.handle_peer_params( - params, - #[cfg(feature = "acktimestamps")] - now, - )?; + self.handle_peer_params(params, now)?; self.issue_first_cids(now); - self.init_0rtt( - #[cfg(feature = "acktimestamps")] - now, - ); + self.init_0rtt(now); } Ok(()) } @@ -2759,7 +2732,6 @@ impl Connection { return Ok(()); } - #[cfg(feature = "acktimestamps")] Frame::AckTimestamps(ack) => { self.on_ack_with_timestamp_received(now, packet.header.space(), ack)?; } @@ -2857,7 +2829,6 @@ impl Connection { self.on_ack_received(now, SpaceId::Data, ack)?; } - #[cfg(feature = "acktimestamps")] Frame::AckTimestamps(ack) => { self.on_ack_with_timestamp_received(now, SpaceId::Data, ack)?; } @@ -3247,7 +3218,6 @@ impl Connection { space, buf, &mut self.stats, - #[cfg(feature = "acktimestamps")] self.peer_ack_timestamp_cfg .as_ref() .map_or(None, |v| Some(v.clone())), @@ -3435,7 +3405,7 @@ impl Connection { space: &mut PacketSpace, buf: &mut Vec, stats: &mut ConnectionStats, - #[cfg(feature = "acktimestamps")] timestamp_config: Option, + timestamp_config: Option, ) { debug_assert!(!space.pending_acks.ranges().is_empty()); @@ -3460,7 +3430,6 @@ impl Connection { delay_micros ); - #[cfg(feature = "acktimestamps")] if timestamp_config.is_some() { let timestamp_config = timestamp_config.unwrap(); AckTimestampFrame::encode( @@ -3476,10 +3445,6 @@ impl Connection { frame::Ack::encode(delay as _, space.pending_acks.ranges(), ecn, buf); } - if !cfg!(feature = "acktimestamps") { - frame::Ack::encode(delay as _, space.pending_acks.ranges(), ecn, buf); - } - stats.frame_tx.acks += 1; } @@ -3499,7 +3464,7 @@ impl Connection { fn handle_peer_params( &mut self, params: TransportParameters, - #[cfg(feature = "acktimestamps")] now: Instant, + now: Instant, ) -> Result<(), TransportError> { if Some(self.orig_rem_cid) != params.initial_src_cid || (self.side.is_client() @@ -3511,20 +3476,12 @@ impl Connection { )); } - self.set_peer_params( - params, - #[cfg(feature = "acktimestamps")] - now, - ); + self.set_peer_params(params, now); Ok(()) } - fn set_peer_params( - &mut self, - params: TransportParameters, - #[cfg(feature = "acktimestamps")] now: Instant, - ) { + fn set_peer_params(&mut self, params: TransportParameters, now: Instant) { self.streams.set_params(¶ms); self.idle_timeout = match (self.config.max_idle_timeout, params.max_idle_timeout) { (None, VarInt(0)) => None, @@ -3546,7 +3503,6 @@ impl Connection { u16::try_from(self.peer_params.max_udp_payload_size.into_inner()).unwrap_or(u16::MAX), ); - #[cfg(feature = "acktimestamps")] { self.peer_ack_timestamp_cfg = if let (Some(max_timestamps_per_ack), Some(exponent)) = ( params.max_recv_timestamps_per_ack, diff --git a/quinn-proto/src/connection/packet_builder.rs b/quinn-proto/src/connection/packet_builder.rs index 85b038547..b7c06854c 100644 --- a/quinn-proto/src/connection/packet_builder.rs +++ b/quinn-proto/src/connection/packet_builder.rs @@ -211,7 +211,6 @@ impl PacketBuilder { retransmits: sent.retransmits, stream_frames: sent.stream_frames, - #[cfg(feature = "acktimestamps")] time_received: None, }; diff --git a/quinn-proto/src/connection/spaces.rs b/quinn-proto/src/connection/spaces.rs index e450a1e5b..02e26d946 100644 --- a/quinn-proto/src/connection/spaces.rs +++ b/quinn-proto/src/connection/spaces.rs @@ -16,7 +16,6 @@ use crate::{ shared::IssuedCid, Dir, StreamId, TransportError, VarInt, }; -#[cfg(feature = "acktimestamps")] use crate::connection::receiver_timestamps::ReceiverTimestamps; pub(super) struct PacketSpace { @@ -282,7 +281,6 @@ pub(super) struct SentPacket { /// The time the packet was sent. pub(super) time_sent: Instant, - #[cfg(feature = "acktimestamps")] /// The time the packet was received by the receiver. The time Instant on this field is /// relative to a basis negotiated by the two connections. Time arithmetic done using the /// time_received field is only useful when compared to other time_received. @@ -597,7 +595,6 @@ pub(super) struct PendingAcks { /// The largest acknowledged packet number sent in an ACK frame largest_acked: Option, - #[cfg(feature = "acktimestamps")] receiver_timestamps: Option, } @@ -615,12 +612,10 @@ impl PendingAcks { largest_ack_eliciting_packet: None, largest_acked: None, - #[cfg(feature = "acktimestamps")] receiver_timestamps: None, } } - #[cfg(feature = "acktimestamps")] pub(super) fn set_receiver_timestamp(&mut self, max_timestamps: usize) { self.receiver_timestamps = Some(ReceiverTimestamps::new(max_timestamps)); } @@ -664,7 +659,6 @@ impl PendingAcks { ack_eliciting: bool, dedup: &Dedup, ) -> bool { - #[cfg(feature = "acktimestamps")] if let Some(ts) = self.receiver_timestamps_as_mut() { ts.add(packet_number, now); } @@ -774,7 +768,6 @@ impl PendingAcks { pub(super) fn subtract_below(&mut self, max: u64) { self.ranges.remove(0..(max + 1)); - #[cfg(feature = "acktimestamps")] self.receiver_timestamps.as_mut().map(|v| { v.subtract_below(max); }); @@ -785,12 +778,10 @@ impl PendingAcks { &self.ranges } - #[cfg(feature = "acktimestamps")] pub(super) fn receiver_timestamps_as_mut(&mut self) -> Option<&mut ReceiverTimestamps> { self.receiver_timestamps.as_mut() } - #[cfg(feature = "acktimestamps")] pub(super) fn receiver_timestamps_as_ref(&self) -> Option<&ReceiverTimestamps> { self.receiver_timestamps.as_ref() } diff --git a/quinn-proto/src/connection/stats.rs b/quinn-proto/src/connection/stats.rs index 189ccf7f3..07b33c962 100644 --- a/quinn-proto/src/connection/stats.rs +++ b/quinn-proto/src/connection/stats.rs @@ -62,7 +62,6 @@ impl FrameStats { Frame::Padding => {} Frame::Ping => self.ping += 1, Frame::Ack(_) => self.acks += 1, - #[cfg(feature = "acktimestamps")] Frame::AckTimestamps(_) => self.acks += 1, Frame::ResetStream(_) => self.reset_stream += 1, Frame::StopSending(_) => self.stop_sending += 1, diff --git a/quinn-proto/src/frame.rs b/quinn-proto/src/frame.rs index 32a2eef13..9c09f154e 100644 --- a/quinn-proto/src/frame.rs +++ b/quinn-proto/src/frame.rs @@ -15,7 +15,6 @@ use crate::{ RESET_TOKEN_SIZE, }; -#[cfg(feature = "acktimestamps")] use crate::ack_timestamp_frame; #[cfg(feature = "arbitrary")] @@ -137,7 +136,6 @@ frame_types! { IMMEDIATE_ACK = 0x1f, // DATAGRAM // Custom frame for https://www.ietf.org/archive/id/draft-smith-quic-receive-ts-00.html - // can't cfg here in macro #[cfg(feature = "acktimestamps")] ACK_RECEIVE_TIMESTAMPS = 0x40, } @@ -149,39 +147,20 @@ pub(crate) enum Frame { Padding, Ping, Ack(Ack), - #[cfg(feature = "acktimestamps")] AckTimestamps(ack_timestamp_frame::AckTimestampFrame), ResetStream(ResetStream), StopSending(StopSending), Crypto(Crypto), - NewToken { - token: Bytes, - }, + NewToken { token: Bytes }, Stream(Stream), MaxData(VarInt), - MaxStreamData { - id: StreamId, - offset: u64, - }, - MaxStreams { - dir: Dir, - count: u64, - }, - DataBlocked { - offset: u64, - }, - StreamDataBlocked { - id: StreamId, - offset: u64, - }, - StreamsBlocked { - dir: Dir, - limit: u64, - }, + MaxStreamData { id: StreamId, offset: u64 }, + MaxStreams { dir: Dir, count: u64 }, + DataBlocked { offset: u64 }, + StreamDataBlocked { id: StreamId, offset: u64 }, + StreamsBlocked { dir: Dir, limit: u64 }, NewConnectionId(NewConnectionId), - RetireConnectionId { - sequence: u64, - }, + RetireConnectionId { sequence: u64 }, PathChallenge(u64), PathResponse(u64), Close(Close), @@ -211,7 +190,6 @@ impl Frame { StopSending { .. } => Type::STOP_SENDING, RetireConnectionId { .. } => Type::RETIRE_CONNECTION_ID, Ack(_) => Type::ACK, - #[cfg(feature = "acktimestamps")] AckTimestamps(_) => Type::ACK_RECEIVE_TIMESTAMPS, Stream(ref x) => { let mut ty = *STREAM_TYS.start(); @@ -669,7 +647,6 @@ impl Iter { }, }) } - #[cfg(feature = "acktimestamps")] Type::ACK_RECEIVE_TIMESTAMPS => { let largest = self.bytes.get_var()?; let delay = self.bytes.get_var()?; @@ -816,7 +793,6 @@ fn scan_ack_blocks(buf: &mut io::Cursor, largest: u64, n: usize) -> Resul Ok(()) } -#[cfg(feature = "acktimestamps")] fn scan_ack_timestamp_blocks( buf: &mut io::Cursor, largest: u64, @@ -1080,7 +1056,6 @@ mod test { } #[cfg(test)] - #[cfg(feature = "acktimestamps")] mod ack_timestamp_tests { use super::*; diff --git a/quinn-proto/src/lib.rs b/quinn-proto/src/lib.rs index fd9cd0bfb..5b1cd5755 100644 --- a/quinn-proto/src/lib.rs +++ b/quinn-proto/src/lib.rs @@ -48,7 +48,6 @@ pub use crate::connection::{ }; mod config; -#[cfg(feature = "acktimestamps")] pub use config::AckTimestampsConfig; pub use config::{ AckFrequencyConfig, ClientConfig, ConfigError, EndpointConfig, IdleTimeout, MtuDiscoveryConfig, @@ -88,7 +87,6 @@ pub use crate::cid_generator::{ mod token; use token::{ResetToken, RetryToken}; -#[cfg(feature = "acktimestamps")] mod ack_timestamp_frame; #[cfg(feature = "arbitrary")] diff --git a/quinn-proto/src/transport_parameters.rs b/quinn-proto/src/transport_parameters.rs index 2cf645614..9bfcfe4c7 100644 --- a/quinn-proto/src/transport_parameters.rs +++ b/quinn-proto/src/transport_parameters.rs @@ -101,9 +101,7 @@ macro_rules! make_struct { pub(crate) preferred_address: Option, - #[cfg(feature = "acktimestamps")] pub(crate) receive_timestamps_exponent: Option, - #[cfg(feature = "acktimestamps")] pub(crate) max_recv_timestamps_per_ack: Option, } @@ -126,9 +124,7 @@ macro_rules! make_struct { retry_src_cid: None, stateless_reset_token: None, preferred_address: None, - #[cfg(feature = "acktimestamps")] receive_timestamps_exponent: None, - #[cfg(feature = "acktimestamps")] max_recv_timestamps_per_ack: None, } } @@ -171,13 +167,11 @@ impl TransportParameters { VarInt::from_u64(u64::try_from(TIMER_GRANULARITY.as_micros()).unwrap()).unwrap(), ), - #[cfg(feature = "acktimestamps")] receive_timestamps_exponent: config .ack_timestamp_config .as_ref() .map_or(None, |cfg| Some(cfg.exponent)), - #[cfg(feature = "acktimestamps")] max_recv_timestamps_per_ack: config .ack_timestamp_config .as_ref() @@ -378,14 +372,12 @@ impl TransportParameters { // The transport parameter values selected are placeholders, using the first 2 reserved values specified // in https://www.rfc-editor.org/rfc/rfc9000#section-22.3 - #[cfg(feature = "acktimestamps")] if let Some(x) = self.max_recv_timestamps_per_ack { w.write_var(0x00f0); w.write_var(x.size() as u64); w.write(x); } - #[cfg(feature = "acktimestamps")] if let Some(x) = self.receive_timestamps_exponent { w.write_var(0x00f1); w.write_var(x.size() as u64); @@ -456,14 +448,12 @@ impl TransportParameters { }, 0xff04de1b => params.min_ack_delay = Some(r.get().unwrap()), - #[cfg(feature = "acktimestamps")] 0x00f0 => { if len > 8 || params.max_recv_timestamps_per_ack.is_some() { return Err(Error::Malformed); } params.max_recv_timestamps_per_ack = Some(r.get().unwrap()); } - #[cfg(feature = "acktimestamps")] 0x00f1 => { if len > 8 || params.receive_timestamps_exponent.is_some() { return Err(Error::Malformed); @@ -523,7 +513,6 @@ impl TransportParameters { } // https://www.ietf.org/archive/id/draft-smith-quic-receive-ts-00.html#name-extension-negotiation - #[cfg(feature = "acktimestamps")] if params .receive_timestamps_exponent .map_or(false, |x| x.0 > 20) @@ -567,9 +556,7 @@ mod test { grease_quic_bit: true, min_ack_delay: Some(2_000u32.into()), - #[cfg(feature = "acktimestamps")] receive_timestamps_exponent: Some(3u32.into()), - #[cfg(feature = "acktimestamps")] max_recv_timestamps_per_ack: Some(5u32.into()), ..TransportParameters::default() From 6c56bc1c7bbb8b6675be12986171479a9c035bbf Mon Sep 17 00:00:00 2001 From: Sterling Deng Date: Tue, 24 Sep 2024 14:53:34 -0700 Subject: [PATCH 13/30] Set created_at time on Connection --- quinn-proto/src/connection/mod.rs | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index d8e232e0a..81dd8cade 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -249,6 +249,8 @@ pub struct Connection { stats: ConnectionStats, /// QUIC version used for the connection. version: u32, + /// Instant created + created_at: Instant, } impl Connection { @@ -370,11 +372,12 @@ impl Connection { rng, stats: ConnectionStats::default(), version, + created_at: now, }; if side.is_client() { // Kick off the connection this.write_crypto(); - this.init_0rtt(now); + this.init_0rtt(); } this } @@ -2070,7 +2073,7 @@ impl Connection { Ok(()) } - fn init_0rtt(&mut self, now: Instant) { + fn init_0rtt(&mut self) { let (header, packet) = match self.crypto.early_crypto() { Some(x) => x, None => return, @@ -2092,7 +2095,7 @@ impl Connection { max_ack_delay: TransportParameters::default().max_ack_delay, ..params }; - self.set_peer_params(params, now); + self.set_peer_params(params); } Err(e) => { error!("session ticket has malformed transport parameters: {}", e); @@ -2613,7 +2616,7 @@ impl Connection { self.endpoint_events .push_back(EndpointEventInner::ResetToken(self.path.remote, token)); } - self.handle_peer_params(params, now)?; + self.handle_peer_params(params)?; self.issue_first_cids(now); } else { // Server-only @@ -2660,9 +2663,9 @@ impl Connection { frame: None, reason: "transport parameters missing".into(), })?; - self.handle_peer_params(params, now)?; + self.handle_peer_params(params)?; self.issue_first_cids(now); - self.init_0rtt(now); + self.init_0rtt(); } Ok(()) } @@ -3461,11 +3464,7 @@ impl Connection { } /// Handle transport parameters received from the peer - fn handle_peer_params( - &mut self, - params: TransportParameters, - now: Instant, - ) -> Result<(), TransportError> { + fn handle_peer_params(&mut self, params: TransportParameters) -> Result<(), TransportError> { if Some(self.orig_rem_cid) != params.initial_src_cid || (self.side.is_client() && (Some(self.initial_dst_cid) != params.original_dst_cid @@ -3476,12 +3475,12 @@ impl Connection { )); } - self.set_peer_params(params, now); + self.set_peer_params(params); Ok(()) } - fn set_peer_params(&mut self, params: TransportParameters, now: Instant) { + fn set_peer_params(&mut self, params: TransportParameters) { self.streams.set_params(¶ms); self.idle_timeout = match (self.config.max_idle_timeout, params.max_idle_timeout) { (None, VarInt(0)) => None, @@ -3516,7 +3515,7 @@ impl Connection { Some(AckTimestampsConfig { exponent, max_timestamps_per_ack, - basis: now, + basis: self.created_at, }) } else { None From 9d9ee2371a2bb241d70a8137fe712fb44e4a12af Mon Sep 17 00:00:00 2001 From: Sterling Deng Date: Tue, 24 Sep 2024 15:11:25 -0700 Subject: [PATCH 14/30] minor cleanup --- quinn-proto/src/config.rs | 5 ++--- quinn-proto/src/connection/receiver_timestamps.rs | 6 +----- quinn-proto/src/transport_parameters.rs | 4 +--- 3 files changed, 4 insertions(+), 11 deletions(-) diff --git a/quinn-proto/src/config.rs b/quinn-proto/src/config.rs index d051651e6..7b429bb00 100644 --- a/quinn-proto/src/config.rs +++ b/quinn-proto/src/config.rs @@ -405,9 +405,8 @@ impl fmt::Debug for TransportConfig { enable_segmentation_offload, ack_timestamp_config, } = self; - let mut s = fmt.debug_struct("TransportConfig"); - - s.field("max_concurrent_bidi_streams", max_concurrent_bidi_streams) + fmt.debug_struct("TransportConfig") + .field("max_concurrent_bidi_streams", max_concurrent_bidi_streams) .field("max_concurrent_uni_streams", max_concurrent_uni_streams) .field("max_idle_timeout", max_idle_timeout) .field("stream_receive_window", stream_receive_window) diff --git a/quinn-proto/src/connection/receiver_timestamps.rs b/quinn-proto/src/connection/receiver_timestamps.rs index 9991ea539..8d5eaedd7 100644 --- a/quinn-proto/src/connection/receiver_timestamps.rs +++ b/quinn-proto/src/connection/receiver_timestamps.rs @@ -67,10 +67,6 @@ impl ReceiverTimestamps { }); } - fn clear(&mut self) { - self.data.clear() - } - pub(crate) fn iter(&self) -> impl DoubleEndedIterator + '_ { self.data.iter().cloned() } @@ -91,7 +87,7 @@ impl ReceiverTimestamps { .data .partition_point(|v| v.packet_number < packet_number); if idx == self.data.len() { - self.clear(); + self.data.clear(); } else { let _ = self.data.drain(0..=idx); } diff --git a/quinn-proto/src/transport_parameters.rs b/quinn-proto/src/transport_parameters.rs index 9bfcfe4c7..39cc16a5b 100644 --- a/quinn-proto/src/transport_parameters.rs +++ b/quinn-proto/src/transport_parameters.rs @@ -369,9 +369,7 @@ impl TransportParameters { // The below 2 fields are for the implementation of // https://www.ietf.org/archive/id/draft-smith-quic-receive-ts-00.html#name-extension-negotiation - // The transport parameter values selected are placeholders, using the first 2 reserved values specified - // in https://www.rfc-editor.org/rfc/rfc9000#section-22.3 - + // Values of 0x00f0 and 0x00f1 arbitrarily chosen. if let Some(x) = self.max_recv_timestamps_per_ack { w.write_var(0x00f0); w.write_var(x.size() as u64); From e47ade62a505afa29c26da1b4902bdf343d7a975 Mon Sep 17 00:00:00 2001 From: Sterling Deng Date: Tue, 24 Sep 2024 15:57:12 -0700 Subject: [PATCH 15/30] Fix lint --- quinn-proto/src/connection/mod.rs | 4 +--- .../src/connection/receiver_timestamps.rs | 22 +++++++++---------- quinn-proto/src/connection/spaces.rs | 4 ++-- quinn-proto/src/transport_parameters.rs | 4 ++-- 4 files changed, 16 insertions(+), 18 deletions(-) diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index 81dd8cade..834d57c25 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -3221,9 +3221,7 @@ impl Connection { space, buf, &mut self.stats, - self.peer_ack_timestamp_cfg - .as_ref() - .map_or(None, |v| Some(v.clone())), + self.peer_ack_timestamp_cfg.clone(), ); } diff --git a/quinn-proto/src/connection/receiver_timestamps.rs b/quinn-proto/src/connection/receiver_timestamps.rs index 8d5eaedd7..3f7e8e3d9 100644 --- a/quinn-proto/src/connection/receiver_timestamps.rs +++ b/quinn-proto/src/connection/receiver_timestamps.rs @@ -11,7 +11,7 @@ pub(crate) struct PacketTimestamp { impl Default for PacketTimestamp { fn default() -> Self { - PacketTimestamp { + Self { packet_number: 0, timestamp: Instant::now(), } @@ -45,7 +45,7 @@ impl std::fmt::Debug for ReceiverTimestamps { impl ReceiverTimestamps { pub(crate) fn new(max: usize) -> Self { - ReceiverTimestamps { + Self { data: VecDeque::with_capacity(max), max, } @@ -101,10 +101,10 @@ mod receiver_timestamp_tests { #[test] fn subtract_below() { let mut ts = ReceiverTimestamps::new(10); - let _ = ts.add(1, Instant::now()); - let _ = ts.add(2, Instant::now()); - let _ = ts.add(3, Instant::now()); - let _ = ts.add(4, Instant::now()); + ts.add(1, Instant::now()); + ts.add(2, Instant::now()); + ts.add(3, Instant::now()); + ts.add(4, Instant::now()); ts.subtract_below(3); assert_eq!(1, ts.len()); } @@ -112,7 +112,7 @@ mod receiver_timestamp_tests { #[test] fn subtract_below_everything() { let mut ts = ReceiverTimestamps::new(10); - let _ = ts.add(5, Instant::now()); + ts.add(5, Instant::now()); ts.subtract_below(10); assert_eq!(0, ts.len()); } @@ -120,10 +120,10 @@ mod receiver_timestamp_tests { #[test] fn receiver_timestamp_max() { let mut ts = ReceiverTimestamps::new(2); - let _ = ts.add(1, Instant::now()); - let _ = ts.add(2, Instant::now()); - let _ = ts.add(3, Instant::now()); - let _ = ts.add(4, Instant::now()); + ts.add(1, Instant::now()); + ts.add(2, Instant::now()); + ts.add(3, Instant::now()); + ts.add(4, Instant::now()); assert_eq!(2, ts.len()); assert_eq!(3, ts.data.front().unwrap().packet_number); assert_eq!(4, ts.data.back().unwrap().packet_number); diff --git a/quinn-proto/src/connection/spaces.rs b/quinn-proto/src/connection/spaces.rs index 02e26d946..a087bb1af 100644 --- a/quinn-proto/src/connection/spaces.rs +++ b/quinn-proto/src/connection/spaces.rs @@ -768,9 +768,9 @@ impl PendingAcks { pub(super) fn subtract_below(&mut self, max: u64) { self.ranges.remove(0..(max + 1)); - self.receiver_timestamps.as_mut().map(|v| { + if let Some(v) = self.receiver_timestamps.as_mut() { v.subtract_below(max); - }); + } } /// Returns the set of currently pending ACK ranges diff --git a/quinn-proto/src/transport_parameters.rs b/quinn-proto/src/transport_parameters.rs index 39cc16a5b..a83f40414 100644 --- a/quinn-proto/src/transport_parameters.rs +++ b/quinn-proto/src/transport_parameters.rs @@ -170,12 +170,12 @@ impl TransportParameters { receive_timestamps_exponent: config .ack_timestamp_config .as_ref() - .map_or(None, |cfg| Some(cfg.exponent)), + .map(|cfg| cfg.exponent), max_recv_timestamps_per_ack: config .ack_timestamp_config .as_ref() - .map_or(None, |cfg| Some(cfg.max_timestamps_per_ack)), + .map(|cfg| cfg.max_timestamps_per_ack), ..Self::default() } From 6ac89dbe84216b335acf33fd898fd0c0cc8f40e9 Mon Sep 17 00:00:00 2001 From: Sterling Deng Date: Wed, 25 Sep 2024 09:32:21 -0700 Subject: [PATCH 16/30] Fix unit tests --- quinn-proto/src/frame.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/quinn-proto/src/frame.rs b/quinn-proto/src/frame.rs index 9c09f154e..1d326d088 100644 --- a/quinn-proto/src/frame.rs +++ b/quinn-proto/src/frame.rs @@ -1084,7 +1084,7 @@ mod test { buf.write_var(3); // pn 7 buf.write_var(1); // gap - buf.write_var(6); // delta count + buf.write_var(5); // delta count buf.write_var(1); // pn 4 buf.write_var(2); // pn 3 buf.write_var(3); // pn 2 @@ -1127,7 +1127,7 @@ mod test { let mut c = io::Cursor::new(buf.freeze()); assert_eq!( - IterErr::Malformed, + IterErr::UnexpectedEnd, scan_ack_timestamp_blocks(&mut c, 5, 2).unwrap_err(), ); } From 96153e62fe451b25bd18b125e8e68725bd8abae0 Mon Sep 17 00:00:00 2001 From: Sterling Deng Date: Thu, 26 Sep 2024 11:26:36 -0700 Subject: [PATCH 17/30] simplify into on_ack_received --- quinn-proto/src/ack_timestamp_frame.rs | 436 ------------------ quinn-proto/src/connection/mod.rs | 205 ++------ .../src/connection/receiver_timestamps.rs | 4 +- quinn-proto/src/connection/stats.rs | 1 - quinn-proto/src/frame.rs | 383 ++++++++++++++- quinn-proto/src/lib.rs | 2 - 6 files changed, 408 insertions(+), 623 deletions(-) delete mode 100644 quinn-proto/src/ack_timestamp_frame.rs diff --git a/quinn-proto/src/ack_timestamp_frame.rs b/quinn-proto/src/ack_timestamp_frame.rs deleted file mode 100644 index 24fadbeb8..000000000 --- a/quinn-proto/src/ack_timestamp_frame.rs +++ /dev/null @@ -1,436 +0,0 @@ -use std::{ - fmt::{self, Write}, - ops::RangeInclusive, - time::{Duration, Instant}, -}; - -use crate::{ - coding::{BufExt, BufMutExt}, - connection::{PacketTimestamp, ReceiverTimestamps}, - frame::{AckIter, Type}, - range_set::ArrayRangeSet, -}; - -use bytes::{Buf, BufMut, Bytes}; - -#[derive(Clone, Eq, PartialEq)] -pub(crate) struct AckTimestampFrame { - pub largest: u64, - pub delay: u64, - pub ranges: Bytes, - pub timestamps: Bytes, -} - -impl AckTimestampFrame { - pub(crate) fn encode( - delay: u64, - ranges: &ArrayRangeSet, - timestamps: &ReceiverTimestamps, - timestamp_basis: Instant, - timestamp_exponent: u64, - max_timestamps: u64, - buf: &mut W, - ) { - let mut rest = ranges.iter().rev(); - let first = rest.next().unwrap(); - let largest = first.end - 1; - let first_size = first.end - first.start; - buf.write(Type::ACK_RECEIVE_TIMESTAMPS); - buf.write_var(largest); - buf.write_var(delay); - buf.write_var(ranges.len() as u64 - 1); - buf.write_var(first_size - 1); - let mut prev = first.start; - for block in rest { - let size = block.end - block.start; - buf.write_var(prev - block.end - 1); - buf.write_var(size - 1); - prev = block.start; - } - - Self::encode_timestamps( - timestamps, - largest, - buf, - timestamp_basis, - timestamp_exponent, - max_timestamps, - ); - } - - // https://www.ietf.org/archive/id/draft-smith-quic-receive-ts-00.html#ts-ranges - fn encode_timestamps( - timestamps: &ReceiverTimestamps, - mut largest: u64, - buf: &mut W, - mut basis: Instant, - exponent: u64, - max_timestamps: u64, - ) { - if timestamps.len() == 0 { - buf.write_var(0); - return; - } - let mut prev: Option = None; - - // segment_idx tracks the positions in `timestamps` in which a gap occurs. - let mut segment_idxs = Vec::::new(); - // iterates from largest number to smallest - for (i, pkt) in timestamps.iter().rev().enumerate() { - if let Some(prev) = prev { - if pkt.packet_number + 1 != prev { - segment_idxs.push(timestamps.len() - i); - } - } - prev = Some(pkt.packet_number); - } - segment_idxs.push(0); - // Timestamp Range Count - buf.write_var(segment_idxs.len() as u64); - - let mut right = timestamps.len(); - let mut first = true; - let mut counter = 0; - - for segment_idx in segment_idxs { - let Some(elt) = timestamps.inner().get(right - 1) else { - debug_assert!( - false, - "an invalid indexing occurred on the ReceiverTimestamps vector" - ); - break; - }; - // *Gap - // For the first Timestamp Range: Gap is the difference between (a) the Largest Acknowledged packet number - // in the frame and (b) the largest packet in the current (first) Timestamp Range. - let gap = if first { - debug_assert!( - elt.packet_number <= largest, - "largest packet number is less than what was found in timestamp vec" - ); - largest - elt.packet_number - } else { - // For subsequent Timestamp Ranges: Gap is the difference between (a) the packet number two lower - // than the smallest packet number in the previous Timestamp Range - // and (b) the largest packet in the current Timestamp Range. - largest - 2 - elt.packet_number - }; - buf.write_var(gap); - // *Timestamp Delta Count - buf.write_var((right - segment_idx) as u64); - // *Timestamp Deltas - for pkt in timestamps.inner().range(segment_idx..right).rev() { - let delta: u64 = if first { - first = false; - // For the first Timestamp Delta of the first Timestamp Range in the frame: the value - // is the difference between (a) the receive timestamp of the largest packet in the - // Timestamp Range (indicated by Gap) and (b) the session receive_timestamp_basis - pkt.timestamp.duration_since(basis).as_micros() as u64 - } else { - // For all other Timestamp Deltas: the value is the difference between - // (a) the receive timestamp specified by the previous Timestamp Delta and - // (b) the receive timestamp of the current packet in the Timestamp Range, decoded as described below. - basis.duration_since(pkt.timestamp).as_micros() as u64 - }; - buf.write_var(delta >> exponent); - basis = pkt.timestamp; - largest = pkt.packet_number; - counter += 1; - } - - right = segment_idx; - } - - debug_assert!( - counter <= max_timestamps, - "the number of timestamps in an ack frame exceeded the max allowed" - ); - } - - /// timestamp_iter returns an iterator that reads the timestamp records from newest to oldest - /// (or highest packet number to smallest). - pub(crate) fn timestamp_iter(&self, basis: Instant, exponent: u64) -> AckTimestampDecoder { - AckTimestampDecoder::new(self.largest, basis, exponent, &self.timestamps[..]) - } - - pub(crate) fn iter(&self) -> AckIter<'_> { - self.into_iter() - } -} - -impl<'a> IntoIterator for &'a AckTimestampFrame { - type Item = RangeInclusive; - type IntoIter = AckIter<'a>; - - fn into_iter(self) -> AckIter<'a> { - AckIter::new(self.largest, &self.ranges[..]) - } -} - -impl fmt::Debug for AckTimestampFrame { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - let mut ranges = "[".to_string(); - let mut first = true; - for range in self.iter() { - if !first { - ranges.push(','); - } - write!(ranges, "{range:?}").unwrap(); - first = false; - } - ranges.push(']'); - - let timestamp_count = self.timestamp_iter(Instant::now(), 0).count(); - - f.debug_struct("AckTimestamp") - .field("largest", &self.largest) - .field("delay", &self.delay) - .field("ranges", &ranges) - .field("timestamps_count", ×tamp_count) - .finish() - } -} - -pub(crate) struct AckTimestampDecoder<'a> { - timestamp_basis: u64, - timestamp_exponent: u64, - timestamp_instant_basis: Instant, - data: &'a [u8], - - deltas_remaining: usize, - first: bool, - next_pn: u64, -} - -impl<'a> AckTimestampDecoder<'a> { - fn new(largest: u64, basis_instant: Instant, exponent: u64, mut data: &'a [u8]) -> Self { - // We read and throw away the Timestamp Range Count value because - // it was already used to properly slice the data. - let _ = data.get_var().unwrap(); - AckTimestampDecoder { - timestamp_basis: 0, - timestamp_exponent: exponent, - timestamp_instant_basis: basis_instant, - data, - deltas_remaining: 0, - first: true, - next_pn: largest, - } - } -} - -impl<'a> Iterator for AckTimestampDecoder<'a> { - type Item = PacketTimestamp; - fn next(&mut self) -> Option { - if !self.data.has_remaining() { - debug_assert!( - self.deltas_remaining == 0, - "timestamp delta remaining should be 0" - ); - return None; - } - if self.deltas_remaining == 0 { - let gap = self.data.get_var().unwrap(); - self.deltas_remaining = self.data.get_var().unwrap() as usize; - if self.first { - self.next_pn -= gap; - } else { - self.next_pn -= gap + 2; - } - } else { - self.next_pn -= 1; - } - - let delta = self.data.get_var().unwrap(); - self.deltas_remaining -= 1; - - if self.first { - self.timestamp_basis = delta << self.timestamp_exponent; - self.first = false; - } else { - self.timestamp_basis -= delta << self.timestamp_exponent; - } - - Some(PacketTimestamp { - packet_number: self.next_pn, - timestamp: self.timestamp_instant_basis + Duration::from_micros(self.timestamp_basis), - }) - } -} - -#[cfg(test)] -mod test { - use super::*; - - #[test] - fn timestamp_iter() { - let mut timestamps = ReceiverTimestamps::new(100); - let second = Duration::from_secs(1); - let t0 = Instant::now(); - timestamps.add(1, t0 + second); - timestamps.add(2, t0 + second * 2); - timestamps.add(3, t0 + second * 3); - let mut buf = bytes::BytesMut::new(); - - AckTimestampFrame::encode_timestamps(×tamps, 12, &mut buf, t0, 0, 10); - - // Manually decode and assert the values in the buffer. - assert_eq!(1, buf.get_var().unwrap()); // timestamp_range_count - assert_eq!(9, buf.get_var().unwrap()); // gap: 12-3 - assert_eq!(3, buf.get_var().unwrap()); // timestamp delta count - assert_eq!(3_000_000, buf.get_var().unwrap()); // timestamp delta: 3_000_000 μs = 3 seconds = diff between largest timestamp and basis - assert_eq!(1_000_000, buf.get_var().unwrap()); // timestamp delta: 1 second diff - assert_eq!(1_000_000, buf.get_var().unwrap()); // timestamp delta: 1 second diff - assert!(buf.get_var().is_err()); - } - - #[test] - fn timestamp_iter_with_gaps() { - let mut timestamps = ReceiverTimestamps::new(100); - let one_second = Duration::from_secs(1); - let t0 = Instant::now(); - vec![(1..=3), (5..=5), (10..=12)] - .into_iter() - .flatten() - .for_each(|i| timestamps.add(i, t0 + one_second * i as u32)); - - let mut buf = bytes::BytesMut::new(); - - AckTimestampFrame::encode_timestamps(×tamps, 12, &mut buf, t0, 0, 10); - // Manually decode and assert the values in the buffer. - assert_eq!(3, buf.get_var().unwrap()); // timestamp_range_count - // - assert_eq!(0, buf.get_var().unwrap()); // gap: 12 - 12 = 0 - assert_eq!(3, buf.get_var().unwrap()); // timestamp_delta_count - assert_eq!(12_000_000, buf.get_var().unwrap()); // delta: 3_000_000 μs = 3 seconds = diff between largest timestamp and basis - assert_eq!(1_000_000, buf.get_var().unwrap()); // delta: 1 second diff - assert_eq!(1_000_000, buf.get_var().unwrap()); // delta: 1 second diff - // - assert_eq!(3, buf.get_var().unwrap()); // gap: 10 - 2 - 5 = 3 - assert_eq!(1, buf.get_var().unwrap()); // timestamp_delta_count - assert_eq!(5_000_000, buf.get_var().unwrap()); // delta: 1 second diff - - assert_eq!(0, buf.get_var().unwrap()); // gap - assert_eq!(3, buf.get_var().unwrap()); // timestamp_delta_count - assert_eq!(2_000_000, buf.get_var().unwrap()); // delta: 2 second diff - assert_eq!(1_000_000, buf.get_var().unwrap()); // delta: 1 second diff - assert_eq!(1_000_000, buf.get_var().unwrap()); // delta: 1 second diff - - // end - assert!(buf.get_var().is_err()); - } - - #[test] - fn timestamp_iter_with_exponent() { - let mut timestamps = ReceiverTimestamps::new(100); - let millisecond = Duration::from_millis(1); - let t0 = Instant::now(); - timestamps.add(1, t0 + millisecond * 200); - timestamps.add(2, t0 + millisecond * 300); - let mut buf = bytes::BytesMut::new(); - - let exponent = 2; - AckTimestampFrame::encode_timestamps(×tamps, 12, &mut buf, t0, exponent, 10); - - // values below are tested in another unit test - buf.get_var().unwrap(); // timestamp_range_count - buf.get_var().unwrap(); // gap - buf.get_var().unwrap(); // timestamp_delta_count - assert_eq!(300_000 >> exponent, buf.get_var().unwrap()); // 300ms diff - assert_eq!(100_000 >> exponent, buf.get_var().unwrap()); // 100ms diff - assert!(buf.get_var().is_err()); - } - - #[test] - fn timestamp_encode_decode() { - let mut timestamps = ReceiverTimestamps::new(100); - let one_second = Duration::from_secs(1); - let t0 = Instant::now(); - timestamps.add(1, t0 + one_second); - timestamps.add(2, t0 + one_second * 2); - timestamps.add(3, t0 + one_second * 3); - - let mut buf = bytes::BytesMut::new(); - - AckTimestampFrame::encode_timestamps(×tamps, 12, &mut buf, t0, 0, 10); - - let decoder = AckTimestampDecoder::new(12, t0, 0, &buf); - - let got: Vec<_> = decoder.collect(); - // [(3, _), (2, _), (1, _)] - assert_eq!(3, got.len()); - assert_eq!(3, got[0].packet_number); - assert_eq!(t0 + (3 * one_second), got[0].timestamp,); - - assert_eq!(2, got[1].packet_number); - assert_eq!(t0 + (2 * one_second), got[1].timestamp,); - - assert_eq!(1, got[2].packet_number); - assert_eq!(t0 + (1 * one_second), got[2].timestamp); - } - - #[test] - fn timestamp_encode_decode_with_gaps() { - let mut timestamps = ReceiverTimestamps::new(100); - let one_second = Duration::from_secs(1); - let t0 = Instant::now(); - let expect: Vec<_> = vec![(1..=3), (5..=5), (10..=12)] - .into_iter() - .flatten() - .collect::>() - .into_iter() - .map(|i| { - let t = t0 + one_second * i as u32; - timestamps.add(i, t); - PacketTimestamp { - packet_number: i, - timestamp: t, - } - }) - .collect(); - - let mut buf = bytes::BytesMut::new(); - - AckTimestampFrame::encode_timestamps(×tamps, 12, &mut buf, t0, 0, 10); - - let decoder = AckTimestampDecoder::new(12, t0, 0, &buf); - let got: Vec<_> = decoder.collect(); - - assert_eq!(7, got.len()); - assert_eq!(expect, got.into_iter().rev().collect::>()); - } - - #[test] - fn timestamp_encode_max_ack() { - // fix this - let mut timestamps = ReceiverTimestamps::new(2); - let one_second = Duration::from_secs(1); - let t0 = Instant::now(); - let expect: Vec<_> = vec![(1..=3), (5..=5), (10..=12)] - .into_iter() - .flatten() - .collect::>() - .into_iter() - .map(|i| { - let t = t0 + one_second * i as u32; - timestamps.add(i, t); - PacketTimestamp { - packet_number: i, - timestamp: t, - } - }) - .collect(); - - let mut buf = bytes::BytesMut::new(); - - AckTimestampFrame::encode_timestamps(×tamps, 12, &mut buf, t0, 0, 2); - let decoder = AckTimestampDecoder::new(12, t0, 0, &buf); - let got: Vec<_> = decoder.collect(); - - assert_eq!(2, got.len()); - assert_eq!( - expect[expect.len() - 2..expect.len()], // the last 2 values - got.into_iter().rev().collect::>() - ); - } -} diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index 834d57c25..2b4f5f56e 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -38,8 +38,6 @@ use crate::{ use crate::config::AckTimestampsConfig; -use crate::ack_timestamp_frame::AckTimestampFrame; - mod ack_frequency; use ack_frequency::AckFrequencyState; @@ -249,8 +247,8 @@ pub struct Connection { stats: ConnectionStats, /// QUIC version used for the connection. version: u32, - /// Instant created - created_at: Instant, + /// Created at time instant. + epoch: Instant, } impl Connection { @@ -372,7 +370,7 @@ impl Connection { rng, stats: ConnectionStats::default(), version, - created_at: now, + epoch: now, }; if side.is_client() { // Kick off the connection @@ -1351,21 +1349,21 @@ impl Connection { } } - fn on_ack_with_timestamp_received( + fn on_ack_received( &mut self, now: Instant, space: SpaceId, - ack: AckTimestampFrame, + ack: frame::Ack, ) -> Result<(), TransportError> { if ack.largest >= self.spaces[space].next_packet_number { return Err(TransportError::PROTOCOL_VIOLATION("unsent packet acked")); } - let Some(timestamp_config) = self.config.ack_timestamp_config.as_ref() else { + if ack.timestamps.is_some() != self.config.ack_timestamp_config.is_some() { return Err(TransportError::PROTOCOL_VIOLATION( - "no timestamp config set", + "ack with timestamps expectation mismatched", )); - }; + } let new_largest = { let space = &mut self.spaces[space]; @@ -1386,14 +1384,6 @@ impl Connection { } }; - let decoder = ack.timestamp_iter(timestamp_config.basis, timestamp_config.exponent.0); - let mut timestamp_iter = { - let mut v: tinyvec::TinyVec<[PacketTimestamp; 10]> = tinyvec::TinyVec::new(); - decoder.for_each(|elt| v.push(elt)); - v.reverse(); - v.into_iter().peekable() - }; - // Avoid DoS from unreasonably huge ack ranges by filtering out just the new acks. let mut newly_acked = ArrayRangeSet::new(); for range in ack.iter() { @@ -1403,6 +1393,14 @@ impl Connection { } } + let mut timestamp_iter = self.config.ack_timestamp_config.as_ref().map(|cfg| { + let decoder = ack.timestamp_iter(cfg.basis, cfg.exponent.0).unwrap(); + let mut v: tinyvec::TinyVec<[PacketTimestamp; 10]> = tinyvec::TinyVec::new(); + decoder.for_each(|elt| v.push(elt)); + v.reverse(); + v.into_iter().peekable() + }); + if newly_acked.is_empty() { return Ok(()); } @@ -1431,125 +1429,24 @@ impl Connection { // Notify ack frequency that a packet was acked, because it might contain an ACK_FREQUENCY frame self.ack_frequency.on_acked(packet); - while let Some(v) = timestamp_iter.peek() { - match v.packet_number.cmp(&packet) { - cmp::Ordering::Less => { - let _ = timestamp_iter.next(); - } - cmp::Ordering::Equal => { - // Unwrap safety is guaranteed because a value was validated - // to exist using peek - let ts = timestamp_iter.next().unwrap(); - info.time_received = Some(ts.timestamp); - } - cmp::Ordering::Greater => { - break; + if let Some(timestamp_iter) = timestamp_iter.as_mut() { + while let Some(v) = timestamp_iter.peek() { + match v.packet_number.cmp(&packet) { + cmp::Ordering::Less => { + let _ = timestamp_iter.next(); + } + cmp::Ordering::Equal => { + // Unwrap safety is guaranteed because a value was validated + // to exist using peek + let ts = timestamp_iter.next().unwrap(); + info.time_received = Some(ts.timestamp); + } + cmp::Ordering::Greater => { + break; + } } } } - self.on_packet_acked(now, packet, info); - } - } - - self.path.congestion.on_end_acks( - now, - self.path.in_flight.bytes, - self.app_limited, - self.spaces[space].largest_acked_packet, - ); - - if new_largest && ack_eliciting_acked { - let ack_delay = if space != SpaceId::Data { - Duration::from_micros(0) - } else { - cmp::min( - self.ack_frequency.peer_max_ack_delay, - Duration::from_micros(ack.delay << self.peer_params.ack_delay_exponent.0), - ) - }; - let rtt = instant_saturating_sub(now, self.spaces[space].largest_acked_packet_sent); - self.path.rtt.update(ack_delay, rtt); - if self.path.first_packet_after_rtt_sample.is_none() { - self.path.first_packet_after_rtt_sample = - Some((space, self.spaces[space].next_packet_number)); - } - } - - // Must be called before crypto/pto_count are clobbered - self.detect_lost_packets(now, space, true); - - if self.peer_completed_address_validation() { - self.pto_count = 0; - } - - self.set_loss_detection_timer(now); - Ok(()) - } - - fn on_ack_received( - &mut self, - now: Instant, - space: SpaceId, - ack: frame::Ack, - ) -> Result<(), TransportError> { - if ack.largest >= self.spaces[space].next_packet_number { - return Err(TransportError::PROTOCOL_VIOLATION("unsent packet acked")); - } - let new_largest = { - let space = &mut self.spaces[space]; - if space - .largest_acked_packet - .map_or(true, |pn| ack.largest > pn) - { - space.largest_acked_packet = Some(ack.largest); - if let Some(info) = space.sent_packets.get(&ack.largest) { - // This should always succeed, but a misbehaving peer might ACK a packet we - // haven't sent. At worst, that will result in us spuriously reducing the - // congestion window. - space.largest_acked_packet_sent = info.time_sent; - } - true - } else { - false - } - }; - - // Avoid DoS from unreasonably huge ack ranges by filtering out just the new acks. - let mut newly_acked = ArrayRangeSet::new(); - for range in ack.iter() { - self.packet_number_filter.check_ack(space, range.clone())?; - for (&pn, _) in self.spaces[space].sent_packets.range(range) { - newly_acked.insert_one(pn); - } - } - - if newly_acked.is_empty() { - return Ok(()); - } - - let mut ack_eliciting_acked = false; - for packet in newly_acked.elts() { - if let Some(info) = self.spaces[space].take(packet) { - if let Some(acked) = info.largest_acked { - // Assume ACKs for all packets below the largest acknowledged in `packet` have - // been received. This can cause the peer to spuriously retransmit if some of - // our earlier ACKs were lost, but allows for simpler state tracking. See - // discussion at - // https://www.rfc-editor.org/rfc/rfc9000.html#name-limiting-ranges-by-tracking - self.spaces[space].pending_acks.subtract_below(acked); - } - ack_eliciting_acked |= info.ack_eliciting; - - // Notify MTU discovery that a packet was acked, because it might be an MTU probe - let mtu_updated = self.path.mtud.on_acked(space, packet, info.size); - if mtu_updated { - self.path - .congestion - .on_mtu_update(self.path.mtud.current_mtu()); - } - - // Notify ack frequency that a packet was acked, because it might contain an ACK_FREQUENCY frame - self.ack_frequency.on_acked(packet); self.on_packet_acked(now, packet, info); } @@ -2734,11 +2631,6 @@ impl Connection { self.state = State::Draining; return Ok(()); } - - Frame::AckTimestamps(ack) => { - self.on_ack_with_timestamp_received(now, packet.header.space(), ack)?; - } - _ => { let mut err = TransportError::PROTOCOL_VIOLATION("illegal frame type in handshake"); @@ -2831,11 +2723,6 @@ impl Connection { Frame::Ack(ack) => { self.on_ack_received(now, SpaceId::Data, ack)?; } - - Frame::AckTimestamps(ack) => { - self.on_ack_with_timestamp_received(now, SpaceId::Data, ack)?; - } - Frame::Padding | Frame::Ping => {} Frame::Close(reason) => { close = Some(reason); @@ -3431,20 +3318,20 @@ impl Connection { delay_micros ); - if timestamp_config.is_some() { - let timestamp_config = timestamp_config.unwrap(); - AckTimestampFrame::encode( - delay as _, - space.pending_acks.ranges(), - space.pending_acks.receiver_timestamps_as_ref().unwrap(), - timestamp_config.basis, - timestamp_config.exponent.0, - timestamp_config.max_timestamps_per_ack.0, - buf, - ); - } else { - frame::Ack::encode(delay as _, space.pending_acks.ranges(), ecn, buf); - } + frame::Ack::encode( + delay as _, + space.pending_acks.ranges(), + ecn, + timestamp_config.map(|cfg| { + ( + space.pending_acks.receiver_timestamps_as_ref().unwrap(), + cfg.basis, + cfg.exponent.0, + cfg.max_timestamps_per_ack.0, + ) + }), + buf, + ); stats.frame_tx.acks += 1; } @@ -3513,7 +3400,7 @@ impl Connection { Some(AckTimestampsConfig { exponent, max_timestamps_per_ack, - basis: self.created_at, + basis: self.epoch, }) } else { None diff --git a/quinn-proto/src/connection/receiver_timestamps.rs b/quinn-proto/src/connection/receiver_timestamps.rs index 3f7e8e3d9..ad0dfc4b2 100644 --- a/quinn-proto/src/connection/receiver_timestamps.rs +++ b/quinn-proto/src/connection/receiver_timestamps.rs @@ -4,7 +4,7 @@ use std::time::Instant; use tracing::warn; #[derive(Debug, Clone, PartialEq, Eq)] -pub(crate) struct PacketTimestamp { +pub struct PacketTimestamp { pub packet_number: u64, pub timestamp: Instant, } @@ -18,7 +18,7 @@ impl Default for PacketTimestamp { } } -pub(crate) struct ReceiverTimestamps { +pub struct ReceiverTimestamps { data: VecDeque, max: usize, } diff --git a/quinn-proto/src/connection/stats.rs b/quinn-proto/src/connection/stats.rs index 07b33c962..fc19c6304 100644 --- a/quinn-proto/src/connection/stats.rs +++ b/quinn-proto/src/connection/stats.rs @@ -62,7 +62,6 @@ impl FrameStats { Frame::Padding => {} Frame::Ping => self.ping += 1, Frame::Ack(_) => self.acks += 1, - Frame::AckTimestamps(_) => self.acks += 1, Frame::ResetStream(_) => self.reset_stream += 1, Frame::StopSending(_) => self.stop_sending += 1, Frame::Crypto(_) => self.crypto += 1, diff --git a/quinn-proto/src/frame.rs b/quinn-proto/src/frame.rs index 1d326d088..7e327d90b 100644 --- a/quinn-proto/src/frame.rs +++ b/quinn-proto/src/frame.rs @@ -2,6 +2,7 @@ use std::{ fmt::{self, Write}, io, mem, ops::{Range, RangeInclusive}, + time::{Duration, Instant}, }; use bytes::{Buf, BufMut, Bytes}; @@ -9,14 +10,13 @@ use tinyvec::TinyVec; use crate::{ coding::{self, BufExt, BufMutExt, UnexpectedEnd}, + connection::{PacketTimestamp, ReceiverTimestamps}, range_set::ArrayRangeSet, shared::{ConnectionId, EcnCodepoint}, Dir, ResetToken, StreamId, TransportError, TransportErrorCode, VarInt, MAX_CID_SIZE, RESET_TOKEN_SIZE, }; -use crate::ack_timestamp_frame; - #[cfg(feature = "arbitrary")] use arbitrary::Arbitrary; @@ -147,7 +147,6 @@ pub(crate) enum Frame { Padding, Ping, Ack(Ack), - AckTimestamps(ack_timestamp_frame::AckTimestampFrame), ResetStream(ResetStream), StopSending(StopSending), Crypto(Crypto), @@ -190,7 +189,6 @@ impl Frame { StopSending { .. } => Type::STOP_SENDING, RetireConnectionId { .. } => Type::RETIRE_CONNECTION_ID, Ack(_) => Type::ACK, - AckTimestamps(_) => Type::ACK_RECEIVE_TIMESTAMPS, Stream(ref x) => { let mut ty = *STREAM_TYS.start(); if x.fin { @@ -349,6 +347,7 @@ pub struct Ack { pub delay: u64, pub additional: Bytes, pub ecn: Option, + pub timestamps: Option, } impl fmt::Debug for Ack { @@ -364,11 +363,16 @@ impl fmt::Debug for Ack { } ranges.push(']'); + let timestamp_count = self + .timestamp_iter(Instant::now(), 0) + .map(|iter| iter.count()); + f.debug_struct("Ack") .field("largest", &self.largest) .field("delay", &self.delay) .field("ecn", &self.ecn) .field("ranges", &ranges) + .field("timestamp_count", ×tamp_count) .finish() } } @@ -387,13 +391,21 @@ impl Ack { delay: u64, ranges: &ArrayRangeSet, ecn: Option<&EcnCounts>, + timestamps: Option<( + &ReceiverTimestamps, + Instant, + u64, // exponent + u64, // max_timestamps + )>, buf: &mut W, ) { let mut rest = ranges.iter().rev(); let first = rest.next().unwrap(); let largest = first.end - 1; let first_size = first.end - first.start; - buf.write(if ecn.is_some() { + buf.write(if timestamps.is_some() { + Type::ACK_RECEIVE_TIMESTAMPS + } else if ecn.is_some() { Type::ACK_ECN } else { Type::ACK @@ -409,7 +421,9 @@ impl Ack { buf.write_var(size - 1); prev = block.start; } - if let Some(x) = ecn { + if let Some(x) = timestamps { + Self::encode_timestamps(x.0, largest, buf, x.1, x.2, x.3) + } else if let Some(x) = ecn { x.encode(buf) } } @@ -417,6 +431,101 @@ impl Ack { pub fn iter(&self) -> AckIter<'_> { self.into_iter() } + + pub fn timestamp_iter(&self, basis: Instant, exponent: u64) -> Option { + self.timestamps + .as_ref() + .map(|v| AckTimestampIter::new(self.largest, basis, exponent, &v[..])) + } + + // https://www.ietf.org/archive/id/draft-smith-quic-receive-ts-00.html#ts-ranges + fn encode_timestamps( + timestamps: &ReceiverTimestamps, + mut largest: u64, + buf: &mut W, + mut basis: Instant, + exponent: u64, + max_timestamps: u64, + ) { + if timestamps.len() == 0 { + buf.write_var(0); + return; + } + let mut prev: Option = None; + + // segment_idx tracks the positions in `timestamps` in which a gap occurs. + let mut segment_idxs = Vec::::new(); + // iterates from largest number to smallest + for (i, pkt) in timestamps.iter().rev().enumerate() { + if let Some(prev) = prev { + if pkt.packet_number + 1 != prev { + segment_idxs.push(timestamps.len() - i); + } + } + prev = Some(pkt.packet_number); + } + segment_idxs.push(0); + // Timestamp Range Count + buf.write_var(segment_idxs.len() as u64); + + let mut right = timestamps.len(); + let mut first = true; + let mut counter = 0; + + for segment_idx in segment_idxs { + let Some(elt) = timestamps.inner().get(right - 1) else { + debug_assert!( + false, + "an invalid indexing occurred on the ReceiverTimestamps vector" + ); + break; + }; + // *Gap + // For the first Timestamp Range: Gap is the difference between (a) the Largest Acknowledged packet number + // in the frame and (b) the largest packet in the current (first) Timestamp Range. + let gap = if first { + debug_assert!( + elt.packet_number <= largest, + "largest packet number is less than what was found in timestamp vec" + ); + largest - elt.packet_number + } else { + // For subsequent Timestamp Ranges: Gap is the difference between (a) the packet number two lower + // than the smallest packet number in the previous Timestamp Range + // and (b) the largest packet in the current Timestamp Range. + largest - 2 - elt.packet_number + }; + buf.write_var(gap); + // *Timestamp Delta Count + buf.write_var((right - segment_idx) as u64); + // *Timestamp Deltas + for pkt in timestamps.inner().range(segment_idx..right).rev() { + let delta: u64 = if first { + first = false; + // For the first Timestamp Delta of the first Timestamp Range in the frame: the value + // is the difference between (a) the receive timestamp of the largest packet in the + // Timestamp Range (indicated by Gap) and (b) the session receive_timestamp_basis + pkt.timestamp.duration_since(basis).as_micros() as u64 + } else { + // For all other Timestamp Deltas: the value is the difference between + // (a) the receive timestamp specified by the previous Timestamp Delta and + // (b) the receive timestamp of the current packet in the Timestamp Range, decoded as described below. + basis.duration_since(pkt.timestamp).as_micros() as u64 + }; + buf.write_var(delta >> exponent); + basis = pkt.timestamp; + largest = pkt.packet_number; + counter += 1; + } + + right = segment_idx; + } + + debug_assert!( + counter <= max_timestamps, + "the number of timestamps in an ack frame exceeded the max allowed" + ); + } } #[derive(Debug, Copy, Clone, Eq, PartialEq)] @@ -625,7 +734,7 @@ impl Iter { Type::RETIRE_CONNECTION_ID => Frame::RetireConnectionId { sequence: self.bytes.get_var()?, }, - Type::ACK | Type::ACK_ECN => { + Type::ACK | Type::ACK_ECN | Type::ACK_RECEIVE_TIMESTAMPS => { let largest = self.bytes.get_var()?; let delay = self.bytes.get_var()?; let extra_blocks = self.bytes.get_var()? as usize; @@ -645,25 +754,14 @@ impl Iter { ce: self.bytes.get_var()?, }) }, - }) - } - Type::ACK_RECEIVE_TIMESTAMPS => { - let largest = self.bytes.get_var()?; - let delay = self.bytes.get_var()?; - let range_count = self.bytes.get_var()? as usize; - let start = self.bytes.position() as usize; - scan_ack_blocks(&mut self.bytes, largest, range_count)?; - let end = self.bytes.position() as usize; - Frame::AckTimestamps(ack_timestamp_frame::AckTimestampFrame { - delay, - largest, - ranges: self.bytes.get_ref().slice(start..end), - timestamps: { + timestamps: if ty != Type::ACK_RECEIVE_TIMESTAMPS { + None + } else { let ts_start = end; let ts_range_count = self.bytes.get_var()?; scan_ack_timestamp_blocks(&mut self.bytes, largest, ts_range_count)?; let ts_end = self.bytes.position() as usize; - self.bytes.get_ref().slice(ts_start..ts_end) + Some(self.bytes.get_ref().slice(ts_start..ts_end)) }, }) } @@ -849,6 +947,73 @@ impl From for IterErr { } } +pub struct AckTimestampIter<'a> { + timestamp_basis: u64, + timestamp_exponent: u64, + timestamp_instant_basis: Instant, + data: &'a [u8], + + deltas_remaining: usize, + first: bool, + next_pn: u64, +} + +impl<'a> AckTimestampIter<'a> { + fn new(largest: u64, basis_instant: Instant, exponent: u64, mut data: &'a [u8]) -> Self { + // We read and throw away the Timestamp Range Count value because + // it was already used to properly slice the data. + let _ = data.get_var().unwrap(); + AckTimestampIter { + timestamp_basis: 0, + timestamp_exponent: exponent, + timestamp_instant_basis: basis_instant, + data, + deltas_remaining: 0, + first: true, + next_pn: largest, + } + } +} + +impl<'a> Iterator for AckTimestampIter<'a> { + type Item = PacketTimestamp; + fn next(&mut self) -> Option { + if !self.data.has_remaining() { + debug_assert!( + self.deltas_remaining == 0, + "timestamp delta remaining should be 0" + ); + return None; + } + if self.deltas_remaining == 0 { + let gap = self.data.get_var().unwrap(); + self.deltas_remaining = self.data.get_var().unwrap() as usize; + if self.first { + self.next_pn -= gap; + } else { + self.next_pn -= gap + 2; + } + } else { + self.next_pn -= 1; + } + + let delta = self.data.get_var().unwrap(); + self.deltas_remaining -= 1; + + if self.first { + self.timestamp_basis = delta << self.timestamp_exponent; + self.first = false; + } else { + self.timestamp_basis -= delta << self.timestamp_exponent; + } + + Some(PacketTimestamp { + packet_number: self.next_pn, + timestamp: self.timestamp_instant_basis + Duration::from_micros(self.timestamp_basis), + }) + } +} + #[derive(Debug, Clone)] pub struct AckIter<'a> { largest: u64, @@ -1014,7 +1179,7 @@ mod test { ect1: 24, ce: 12, }; - Ack::encode(42, &ranges, Some(&ECN), &mut buf); + Ack::encode(42, &ranges, Some(&ECN), None, &mut buf); let frames = frames(buf); assert_eq!(frames.len(), 1); match frames[0] { @@ -1131,5 +1296,177 @@ mod test { scan_ack_timestamp_blocks(&mut c, 5, 2).unwrap_err(), ); } + + #[test] + fn timestamp_iter() { + let mut timestamps = ReceiverTimestamps::new(100); + let second = Duration::from_secs(1); + let t0 = Instant::now(); + timestamps.add(1, t0 + second); + timestamps.add(2, t0 + second * 2); + timestamps.add(3, t0 + second * 3); + let mut buf = bytes::BytesMut::new(); + + Ack::encode_timestamps(×tamps, 12, &mut buf, t0, 0, 10); + + // Manually decode and assert the values in the buffer. + assert_eq!(1, buf.get_var().unwrap()); // timestamp_range_count + assert_eq!(9, buf.get_var().unwrap()); // gap: 12-3 + assert_eq!(3, buf.get_var().unwrap()); // timestamp delta count + assert_eq!(3_000_000, buf.get_var().unwrap()); // timestamp delta: 3_000_000 μs = 3 seconds = diff between largest timestamp and basis + assert_eq!(1_000_000, buf.get_var().unwrap()); // timestamp delta: 1 second diff + assert_eq!(1_000_000, buf.get_var().unwrap()); // timestamp delta: 1 second diff + assert!(buf.get_var().is_err()); + } + + #[test] + fn timestamp_iter_with_gaps() { + let mut timestamps = ReceiverTimestamps::new(100); + let one_second = Duration::from_secs(1); + let t0 = Instant::now(); + vec![(1..=3), (5..=5), (10..=12)] + .into_iter() + .flatten() + .for_each(|i| timestamps.add(i, t0 + one_second * i as u32)); + + let mut buf = bytes::BytesMut::new(); + + Ack::encode_timestamps(×tamps, 12, &mut buf, t0, 0, 10); + // Manually decode and assert the values in the buffer. + assert_eq!(3, buf.get_var().unwrap()); // timestamp_range_count + // + assert_eq!(0, buf.get_var().unwrap()); // gap: 12 - 12 = 0 + assert_eq!(3, buf.get_var().unwrap()); // timestamp_delta_count + assert_eq!(12_000_000, buf.get_var().unwrap()); // delta: 3_000_000 μs = 3 seconds = diff between largest timestamp and basis + assert_eq!(1_000_000, buf.get_var().unwrap()); // delta: 1 second diff + assert_eq!(1_000_000, buf.get_var().unwrap()); // delta: 1 second diff + // + assert_eq!(3, buf.get_var().unwrap()); // gap: 10 - 2 - 5 = 3 + assert_eq!(1, buf.get_var().unwrap()); // timestamp_delta_count + assert_eq!(5_000_000, buf.get_var().unwrap()); // delta: 1 second diff + + assert_eq!(0, buf.get_var().unwrap()); // gap + assert_eq!(3, buf.get_var().unwrap()); // timestamp_delta_count + assert_eq!(2_000_000, buf.get_var().unwrap()); // delta: 2 second diff + assert_eq!(1_000_000, buf.get_var().unwrap()); // delta: 1 second diff + assert_eq!(1_000_000, buf.get_var().unwrap()); // delta: 1 second diff + + // end + assert!(buf.get_var().is_err()); + } + + #[test] + fn timestamp_iter_with_exponent() { + let mut timestamps = ReceiverTimestamps::new(100); + let millisecond = Duration::from_millis(1); + let t0 = Instant::now(); + timestamps.add(1, t0 + millisecond * 200); + timestamps.add(2, t0 + millisecond * 300); + let mut buf = bytes::BytesMut::new(); + + let exponent = 2; + Ack::encode_timestamps(×tamps, 12, &mut buf, t0, exponent, 10); + + // values below are tested in another unit test + buf.get_var().unwrap(); // timestamp_range_count + buf.get_var().unwrap(); // gap + buf.get_var().unwrap(); // timestamp_delta_count + assert_eq!(300_000 >> exponent, buf.get_var().unwrap()); // 300ms diff + assert_eq!(100_000 >> exponent, buf.get_var().unwrap()); // 100ms diff + assert!(buf.get_var().is_err()); + } + + #[test] + fn timestamp_encode_decode() { + let mut timestamps = ReceiverTimestamps::new(100); + let one_second = Duration::from_secs(1); + let t0 = Instant::now(); + timestamps.add(1, t0 + one_second); + timestamps.add(2, t0 + one_second * 2); + timestamps.add(3, t0 + one_second * 3); + + let mut buf = bytes::BytesMut::new(); + + Ack::encode_timestamps(×tamps, 12, &mut buf, t0, 0, 10); + + let decoder = AckTimestampIter::new(12, t0, 0, &buf); + + let got: Vec<_> = decoder.collect(); + // [(3, _), (2, _), (1, _)] + assert_eq!(3, got.len()); + assert_eq!(3, got[0].packet_number); + assert_eq!(t0 + (3 * one_second), got[0].timestamp,); + + assert_eq!(2, got[1].packet_number); + assert_eq!(t0 + (2 * one_second), got[1].timestamp,); + + assert_eq!(1, got[2].packet_number); + assert_eq!(t0 + (1 * one_second), got[2].timestamp); + } + + #[test] + fn timestamp_encode_decode_with_gaps() { + let mut timestamps = ReceiverTimestamps::new(100); + let one_second = Duration::from_secs(1); + let t0 = Instant::now(); + let expect: Vec<_> = vec![(1..=3), (5..=5), (10..=12)] + .into_iter() + .flatten() + .collect::>() + .into_iter() + .map(|i| { + let t = t0 + one_second * i as u32; + timestamps.add(i, t); + PacketTimestamp { + packet_number: i, + timestamp: t, + } + }) + .collect(); + + let mut buf = bytes::BytesMut::new(); + + Ack::encode_timestamps(×tamps, 12, &mut buf, t0, 0, 10); + + let decoder = AckTimestampIter::new(12, t0, 0, &buf); + let got: Vec<_> = decoder.collect(); + + assert_eq!(7, got.len()); + assert_eq!(expect, got.into_iter().rev().collect::>()); + } + + #[test] + fn timestamp_encode_max_ack() { + // fix this + let mut timestamps = ReceiverTimestamps::new(2); + let one_second = Duration::from_secs(1); + let t0 = Instant::now(); + let expect: Vec<_> = vec![(1..=3), (5..=5), (10..=12)] + .into_iter() + .flatten() + .collect::>() + .into_iter() + .map(|i| { + let t = t0 + one_second * i as u32; + timestamps.add(i, t); + PacketTimestamp { + packet_number: i, + timestamp: t, + } + }) + .collect(); + + let mut buf = bytes::BytesMut::new(); + + Ack::encode_timestamps(×tamps, 12, &mut buf, t0, 0, 2); + let decoder = AckTimestampIter::new(12, t0, 0, &buf); + let got: Vec<_> = decoder.collect(); + + assert_eq!(2, got.len()); + assert_eq!( + expect[expect.len() - 2..expect.len()], // the last 2 values + got.into_iter().rev().collect::>() + ); + } } } diff --git a/quinn-proto/src/lib.rs b/quinn-proto/src/lib.rs index 5b1cd5755..9cb196184 100644 --- a/quinn-proto/src/lib.rs +++ b/quinn-proto/src/lib.rs @@ -87,8 +87,6 @@ pub use crate::cid_generator::{ mod token; use token::{ResetToken, RetryToken}; -mod ack_timestamp_frame; - #[cfg(feature = "arbitrary")] use arbitrary::Arbitrary; From ea3bb6a65d8f9e176679940b32795998bcc40795 Mon Sep 17 00:00:00 2001 From: Sterling Deng Date: Thu, 26 Sep 2024 11:57:44 -0700 Subject: [PATCH 18/30] add comment --- quinn-proto/src/connection/mod.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index 2b4f5f56e..89b124e1a 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -3293,7 +3293,7 @@ impl Connection { space: &mut PacketSpace, buf: &mut Vec, stats: &mut ConnectionStats, - timestamp_config: Option, + peer_timestamp_config: Option, ) { debug_assert!(!space.pending_acks.ranges().is_empty()); @@ -3322,8 +3322,9 @@ impl Connection { delay as _, space.pending_acks.ranges(), ecn, - timestamp_config.map(|cfg| { + peer_timestamp_config.map(|cfg| { ( + // Safety: If peer_timestamp_config is set, receiver_timestamps must be set. space.pending_acks.receiver_timestamps_as_ref().unwrap(), cfg.basis, cfg.exponent.0, From caa86bda2386e32aebec1bed028545bcb9affa61 Mon Sep 17 00:00:00 2001 From: Sterling Deng Date: Fri, 27 Sep 2024 10:44:53 -0700 Subject: [PATCH 19/30] revert some changes --- quinn-proto/src/connection/mod.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index 89b124e1a..8165e2a77 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -2243,10 +2243,8 @@ impl Connection { } Ok((packet, number)) => { let span = match number { - Some(pn) => { - trace_span!("recv", space = ?packet.header.space(), pn, side=?self.side) - } - None => trace_span!("recv", space = ?packet.header.space(), side=?self.side), + Some(pn) => trace_span!("recv", space = ?packet.header.space(), pn), + None => trace_span!("recv", space = ?packet.header.space()), }; let _guard = span.enter(); From 1e3c9361a39cb1783be86debe015198644a543e0 Mon Sep 17 00:00:00 2001 From: Sterling Deng Date: Tue, 1 Oct 2024 09:55:22 -0700 Subject: [PATCH 20/30] fix easier PR comments --- quinn-proto/src/config.rs | 4 +-- quinn-proto/src/connection/mod.rs | 4 +-- .../src/connection/receiver_timestamps.rs | 31 +++++++++---------- quinn-proto/src/connection/spaces.rs | 11 ++++--- quinn-proto/src/frame.rs | 12 +++---- quinn-proto/src/lib.rs | 5 ++- 6 files changed, 33 insertions(+), 34 deletions(-) diff --git a/quinn-proto/src/config.rs b/quinn-proto/src/config.rs index 7b429bb00..aabb08142 100644 --- a/quinn-proto/src/config.rs +++ b/quinn-proto/src/config.rs @@ -3,7 +3,7 @@ use std::{ net::{SocketAddrV4, SocketAddrV6}, num::TryFromIntError, sync::Arc, - time::Duration, + time::{Duration, Instant}, }; #[cfg(feature = "ring")] @@ -459,7 +459,7 @@ impl AckTimestampsConfig { /// Sets the time base for which all timestamps are anchored on. /// Defaults to Instant::now of when the default struct was created. - pub fn basis(&mut self, instant: std::time::Instant) -> &mut Self { + pub fn basis(&mut self, instant: Instant) -> &mut Self { self.basis = instant; self } diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index 8165e2a77..ba434ec1f 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -18,7 +18,7 @@ use crate::{ cid_generator::ConnectionIdGenerator, cid_queue::CidQueue, coding::BufMutExt, - config::{ServerConfig, TransportConfig}, + config::{AckTimestampsConfig, ServerConfig, TransportConfig}, crypto::{self, KeyPair, Keys, PacketKey}, frame::{self, Close, Datagram, FrameStruct}, packet::{ @@ -36,8 +36,6 @@ use crate::{ VarInt, MAX_STREAM_COUNT, MIN_INITIAL_SIZE, TIMER_GRANULARITY, }; -use crate::config::AckTimestampsConfig; - mod ack_frequency; use ack_frequency::AckFrequencyState; diff --git a/quinn-proto/src/connection/receiver_timestamps.rs b/quinn-proto/src/connection/receiver_timestamps.rs index ad0dfc4b2..03f69e0e9 100644 --- a/quinn-proto/src/connection/receiver_timestamps.rs +++ b/quinn-proto/src/connection/receiver_timestamps.rs @@ -1,9 +1,8 @@ -use std::collections::VecDeque; -use std::time::Instant; +use std::{collections::VecDeque, fmt, time::Instant}; use tracing::warn; -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Debug, Clone, PartialEq, Eq, Copy)] pub struct PacketTimestamp { pub packet_number: u64, pub timestamp: Instant, @@ -23,7 +22,7 @@ pub struct ReceiverTimestamps { max: usize, } -impl std::fmt::Debug for ReceiverTimestamps { +impl fmt::Debug for ReceiverTimestamps { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { let mut l = f.debug_list(); let mut last: Option<(u64, Instant)> = None; @@ -67,18 +66,6 @@ impl ReceiverTimestamps { }); } - pub(crate) fn iter(&self) -> impl DoubleEndedIterator + '_ { - self.data.iter().cloned() - } - - pub(crate) fn len(&self) -> usize { - self.data.len() - } - - pub(crate) fn inner(&self) -> &VecDeque { - &self.data - } - pub(crate) fn subtract_below(&mut self, packet_number: u64) { if self.data.is_empty() { return; @@ -92,6 +79,18 @@ impl ReceiverTimestamps { let _ = self.data.drain(0..=idx); } } + + pub(crate) fn iter(&self) -> impl DoubleEndedIterator + '_ { + self.data.iter().cloned() + } + + pub(crate) fn len(&self) -> usize { + self.data.len() + } + + pub(crate) fn inner(&self) -> &VecDeque { + &self.data + } } #[cfg(test)] diff --git a/quinn-proto/src/connection/spaces.rs b/quinn-proto/src/connection/spaces.rs index a087bb1af..a6baebc09 100644 --- a/quinn-proto/src/connection/spaces.rs +++ b/quinn-proto/src/connection/spaces.rs @@ -12,12 +12,15 @@ use tracing::trace; use super::assembler::Assembler; use crate::{ - connection::StreamsState, crypto::Keys, frame, packet::SpaceId, range_set::ArrayRangeSet, - shared::IssuedCid, Dir, StreamId, TransportError, VarInt, + connection::{receiver_timestamps::ReceiverTimestamps, StreamsState}, + crypto::Keys, + frame, + packet::SpaceId, + range_set::ArrayRangeSet, + shared::IssuedCid, + Dir, StreamId, TransportError, VarInt, }; -use crate::connection::receiver_timestamps::ReceiverTimestamps; - pub(super) struct PacketSpace { pub(super) crypto: Option, pub(super) dedup: Dedup, diff --git a/quinn-proto/src/frame.rs b/quinn-proto/src/frame.rs index 7e327d90b..4c5b5faf8 100644 --- a/quinn-proto/src/frame.rs +++ b/quinn-proto/src/frame.rs @@ -439,10 +439,10 @@ impl Ack { } // https://www.ietf.org/archive/id/draft-smith-quic-receive-ts-00.html#ts-ranges - fn encode_timestamps( + fn encode_timestamps( timestamps: &ReceiverTimestamps, mut largest: u64, - buf: &mut W, + buf: &mut impl BufMut, mut basis: Instant, exponent: u64, max_timestamps: u64, @@ -451,7 +451,7 @@ impl Ack { buf.write_var(0); return; } - let mut prev: Option = None; + let mut prev = None; // segment_idx tracks the positions in `timestamps` in which a gap occurs. let mut segment_idxs = Vec::::new(); @@ -924,7 +924,7 @@ enum IterErr { Malformed, } -impl std::fmt::Debug for IterErr { +impl fmt::Debug for IterErr { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.write_str(self.reason()) } @@ -979,8 +979,8 @@ impl<'a> Iterator for AckTimestampIter<'a> { type Item = PacketTimestamp; fn next(&mut self) -> Option { if !self.data.has_remaining() { - debug_assert!( - self.deltas_remaining == 0, + debug_assert_eq!( + self.deltas_remaining, 0, "timestamp delta remaining should be 0" ); return None; diff --git a/quinn-proto/src/lib.rs b/quinn-proto/src/lib.rs index 9cb196184..fd6f2e393 100644 --- a/quinn-proto/src/lib.rs +++ b/quinn-proto/src/lib.rs @@ -48,10 +48,9 @@ pub use crate::connection::{ }; mod config; -pub use config::AckTimestampsConfig; pub use config::{ - AckFrequencyConfig, ClientConfig, ConfigError, EndpointConfig, IdleTimeout, MtuDiscoveryConfig, - ServerConfig, TransportConfig, + AckFrequencyConfig, AckTimestampsConfig, ClientConfig, ConfigError, EndpointConfig, + IdleTimeout, MtuDiscoveryConfig, ServerConfig, TransportConfig, }; pub mod crypto; From 1cc4043a3d12ebabccc30dbdd348944a60a6dac4 Mon Sep 17 00:00:00 2001 From: Sterling Deng Date: Tue, 1 Oct 2024 11:27:18 -0700 Subject: [PATCH 21/30] refactor on nits change field name and arg names --- quinn-proto/src/config.rs | 12 +----------- quinn-proto/src/connection/mod.rs | 19 ++++++++++--------- quinn-proto/src/frame.rs | 12 ++++++------ 3 files changed, 17 insertions(+), 26 deletions(-) diff --git a/quinn-proto/src/config.rs b/quinn-proto/src/config.rs index aabb08142..15839b20d 100644 --- a/quinn-proto/src/config.rs +++ b/quinn-proto/src/config.rs @@ -3,7 +3,7 @@ use std::{ net::{SocketAddrV4, SocketAddrV6}, num::TryFromIntError, sync::Arc, - time::{Duration, Instant}, + time::Duration, }; #[cfg(feature = "ring")] @@ -52,7 +52,6 @@ pub struct TransportConfig { pub(crate) min_mtu: u16, pub(crate) mtu_discovery_config: Option, pub(crate) ack_frequency_config: Option, - pub(crate) ack_timestamp_config: Option, pub(crate) persistent_congestion_threshold: u32, @@ -440,7 +439,6 @@ impl fmt::Debug for TransportConfig { pub struct AckTimestampsConfig { pub(crate) max_timestamps_per_ack: VarInt, pub(crate) exponent: VarInt, - pub(crate) basis: std::time::Instant, } impl AckTimestampsConfig { @@ -456,13 +454,6 @@ impl AckTimestampsConfig { self.exponent = value; self } - - /// Sets the time base for which all timestamps are anchored on. - /// Defaults to Instant::now of when the default struct was created. - pub fn basis(&mut self, instant: Instant) -> &mut Self { - self.basis = instant; - self - } } impl Default for AckTimestampsConfig { @@ -470,7 +461,6 @@ impl Default for AckTimestampsConfig { Self { max_timestamps_per_ack: 10u32.into(), exponent: 0u32.into(), - basis: std::time::Instant::now(), } } } diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index ba434ec1f..c7e5e71a9 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -229,10 +229,9 @@ pub struct Connection { /// no outgoing application data. app_limited: bool, - // // Ack Receive Timestamps - // - peer_ack_timestamp_cfg: Option, + // The timestamp config of the peer. + ack_timestamp_cfg: Option, streams: StreamsState, /// Surplus remote CIDs for future use on new paths @@ -346,7 +345,7 @@ impl Connection { &TransportParameters::default(), )), - peer_ack_timestamp_cfg: None, + ack_timestamp_cfg: None, pto_count: 0, @@ -830,6 +829,7 @@ impl Connection { buf, &mut self.stats, None, + self.epoch, ); } @@ -1392,7 +1392,7 @@ impl Connection { } let mut timestamp_iter = self.config.ack_timestamp_config.as_ref().map(|cfg| { - let decoder = ack.timestamp_iter(cfg.basis, cfg.exponent.0).unwrap(); + let decoder = ack.timestamp_iter(self.epoch, cfg.exponent.0).unwrap(); let mut v: tinyvec::TinyVec<[PacketTimestamp; 10]> = tinyvec::TinyVec::new(); decoder.for_each(|elt| v.push(elt)); v.reverse(); @@ -3104,7 +3104,8 @@ impl Connection { space, buf, &mut self.stats, - self.peer_ack_timestamp_cfg.clone(), + self.ack_timestamp_cfg.clone(), + self.epoch, ); } @@ -3290,6 +3291,7 @@ impl Connection { buf: &mut Vec, stats: &mut ConnectionStats, peer_timestamp_config: Option, + epoch: Instant, ) { debug_assert!(!space.pending_acks.ranges().is_empty()); @@ -3322,7 +3324,7 @@ impl Connection { ( // Safety: If peer_timestamp_config is set, receiver_timestamps must be set. space.pending_acks.receiver_timestamps_as_ref().unwrap(), - cfg.basis, + epoch, cfg.exponent.0, cfg.max_timestamps_per_ack.0, ) @@ -3385,7 +3387,7 @@ impl Connection { ); { - self.peer_ack_timestamp_cfg = if let (Some(max_timestamps_per_ack), Some(exponent)) = ( + self.ack_timestamp_cfg = if let (Some(max_timestamps_per_ack), Some(exponent)) = ( params.max_recv_timestamps_per_ack, params.receive_timestamps_exponent, ) { @@ -3397,7 +3399,6 @@ impl Connection { Some(AckTimestampsConfig { exponent, max_timestamps_per_ack, - basis: self.epoch, }) } else { None diff --git a/quinn-proto/src/frame.rs b/quinn-proto/src/frame.rs index 4c5b5faf8..88d093553 100644 --- a/quinn-proto/src/frame.rs +++ b/quinn-proto/src/frame.rs @@ -432,10 +432,10 @@ impl Ack { self.into_iter() } - pub fn timestamp_iter(&self, basis: Instant, exponent: u64) -> Option { + pub fn timestamp_iter(&self, epoch: Instant, exponent: u64) -> Option { self.timestamps .as_ref() - .map(|v| AckTimestampIter::new(self.largest, basis, exponent, &v[..])) + .map(|v| AckTimestampIter::new(self.largest, epoch, exponent, &v[..])) } // https://www.ietf.org/archive/id/draft-smith-quic-receive-ts-00.html#ts-ranges @@ -950,7 +950,7 @@ impl From for IterErr { pub struct AckTimestampIter<'a> { timestamp_basis: u64, timestamp_exponent: u64, - timestamp_instant_basis: Instant, + epoch: Instant, data: &'a [u8], deltas_remaining: usize, @@ -959,14 +959,14 @@ pub struct AckTimestampIter<'a> { } impl<'a> AckTimestampIter<'a> { - fn new(largest: u64, basis_instant: Instant, exponent: u64, mut data: &'a [u8]) -> Self { + fn new(largest: u64, epoch: Instant, exponent: u64, mut data: &'a [u8]) -> Self { // We read and throw away the Timestamp Range Count value because // it was already used to properly slice the data. let _ = data.get_var().unwrap(); AckTimestampIter { timestamp_basis: 0, timestamp_exponent: exponent, - timestamp_instant_basis: basis_instant, + epoch, data, deltas_remaining: 0, first: true, @@ -1009,7 +1009,7 @@ impl<'a> Iterator for AckTimestampIter<'a> { Some(PacketTimestamp { packet_number: self.next_pn, - timestamp: self.timestamp_instant_basis + Duration::from_micros(self.timestamp_basis), + timestamp: self.epoch + Duration::from_micros(self.timestamp_basis), }) } } From 67af041a0b0b8f1a6194fe13c2eace0538788819 Mon Sep 17 00:00:00 2001 From: Sterling Deng Date: Tue, 1 Oct 2024 12:23:24 -0700 Subject: [PATCH 22/30] refactor transport parameter --- quinn-proto/src/config.rs | 16 +++--- quinn-proto/src/connection/mod.rs | 66 ++++++++++++------------- quinn-proto/src/transport_parameters.rs | 53 ++++++++------------ 3 files changed, 62 insertions(+), 73 deletions(-) diff --git a/quinn-proto/src/config.rs b/quinn-proto/src/config.rs index 15839b20d..e9f6602eb 100644 --- a/quinn-proto/src/config.rs +++ b/quinn-proto/src/config.rs @@ -52,7 +52,7 @@ pub struct TransportConfig { pub(crate) min_mtu: u16, pub(crate) mtu_discovery_config: Option, pub(crate) ack_frequency_config: Option, - pub(crate) ack_timestamp_config: Option, + pub(crate) ack_timestamp_config: AckTimestampsConfig, pub(crate) persistent_congestion_threshold: u32, pub(crate) keep_alive_interval: Option, @@ -228,7 +228,7 @@ impl TransportConfig { /// Defaults to `None`, which disables receiving acknowledgement timestamps from the sender. /// If `Some`, TransportParameters are sent to the peer to enable acknowledgement timestamps /// if supported. - pub fn ack_timestamp_config(&mut self, value: Option) -> &mut Self { + pub fn ack_timestamp_config(&mut self, value: AckTimestampsConfig) -> &mut Self { self.ack_timestamp_config = value; self } @@ -371,7 +371,7 @@ impl Default for TransportConfig { enable_segmentation_offload: true, - ack_timestamp_config: None, + ack_timestamp_config: AckTimestampsConfig::default(), } } } @@ -435,16 +435,17 @@ impl fmt::Debug for TransportConfig { } /// Parameters for controlling the peer's acknowledgements with receiver timestamps. -#[derive(Clone, Debug)] +#[derive(Clone, Debug, PartialEq, Eq, Copy)] pub struct AckTimestampsConfig { - pub(crate) max_timestamps_per_ack: VarInt, + /// If max_timestamp_per_ack is None, this feature is disabled. + pub(crate) max_timestamps_per_ack: Option, pub(crate) exponent: VarInt, } impl AckTimestampsConfig { /// Sets the maximum number of timestamp entries per ACK frame. pub fn max_timestamps_per_ack(&mut self, value: VarInt) -> &mut Self { - self.max_timestamps_per_ack = value; + self.max_timestamps_per_ack = Some(value); self } @@ -459,7 +460,8 @@ impl AckTimestampsConfig { impl Default for AckTimestampsConfig { fn default() -> Self { Self { - max_timestamps_per_ack: 10u32.into(), + max_timestamps_per_ack: None, + // Default to 0 as per draft. exponent: 0u32.into(), } } diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index c7e5e71a9..2066a1bd1 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -231,7 +231,7 @@ pub struct Connection { // Ack Receive Timestamps // The timestamp config of the peer. - ack_timestamp_cfg: Option, + ack_timestamp_cfg: AckTimestampsConfig, streams: StreamsState, /// Surplus remote CIDs for future use on new paths @@ -345,7 +345,7 @@ impl Connection { &TransportParameters::default(), )), - ack_timestamp_cfg: None, + ack_timestamp_cfg: AckTimestampsConfig::default(), pto_count: 0, @@ -828,7 +828,7 @@ impl Connection { &mut self.spaces[space_id], buf, &mut self.stats, - None, + self.ack_timestamp_cfg, self.epoch, ); } @@ -1357,7 +1357,13 @@ impl Connection { return Err(TransportError::PROTOCOL_VIOLATION("unsent packet acked")); } - if ack.timestamps.is_some() != self.config.ack_timestamp_config.is_some() { + if ack.timestamps.is_some() + != self + .config + .ack_timestamp_config + .max_timestamps_per_ack + .is_some() + { return Err(TransportError::PROTOCOL_VIOLATION( "ack with timestamps expectation mismatched", )); @@ -1391,13 +1397,18 @@ impl Connection { } } - let mut timestamp_iter = self.config.ack_timestamp_config.as_ref().map(|cfg| { - let decoder = ack.timestamp_iter(self.epoch, cfg.exponent.0).unwrap(); - let mut v: tinyvec::TinyVec<[PacketTimestamp; 10]> = tinyvec::TinyVec::new(); - decoder.for_each(|elt| v.push(elt)); - v.reverse(); - v.into_iter().peekable() - }); + let mut timestamp_iter = + if let Some(_) = self.config.ack_timestamp_config.max_timestamps_per_ack { + let decoder = ack + .timestamp_iter(self.epoch, self.config.ack_timestamp_config.exponent.0) + .unwrap(); + let mut v: tinyvec::TinyVec<[PacketTimestamp; 10]> = tinyvec::TinyVec::new(); + decoder.for_each(|elt| v.push(elt)); + v.reverse(); + Some(v.into_iter().peekable()) + } else { + None + }; if newly_acked.is_empty() { return Ok(()); @@ -3290,7 +3301,7 @@ impl Connection { space: &mut PacketSpace, buf: &mut Vec, stats: &mut ConnectionStats, - peer_timestamp_config: Option, + ack_timestamps_config: AckTimestampsConfig, epoch: Instant, ) { debug_assert!(!space.pending_acks.ranges().is_empty()); @@ -3320,13 +3331,13 @@ impl Connection { delay as _, space.pending_acks.ranges(), ecn, - peer_timestamp_config.map(|cfg| { + ack_timestamps_config.max_timestamps_per_ack.map(|max| { ( // Safety: If peer_timestamp_config is set, receiver_timestamps must be set. space.pending_acks.receiver_timestamps_as_ref().unwrap(), epoch, - cfg.exponent.0, - cfg.max_timestamps_per_ack.0, + ack_timestamps_config.exponent.0, + max.0, ) }), buf, @@ -3385,25 +3396,12 @@ impl Connection { self.path.mtud.on_peer_max_udp_payload_size_received( u16::try_from(self.peer_params.max_udp_payload_size.into_inner()).unwrap_or(u16::MAX), ); - - { - self.ack_timestamp_cfg = if let (Some(max_timestamps_per_ack), Some(exponent)) = ( - params.max_recv_timestamps_per_ack, - params.receive_timestamps_exponent, - ) { - for space in self.spaces.iter_mut() { - space - .pending_acks - .set_receiver_timestamp(max_timestamps_per_ack.0 as usize); - } - Some(AckTimestampsConfig { - exponent, - max_timestamps_per_ack, - }) - } else { - None - }; - } + self.ack_timestamp_cfg = params.ack_timestamps_cfg; + if let Some(max) = params.ack_timestamps_cfg.max_timestamps_per_ack { + for space in self.spaces.iter_mut() { + space.pending_acks.set_receiver_timestamp(max.0 as usize); + } + }; } fn decrypt_packet( diff --git a/quinn-proto/src/transport_parameters.rs b/quinn-proto/src/transport_parameters.rs index a83f40414..f24fca4b3 100644 --- a/quinn-proto/src/transport_parameters.rs +++ b/quinn-proto/src/transport_parameters.rs @@ -18,7 +18,7 @@ use crate::{ cid_generator::ConnectionIdGenerator, cid_queue::CidQueue, coding::{BufExt, BufMutExt, UnexpectedEnd}, - config::{EndpointConfig, ServerConfig, TransportConfig}, + config::{AckTimestampsConfig, EndpointConfig, ServerConfig, TransportConfig}, shared::ConnectionId, ResetToken, Side, TransportError, VarInt, LOC_CID_COUNT, MAX_CID_SIZE, MAX_STREAM_COUNT, RESET_TOKEN_SIZE, TIMER_GRANULARITY, @@ -101,8 +101,7 @@ macro_rules! make_struct { pub(crate) preferred_address: Option, - pub(crate) receive_timestamps_exponent: Option, - pub(crate) max_recv_timestamps_per_ack: Option, + pub(crate) ack_timestamps_cfg: AckTimestampsConfig, } // We deliberately don't implement the `Default` trait, since that would be public, and @@ -124,8 +123,7 @@ macro_rules! make_struct { retry_src_cid: None, stateless_reset_token: None, preferred_address: None, - receive_timestamps_exponent: None, - max_recv_timestamps_per_ack: None, + ack_timestamps_cfg: AckTimestampsConfig::default(), } } } @@ -167,15 +165,7 @@ impl TransportParameters { VarInt::from_u64(u64::try_from(TIMER_GRANULARITY.as_micros()).unwrap()).unwrap(), ), - receive_timestamps_exponent: config - .ack_timestamp_config - .as_ref() - .map(|cfg| cfg.exponent), - - max_recv_timestamps_per_ack: config - .ack_timestamp_config - .as_ref() - .map(|cfg| cfg.max_timestamps_per_ack), + ack_timestamps_cfg: config.ack_timestamp_config, ..Self::default() } @@ -370,16 +360,15 @@ impl TransportParameters { // The below 2 fields are for the implementation of // https://www.ietf.org/archive/id/draft-smith-quic-receive-ts-00.html#name-extension-negotiation // Values of 0x00f0 and 0x00f1 arbitrarily chosen. - if let Some(x) = self.max_recv_timestamps_per_ack { + if let Some(max) = self.ack_timestamps_cfg.max_timestamps_per_ack { w.write_var(0x00f0); - w.write_var(x.size() as u64); - w.write(x); - } + w.write_var(max.size() as u64); + w.write(max); - if let Some(x) = self.receive_timestamps_exponent { + let exponent = self.ack_timestamps_cfg.exponent; w.write_var(0x00f1); - w.write_var(x.size() as u64); - w.write(x); + w.write_var(exponent.size() as u64); + w.write(exponent); } } @@ -447,16 +436,18 @@ impl TransportParameters { 0xff04de1b => params.min_ack_delay = Some(r.get().unwrap()), 0x00f0 => { - if len > 8 || params.max_recv_timestamps_per_ack.is_some() { + if len > 8 || params.ack_timestamps_cfg.max_timestamps_per_ack.is_some() { return Err(Error::Malformed); } - params.max_recv_timestamps_per_ack = Some(r.get().unwrap()); + params + .ack_timestamps_cfg + .max_timestamps_per_ack(r.get().unwrap()); } 0x00f1 => { - if len > 8 || params.receive_timestamps_exponent.is_some() { + if len > 8 { return Err(Error::Malformed); } - params.receive_timestamps_exponent = Some(r.get().unwrap()); + params.ack_timestamps_cfg.exponent(r.get().unwrap()); } _ => { @@ -511,10 +502,7 @@ impl TransportParameters { } // https://www.ietf.org/archive/id/draft-smith-quic-receive-ts-00.html#name-extension-negotiation - if params - .receive_timestamps_exponent - .map_or(false, |x| x.0 > 20) - { + if params.ack_timestamps_cfg.exponent.0 > 20 { return Err(Error::IllegalValue); } @@ -554,9 +542,10 @@ mod test { grease_quic_bit: true, min_ack_delay: Some(2_000u32.into()), - receive_timestamps_exponent: Some(3u32.into()), - max_recv_timestamps_per_ack: Some(5u32.into()), - + ack_timestamps_cfg: AckTimestampsConfig { + max_timestamps_per_ack: Some(10u32.into()), + exponent: 2u32.into(), + }, ..TransportParameters::default() }; params.write(&mut buf); From 55d207d079a05c3c175ad1dcf532b442690e1b1f Mon Sep 17 00:00:00 2001 From: Sterling Deng Date: Tue, 1 Oct 2024 13:02:15 -0700 Subject: [PATCH 23/30] refactor nits --- quinn-proto/src/frame.rs | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/quinn-proto/src/frame.rs b/quinn-proto/src/frame.rs index 88d093553..7f72cd06c 100644 --- a/quinn-proto/src/frame.rs +++ b/quinn-proto/src/frame.rs @@ -949,7 +949,7 @@ impl From for IterErr { pub struct AckTimestampIter<'a> { timestamp_basis: u64, - timestamp_exponent: u64, + exponent: u64, epoch: Instant, data: &'a [u8], @@ -965,7 +965,7 @@ impl<'a> AckTimestampIter<'a> { let _ = data.get_var().unwrap(); AckTimestampIter { timestamp_basis: 0, - timestamp_exponent: exponent, + exponent, epoch, data, deltas_remaining: 0, @@ -988,11 +988,10 @@ impl<'a> Iterator for AckTimestampIter<'a> { if self.deltas_remaining == 0 { let gap = self.data.get_var().unwrap(); self.deltas_remaining = self.data.get_var().unwrap() as usize; - if self.first { - self.next_pn -= gap; - } else { - self.next_pn -= gap + 2; - } + self.next_pn -= match self.first { + true => gap, + false => gap + 2, + }; } else { self.next_pn -= 1; } @@ -1001,10 +1000,10 @@ impl<'a> Iterator for AckTimestampIter<'a> { self.deltas_remaining -= 1; if self.first { - self.timestamp_basis = delta << self.timestamp_exponent; + self.timestamp_basis = delta << self.exponent; self.first = false; } else { - self.timestamp_basis -= delta << self.timestamp_exponent; + self.timestamp_basis -= delta << self.exponent; } Some(PacketTimestamp { From e85e8f11916898e9ad6a7c5a404279e549ac4d02 Mon Sep 17 00:00:00 2001 From: Sterling Deng Date: Tue, 1 Oct 2024 13:09:46 -0700 Subject: [PATCH 24/30] consistent naming --- quinn-proto/src/config.rs | 18 ++++++++++++------ quinn-proto/src/connection/mod.rs | 16 ++++++++-------- quinn-proto/src/frame.rs | 22 +++++++++++----------- quinn-proto/src/transport_parameters.rs | 2 +- 4 files changed, 32 insertions(+), 26 deletions(-) diff --git a/quinn-proto/src/config.rs b/quinn-proto/src/config.rs index e9f6602eb..6894265a9 100644 --- a/quinn-proto/src/config.rs +++ b/quinn-proto/src/config.rs @@ -52,7 +52,7 @@ pub struct TransportConfig { pub(crate) min_mtu: u16, pub(crate) mtu_discovery_config: Option, pub(crate) ack_frequency_config: Option, - pub(crate) ack_timestamp_config: AckTimestampsConfig, + pub(crate) ack_timestamps_config: AckTimestampsConfig, pub(crate) persistent_congestion_threshold: u32, pub(crate) keep_alive_interval: Option, @@ -228,8 +228,14 @@ impl TransportConfig { /// Defaults to `None`, which disables receiving acknowledgement timestamps from the sender. /// If `Some`, TransportParameters are sent to the peer to enable acknowledgement timestamps /// if supported. - pub fn ack_timestamp_config(&mut self, value: AckTimestampsConfig) -> &mut Self { - self.ack_timestamp_config = value; + pub fn max_ack_timestamps(&mut self, value: VarInt) -> &mut Self { + self.ack_timestamps_config.max_timestamps_per_ack = Some(value); + self + } + + /// Specifies the exponent used when encoding the timestamps. + pub fn ack_timestamps_exponent(&mut self, value: VarInt) -> &mut Self { + self.ack_timestamps_config.exponent = value; self } @@ -371,7 +377,7 @@ impl Default for TransportConfig { enable_segmentation_offload: true, - ack_timestamp_config: AckTimestampsConfig::default(), + ack_timestamps_config: AckTimestampsConfig::default(), } } } @@ -402,7 +408,7 @@ impl fmt::Debug for TransportConfig { deterministic_packet_numbers: _, congestion_controller_factory: _, enable_segmentation_offload, - ack_timestamp_config, + ack_timestamps_config, } = self; fmt.debug_struct("TransportConfig") .field("max_concurrent_bidi_streams", max_concurrent_bidi_streams) @@ -429,7 +435,7 @@ impl fmt::Debug for TransportConfig { .field("datagram_send_buffer_size", datagram_send_buffer_size) .field("congestion_controller_factory", &"[ opaque ]") .field("enable_segmentation_offload", enable_segmentation_offload) - .field("ack_timestamp_config", ack_timestamp_config) + .field("ack_timestamps_config", ack_timestamps_config) .finish() } } diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index 2066a1bd1..0936bdd6e 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -231,7 +231,7 @@ pub struct Connection { // Ack Receive Timestamps // The timestamp config of the peer. - ack_timestamp_cfg: AckTimestampsConfig, + ack_timestamps_cfg: AckTimestampsConfig, streams: StreamsState, /// Surplus remote CIDs for future use on new paths @@ -345,7 +345,7 @@ impl Connection { &TransportParameters::default(), )), - ack_timestamp_cfg: AckTimestampsConfig::default(), + ack_timestamps_cfg: AckTimestampsConfig::default(), pto_count: 0, @@ -828,7 +828,7 @@ impl Connection { &mut self.spaces[space_id], buf, &mut self.stats, - self.ack_timestamp_cfg, + self.ack_timestamps_cfg, self.epoch, ); } @@ -1360,7 +1360,7 @@ impl Connection { if ack.timestamps.is_some() != self .config - .ack_timestamp_config + .ack_timestamps_config .max_timestamps_per_ack .is_some() { @@ -1398,9 +1398,9 @@ impl Connection { } let mut timestamp_iter = - if let Some(_) = self.config.ack_timestamp_config.max_timestamps_per_ack { + if let Some(_) = self.config.ack_timestamps_config.max_timestamps_per_ack { let decoder = ack - .timestamp_iter(self.epoch, self.config.ack_timestamp_config.exponent.0) + .timestamp_iter(self.epoch, self.config.ack_timestamps_config.exponent.0) .unwrap(); let mut v: tinyvec::TinyVec<[PacketTimestamp; 10]> = tinyvec::TinyVec::new(); decoder.for_each(|elt| v.push(elt)); @@ -3115,7 +3115,7 @@ impl Connection { space, buf, &mut self.stats, - self.ack_timestamp_cfg.clone(), + self.ack_timestamps_cfg.clone(), self.epoch, ); } @@ -3396,7 +3396,7 @@ impl Connection { self.path.mtud.on_peer_max_udp_payload_size_received( u16::try_from(self.peer_params.max_udp_payload_size.into_inner()).unwrap_or(u16::MAX), ); - self.ack_timestamp_cfg = params.ack_timestamps_cfg; + self.ack_timestamps_cfg = params.ack_timestamps_cfg; if let Some(max) = params.ack_timestamps_cfg.max_timestamps_per_ack { for space in self.spaces.iter_mut() { space.pending_acks.set_receiver_timestamp(max.0 as usize); diff --git a/quinn-proto/src/frame.rs b/quinn-proto/src/frame.rs index 7f72cd06c..11ddef0e6 100644 --- a/quinn-proto/src/frame.rs +++ b/quinn-proto/src/frame.rs @@ -759,7 +759,7 @@ impl Iter { } else { let ts_start = end; let ts_range_count = self.bytes.get_var()?; - scan_ack_timestamp_blocks(&mut self.bytes, largest, ts_range_count)?; + scan_ack_timestamps_blocks(&mut self.bytes, largest, ts_range_count)?; let ts_end = self.bytes.position() as usize; Some(self.bytes.get_ref().slice(ts_start..ts_end)) }, @@ -891,7 +891,7 @@ fn scan_ack_blocks(buf: &mut io::Cursor, largest: u64, n: usize) -> Resul Ok(()) } -fn scan_ack_timestamp_blocks( +fn scan_ack_timestamps_blocks( buf: &mut io::Cursor, largest: u64, range_count: u64, @@ -1220,11 +1220,11 @@ mod test { } #[cfg(test)] - mod ack_timestamp_tests { + mod ack_timestamps_tests { use super::*; #[test] - fn test_scan_ack_timestamp_block() { + fn test_scan_ack_timestamps_block() { let mut buf = bytes::BytesMut::new(); buf.write_var(0); // gap buf.write_var(3); // delta count @@ -1234,12 +1234,12 @@ mod test { let buf_len = buf.len() as u64; let mut c = io::Cursor::new(buf.freeze()); - scan_ack_timestamp_blocks(&mut c, 3, 1).unwrap(); + scan_ack_timestamps_blocks(&mut c, 3, 1).unwrap(); assert_eq!(buf_len, c.position()); } #[test] - fn test_scan_ack_timestamp_block_with_gap() { + fn test_scan_ack_timestamps_block_with_gap() { let mut buf = bytes::BytesMut::new(); buf.write_var(1); // gap buf.write_var(3); // delta count @@ -1257,12 +1257,12 @@ mod test { let buf_len = buf.len() as u64; let mut c = io::Cursor::new(buf.freeze()); - scan_ack_timestamp_blocks(&mut c, 10, 2).unwrap(); + scan_ack_timestamps_blocks(&mut c, 10, 2).unwrap(); assert_eq!(buf_len, c.position()); } #[test] - fn test_scan_ack_timestamp_block_with_gap_malforned() { + fn test_scan_ack_timestamps_block_with_gap_malforned() { let mut buf = bytes::BytesMut::new(); buf.write_var(1); // gap buf.write_var(3); // delta count @@ -1279,11 +1279,11 @@ mod test { buf.write_var(3); // pn -1 this will cause an error let mut c = io::Cursor::new(buf.freeze()); - assert!(scan_ack_timestamp_blocks(&mut c, 10, 2).is_err()); + assert!(scan_ack_timestamps_blocks(&mut c, 10, 2).is_err()); } #[test] - fn test_scan_ack_timestamp_block_malformed() { + fn test_scan_ack_timestamps_block_malformed() { let mut buf = bytes::BytesMut::new(); buf.write_var(5); // gap buf.write_var(1); // delta count @@ -1292,7 +1292,7 @@ mod test { let mut c = io::Cursor::new(buf.freeze()); assert_eq!( IterErr::UnexpectedEnd, - scan_ack_timestamp_blocks(&mut c, 5, 2).unwrap_err(), + scan_ack_timestamps_blocks(&mut c, 5, 2).unwrap_err(), ); } diff --git a/quinn-proto/src/transport_parameters.rs b/quinn-proto/src/transport_parameters.rs index f24fca4b3..e6172c15f 100644 --- a/quinn-proto/src/transport_parameters.rs +++ b/quinn-proto/src/transport_parameters.rs @@ -165,7 +165,7 @@ impl TransportParameters { VarInt::from_u64(u64::try_from(TIMER_GRANULARITY.as_micros()).unwrap()).unwrap(), ), - ack_timestamps_cfg: config.ack_timestamp_config, + ack_timestamps_cfg: config.ack_timestamps_config, ..Self::default() } From 64acadabb0b57272b8d403d205f453215f1b65f5 Mon Sep 17 00:00:00 2001 From: Sterling Deng Date: Tue, 1 Oct 2024 14:01:07 -0700 Subject: [PATCH 25/30] lint and test --- quinn-proto/src/congestion.rs | 1 + quinn-proto/src/connection/mod.rs | 38 ++++++++++++++----------------- quinn-proto/src/frame.rs | 1 + 3 files changed, 19 insertions(+), 21 deletions(-) diff --git a/quinn-proto/src/congestion.rs b/quinn-proto/src/congestion.rs index 1ae3757a7..badeb0ecd 100644 --- a/quinn-proto/src/congestion.rs +++ b/quinn-proto/src/congestion.rs @@ -46,6 +46,7 @@ pub trait Controller: Send + Sync { app_limited: bool, rtt: &RttEstimator, ) { + self.on_ack(now, sent, bytes, app_limited, rtt); } /// Packets are acked in batches, all with the same `now` argument. This indicates one of those batches has completed. diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index 0936bdd6e..25d413fec 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -1397,18 +1397,22 @@ impl Connection { } } - let mut timestamp_iter = - if let Some(_) = self.config.ack_timestamps_config.max_timestamps_per_ack { - let decoder = ack - .timestamp_iter(self.epoch, self.config.ack_timestamps_config.exponent.0) - .unwrap(); - let mut v: tinyvec::TinyVec<[PacketTimestamp; 10]> = tinyvec::TinyVec::new(); - decoder.for_each(|elt| v.push(elt)); - v.reverse(); - Some(v.into_iter().peekable()) - } else { - None - }; + let mut timestamp_iter = if self + .config + .ack_timestamps_config + .max_timestamps_per_ack + .is_some() + { + let decoder = ack + .timestamp_iter(self.epoch, self.config.ack_timestamps_config.exponent.0) + .unwrap(); + let mut v: tinyvec::TinyVec<[PacketTimestamp; 10]> = tinyvec::TinyVec::new(); + decoder.for_each(|elt| v.push(elt)); + v.reverse(); + Some(v.into_iter().peekable()) + } else { + None + }; if newly_acked.is_empty() { return Ok(()); @@ -1548,14 +1552,6 @@ impl Connection { if info.ack_eliciting && self.path.challenge.is_none() { // Only pass ACKs to the congestion controller if we are not validating the current // path, so as to ignore any ACKs from older paths still coming in. - self.path.congestion.on_ack( - now, - info.time_sent, - info.size.into(), - self.app_limited, - &self.path.rtt, - ); - self.path.congestion.on_ack_packet( pn, now, @@ -3115,7 +3111,7 @@ impl Connection { space, buf, &mut self.stats, - self.ack_timestamps_cfg.clone(), + self.ack_timestamps_cfg, self.epoch, ); } diff --git a/quinn-proto/src/frame.rs b/quinn-proto/src/frame.rs index 11ddef0e6..269f1f640 100644 --- a/quinn-proto/src/frame.rs +++ b/quinn-proto/src/frame.rs @@ -962,6 +962,7 @@ impl<'a> AckTimestampIter<'a> { fn new(largest: u64, epoch: Instant, exponent: u64, mut data: &'a [u8]) -> Self { // We read and throw away the Timestamp Range Count value because // it was already used to properly slice the data. + // Unwrap safety: this byte block was scanned prior using scan_ack_timestamps_blocks. let _ = data.get_var().unwrap(); AckTimestampIter { timestamp_basis: 0, From fa004c54efce4a0643aa2a575b3fa434f2c7aa30 Mon Sep 17 00:00:00 2001 From: Sterling Deng Date: Wed, 2 Oct 2024 13:24:33 -0700 Subject: [PATCH 26/30] refactor some unidiomatic code --- quinn-proto/src/config.rs | 5 +++ quinn-proto/src/connection/mod.rs | 49 ++++++++-------------------- quinn-proto/src/connection/spaces.rs | 4 +++ quinn-proto/src/frame.rs | 4 +-- 4 files changed, 24 insertions(+), 38 deletions(-) diff --git a/quinn-proto/src/config.rs b/quinn-proto/src/config.rs index 6894265a9..7c4a4a0e8 100644 --- a/quinn-proto/src/config.rs +++ b/quinn-proto/src/config.rs @@ -449,6 +449,11 @@ pub struct AckTimestampsConfig { } impl AckTimestampsConfig { + /// Enabled returns true if the feature is enabled. + pub fn enabled(&self) -> bool { + self.max_timestamps_per_ack.is_some() + } + /// Sets the maximum number of timestamp entries per ACK frame. pub fn max_timestamps_per_ack(&mut self, value: VarInt) -> &mut Self { self.max_timestamps_per_ack = Some(value); diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index 25d413fec..ffeced3b6 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -1397,22 +1397,18 @@ impl Connection { } } - let mut timestamp_iter = if self - .config - .ack_timestamps_config - .max_timestamps_per_ack - .is_some() - { - let decoder = ack - .timestamp_iter(self.epoch, self.config.ack_timestamps_config.exponent.0) - .unwrap(); - let mut v: tinyvec::TinyVec<[PacketTimestamp; 10]> = tinyvec::TinyVec::new(); - decoder.for_each(|elt| v.push(elt)); - v.reverse(); - Some(v.into_iter().peekable()) - } else { - None - }; + let timestamp_iter = + ack.timestamps_iter(self.epoch, self.config.ack_timestamps_config.exponent.0); + if self.config.ack_timestamps_config.enabled() && timestamp_iter.is_some() { + // Safety: checked by is_some() + let iter = timestamp_iter.unwrap(); + let packet_space = &mut self.spaces[space]; + for pkt in iter { + if let Some(sent_packet) = packet_space.get_mut_sent_packet(pkt.packet_number) { + sent_packet.time_received = Some(pkt.timestamp); + } + } + } if newly_acked.is_empty() { return Ok(()); @@ -1420,7 +1416,7 @@ impl Connection { let mut ack_eliciting_acked = false; for packet in newly_acked.elts() { - if let Some(mut info) = self.spaces[space].take(packet) { + if let Some(info) = self.spaces[space].take(packet) { if let Some(acked) = info.largest_acked { // Assume ACKs for all packets below the largest acknowledged in `packet` have // been received. This can cause the peer to spuriously retransmit if some of @@ -1442,25 +1438,6 @@ impl Connection { // Notify ack frequency that a packet was acked, because it might contain an ACK_FREQUENCY frame self.ack_frequency.on_acked(packet); - if let Some(timestamp_iter) = timestamp_iter.as_mut() { - while let Some(v) = timestamp_iter.peek() { - match v.packet_number.cmp(&packet) { - cmp::Ordering::Less => { - let _ = timestamp_iter.next(); - } - cmp::Ordering::Equal => { - // Unwrap safety is guaranteed because a value was validated - // to exist using peek - let ts = timestamp_iter.next().unwrap(); - info.time_received = Some(ts.timestamp); - } - cmp::Ordering::Greater => { - break; - } - } - } - } - self.on_packet_acked(now, packet, info); } } diff --git a/quinn-proto/src/connection/spaces.rs b/quinn-proto/src/connection/spaces.rs index a6baebc09..5852302f7 100644 --- a/quinn-proto/src/connection/spaces.rs +++ b/quinn-proto/src/connection/spaces.rs @@ -221,6 +221,10 @@ impl PacketSpace { Some(packet) } + pub(super) fn get_mut_sent_packet(&mut self, number: u64) -> Option<&mut SentPacket> { + self.sent_packets.get_mut(&number) + } + /// Returns the number of bytes to *remove* from the connection's in-flight count pub(super) fn sent(&mut self, number: u64, packet: SentPacket) -> u64 { // Retain state for at most this many non-ACK-eliciting packets sent after the most recently diff --git a/quinn-proto/src/frame.rs b/quinn-proto/src/frame.rs index 269f1f640..b08d2e2a3 100644 --- a/quinn-proto/src/frame.rs +++ b/quinn-proto/src/frame.rs @@ -364,7 +364,7 @@ impl fmt::Debug for Ack { ranges.push(']'); let timestamp_count = self - .timestamp_iter(Instant::now(), 0) + .timestamps_iter(Instant::now(), 0) .map(|iter| iter.count()); f.debug_struct("Ack") @@ -432,7 +432,7 @@ impl Ack { self.into_iter() } - pub fn timestamp_iter(&self, epoch: Instant, exponent: u64) -> Option { + pub fn timestamps_iter(&self, epoch: Instant, exponent: u64) -> Option { self.timestamps .as_ref() .map(|v| AckTimestampIter::new(self.largest, epoch, exponent, &v[..])) From 53bcc35b0e9c79115c034cb8edb3637d112bcf24 Mon Sep 17 00:00:00 2001 From: Sterling Deng Date: Wed, 2 Oct 2024 14:19:22 -0700 Subject: [PATCH 27/30] refactor some unwraps --- quinn-proto/src/config.rs | 5 ---- quinn-proto/src/connection/mod.rs | 25 ++++++++----------- .../src/connection/receiver_timestamps.rs | 6 +---- quinn-proto/src/frame.rs | 4 +-- 4 files changed, 13 insertions(+), 27 deletions(-) diff --git a/quinn-proto/src/config.rs b/quinn-proto/src/config.rs index 7c4a4a0e8..6894265a9 100644 --- a/quinn-proto/src/config.rs +++ b/quinn-proto/src/config.rs @@ -449,11 +449,6 @@ pub struct AckTimestampsConfig { } impl AckTimestampsConfig { - /// Enabled returns true if the feature is enabled. - pub fn enabled(&self) -> bool { - self.max_timestamps_per_ack.is_some() - } - /// Sets the maximum number of timestamp entries per ACK frame. pub fn max_timestamps_per_ack(&mut self, value: VarInt) -> &mut Self { self.max_timestamps_per_ack = Some(value); diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index ffeced3b6..f9ba10ec1 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -1357,18 +1357,6 @@ impl Connection { return Err(TransportError::PROTOCOL_VIOLATION("unsent packet acked")); } - if ack.timestamps.is_some() - != self - .config - .ack_timestamps_config - .max_timestamps_per_ack - .is_some() - { - return Err(TransportError::PROTOCOL_VIOLATION( - "ack with timestamps expectation mismatched", - )); - } - let new_largest = { let space = &mut self.spaces[space]; if space @@ -1399,14 +1387,21 @@ impl Connection { let timestamp_iter = ack.timestamps_iter(self.epoch, self.config.ack_timestamps_config.exponent.0); - if self.config.ack_timestamps_config.enabled() && timestamp_iter.is_some() { - // Safety: checked by is_some() - let iter = timestamp_iter.unwrap(); + if let (Some(max), Some(iter)) = ( + self.config.ack_timestamps_config.max_timestamps_per_ack, + timestamp_iter, + ) { let packet_space = &mut self.spaces[space]; + let mut n = 0; for pkt in iter { + if n > max.0 { + warn!("peer is sending more timestamps than max requested"); + break; + } if let Some(sent_packet) = packet_space.get_mut_sent_packet(pkt.packet_number) { sent_packet.time_received = Some(pkt.timestamp); } + n += 1; } } diff --git a/quinn-proto/src/connection/receiver_timestamps.rs b/quinn-proto/src/connection/receiver_timestamps.rs index 03f69e0e9..be7859b0d 100644 --- a/quinn-proto/src/connection/receiver_timestamps.rs +++ b/quinn-proto/src/connection/receiver_timestamps.rs @@ -18,7 +18,7 @@ impl Default for PacketTimestamp { } pub struct ReceiverTimestamps { - data: VecDeque, + pub data: VecDeque, max: usize, } @@ -87,10 +87,6 @@ impl ReceiverTimestamps { pub(crate) fn len(&self) -> usize { self.data.len() } - - pub(crate) fn inner(&self) -> &VecDeque { - &self.data - } } #[cfg(test)] diff --git a/quinn-proto/src/frame.rs b/quinn-proto/src/frame.rs index b08d2e2a3..0d090d89a 100644 --- a/quinn-proto/src/frame.rs +++ b/quinn-proto/src/frame.rs @@ -473,7 +473,7 @@ impl Ack { let mut counter = 0; for segment_idx in segment_idxs { - let Some(elt) = timestamps.inner().get(right - 1) else { + let Some(elt) = timestamps.data.get(right - 1) else { debug_assert!( false, "an invalid indexing occurred on the ReceiverTimestamps vector" @@ -499,7 +499,7 @@ impl Ack { // *Timestamp Delta Count buf.write_var((right - segment_idx) as u64); // *Timestamp Deltas - for pkt in timestamps.inner().range(segment_idx..right).rev() { + for pkt in timestamps.data.range(segment_idx..right).rev() { let delta: u64 = if first { first = false; // For the first Timestamp Delta of the first Timestamp Range in the frame: the value From 7bee8454219b2cafb6b33bed3d8b1d2ba5672a0d Mon Sep 17 00:00:00 2001 From: Sterling Deng Date: Wed, 2 Oct 2024 15:22:27 -0700 Subject: [PATCH 28/30] fix lint --- quinn-proto/src/connection/mod.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index f9ba10ec1..6d66397c4 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -1392,16 +1392,14 @@ impl Connection { timestamp_iter, ) { let packet_space = &mut self.spaces[space]; - let mut n = 0; - for pkt in iter { - if n > max.0 { + for (i, pkt) in iter.enumerate() { + if i > max.0 as usize { warn!("peer is sending more timestamps than max requested"); break; } if let Some(sent_packet) = packet_space.get_mut_sent_packet(pkt.packet_number) { sent_packet.time_received = Some(pkt.timestamp); } - n += 1; } } From 8bc98b1ec43329f66fa086b2d45cc27bd45ca98b Mon Sep 17 00:00:00 2001 From: sterlingdeng Date: Tue, 22 Oct 2024 07:14:51 +0200 Subject: [PATCH 29/30] timestamp as duration --- quinn-proto/src/congestion.rs | 6 +- quinn-proto/src/connection/mod.rs | 25 ++-- .../src/connection/receiver_timestamps.rs | 56 +++++---- quinn-proto/src/connection/spaces.rs | 6 +- quinn-proto/src/frame.rs | 107 ++++++++++-------- 5 files changed, 101 insertions(+), 99 deletions(-) diff --git a/quinn-proto/src/congestion.rs b/quinn-proto/src/congestion.rs index badeb0ecd..b41b7dc38 100644 --- a/quinn-proto/src/congestion.rs +++ b/quinn-proto/src/congestion.rs @@ -3,7 +3,7 @@ use crate::connection::RttEstimator; use std::any::Any; use std::sync::Arc; -use std::time::Instant; +use std::time::{Duration, Instant}; mod bbr; mod cubic; @@ -36,12 +36,12 @@ pub trait Controller: Send + Sync { #[allow(unused_variables)] /// Packet deliveries were confirmed with timestamps information. - fn on_ack_packet( + fn on_ack_timestamped( &mut self, pn: u64, now: Instant, sent: Instant, - received: Option, + received: Option, bytes: u64, app_limited: bool, rtt: &RttEstimator, diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index 6d66397c4..4253e013f 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -9,7 +9,7 @@ use std::{ }; use bytes::{Bytes, BytesMut}; -use frame::StreamMetaVec; +use frame::{AckTimestampEncodeParams, StreamMetaVec}; use rand::{rngs::StdRng, Rng, SeedableRng}; use thiserror::Error; use tracing::{debug, error, trace, trace_span, warn}; @@ -829,7 +829,6 @@ impl Connection { buf, &mut self.stats, self.ack_timestamps_cfg, - self.epoch, ); } @@ -1385,8 +1384,7 @@ impl Connection { } } - let timestamp_iter = - ack.timestamps_iter(self.epoch, self.config.ack_timestamps_config.exponent.0); + let timestamp_iter = ack.timestamps_iter(self.config.ack_timestamps_config.exponent.0); if let (Some(max), Some(iter)) = ( self.config.ack_timestamps_config.max_timestamps_per_ack, timestamp_iter, @@ -1522,7 +1520,7 @@ impl Connection { if info.ack_eliciting && self.path.challenge.is_none() { // Only pass ACKs to the congestion controller if we are not validating the current // path, so as to ignore any ACKs from older paths still coming in. - self.path.congestion.on_ack_packet( + self.path.congestion.on_ack_timestamped( pn, now, info.time_sent, @@ -3082,7 +3080,6 @@ impl Connection { buf, &mut self.stats, self.ack_timestamps_cfg, - self.epoch, ); } @@ -3268,7 +3265,6 @@ impl Connection { buf: &mut Vec, stats: &mut ConnectionStats, ack_timestamps_config: AckTimestampsConfig, - epoch: Instant, ) { debug_assert!(!space.pending_acks.ranges().is_empty()); @@ -3298,13 +3294,12 @@ impl Connection { space.pending_acks.ranges(), ecn, ack_timestamps_config.max_timestamps_per_ack.map(|max| { - ( + AckTimestampEncodeParams { // Safety: If peer_timestamp_config is set, receiver_timestamps must be set. - space.pending_acks.receiver_timestamps_as_ref().unwrap(), - epoch, - ack_timestamps_config.exponent.0, - max.0, - ) + receiver_timestamps: space.pending_acks.receiver_timestamps_as_ref().unwrap(), + exponent: ack_timestamps_config.exponent.0, + max_timestamps: max.0, + } }), buf, ); @@ -3365,7 +3360,9 @@ impl Connection { self.ack_timestamps_cfg = params.ack_timestamps_cfg; if let Some(max) = params.ack_timestamps_cfg.max_timestamps_per_ack { for space in self.spaces.iter_mut() { - space.pending_acks.set_receiver_timestamp(max.0 as usize); + space + .pending_acks + .set_receiver_timestamp(max.0 as usize, self.epoch); } }; } diff --git a/quinn-proto/src/connection/receiver_timestamps.rs b/quinn-proto/src/connection/receiver_timestamps.rs index be7859b0d..138446a05 100644 --- a/quinn-proto/src/connection/receiver_timestamps.rs +++ b/quinn-proto/src/connection/receiver_timestamps.rs @@ -1,38 +1,32 @@ -use std::{collections::VecDeque, fmt, time::Instant}; +use std::{collections::VecDeque, fmt}; + +use std::time::{Duration, Instant}; use tracing::warn; #[derive(Debug, Clone, PartialEq, Eq, Copy)] pub struct PacketTimestamp { pub packet_number: u64, - pub timestamp: Instant, -} - -impl Default for PacketTimestamp { - fn default() -> Self { - Self { - packet_number: 0, - timestamp: Instant::now(), - } - } + pub timestamp: Duration, } -pub struct ReceiverTimestamps { - pub data: VecDeque, +pub(crate) struct ReceiverTimestamps { + pub(crate) data: VecDeque, max: usize, + epoch: Instant, } impl fmt::Debug for ReceiverTimestamps { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { let mut l = f.debug_list(); - let mut last: Option<(u64, Instant)> = None; + let mut last: Option<(u64, Duration)> = None; for curr in self.data.iter() { if let Some(last) = last.take() { let s = format!( "{}..{} diff_micros: {}", last.0, curr.packet_number, - curr.timestamp.duration_since(last.1).as_micros(), + (curr.timestamp - last.1).as_micros(), ); l.entry(&s); } @@ -43,10 +37,11 @@ impl fmt::Debug for ReceiverTimestamps { } impl ReceiverTimestamps { - pub(crate) fn new(max: usize) -> Self { + pub(crate) fn new(max: usize, epoch: Instant) -> Self { Self { data: VecDeque::with_capacity(max), max, + epoch, } } @@ -62,7 +57,7 @@ impl ReceiverTimestamps { } self.data.push_back(PacketTimestamp { packet_number, - timestamp, + timestamp: timestamp - self.epoch, }); } @@ -95,30 +90,33 @@ mod receiver_timestamp_tests { #[test] fn subtract_below() { - let mut ts = ReceiverTimestamps::new(10); - ts.add(1, Instant::now()); - ts.add(2, Instant::now()); - ts.add(3, Instant::now()); - ts.add(4, Instant::now()); + let t0 = Instant::now(); + let mut ts = ReceiverTimestamps::new(10, t0); + ts.add(1, t0); + ts.add(2, t0); + ts.add(3, t0); + ts.add(4, t0); ts.subtract_below(3); assert_eq!(1, ts.len()); } #[test] fn subtract_below_everything() { - let mut ts = ReceiverTimestamps::new(10); - ts.add(5, Instant::now()); + let t0 = Instant::now(); + let mut ts = ReceiverTimestamps::new(10, t0); + ts.add(5, t0); ts.subtract_below(10); assert_eq!(0, ts.len()); } #[test] fn receiver_timestamp_max() { - let mut ts = ReceiverTimestamps::new(2); - ts.add(1, Instant::now()); - ts.add(2, Instant::now()); - ts.add(3, Instant::now()); - ts.add(4, Instant::now()); + let t0 = Instant::now(); + let mut ts = ReceiverTimestamps::new(2, t0); + ts.add(1, t0); + ts.add(2, t0); + ts.add(3, t0); + ts.add(4, t0); assert_eq!(2, ts.len()); assert_eq!(3, ts.data.front().unwrap().packet_number); assert_eq!(4, ts.data.back().unwrap().packet_number); diff --git a/quinn-proto/src/connection/spaces.rs b/quinn-proto/src/connection/spaces.rs index 5852302f7..f4e943228 100644 --- a/quinn-proto/src/connection/spaces.rs +++ b/quinn-proto/src/connection/spaces.rs @@ -291,7 +291,7 @@ pub(super) struct SentPacket { /// The time the packet was received by the receiver. The time Instant on this field is /// relative to a basis negotiated by the two connections. Time arithmetic done using the /// time_received field is only useful when compared to other time_received. - pub(super) time_received: Option, + pub(super) time_received: Option, /// The number of bytes sent in the packet, not including UDP or IP overhead, but including QUIC /// framing overhead. Zero if this packet is not counted towards congestion control, i.e. not an /// "in flight" packet. @@ -623,8 +623,8 @@ impl PendingAcks { } } - pub(super) fn set_receiver_timestamp(&mut self, max_timestamps: usize) { - self.receiver_timestamps = Some(ReceiverTimestamps::new(max_timestamps)); + pub(super) fn set_receiver_timestamp(&mut self, max_timestamps: usize, epoch: Instant) { + self.receiver_timestamps = Some(ReceiverTimestamps::new(max_timestamps, epoch)); } pub(super) fn set_ack_frequency_params(&mut self, frame: &frame::AckFrequency) { diff --git a/quinn-proto/src/frame.rs b/quinn-proto/src/frame.rs index 0d090d89a..d9dbfbbbf 100644 --- a/quinn-proto/src/frame.rs +++ b/quinn-proto/src/frame.rs @@ -2,7 +2,7 @@ use std::{ fmt::{self, Write}, io, mem, ops::{Range, RangeInclusive}, - time::{Duration, Instant}, + time::Duration, }; use bytes::{Buf, BufMut, Bytes}; @@ -363,9 +363,7 @@ impl fmt::Debug for Ack { } ranges.push(']'); - let timestamp_count = self - .timestamps_iter(Instant::now(), 0) - .map(|iter| iter.count()); + let timestamp_count = self.timestamps_iter(0).map(|iter| iter.count()); f.debug_struct("Ack") .field("largest", &self.largest) @@ -386,24 +384,25 @@ impl<'a> IntoIterator for &'a Ack { } } +pub struct AckTimestampEncodeParams<'a> { + pub(crate) receiver_timestamps: &'a ReceiverTimestamps, + pub(crate) exponent: u64, + pub(crate) max_timestamps: u64, +} + impl Ack { pub fn encode( delay: u64, ranges: &ArrayRangeSet, ecn: Option<&EcnCounts>, - timestamps: Option<( - &ReceiverTimestamps, - Instant, - u64, // exponent - u64, // max_timestamps - )>, + ts_params: Option, buf: &mut W, ) { let mut rest = ranges.iter().rev(); let first = rest.next().unwrap(); let largest = first.end - 1; let first_size = first.end - first.start; - buf.write(if timestamps.is_some() { + buf.write(if ts_params.is_some() { Type::ACK_RECEIVE_TIMESTAMPS } else if ecn.is_some() { Type::ACK_ECN @@ -421,8 +420,14 @@ impl Ack { buf.write_var(size - 1); prev = block.start; } - if let Some(x) = timestamps { - Self::encode_timestamps(x.0, largest, buf, x.1, x.2, x.3) + if let Some(x) = ts_params { + Self::encode_timestamps( + x.receiver_timestamps, + largest, + buf, + x.exponent, + x.max_timestamps, + ) } else if let Some(x) = ecn { x.encode(buf) } @@ -432,10 +437,10 @@ impl Ack { self.into_iter() } - pub fn timestamps_iter(&self, epoch: Instant, exponent: u64) -> Option { + pub fn timestamps_iter(&self, exponent: u64) -> Option { self.timestamps .as_ref() - .map(|v| AckTimestampIter::new(self.largest, epoch, exponent, &v[..])) + .map(|v| AckTimestampIter::new(self.largest, exponent, &v[..])) } // https://www.ietf.org/archive/id/draft-smith-quic-receive-ts-00.html#ts-ranges @@ -443,7 +448,6 @@ impl Ack { timestamps: &ReceiverTimestamps, mut largest: u64, buf: &mut impl BufMut, - mut basis: Instant, exponent: u64, max_timestamps: u64, ) { @@ -471,6 +475,7 @@ impl Ack { let mut right = timestamps.len(); let mut first = true; let mut counter = 0; + let mut basis = Duration::ZERO; for segment_idx in segment_idxs { let Some(elt) = timestamps.data.get(right - 1) else { @@ -505,12 +510,12 @@ impl Ack { // For the first Timestamp Delta of the first Timestamp Range in the frame: the value // is the difference between (a) the receive timestamp of the largest packet in the // Timestamp Range (indicated by Gap) and (b) the session receive_timestamp_basis - pkt.timestamp.duration_since(basis).as_micros() as u64 + pkt.timestamp.as_micros() as u64 } else { // For all other Timestamp Deltas: the value is the difference between // (a) the receive timestamp specified by the previous Timestamp Delta and // (b) the receive timestamp of the current packet in the Timestamp Range, decoded as described below. - basis.duration_since(pkt.timestamp).as_micros() as u64 + (basis - pkt.timestamp).as_micros() as u64 }; buf.write_var(delta >> exponent); basis = pkt.timestamp; @@ -950,7 +955,6 @@ impl From for IterErr { pub struct AckTimestampIter<'a> { timestamp_basis: u64, exponent: u64, - epoch: Instant, data: &'a [u8], deltas_remaining: usize, @@ -959,7 +963,7 @@ pub struct AckTimestampIter<'a> { } impl<'a> AckTimestampIter<'a> { - fn new(largest: u64, epoch: Instant, exponent: u64, mut data: &'a [u8]) -> Self { + fn new(largest: u64, exponent: u64, mut data: &'a [u8]) -> Self { // We read and throw away the Timestamp Range Count value because // it was already used to properly slice the data. // Unwrap safety: this byte block was scanned prior using scan_ack_timestamps_blocks. @@ -967,7 +971,6 @@ impl<'a> AckTimestampIter<'a> { AckTimestampIter { timestamp_basis: 0, exponent, - epoch, data, deltas_remaining: 0, first: true, @@ -1009,7 +1012,7 @@ impl<'a> Iterator for AckTimestampIter<'a> { Some(PacketTimestamp { packet_number: self.next_pn, - timestamp: self.epoch + Duration::from_micros(self.timestamp_basis), + timestamp: Duration::from_micros(self.timestamp_basis), }) } } @@ -1223,6 +1226,7 @@ mod test { #[cfg(test)] mod ack_timestamps_tests { use super::*; + use std::time::Instant; #[test] fn test_scan_ack_timestamps_block() { @@ -1299,15 +1303,17 @@ mod test { #[test] fn timestamp_iter() { - let mut timestamps = ReceiverTimestamps::new(100); let second = Duration::from_secs(1); let t0 = Instant::now(); + + let mut timestamps = ReceiverTimestamps::new(100, t0); + timestamps.add(1, t0 + second); timestamps.add(2, t0 + second * 2); timestamps.add(3, t0 + second * 3); let mut buf = bytes::BytesMut::new(); - Ack::encode_timestamps(×tamps, 12, &mut buf, t0, 0, 10); + Ack::encode_timestamps(×tamps, 12, &mut buf, 0, 10); // Manually decode and assert the values in the buffer. assert_eq!(1, buf.get_var().unwrap()); // timestamp_range_count @@ -1321,7 +1327,8 @@ mod test { #[test] fn timestamp_iter_with_gaps() { - let mut timestamps = ReceiverTimestamps::new(100); + let t0 = Instant::now(); + let mut timestamps = ReceiverTimestamps::new(100, t0); let one_second = Duration::from_secs(1); let t0 = Instant::now(); vec![(1..=3), (5..=5), (10..=12)] @@ -1331,7 +1338,7 @@ mod test { let mut buf = bytes::BytesMut::new(); - Ack::encode_timestamps(×tamps, 12, &mut buf, t0, 0, 10); + Ack::encode_timestamps(×tamps, 12, &mut buf, 0, 10); // Manually decode and assert the values in the buffer. assert_eq!(3, buf.get_var().unwrap()); // timestamp_range_count // @@ -1357,15 +1364,15 @@ mod test { #[test] fn timestamp_iter_with_exponent() { - let mut timestamps = ReceiverTimestamps::new(100); - let millisecond = Duration::from_millis(1); let t0 = Instant::now(); + let mut timestamps = ReceiverTimestamps::new(100, t0); + let millisecond = Duration::from_millis(1); timestamps.add(1, t0 + millisecond * 200); timestamps.add(2, t0 + millisecond * 300); let mut buf = bytes::BytesMut::new(); let exponent = 2; - Ack::encode_timestamps(×tamps, 12, &mut buf, t0, exponent, 10); + Ack::encode_timestamps(×tamps, 12, &mut buf, exponent, 10); // values below are tested in another unit test buf.get_var().unwrap(); // timestamp_range_count @@ -1378,57 +1385,57 @@ mod test { #[test] fn timestamp_encode_decode() { - let mut timestamps = ReceiverTimestamps::new(100); - let one_second = Duration::from_secs(1); let t0 = Instant::now(); + let mut timestamps = ReceiverTimestamps::new(100, t0); + let one_second = Duration::from_secs(1); timestamps.add(1, t0 + one_second); timestamps.add(2, t0 + one_second * 2); timestamps.add(3, t0 + one_second * 3); let mut buf = bytes::BytesMut::new(); - Ack::encode_timestamps(×tamps, 12, &mut buf, t0, 0, 10); + Ack::encode_timestamps(×tamps, 12, &mut buf, 0, 10); - let decoder = AckTimestampIter::new(12, t0, 0, &buf); + let decoder = AckTimestampIter::new(12, 0, &buf); let got: Vec<_> = decoder.collect(); // [(3, _), (2, _), (1, _)] assert_eq!(3, got.len()); assert_eq!(3, got[0].packet_number); - assert_eq!(t0 + (3 * one_second), got[0].timestamp,); + assert_eq!(3 * one_second, got[0].timestamp,); assert_eq!(2, got[1].packet_number); - assert_eq!(t0 + (2 * one_second), got[1].timestamp,); + assert_eq!(2 * one_second, got[1].timestamp,); assert_eq!(1, got[2].packet_number); - assert_eq!(t0 + (1 * one_second), got[2].timestamp); + assert_eq!(1 * one_second, got[2].timestamp); } #[test] fn timestamp_encode_decode_with_gaps() { - let mut timestamps = ReceiverTimestamps::new(100); - let one_second = Duration::from_secs(1); let t0 = Instant::now(); + let mut timestamps = ReceiverTimestamps::new(100, t0); + let one_second = Duration::from_secs(1); let expect: Vec<_> = vec![(1..=3), (5..=5), (10..=12)] .into_iter() .flatten() .collect::>() .into_iter() .map(|i| { - let t = t0 + one_second * i as u32; - timestamps.add(i, t); + let dt = one_second * i as u32; + timestamps.add(i, t0 + dt); PacketTimestamp { packet_number: i, - timestamp: t, + timestamp: dt, } }) .collect(); let mut buf = bytes::BytesMut::new(); - Ack::encode_timestamps(×tamps, 12, &mut buf, t0, 0, 10); + Ack::encode_timestamps(×tamps, 12, &mut buf, 0, 10); - let decoder = AckTimestampIter::new(12, t0, 0, &buf); + let decoder = AckTimestampIter::new(12, 0, &buf); let got: Vec<_> = decoder.collect(); assert_eq!(7, got.len()); @@ -1438,28 +1445,28 @@ mod test { #[test] fn timestamp_encode_max_ack() { // fix this - let mut timestamps = ReceiverTimestamps::new(2); - let one_second = Duration::from_secs(1); let t0 = Instant::now(); + let mut timestamps = ReceiverTimestamps::new(2, t0); + let one_second = Duration::from_secs(1); let expect: Vec<_> = vec![(1..=3), (5..=5), (10..=12)] .into_iter() .flatten() .collect::>() .into_iter() .map(|i| { - let t = t0 + one_second * i as u32; - timestamps.add(i, t); + let dt = one_second * i as u32; + timestamps.add(i, t0 + dt); PacketTimestamp { packet_number: i, - timestamp: t, + timestamp: dt, } }) .collect(); let mut buf = bytes::BytesMut::new(); - Ack::encode_timestamps(×tamps, 12, &mut buf, t0, 0, 2); - let decoder = AckTimestampIter::new(12, t0, 0, &buf); + Ack::encode_timestamps(×tamps, 12, &mut buf, 0, 2); + let decoder = AckTimestampIter::new(12, 0, &buf); let got: Vec<_> = decoder.collect(); assert_eq!(2, got.len()); From bb8df6b8055ecc67f955015059782c80dced82ca Mon Sep 17 00:00:00 2001 From: sterlingdeng Date: Tue, 22 Oct 2024 07:43:32 +0200 Subject: [PATCH 30/30] best effort timestamp send --- quinn-proto/src/connection/mod.rs | 5 +++++ quinn-proto/src/connection/receiver_timestamps.rs | 5 +++++ quinn-proto/src/connection/spaces.rs | 4 ---- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index 4253e013f..a5d171d33 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -3304,6 +3304,11 @@ impl Connection { buf, ); + if let Some(ts) = space.pending_acks.receiver_timestamps_as_mut() { + // Best effort / one try to send the timestamps to the peer. + ts.clear(); + } + stats.frame_tx.acks += 1; } diff --git a/quinn-proto/src/connection/receiver_timestamps.rs b/quinn-proto/src/connection/receiver_timestamps.rs index 138446a05..80114874e 100644 --- a/quinn-proto/src/connection/receiver_timestamps.rs +++ b/quinn-proto/src/connection/receiver_timestamps.rs @@ -61,6 +61,11 @@ impl ReceiverTimestamps { }); } + pub(crate) fn clear(&mut self) { + self.data.clear(); + } + + #[allow(dead_code)] pub(crate) fn subtract_below(&mut self, packet_number: u64) { if self.data.is_empty() { return; diff --git a/quinn-proto/src/connection/spaces.rs b/quinn-proto/src/connection/spaces.rs index f4e943228..00019cd8f 100644 --- a/quinn-proto/src/connection/spaces.rs +++ b/quinn-proto/src/connection/spaces.rs @@ -774,10 +774,6 @@ impl PendingAcks { /// Remove ACKs of packets numbered at or below `max` from the set of pending ACKs pub(super) fn subtract_below(&mut self, max: u64) { self.ranges.remove(0..(max + 1)); - - if let Some(v) = self.receiver_timestamps.as_mut() { - v.subtract_below(max); - } } /// Returns the set of currently pending ACK ranges