diff --git a/quic/s2n-quic-core/src/recovery/bandwidth/estimator.rs b/quic/s2n-quic-core/src/recovery/bandwidth/estimator.rs index 8fe5e85ebc..e6e30b0554 100644 --- a/quic/s2n-quic-core/src/recovery/bandwidth/estimator.rs +++ b/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, + /// 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, 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, 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 diff --git a/quic/s2n-quic-core/src/recovery/bandwidth/estimator/tests.rs b/quic/s2n-quic-core/src/recovery/bandwidth/estimator/tests.rs index 95004a2ebb..882a00fccc 100644 --- a/quic/s2n-quic-core/src/recovery/bandwidth/estimator/tests.rs +++ b/quic/s2n-quic-core/src/recovery/bandwidth/estimator/tests.rs @@ -84,7 +84,7 @@ 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); @@ -92,7 +92,7 @@ fn on_packet_sent_timestamp_initialization() { // 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); @@ -100,7 +100,7 @@ fn on_packet_sent_timestamp_initialization() { // 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); diff --git a/quic/s2n-quic-core/src/recovery/bbr.rs b/quic/s2n-quic-core/src/recovery/bbr.rs index 0f6e54f69c..4e67c41a81 100644 --- a/quic/s2n-quic-core/src/recovery/bbr.rs +++ b/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, _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) { diff --git a/quic/s2n-quic-core/src/recovery/congestion_controller.rs b/quic/s2n-quic-core/src/recovery/congestion_controller.rs index 104ed6dd6b..74e07dbce6 100644 --- a/quic/s2n-quic-core/src/recovery/congestion_controller.rs +++ b/quic/s2n-quic-core/src/recovery/congestion_controller.rs @@ -63,6 +63,9 @@ 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. @@ -70,6 +73,7 @@ pub trait CongestionController: 'static + Clone + Send + Debug { &mut self, time_sent: Timestamp, sent_bytes: usize, + app_limited: Option, rtt_estimator: &RttEstimator, ) -> Self::PacketInfo; @@ -182,6 +186,7 @@ pub mod testing { &mut self, _time_sent: Timestamp, _bytes_sent: usize, + _app_limited: Option, _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, } 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, _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) { diff --git a/quic/s2n-quic-core/src/recovery/cubic.rs b/quic/s2n-quic-core/src/recovery/cubic.rs index 82bd1342d6..313fe86842 100644 --- a/quic/s2n-quic-core/src/recovery/cubic.rs +++ b/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, 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) diff --git a/quic/s2n-quic-core/src/recovery/cubic/tests.rs b/quic/s2n-quic-core/src/recovery/cubic/tests.rs index 6657e510aa..58c0d0983a 100644 --- a/quic/s2n-quic-core/src/recovery/cubic/tests.rs +++ b/quic/s2n-quic-core/src/recovery/cubic/tests.rs @@ -216,7 +216,7 @@ fn on_packet_sent() { cc.congestion_window = 100_000.0; // Last sent packet time updated to t10 - cc.on_packet_sent(now + Duration::from_secs(10), 1, &rtt_estimator); + cc.on_packet_sent(now + Duration::from_secs(10), 1, None, &rtt_estimator); assert_eq!(cc.bytes_in_flight, 1); @@ -250,7 +250,7 @@ fn on_packet_sent() { ); // Last sent packet time updated to t20 - cc.on_packet_sent(now + Duration::from_secs(20), 1, &rtt_estimator); + cc.on_packet_sent(now + Duration::from_secs(20), 1, None, &rtt_estimator); assert_eq!(cc.bytes_in_flight, 2); @@ -273,7 +273,7 @@ fn on_packet_sent_application_limited() { cc.state = SlowStart; // t0: Send a packet in Slow Start - cc.on_packet_sent(now, 1000, &rtt_estimator); + cc.on_packet_sent(now, 1000, Some(true), &rtt_estimator); assert_eq!(cc.bytes_in_flight, 93_500); assert_eq!(cc.time_of_last_sent_packet, Some(now)); @@ -284,7 +284,12 @@ fn on_packet_sent_application_limited() { assert!(!cc.under_utilized); // t15: Send a packet in Congestion Avoidance - cc.on_packet_sent(now + Duration::from_secs(15), 1000, &rtt_estimator); + cc.on_packet_sent( + now + Duration::from_secs(15), + 1000, + Some(true), + &rtt_estimator, + ); assert_eq!(cc.bytes_in_flight, 94_500); assert_eq!( @@ -295,7 +300,12 @@ fn on_packet_sent_application_limited() { // t20: Send packets to fully utilize the congestion window while cc.bytes_in_flight < cc.congestion_window() { - cc.on_packet_sent(now + Duration::from_secs(20), 1000, &rtt_estimator); + cc.on_packet_sent( + now + Duration::from_secs(20), + 1000, + Some(true), + &rtt_estimator, + ); } assert!(!cc.under_utilized); @@ -311,7 +321,7 @@ fn on_packet_sent_fast_retransmission() { cc.bytes_in_flight = BytesInFlight::new(99900); cc.state = Recovery(now, RequiresTransmission); - cc.on_packet_sent(now + Duration::from_secs(10), 100, &rtt_estimator); + cc.on_packet_sent(now + Duration::from_secs(10), 100, None, &rtt_estimator); assert_eq!(cc.state, Recovery(now, Idle)); } @@ -334,7 +344,7 @@ fn congestion_avoidance_after_idle_period() { cc.state = SlowStart; // t0: Send a packet in Slow Start - cc.on_packet_sent(now, 1000, rtt_estimator); + cc.on_packet_sent(now, 1000, Some(true), rtt_estimator); assert_eq!(cc.bytes_in_flight, 1000); @@ -343,7 +353,12 @@ fn congestion_avoidance_after_idle_period() { cc.state = State::congestion_avoidance(now + Duration::from_secs(10)); // t15: Send a packet in Congestion Avoidance while under utilized - cc.on_packet_sent(now + Duration::from_secs(15), 1000, rtt_estimator); + cc.on_packet_sent( + now + Duration::from_secs(15), + 1000, + Some(true), + rtt_estimator, + ); assert!(cc.is_congestion_window_under_utilized()); assert_eq!(cc.bytes_in_flight, 2000); @@ -371,7 +386,12 @@ fn congestion_avoidance_after_idle_period() { // t20: Send packets to fully utilize the congestion window while cc.bytes_in_flight < cc.congestion_window() { - cc.on_packet_sent(now + Duration::from_secs(20), 1000, rtt_estimator); + cc.on_packet_sent( + now + Duration::from_secs(20), + 1000, + Some(false), + rtt_estimator, + ); } assert!(!cc.is_congestion_window_under_utilized()); @@ -729,7 +749,7 @@ fn on_packet_ack_utilized_then_under_utilized() { cc.congestion_window = 100_000.0; cc.state = SlowStart; - cc.on_packet_sent(now, 60_000, &rtt_estimator); + cc.on_packet_sent(now, 60_000, Some(true), &rtt_estimator); cc.on_ack(now, 50_000, (), &rtt_estimator, random, now); let cwnd = cc.congestion_window(); @@ -752,7 +772,7 @@ fn on_packet_ack_utilized_then_under_utilized() { // Now the application has had a chance to send more data, but it didn't send enough to // utilize the congestion window, so the window does not grow. - cc.on_packet_sent(now, 1200, &rtt_estimator); + cc.on_packet_sent(now, 1200, Some(true), &rtt_estimator); assert!(cc.under_utilized); cc.on_ack( now, diff --git a/quic/s2n-quic-core/tests/recovery/simulation.rs b/quic/s2n-quic-core/tests/recovery/simulation.rs index a5be455f15..fac37029f6 100644 --- a/quic/s2n-quic-core/tests/recovery/simulation.rs +++ b/quic/s2n-quic-core/tests/recovery/simulation.rs @@ -219,7 +219,7 @@ fn minimum_window( let random = &mut random::testing::Generator::default(); let packet_info = - congestion_controller.on_packet_sent(time_zero, MINIMUM_MTU as usize, &rtt_estimator); + congestion_controller.on_packet_sent(time_zero, MINIMUM_MTU as usize, None, &rtt_estimator); // Experience persistent congestion to drop to the minimum window congestion_controller.on_packet_lost( MINIMUM_MTU as u32, @@ -230,7 +230,7 @@ fn minimum_window( time_zero, ); let packet_info = - congestion_controller.on_packet_sent(time_zero, MINIMUM_MTU as usize, &rtt_estimator); + congestion_controller.on_packet_sent(time_zero, MINIMUM_MTU as usize, None, &rtt_estimator); // Lose a packet to exit slow start congestion_controller.on_packet_lost( MINIMUM_MTU as u32, @@ -307,6 +307,7 @@ fn simulate_constant_rtt( let packet_info = congestion_controller.on_packet_sent( round_start, MINIMUM_MTU as usize, + None, &rtt_estimator, ); congestion_controller.on_packet_lost( @@ -320,7 +321,7 @@ fn simulate_constant_rtt( drop_index += 1; } else { let send_bytes = (congestion_controller.congestion_window() as usize) - .min(app_limit.unwrap_or(usize::max_value())); + .min(app_limit.unwrap_or(usize::MAX)); // Send and ack the full congestion window send_and_ack( @@ -343,32 +344,57 @@ fn send_and_ack( bytes: usize, ) { let random = &mut random::testing::Generator::default(); - let mut remaining = bytes; + let mut tx_remaining = bytes; + let mut rx_remaining = 0; + let mut now = timestamp; + let ack_receive_time = now + rtt_estimator.min_rtt(); + // Allow acks to start being received after this time, to simulate + // acks arriving while sending is paused by the pacer. + let earliest_ack_receive_time = ack_receive_time - Duration::from_millis(50); + let sending_full_cwnd = bytes as u32 == congestion_controller.congestion_window(); let mut packet_info = None; - while remaining > 0 { - let bytes_sent = remaining.min(MINIMUM_MTU as usize); - packet_info = - Some(congestion_controller.on_packet_sent(timestamp, bytes_sent, rtt_estimator)); - remaining -= bytes_sent; - } - - let ack_receive_time = timestamp + rtt_estimator.min_rtt(); + while tx_remaining > 0 || rx_remaining > 0 { + while tx_remaining > 0 { + if let Some(edt) = congestion_controller.earliest_departure_time() { + if !edt.has_elapsed(now) { + // We are blocked by the pacer, stop sending and fast forward to the earliest departure time + now = edt; + break; + } + } + + let bytes_sent = tx_remaining.min(MINIMUM_MTU as usize); + let app_limited = tx_remaining - bytes_sent == 0 && !sending_full_cwnd; + + packet_info = Some(congestion_controller.on_packet_sent( + now, + bytes_sent, + Some(app_limited), + rtt_estimator, + )); + tx_remaining -= bytes_sent; + rx_remaining += bytes_sent; + } - let mut remaining = bytes; + if tx_remaining == 0 { + // Nothing left to send, so fast forward to when we receive acks for everything + now = ack_receive_time; + } - while remaining > 0 { - let bytes_sent = remaining.min(MINIMUM_MTU as usize); + while now >= earliest_ack_receive_time && rx_remaining > 0 { + let bytes_acked = rx_remaining.min(MINIMUM_MTU as usize); - congestion_controller.on_ack( - ack_receive_time, - bytes_sent, - packet_info.unwrap(), - rtt_estimator, - random, - ack_receive_time, - ); - remaining -= bytes_sent; + congestion_controller.on_ack( + now, + bytes_acked, + packet_info.unwrap(), + rtt_estimator, + random, + now, + ); + rx_remaining -= bytes_acked; + } } } diff --git a/quic/s2n-quic-core/tests/recovery/snapshots/recovery_simulation__AppLimited1MB-CubicCongestionController.snap b/quic/s2n-quic-core/tests/recovery/snapshots/recovery_simulation__AppLimited1MB-CubicCongestionController.snap index 8733761208..aae102ef80 100644 --- a/quic/s2n-quic-core/tests/recovery/snapshots/recovery_simulation__AppLimited1MB-CubicCongestionController.snap +++ b/quic/s2n-quic-core/tests/recovery/snapshots/recovery_simulation__AppLimited1MB-CubicCongestionController.snap @@ -1,8 +1,6 @@ --- source: quic/s2n-quic-core/tests/recovery/simulation.rs -assertion_line: 148 expression: self - --- Simulation { name: "App Limited 1MB", @@ -87,47 +85,47 @@ Simulation { 75: pkts: 712, 76: pkts: 720, 77: pkts: 728, - 78: pkts: 738, + 78: pkts: 737, 79: pkts: 747, 80: pkts: 758, - 81: pkts: 769, + 81: pkts: 768, 82: pkts: 780, 83: pkts: 792, 84: pkts: 805, 85: pkts: 819, - 86: pkts: 834, - 87: pkts: 849, - 88: pkts: 849, - 89: pkts: 849, - 90: pkts: 849, - 91: pkts: 849, - 92: pkts: 849, - 93: pkts: 849, - 94: pkts: 849, - 95: pkts: 849, - 96: pkts: 849, - 97: pkts: 849, - 98: pkts: 849, - 99: pkts: 849, - 100: pkts: 849, - 101: pkts: 849, - 102: pkts: 849, - 103: pkts: 849, - 104: pkts: 849, - 105: pkts: 849, - 106: pkts: 849, - 107: pkts: 849, - 108: pkts: 849, - 109: pkts: 849, - 110: pkts: 849, - 111: pkts: 849, - 112: pkts: 849, - 113: pkts: 849, - 114: pkts: 849, - 115: pkts: 849, - 116: pkts: 849, - 117: pkts: 849, - 118: pkts: 849, - 119: pkts: 849, + 86: pkts: 833, + 87: pkts: 848, + 88: pkts: 862, + 89: pkts: 875, + 90: pkts: 875, + 91: pkts: 888, + 92: pkts: 888, + 93: pkts: 888, + 94: pkts: 888, + 95: pkts: 888, + 96: pkts: 888, + 97: pkts: 888, + 98: pkts: 888, + 99: pkts: 888, + 100: pkts: 888, + 101: pkts: 888, + 102: pkts: 888, + 103: pkts: 888, + 104: pkts: 888, + 105: pkts: 888, + 106: pkts: 888, + 107: pkts: 888, + 108: pkts: 888, + 109: pkts: 888, + 110: pkts: 888, + 111: pkts: 888, + 112: pkts: 888, + 113: pkts: 888, + 114: pkts: 888, + 115: pkts: 888, + 116: pkts: 888, + 117: pkts: 888, + 118: pkts: 888, + 119: pkts: 888, ], } diff --git a/quic/s2n-quic-core/tests/recovery/snapshots/recovery_simulation__Lossat3MB-CubicCongestionController.snap b/quic/s2n-quic-core/tests/recovery/snapshots/recovery_simulation__Lossat3MB-CubicCongestionController.snap index bb766139a6..031d3ec4d2 100644 --- a/quic/s2n-quic-core/tests/recovery/snapshots/recovery_simulation__Lossat3MB-CubicCongestionController.snap +++ b/quic/s2n-quic-core/tests/recovery/snapshots/recovery_simulation__Lossat3MB-CubicCongestionController.snap @@ -1,8 +1,6 @@ --- source: quic/s2n-quic-core/tests/recovery/simulation.rs -assertion_line: 148 expression: self - --- Simulation { name: "Loss at 3MB", @@ -19,7 +17,7 @@ Simulation { 7: pkts: 1280, 8: pkts: 2560, 9: pkts: 1792, - 10: pkts: 1814, + 10: pkts: 1815, 11: pkts: 1845, 12: pkts: 1878, 13: pkts: 1911, @@ -34,7 +32,7 @@ Simulation { 22: pkts: 2165, 23: pkts: 2189, 24: pkts: 2211, - 25: pkts: 2233, + 25: pkts: 2232, 26: pkts: 2253, 27: pkts: 2273, 28: pkts: 2291, @@ -118,7 +116,7 @@ Simulation { 106: pkts: 2689, 107: pkts: 2700, 108: pkts: 2713, - 109: pkts: 2726, + 109: pkts: 2725, 110: pkts: 2739, 111: pkts: 2754, 112: pkts: 2769, diff --git a/quic/s2n-quic-core/tests/recovery/snapshots/recovery_simulation__Lossat3MBand2_75MB-CubicCongestionController.snap b/quic/s2n-quic-core/tests/recovery/snapshots/recovery_simulation__Lossat3MBand2_75MB-CubicCongestionController.snap index 957dd357cd..cfa3c550ac 100644 --- a/quic/s2n-quic-core/tests/recovery/snapshots/recovery_simulation__Lossat3MBand2_75MB-CubicCongestionController.snap +++ b/quic/s2n-quic-core/tests/recovery/snapshots/recovery_simulation__Lossat3MBand2_75MB-CubicCongestionController.snap @@ -1,8 +1,6 @@ --- source: quic/s2n-quic-core/tests/recovery/simulation.rs -assertion_line: 148 expression: self - --- Simulation { name: "Loss at 3MB and 2.75MB", @@ -19,7 +17,7 @@ Simulation { 7: pkts: 1280, 8: pkts: 2560, 9: pkts: 1792, - 10: pkts: 1814, + 10: pkts: 1815, 11: pkts: 1845, 12: pkts: 1878, 13: pkts: 1911, @@ -34,7 +32,7 @@ Simulation { 22: pkts: 2165, 23: pkts: 2189, 24: pkts: 2211, - 25: pkts: 2233, + 25: pkts: 2232, 26: pkts: 2253, 27: pkts: 2273, 28: pkts: 2291, @@ -50,7 +48,7 @@ Simulation { 38: pkts: 1756, 39: pkts: 1770, 40: pkts: 1784, - 41: pkts: 1797, + 41: pkts: 1796, 42: pkts: 1809, 43: pkts: 1820, 44: pkts: 1831, diff --git a/quic/s2n-quic-transport/src/path/mod.rs b/quic/s2n-quic-transport/src/path/mod.rs index 0d56d57796..93ffef38d2 100644 --- a/quic/s2n-quic-transport/src/path/mod.rs +++ b/quic/s2n-quic-transport/src/path/mod.rs @@ -538,6 +538,20 @@ impl Path { self.mtu_controller.max_mtu() } + /// Returns `true` if the congestion window does not have sufficient space for a packet of + /// size `mtu` considering the current bytes in flight and the additional `bytes_sent` provided + #[inline] + pub fn is_congestion_limited(&self, bytes_sent: usize) -> bool { + let cwnd = self.congestion_controller.congestion_window(); + let bytes_in_flight = self + .congestion_controller + .bytes_in_flight() + .saturating_add(bytes_sent as u32); + let mtu = self.mtu(transmission::Mode::Normal) as u32; + + cwnd.saturating_sub(bytes_in_flight) < mtu + } + // Compare a Path based on its PathHandle. // // Currently the local_address on the Client connection is unknown and set to @@ -1131,6 +1145,7 @@ mod tests { let packet_info = path.congestion_controller.on_packet_sent( now, path.congestion_controller.congestion_window() as usize, + None, &path.rtt_estimator, ); @@ -1202,4 +1217,19 @@ mod tests { } } } + + #[test] + fn is_congestion_limited() { + let mut path = testing::helper_path_client(); + let mtu = path.mtu_controller.mtu() as u32; + + path.congestion_controller.congestion_window = 12000; + path.congestion_controller.bytes_in_flight = 12000 - 500 - mtu; + + // There is room for an MTU sized packet after including the 500 bytes, so the path is not congestion limited + assert!(!path.is_congestion_limited(500)); + + // There isn't room for an MTU sized packet after including the 501 bytes, so the path is congestion limited + assert!(path.is_congestion_limited(501)); + } } diff --git a/quic/s2n-quic-transport/src/recovery/manager.rs b/quic/s2n-quic-transport/src/recovery/manager.rs index 5048b64d32..501e8789ca 100644 --- a/quic/s2n-quic-transport/src/recovery/manager.rs +++ b/quic/s2n-quic-transport/src/recovery/manager.rs @@ -188,12 +188,14 @@ impl Manager { //= https://www.rfc-editor.org/rfc/rfc9002#section-A.5 //# After a packet is sent, information about the packet is stored. + #[allow(clippy::too_many_arguments)] pub fn on_packet_sent, Pub: event::ConnectionPublisher>( &mut self, packet_number: PacketNumber, outcome: transmission::Outcome, time_sent: Timestamp, ecn: ExplicitCongestionNotification, + app_limited: Option, context: &mut Ctx, publisher: &mut Pub, ) { @@ -217,6 +219,7 @@ impl Manager { let cc_packet_info = path.congestion_controller.on_packet_sent( time_sent, congestion_controlled_bytes, + app_limited, &path.rtt_estimator, ); diff --git a/quic/s2n-quic-transport/src/recovery/manager/tests.rs b/quic/s2n-quic-transport/src/recovery/manager/tests.rs index ba001790fa..150dc18d8b 100644 --- a/quic/s2n-quic-transport/src/recovery/manager/tests.rs +++ b/quic/s2n-quic-transport/src/recovery/manager/tests.rs @@ -103,6 +103,7 @@ fn on_packet_sent() { } else { AckElicitation::NonEliciting }; + let app_limited = if i % 2 == 0 { Some(true) } else { Some(false) }; let outcome = transmission::Outcome { ack_elicitation, @@ -116,6 +117,7 @@ fn on_packet_sent() { outcome, time_sent, ecn, + app_limited, &mut context, &mut publisher, ); @@ -128,6 +130,10 @@ fn on_packet_sent() { ); assert_eq!(actual_sent_packet.time_sent, time_sent); assert_eq!(actual_sent_packet.ecn, ecn); + assert_eq!( + app_limited, + context.path().congestion_controller.app_limited + ); if outcome.is_congestion_controlled { assert_eq!(actual_sent_packet.sent_bytes as usize, outcome.bytes_sent); @@ -247,6 +253,7 @@ fn on_packet_sent_across_multiple_paths() { outcome, time_sent, ecn, + None, &mut context, &mut publisher, ); @@ -292,6 +299,7 @@ fn on_packet_sent_across_multiple_paths() { outcome, time_sent, ecn, + None, &mut context, &mut publisher, ); @@ -346,6 +354,7 @@ fn on_ack_frame() { }, time_sent, ecn, + None, &mut context, &mut publisher, ); @@ -484,6 +493,7 @@ fn on_ack_frame() { }, time_sent, ecn, + None, &mut context, &mut publisher, ); @@ -553,6 +563,7 @@ fn process_new_acked_packets_update_pto_timer() { }, time_sent, ecn, + None, &mut context, &mut publisher, ); @@ -568,6 +579,7 @@ fn process_new_acked_packets_update_pto_timer() { }, time_sent, ecn, + None, &mut context, &mut publisher, ); @@ -666,6 +678,7 @@ fn process_new_acked_packets_congestion_controller() { }, time_sent, ecn, + None, &mut context, &mut publisher, ); @@ -681,6 +694,7 @@ fn process_new_acked_packets_congestion_controller() { }, time_sent, ecn, + None, &mut context, &mut publisher, ); @@ -790,6 +804,7 @@ fn process_new_acked_packets_pto_timer() { }, time_sent, ecn, + None, &mut context, &mut publisher, ); @@ -805,6 +820,7 @@ fn process_new_acked_packets_pto_timer() { }, time_sent, ecn, + None, &mut context, &mut publisher, ); @@ -840,6 +856,7 @@ fn process_new_acked_packets_pto_timer() { }, time_sent, ecn, + None, &mut context, &mut publisher, ); @@ -902,6 +919,7 @@ fn process_new_acked_packets_process_ecn() { }, time_sent, ExplicitCongestionNotification::Ect0, + None, &mut context, &mut publisher, ); @@ -1002,6 +1020,7 @@ fn process_new_acked_packets_failed_ecn_validation_does_not_cause_congestion_eve }, time_sent, ExplicitCongestionNotification::Ect0, + None, &mut context, &mut publisher, ); @@ -1058,6 +1077,7 @@ fn no_rtt_update_when_not_acknowledging_the_largest_acknowledged_packet() { }, time_sent, ecn, + None, &mut context, &mut publisher, ); @@ -1071,6 +1091,7 @@ fn no_rtt_update_when_not_acknowledging_the_largest_acknowledged_packet() { }, time_sent, ecn, + None, &mut context, &mut publisher, ); @@ -1147,6 +1168,7 @@ fn no_rtt_update_when_receiving_packet_on_different_path() { }, time_sent, ecn, + None, &mut context, &mut publisher, ); @@ -1160,6 +1182,7 @@ fn no_rtt_update_when_receiving_packet_on_different_path() { }, time_sent, ecn, + None, &mut context, &mut publisher, ); @@ -1261,6 +1284,7 @@ fn rtt_update_when_receiving_ack_from_multiple_paths() { }, sent_time, ecn, + None, &mut context, &mut publisher, ); @@ -1277,6 +1301,7 @@ fn rtt_update_when_receiving_ack_from_multiple_paths() { }, sent_time, ecn, + None, &mut context, &mut publisher, ); @@ -1350,6 +1375,7 @@ fn detect_and_remove_lost_packets() { outcome, time_sent, ecn, + None, &mut context, &mut publisher, ); @@ -1380,6 +1406,7 @@ fn detect_and_remove_lost_packets() { outcome, time_sent, ecn, + None, &mut context, &mut publisher, ); @@ -1391,6 +1418,7 @@ fn detect_and_remove_lost_packets() { outcome, time_sent, ecn, + None, &mut context, &mut publisher, ); @@ -1402,6 +1430,7 @@ fn detect_and_remove_lost_packets() { outcome, time_sent, ecn, + None, &mut context, &mut publisher, ); @@ -1525,6 +1554,7 @@ fn detect_lost_packets_persistent_congestion_path_aware() { outcome, now, ecn, + None, &mut context, &mut publisher, ); @@ -1538,6 +1568,7 @@ fn detect_lost_packets_persistent_congestion_path_aware() { outcome, now, ecn, + None, &mut context, &mut publisher, ); @@ -1551,6 +1582,7 @@ fn detect_lost_packets_persistent_congestion_path_aware() { outcome, now, ecn, + None, &mut context, &mut publisher, ); @@ -1747,6 +1779,7 @@ fn detect_and_remove_lost_packets_nothing_lost() { outcome, time_sent, ecn, + None, &mut context, &mut publisher, ); @@ -1798,6 +1831,7 @@ fn detect_and_remove_lost_packets_mtu_probe() { outcome, time_sent, ecn, + None, &mut context, &mut publisher, ); @@ -1856,6 +1890,7 @@ fn persistent_congestion() { outcome, time_zero, ecn, + None, &mut context, &mut publisher, ); @@ -1866,6 +1901,7 @@ fn persistent_congestion() { outcome, time_zero + Duration::from_secs(1), ecn, + None, &mut context, &mut publisher, ); @@ -1890,6 +1926,7 @@ fn persistent_congestion() { outcome, time_zero + Duration::from_secs(t.into()), ecn, + None, &mut context, &mut publisher, ); @@ -1902,6 +1939,7 @@ fn persistent_congestion() { outcome, time_zero + Duration::from_secs(8), ecn, + None, &mut context, &mut publisher, ); @@ -1912,6 +1950,7 @@ fn persistent_congestion() { outcome, time_zero + Duration::from_secs(12), ecn, + None, &mut context, &mut publisher, ); @@ -1954,6 +1993,7 @@ fn persistent_congestion() { outcome, time_zero + Duration::from_secs(20), ecn, + None, &mut context, &mut publisher, ); @@ -2007,6 +2047,7 @@ fn persistent_congestion_multiple_periods() { outcome, time_zero, ecn, + None, &mut context, &mut publisher, ); @@ -2017,6 +2058,7 @@ fn persistent_congestion_multiple_periods() { outcome, time_zero + Duration::from_secs(1), ecn, + None, &mut context, &mut publisher, ); @@ -2038,6 +2080,7 @@ fn persistent_congestion_multiple_periods() { outcome, time_zero + Duration::from_secs(t.into()), ecn, + None, &mut context, &mut publisher, ); @@ -2051,6 +2094,7 @@ fn persistent_congestion_multiple_periods() { outcome, time_zero + Duration::from_secs(8), ecn, + None, &mut context, &mut publisher, ); @@ -2061,6 +2105,7 @@ fn persistent_congestion_multiple_periods() { outcome, time_zero + Duration::from_secs(20), ecn, + None, &mut context, &mut publisher, ); @@ -2071,6 +2116,7 @@ fn persistent_congestion_multiple_periods() { outcome, time_zero + Duration::from_secs(30), ecn, + None, &mut context, &mut publisher, ); @@ -2130,6 +2176,7 @@ fn persistent_congestion_period_does_not_start_until_rtt_sample() { outcome, time_zero, ecn, + None, &mut context, &mut publisher, ); @@ -2140,6 +2187,7 @@ fn persistent_congestion_period_does_not_start_until_rtt_sample() { outcome, time_zero + Duration::from_secs(10), ecn, + None, &mut context, &mut publisher, ); @@ -2150,6 +2198,7 @@ fn persistent_congestion_period_does_not_start_until_rtt_sample() { outcome, time_zero + Duration::from_secs(20), ecn, + None, &mut context, &mut publisher, ); @@ -2210,6 +2259,7 @@ fn persistent_congestion_not_ack_eliciting() { outcome, time_zero, ecn, + None, &mut context, &mut publisher, ); @@ -2223,6 +2273,7 @@ fn persistent_congestion_not_ack_eliciting() { outcome, time_zero + Duration::from_secs(10), ecn, + None, &mut context, &mut publisher, ); @@ -2233,6 +2284,7 @@ fn persistent_congestion_not_ack_eliciting() { outcome, time_zero + Duration::from_secs(20), ecn, + None, &mut context, &mut publisher, ); @@ -2349,6 +2401,7 @@ fn update_pto_timer() { }, now, ecn, + None, &mut context, &mut publisher, ); @@ -2486,6 +2539,7 @@ fn on_timeout() { }, now - Duration::from_secs(5), ecn, + None, &mut context, &mut publisher, ); @@ -2792,6 +2846,7 @@ fn probe_packets_count_towards_bytes_in_flight() { outcome, s2n_quic_platform::time::now(), ecn, + None, &mut context, &mut publisher, ); @@ -2883,6 +2938,7 @@ fn packet_declared_lost_less_than_1_ms_from_loss_threshold() { outcome, sent_time, ecn, + None, &mut context, &mut publisher, ); diff --git a/quic/s2n-quic-transport/src/space/application.rs b/quic/s2n-quic-transport/src/space/application.rs index a5b53c5acd..4777d2528b 100644 --- a/quic/s2n-quic-transport/src/space/application.rs +++ b/quic/s2n-quic-transport/src/space/application.rs @@ -12,6 +12,7 @@ use crate::{ stream::AbstractStreamManager, sync::flag, transmission, + transmission::interest::Provider, }; use core::{convert::TryInto, fmt, marker::PhantomData}; use once_cell::sync::OnceCell; @@ -215,6 +216,8 @@ impl ApplicationSpace { outcome.bytes_progressed += (self.stream_manager.outgoing_bytes_progressed() - bytes_progressed).as_u64() as usize; + let app_limited = self.is_app_limited(context.path(), outcome.bytes_sent); + let (recovery_manager, mut recovery_context, _) = self.recovery( handshake_status, context.local_id_registry, @@ -226,6 +229,7 @@ impl ApplicationSpace { outcome, context.timestamp, context.ecn, + Some(app_limited), &mut recovery_context, context.publisher, ); @@ -440,6 +444,14 @@ impl ApplicationSpace { ) } + /// Returns `true` if sending is limited by the application and not the congestion controller + /// + /// Sending is app limited if the application is not fully utilizing the available + /// congestion window currently and there is no more application data remaining to send. + fn is_app_limited(&self, path: &Path, bytes_sent: usize) -> bool { + !path.is_congestion_limited(bytes_sent) && !self.has_transmission_interest() + } + /// Validate packets in the Application packet space pub fn validate_and_decrypt_packet<'a, Pub: event::ConnectionPublisher>( &mut self, diff --git a/quic/s2n-quic-transport/src/space/handshake.rs b/quic/s2n-quic-transport/src/space/handshake.rs index bbafd23bfe..a0461940aa 100644 --- a/quic/s2n-quic-transport/src/space/handshake.rs +++ b/quic/s2n-quic-transport/src/space/handshake.rs @@ -166,6 +166,7 @@ impl HandshakeSpace { outcome, time_sent, context.ecn, + None, &mut recovery_context, context.publisher, ); diff --git a/quic/s2n-quic-transport/src/space/initial.rs b/quic/s2n-quic-transport/src/space/initial.rs index 717dc0f152..94c8e281d8 100644 --- a/quic/s2n-quic-transport/src/space/initial.rs +++ b/quic/s2n-quic-transport/src/space/initial.rs @@ -212,6 +212,7 @@ impl InitialSpace { outcome, time_sent, context.ecn, + None, &mut recovery_context, context.publisher, );