Proposed fix to mozilla#585

Instead of tracking largest acked sent time, track oldest lost packet
time. Compare against most recent lost packet

neqo-transport/src/cc.rs

@@ -6,7 +6,7 @@
 
 // Congestion control
 
-use std::cmp::max;
+use std::cmp::{max, min};
 use std::fmt::{self, Display};
 use std::time::{Duration, Instant};
 
@@ -29,6 +29,8 @@ pub struct CongestionControl {
     bytes_in_flight: usize,
     congestion_recovery_start_time: Option<Instant>,
     ssthresh: usize,
+
+    oldest_lost_pkt_time: Option<Instant>,
 }
 
 impl Default for CongestionControl {
@@ -38,6 +40,7 @@ impl Default for CongestionControl {
             bytes_in_flight: 0,
             congestion_recovery_start_time: None,
             ssthresh: std::usize::MAX,
+            oldest_lost_pkt_time: None,
         }
     }
 }
@@ -97,20 +100,23 @@ impl CongestionControl {
         }
     }
 
-    pub fn on_packets_lost(
-        &mut self,
-        now: Instant,
-        largest_acked_sent: Option<Instant>,
-        pto: Duration,
-        lost_packets: &[SentPacket],
-    ) {
+    pub fn clear_oldest_lost_pkt_time(&mut self) {
+        self.oldest_lost_pkt_time = None;
+    }
+
+    pub fn on_packets_lost(&mut self, now: Instant, pto: Duration, lost_packets: &[SentPacket]) {
         if lost_packets.is_empty() {
             return;
         }
 
         for pkt in lost_packets.iter().filter(|pkt| pkt.cc_in_flight()) {
             assert!(self.bytes_in_flight >= pkt.size);
             self.bytes_in_flight -= pkt.size;
+
+            self.oldest_lost_pkt_time = match self.oldest_lost_pkt_time {
+                None => Some(pkt.time_sent),
+                Some(prev_oldest) => Some(min(prev_oldest, pkt.time_sent)),
+            }
         }
 
         qdebug!([self], "Pkts lost {}", lost_packets.len());
@@ -121,12 +127,11 @@ impl CongestionControl {
         let in_persistent_congestion = {
             let congestion_period = pto * PERSISTENT_CONG_THRESH;
 
-            match largest_acked_sent {
-                Some(las) => las < last_lost_pkt.time_sent - congestion_period,
+            match self.oldest_lost_pkt_time {
+                Some(olpt) => olpt < last_lost_pkt.time_sent - congestion_period,
                 None => {
-                    // Nothing has ever been acked. Could still be PC.
-                    let first_lost_pkt_sent = lost_packets.first().unwrap().time_sent;
-                    last_lost_pkt.time_sent - first_lost_pkt_sent > congestion_period
+                    // No previous lost packets, it's not PC.
+                    false
                 }
             }
         };

neqo-transport/src/recovery.rs

@@ -101,7 +101,6 @@ impl RttVals {
 pub(crate) struct LossRecoverySpace {
     space: PNSpace,
     largest_acked: Option<u64>,
-    largest_acked_sent_time: Option<Instant>,
     time_of_last_sent_ack_eliciting_packet: Option<Instant>,
     ack_eliciting_outstanding: u64,
     sent_packets: BTreeMap<u64, SentPacket>,
@@ -116,7 +115,6 @@ impl LossRecoverySpace {
         Self {
             space,
             largest_acked: None,
-            largest_acked_sent_time: None,
             time_of_last_sent_ack_eliciting_packet: None,
             ack_eliciting_outstanding: 0,
             sent_packets: BTreeMap::default(),
@@ -490,14 +488,12 @@ impl LossRecovery {
         }
 
         // Track largest PN acked per space
-        let prev_largest_acked_sent_time = space.largest_acked_sent_time;
         if Some(largest_acked) > space.largest_acked {
             space.largest_acked = Some(largest_acked);
 
             // If the largest acknowledged is newly acked and any newly acked
             // packet was ack-eliciting, update the RTT. (-recovery 5.1)
             let largest_acked_pkt = acked_packets.last().expect("must be there");
-            space.largest_acked_sent_time = Some(largest_acked_pkt.time_sent);
             if any_ack_eliciting {
                 let latest_rtt = now - largest_acked_pkt.time_sent;
                 self.rtt_vals.update_rtt(latest_rtt, ack_delay);
@@ -513,12 +509,10 @@ impl LossRecovery {
             &mut lost_packets,
         );
         // TODO Process ECN information if present.
-        self.cc.on_packets_lost(
-            now,
-            prev_largest_acked_sent_time,
-            self.rtt_vals.pto(pn_space),
-            &lost_packets,
-        );
+        self.cc
+            .on_packets_lost(now, self.rtt_vals.pto(pn_space), &lost_packets);
+
+        self.cc.clear_oldest_lost_pkt_time();
 
         self.pto_state = None;
 
@@ -661,7 +655,6 @@ impl LossRecovery {
             space.detect_lost_packets(now, loss_delay, &mut lost_packets);
             self.cc.on_packets_lost(
                 now,
-                space.largest_acked_sent_time,
                 self.rtt_vals.pto(space.space()),
                 &lost_packets[first..],
             )