Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Revert "fix: Path is not app limited if more data is available to transmit (#1326)" #1350

Merged
merged 1 commit into from
Jun 3, 2022
Merged

Revert "fix: Path is not app limited if more data is available to transmit (#1326)" #1350

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 2 additions & 19 deletions quic/s2n-quic-core/src/recovery/bandwidth/estimator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,9 +139,6 @@ 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,
}

Expand All @@ -161,7 +158,7 @@ impl Estimator {
pub fn on_packet_sent(
&mut self,
bytes_in_flight: u32,
app_limited: Option<bool>,
is_app_limited: bool,
now: Timestamp,
) -> PacketInfo {
//= https://tools.ietf.org/id/draft-cheng-iccrg-delivery-rate-estimation-02#3.2
Expand All @@ -174,10 +171,6 @@ 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
Expand All @@ -188,7 +181,7 @@ impl Estimator {
.first_sent_time
.expect("initialized on first sent packet"),
bytes_in_flight,
is_app_limited: self.app_limited_delivered_bytes.is_some(),
is_app_limited,
}
}

Expand All @@ -207,16 +200,6 @@ 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
Expand Down
68 changes: 9 additions & 59 deletions quic/s2n-quic-core/src/recovery/bandwidth/estimator/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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, None, t0);
let packet_info = bw_estimator.on_packet_sent(0, false, 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, None, t1);
let packet_info = bw_estimator.on_packet_sent(1500, false, 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, None, t2);
let packet_info = bw_estimator.on_packet_sent(0, false, 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);
Expand All @@ -116,66 +116,16 @@ 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, Some(true), first_sent_time);
let packet_info = bw_estimator.on_packet_sent(500, 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]
Expand All @@ -187,11 +137,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, Some(false), t0);
let packet_1 = bw_estimator.on_packet_sent(0, false, t0);
bw_estimator.delivered_bytes = 100000;
let packet_2 = bw_estimator.on_packet_sent(1500, Some(true), t1);
let packet_2 = bw_estimator.on_packet_sent(1500, true, t1);
bw_estimator.delivered_bytes = 200000;
let packet_3 = bw_estimator.on_packet_sent(3000, Some(false), t2);
let packet_3 = bw_estimator.on_packet_sent(3000, false, t2);

let now = t0 + Duration::from_secs(10);
let delivered_bytes = bw_estimator.delivered_bytes;
Expand Down Expand Up @@ -295,13 +245,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, Some(false), t0);
let packet_info = bw_estimator.on_packet_sent(0, 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, Some(false), t5);
let packet_info = bw_estimator.on_packet_sent(1500, false, t5);
let now = t0 + Duration::from_secs(6);
bw_estimator.on_ack(1500, t0 + Duration::from_secs(5), packet_info, now);

Expand Down
9 changes: 6 additions & 3 deletions quic/s2n-quic-core/src/recovery/bbr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,9 +100,13 @@ 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();

Expand All @@ -111,8 +115,7 @@ impl CongestionController for BbrCongestionController {
.expect("sent_bytes should not exceed u32::MAX");
}

self.bw_estimator
.on_packet_sent(*self.bytes_in_flight, app_limited, time_sent)
packet_info
}

fn on_rtt_update(&mut self, _time_sent: Timestamp, _rtt_estimator: &RttEstimator) {
Expand Down
9 changes: 0 additions & 9 deletions quic/s2n-quic-core/src/recovery/congestion_controller.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,17 +63,13 @@ 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;

Expand Down Expand Up @@ -186,7 +182,6 @@ pub mod testing {
&mut self,
_time_sent: Timestamp,
_bytes_sent: usize,
_app_limited: Option<bool>,
_rtt_estimator: &RttEstimator,
) {
}
Expand Down Expand Up @@ -257,7 +252,6 @@ 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 {
Expand All @@ -274,7 +268,6 @@ pub mod testing {
congestion_events: 0,
requires_fast_retransmission: false,
loss_bursts: 0,
app_limited: None,
}
}
}
Expand Down Expand Up @@ -302,12 +295,10 @@ 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) {
Expand Down
10 changes: 1 addition & 9 deletions quic/s2n-quic-core/src/recovery/cubic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,6 @@ impl CongestionController for CubicCongestionController {
&mut self,
time_sent: Timestamp,
bytes_sent: usize,
app_limited: Option<bool>,
rtt_estimator: &RttEstimator,
) {
if bytes_sent == 0 {
Expand All @@ -205,14 +204,7 @@ impl CongestionController for CubicCongestionController {
.try_add(bytes_sent)
.expect("bytes sent should not exceed u32::MAX");

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();
}
self.under_utilized = 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)
Expand Down
Loading