fix: Path is not app limited if more data is available to transmit (#…

…1326)

* fix: Path is not app limited if more data is available to transmit

* Update bandwidth estimator to track app-limited period

* Update recovery simulation to incorporate pacing

* Move congestion limited to the path

* Add comment

quic/s2n-quic-core/src/recovery/bandwidth/estimator.rs

@@ -139,6 +139,9 @@ pub struct Estimator {
     /// The send time of the packet that was most recently marked as delivered, or if the connection
     /// was recently idle, the send time of the first packet sent after resuming from idle.
     first_sent_time: Option<Timestamp>,
+    /// The `delivered_bytes` that marks the end of the current application-limited period, or
+    /// `None` if the connection is not currently application-limited.
+    app_limited_delivered_bytes: Option<u64>,
     rate_sample: RateSample,
 }
 
@@ -158,7 +161,7 @@ impl Estimator {
     pub fn on_packet_sent(
         &mut self,
         bytes_in_flight: u32,
-        is_app_limited: bool,
+        app_limited: Option<bool>,
         now: Timestamp,
     ) -> PacketInfo {
         //= https://tools.ietf.org/id/draft-cheng-iccrg-delivery-rate-estimation-02#3.2
@@ -171,6 +174,10 @@ impl Estimator {
             self.delivered_time = Some(now);
         }
 
+        if app_limited.unwrap_or(false) {
+            self.app_limited_delivered_bytes = Some(self.delivered_bytes + bytes_in_flight as u64);
+        }
+
         PacketInfo {
             delivered_bytes: self.delivered_bytes,
             delivered_time: self
@@ -181,7 +188,7 @@ impl Estimator {
                 .first_sent_time
                 .expect("initialized on first sent packet"),
             bytes_in_flight,
-            is_app_limited,
+            is_app_limited: self.app_limited_delivered_bytes.is_some(),
         }
     }
 
@@ -200,6 +207,16 @@ impl Estimator {
         self.delivered_bytes += bytes_acknowledged as u64;
         self.delivered_time = Some(now);
 
+        if self
+            .app_limited_delivered_bytes
+            .map_or(false, |app_limited_bytes| {
+                self.delivered_bytes > app_limited_bytes
+            })
+        {
+            // Clear app-limited field if bubble is ACKed and gone
+            self.app_limited_delivered_bytes = None;
+        }
+
         //= https://tools.ietf.org/id/draft-cheng-iccrg-delivery-rate-estimation-02#3.3
         //# UpdateRateSample() is invoked multiple times when a stretched ACK acknowledges
         //# multiple data packets. In this case we use the information from the most recently

quic/s2n-quic-core/src/recovery/bandwidth/estimator/tests.rs

@@ -84,23 +84,23 @@ fn on_packet_sent_timestamp_initialization() {
     let mut bw_estimator = Estimator::default();
 
     // Test that first_sent_time and delivered_time are updated on the first sent packet
-    let packet_info = bw_estimator.on_packet_sent(0, false, t0);
+    let packet_info = bw_estimator.on_packet_sent(0, None, t0);
     assert_eq!(t0, packet_info.first_sent_time);
     assert_eq!(t0, packet_info.delivered_time);
     assert_eq!(Some(t0), bw_estimator.first_sent_time);
     assert_eq!(Some(t0), bw_estimator.delivered_time);
 
     // Test that first_sent_time and delivered_time are not updated if packets are in flight
     let t1 = t0 + Duration::from_secs(1);
-    let packet_info = bw_estimator.on_packet_sent(1500, false, t1);
+    let packet_info = bw_estimator.on_packet_sent(1500, None, t1);
     assert_eq!(t0, packet_info.first_sent_time);
     assert_eq!(t0, packet_info.delivered_time);
     assert_eq!(Some(t0), bw_estimator.first_sent_time);
     assert_eq!(Some(t0), bw_estimator.delivered_time);
 
     // Test that first_sent_time and delivered_time are updated after an idle period
     let t2 = t0 + Duration::from_secs(2);
-    let packet_info = bw_estimator.on_packet_sent(0, false, t2);
+    let packet_info = bw_estimator.on_packet_sent(0, None, t2);
     assert_eq!(t2, packet_info.first_sent_time);
     assert_eq!(t2, packet_info.delivered_time);
     assert_eq!(Some(t2), bw_estimator.first_sent_time);
@@ -116,16 +116,66 @@ fn on_packet_sent() {
         delivered_time: Some(delivered_time),
         lost_bytes: 100,
         first_sent_time: Some(first_sent_time),
+        app_limited_delivered_bytes: None,
         rate_sample: Default::default(),
     };
 
-    let packet_info = bw_estimator.on_packet_sent(500, true, first_sent_time);
+    let packet_info = bw_estimator.on_packet_sent(500, Some(true), first_sent_time);
     assert_eq!(first_sent_time, packet_info.first_sent_time);
     assert_eq!(delivered_time, packet_info.delivered_time);
     assert_eq!(15000, packet_info.delivered_bytes);
     assert_eq!(100, packet_info.lost_bytes);
     assert!(packet_info.is_app_limited);
     assert_eq!(500, packet_info.bytes_in_flight);
+    assert_eq!(Some(500 + 15000), bw_estimator.app_limited_delivered_bytes);
+}
+
+#[test]
+fn app_limited() {
+    let first_sent_time = NoopClock.get_time();
+    let delivered_time = first_sent_time + Duration::from_secs(1);
+    let mut bw_estimator = Estimator {
+        delivered_bytes: 15000,
+        delivered_time: Some(delivered_time),
+        lost_bytes: 100,
+        first_sent_time: Some(first_sent_time),
+        app_limited_delivered_bytes: None,
+        rate_sample: Default::default(),
+    };
+
+    // Packet is sent while app-limited, starting the app limited period
+    let packet_info = bw_estimator.on_packet_sent(1500, Some(true), first_sent_time);
+    assert!(packet_info.is_app_limited);
+    assert_eq!(Some(1500 + 15000), bw_estimator.app_limited_delivered_bytes);
+
+    // Packet is sent while not app-limited, but the app limited continues until all previous bytes in flight have been acknowledged
+    let packet_info = bw_estimator.on_packet_sent(500, Some(false), first_sent_time);
+    assert!(packet_info.is_app_limited);
+    assert_eq!(Some(1500 + 15000), bw_estimator.app_limited_delivered_bytes);
+
+    // Packet is sent while app-limited is not determined, but the app limited continues until all previous bytes in flight have been acknowledged
+    let packet_info = bw_estimator.on_packet_sent(500, None, first_sent_time);
+    assert!(packet_info.is_app_limited);
+    assert_eq!(Some(1500 + 15000), bw_estimator.app_limited_delivered_bytes);
+
+    let packet_info = PacketInfo {
+        delivered_bytes: bw_estimator.delivered_bytes,
+        delivered_time,
+        lost_bytes: 0,
+        first_sent_time,
+        bytes_in_flight: 1500,
+        is_app_limited: false,
+    };
+
+    // Acknowledge all the bytes that were inflight when the app-limited period began
+    bw_estimator.on_ack(1500, delivered_time, packet_info, delivered_time);
+    // Still app_limited, since we need bytes to be acknowledged after the app limited period
+    assert_eq!(Some(1500 + 15000), bw_estimator.app_limited_delivered_bytes);
+
+    // Acknowledge one more byte
+    bw_estimator.on_ack(1, delivered_time, packet_info, delivered_time);
+    // Now the app limited period is over
+    assert_eq!(None, bw_estimator.app_limited_delivered_bytes);
 }
 
 #[test]
@@ -137,11 +187,11 @@ fn on_packet_ack_rate_sample() {
 
     // Send three packets. In between each send, other packets were acknowledged, and thus the
     // delivered_bytes amount is increased.
-    let packet_1 = bw_estimator.on_packet_sent(0, false, t0);
+    let packet_1 = bw_estimator.on_packet_sent(0, Some(false), t0);
     bw_estimator.delivered_bytes = 100000;
-    let packet_2 = bw_estimator.on_packet_sent(1500, true, t1);
+    let packet_2 = bw_estimator.on_packet_sent(1500, Some(true), t1);
     bw_estimator.delivered_bytes = 200000;
-    let packet_3 = bw_estimator.on_packet_sent(3000, false, t2);
+    let packet_3 = bw_estimator.on_packet_sent(3000, Some(false), t2);
 
     let now = t0 + Duration::from_secs(10);
     let delivered_bytes = bw_estimator.delivered_bytes;
@@ -245,13 +295,13 @@ fn on_packet_ack_implausible_ack_rate() {
     let mut bw_estimator = Estimator::default();
 
     // A packet is sent and acknowledged 4 seconds later
-    let packet_info = bw_estimator.on_packet_sent(0, false, t0);
+    let packet_info = bw_estimator.on_packet_sent(0, Some(false), t0);
     let t4 = t0 + Duration::from_secs(4);
     bw_estimator.on_ack(1500, t0, packet_info, t4);
 
     // A packet is sent and acknowledged 1 second later
     let t5 = t0 + Duration::from_secs(5);
-    let packet_info = bw_estimator.on_packet_sent(1500, false, t5);
+    let packet_info = bw_estimator.on_packet_sent(1500, Some(false), t5);
     let now = t0 + Duration::from_secs(6);
     bw_estimator.on_ack(1500, t0 + Duration::from_secs(5), packet_info, now);
 

quic/s2n-quic-core/src/recovery/bbr.rs

@@ -100,13 +100,9 @@ impl CongestionController for BbrCongestionController {
         &mut self,
         time_sent: Timestamp,
         sent_bytes: usize,
+        app_limited: Option<bool>,
         _rtt_estimator: &RttEstimator,
     ) -> Self::PacketInfo {
-        let is_app_limited = false; // TODO: determine if app limited
-        let packet_info =
-            self.bw_estimator
-                .on_packet_sent(*self.bytes_in_flight, is_app_limited, time_sent);
-
         if sent_bytes > 0 {
             self.recovery_state.on_packet_sent();
 
@@ -115,7 +111,8 @@ impl CongestionController for BbrCongestionController {
                 .expect("sent_bytes should not exceed u32::MAX");
         }
 
-        packet_info
+        self.bw_estimator
+            .on_packet_sent(*self.bytes_in_flight, app_limited, time_sent)
     }
 
     fn on_rtt_update(&mut self, _time_sent: Timestamp, _rtt_estimator: &RttEstimator) {

quic/s2n-quic-core/src/recovery/congestion_controller.rs

@@ -63,13 +63,17 @@ pub trait CongestionController: 'static + Clone + Send + Debug {
     /// the packet is acknowledged and the packet was the newest acknowledged in the ACK frame,
     /// or to `on_packet_lost` if the packet was declared lost.
     ///
+    /// `app_limited` indicates whether the application has enough data to send to fill the
+    /// congestion window. This value will be `None` for Initial and Handshake packets.
+    ///
     /// Note: Sent bytes may be 0 in the case the packet being sent contains only ACK frames.
     /// These pure ACK packets are not congestion-controlled to ensure congestion control
     /// does not impede congestion feedback.
     fn on_packet_sent(
         &mut self,
         time_sent: Timestamp,
         sent_bytes: usize,
+        app_limited: Option<bool>,
         rtt_estimator: &RttEstimator,
     ) -> Self::PacketInfo;
 
@@ -182,6 +186,7 @@ pub mod testing {
                 &mut self,
                 _time_sent: Timestamp,
                 _bytes_sent: usize,
+                _app_limited: Option<bool>,
                 _rtt_estimator: &RttEstimator,
             ) {
             }
@@ -252,6 +257,7 @@ pub mod testing {
             pub congestion_events: u32,
             pub requires_fast_retransmission: bool,
             pub loss_bursts: u32,
+            pub app_limited: Option<bool>,
         }
 
         impl Default for CongestionController {
@@ -268,6 +274,7 @@ pub mod testing {
                     congestion_events: 0,
                     requires_fast_retransmission: false,
                     loss_bursts: 0,
+                    app_limited: None,
                 }
             }
         }
@@ -295,10 +302,12 @@ pub mod testing {
                 &mut self,
                 _time_sent: Timestamp,
                 bytes_sent: usize,
+                app_limited: Option<bool>,
                 _rtt_estimator: &RttEstimator,
             ) {
                 self.bytes_in_flight += bytes_sent as u32;
                 self.requires_fast_retransmission = false;
+                self.app_limited = app_limited;
             }
 
             fn on_rtt_update(&mut self, _time_sent: Timestamp, _rtt_estimator: &RttEstimator) {

quic/s2n-quic-core/src/recovery/cubic.rs

@@ -193,6 +193,7 @@ impl CongestionController for CubicCongestionController {
         &mut self,
         time_sent: Timestamp,
         bytes_sent: usize,
+        app_limited: Option<bool>,
         rtt_estimator: &RttEstimator,
     ) {
         if bytes_sent == 0 {
@@ -204,7 +205,14 @@ impl CongestionController for CubicCongestionController {
             .try_add(bytes_sent)
             .expect("bytes sent should not exceed u32::MAX");
 
-        self.under_utilized = self.is_congestion_window_under_utilized();
+        if let Some(app_limited) = app_limited {
+            // We check both the given `app_limited` value and is_congestion_window_under_utilized()
+            // as is_congestion_window_under_utilized() is more lenient with respect to the utilization
+            // of the congestion window than the app_limited check. is_congestion_window_under_utilized()
+            // returns true if there are more than 3 MTU's of space left in the cwnd, or less than
+            // half the cwnd is utilized in slow start.
+            self.under_utilized = app_limited && self.is_congestion_window_under_utilized();
+        }
 
         if let Recovery(recovery_start_time, RequiresTransmission) = self.state {
             // A packet has been sent since we entered recovery (fast retransmission)