diff --git a/neqo-transport/src/cc/classic_cc.rs b/neqo-transport/src/cc/classic_cc.rs index 0286b29913..bb0255ee9b 100644 --- a/neqo-transport/src/cc/classic_cc.rs +++ b/neqo-transport/src/cc/classic_cc.rs @@ -1597,8 +1597,8 @@ mod tests { // 2. Lose packets (1, 2) --> `RecoveryStart`, 1 event, reduced cwnd let mut lost_pkt1 = pkt1.clone(); let mut lost_pkt2 = pkt2.clone(); - lost_pkt1.declare_lost(now); - lost_pkt2.declare_lost(now); + lost_pkt1.declare_lost(now, sent::LossTrigger::TimeThreshold); + lost_pkt2.declare_lost(now, sent::LossTrigger::TimeThreshold); cc.on_packets_lost( Some(now), None, @@ -1747,7 +1747,7 @@ mod tests { assert_eq!(cc_stats.congestion_events[CongestionEvent::Spurious], 0); let mut lost_pkt1 = pkt1.clone(); - lost_pkt1.declare_lost(now); + lost_pkt1.declare_lost(now, sent::LossTrigger::TimeThreshold); // Step 2: Lose packet 1 → congestion event #1 cc.on_packets_lost( @@ -1775,7 +1775,7 @@ mod tests { assert_eq!(cc_stats.congestion_events[CongestionEvent::Spurious], 1); let mut lost_pkt2 = pkt2.clone(); - lost_pkt2.declare_lost(now); + lost_pkt2.declare_lost(now, sent::LossTrigger::TimeThreshold); // Step 5. Lose packet 2 → New congestion event as we left recovery when restoring the // previous params. diff --git a/neqo-transport/src/qlog.rs b/neqo-transport/src/qlog.rs index b50f897b36..9d1a7157a3 100644 --- a/neqo-transport/src/qlog.rs +++ b/neqo-transport/src/qlog.rs @@ -21,9 +21,9 @@ use qlog::events::{ quic::{ AckedRanges, CongestionStateUpdated, CongestionStateUpdatedTrigger, ErrorSpace, LossTimerEventType, LossTimerUpdated, MetricsUpdated, PacketDropped, PacketDroppedTrigger, - PacketHeader, PacketLost, PacketNumberSpace as QlogPacketNumberSpace, PacketReceived, - PacketSent, PacketsAcked, QuicFrame, RecoveryParametersSet, StreamType, TimerType, - VersionInformation, + PacketHeader, PacketLost, PacketLostTrigger, PacketNumberSpace as QlogPacketNumberSpace, + PacketReceived, PacketSent, PacketsAcked, QuicFrame, RecoveryParametersSet, StreamType, + TimerType, VersionInformation, }, }; use smallvec::SmallVec; @@ -288,8 +288,14 @@ pub fn packets_lost(qlog: &mut Qlog, pkts: &[sent::Packet], now: Instant) { let header = PacketHeader::with_type(pkt.packet_type().into(), Some(pkt.pn()), None, None, None); + let trigger = pkt + .loss_info() + .map(|info| PacketLostTrigger::from(info.trigger)) + .or_else(|| pkt.pto_fired().then_some(PacketLostTrigger::PtoExpired)); + let ev_data = EventData::PacketLost(PacketLost { header: Some(header), + trigger, ..Default::default() }); @@ -817,6 +823,15 @@ impl From for TimerType { } } +impl From for PacketLostTrigger { + fn from(value: sent::LossTrigger) -> Self { + match value { + sent::LossTrigger::TimeThreshold => Self::TimeThreshold, + sent::LossTrigger::ReorderingThreshold => Self::ReorderingThreshold, + } + } +} + impl From for qlog::events::quic::PacketType { fn from(value: packet::Type) -> Self { match value { diff --git a/neqo-transport/src/recovery/mod.rs b/neqo-transport/src/recovery/mod.rs index 8e2789099b..e4cbefff38 100644 --- a/neqo-transport/src/recovery/mod.rs +++ b/neqo-transport/src/recovery/mod.rs @@ -330,26 +330,28 @@ impl LossRecoverySpace { .take_while(|p| largest_acked.is_some_and(|largest_ack| p.pn() < largest_ack)) { // Packets sent before now - loss_delay are deemed lost. - if packet.time_sent() + loss_delay <= now { + let trigger = if packet.time_sent() + loss_delay <= now { qtrace!( "lost={}, time sent {:?} is before lost_delay {loss_delay:?}", packet.pn(), packet.time_sent() ); + sent::LossTrigger::TimeThreshold } else if largest_acked >= Some(packet.pn() + PACKET_THRESHOLD) { qtrace!( "lost={}, is >= {PACKET_THRESHOLD} from largest acked {largest_acked:?}", packet.pn() ); + sent::LossTrigger::ReorderingThreshold } else { if largest_acked.is_some() { self.first_ooo_time = Some(packet.time_sent()); } // No more packets can be declared lost after this one. break; - } + }; - if packet.declare_lost(now) { + if packet.declare_lost(now, trigger) { lost_packets.push(packet.clone()); } } diff --git a/neqo-transport/src/recovery/sent.rs b/neqo-transport/src/recovery/sent.rs index efa70e4f01..d2346176f5 100644 --- a/neqo-transport/src/recovery/sent.rs +++ b/neqo-transport/src/recovery/sent.rs @@ -15,6 +15,20 @@ use std::{ use crate::{packet, recovery}; +/// The reason a packet was declared lost. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum LossTrigger { + TimeThreshold, + ReorderingThreshold, +} + +/// Information recorded when a packet is declared lost. +#[derive(Debug, Clone, Copy)] +pub struct LossInfo { + pub time: Instant, + pub trigger: LossTrigger, +} + #[derive(Debug, Clone)] pub struct Packet { pt: packet::Type, @@ -24,7 +38,7 @@ pub struct Packet { primary_path: bool, tokens: Rc, - time_declared_lost: Option, + loss_info: Option, /// After a PTO, this is true when the packet has been released. pto: bool, @@ -48,7 +62,7 @@ impl Packet { ack_eliciting, primary_path: true, tokens: Rc::new(tokens), - time_declared_lost: None, + loss_info: None, pto: false, len, } @@ -132,7 +146,7 @@ impl Packet { /// Whether the packet has been declared lost. #[must_use] pub const fn lost(&self) -> bool { - self.time_declared_lost.is_some() + self.loss_info.is_some() } /// Whether accounting for the loss or acknowledgement in the @@ -152,12 +166,13 @@ impl Packet { self.ack_eliciting() && self.on_primary_path() } - /// Declare the packet as lost. Returns `true` if this is the first time. - pub const fn declare_lost(&mut self, now: Instant) -> bool { + /// Declare the packet as lost with the given trigger. Returns `true` if + /// this is the first time. + pub const fn declare_lost(&mut self, now: Instant, trigger: LossTrigger) -> bool { if self.lost() { false } else { - self.time_declared_lost = Some(now); + self.loss_info = Some(LossInfo { time: now, trigger }); true } } @@ -166,8 +181,8 @@ impl Packet { /// that it can be expired and no longer tracked. #[must_use] pub fn expired(&self, now: Instant, expiration_period: Duration) -> bool { - self.time_declared_lost - .is_some_and(|loss_time| (loss_time + expiration_period) <= now) + self.loss_info + .is_some_and(|info| (info.time + expiration_period) <= now) } /// Whether the packet contents were cleared out after a PTO. @@ -176,6 +191,12 @@ impl Packet { self.pto } + /// Loss information recorded when this packet was declared lost. + #[must_use] + pub const fn loss_info(&self) -> Option { + self.loss_info + } + /// On PTO, we need to get the recovery tokens so that we can ensure that /// the frames we sent can be sent again in the PTO packet(s). Do that just once. #[must_use] @@ -334,7 +355,7 @@ mod tests { time::{Duration, Instant}, }; - use super::{Packet, Packets}; + use super::{LossTrigger, Packet, Packets}; use crate::{packet, recovery}; const PACKET_GAP: Duration = Duration::from_secs(1); @@ -435,7 +456,7 @@ mod tests { remove_one(&mut pkts, 0); for p in pkts.iter_mut() { - p.declare_lost(p.time_sent); // just to keep things simple. + p.declare_lost(p.time_sent, LossTrigger::TimeThreshold); // just to keep things simple. } // Expire up to pkt(1). @@ -470,7 +491,27 @@ mod tests { #[test] fn pto_after_lost() { let mut p = pkt(0); - p.declare_lost(start_time()); + p.declare_lost(start_time(), LossTrigger::TimeThreshold); assert!(!p.pto()); // Lost packet returns false } + + #[test] + fn loss_info_default() { + let p = pkt(0); + assert!(p.loss_info().is_none()); + } + + #[test] + fn loss_info_declared() { + let t = start_time(); + let mut p = pkt(0); + assert!(p.declare_lost(t, LossTrigger::TimeThreshold)); + let info = p.loss_info().unwrap(); + assert_eq!(info.time, t); + assert_eq!(info.trigger, LossTrigger::TimeThreshold); + + // Second declaration is ignored. + assert!(!p.declare_lost(t, LossTrigger::ReorderingThreshold)); + assert_eq!(p.loss_info().unwrap().trigger, LossTrigger::TimeThreshold); + } }