Skip to content

Commit

Permalink
Improve app_limit detection by keeping track on app_limited state in …
Browse files Browse the repository at this point in the history 
…on_packet_sent

Fixes mozilla#1475
  • Loading branch information
@mb
mb committed Oct 26, 2023
1 parent 6c05ac3 commit 82f05b1
Showing 1 changed file with 92 additions and 94 deletions.
186 changes: 92 additions & 94 deletions neqo-transport/src/cc/classic_cc.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ use std::time::{Duration, Instant};
use super::CongestionControl;

use crate::cc::MAX_DATAGRAM_SIZE;
use crate::packet::PacketNumber;
use crate::qlog::{self, QlogMetric};
use crate::sender::PACING_BURST_SIZE;
use crate::tracking::SentPacket;
Expand Down Expand Up @@ -110,6 +111,14 @@ pub struct ClassicCongestionControl<T> {
acked_bytes: usize,
ssthresh: usize,
recovery_start: Option<Instant>,
/// `first_app_limited` indicates the packet number after which the application might be
/// underutilizing the congestion window. When underutilizing the congestion window due to not
/// sending out enough data, we SHOULD NOT increase the congestion window.[1] Packets sent
/// before this point are deemed to fully utilize the congestion window and count towards
/// increasing the congestion window.
///
/// [1]: https://datatracker.ietf.org/doc/html/rfc9002#section-7.8
first_app_limited: PacketNumber,

qlog: NeqoQlog,
}
Expand Down Expand Up @@ -149,21 +158,12 @@ impl<T: WindowAdjustment> CongestionControl for ClassicCongestionControl<T> {

// Multi-packet version of OnPacketAckedCC
fn on_packets_acked(&mut self, acked_pkts: &[SentPacket], min_rtt: Duration, now: Instant) {
// Check whether we are app limited before acked packets are removed
// from bytes_in_flight.
let is_app_limited = self.app_limited();
qtrace!(
[self],
"limited={}, bytes_in_flight={}, cwnd={}, state={:?} pacing_burst_size={}",
is_app_limited,
self.bytes_in_flight,
self.congestion_window,
self.state,
MAX_DATAGRAM_SIZE * PACING_BURST_SIZE,
);

let mut new_acked = 0;
let mut is_app_limited = true;
for pkt in acked_pkts.iter().filter(|pkt| pkt.cc_outstanding()) {
if pkt.pn < self.first_app_limited {
is_app_limited = false;
}
assert!(self.bytes_in_flight >= pkt.size);
self.bytes_in_flight -= pkt.size;

Expand All @@ -181,6 +181,16 @@ impl<T: WindowAdjustment> CongestionControl for ClassicCongestionControl<T> {
new_acked += pkt.size;
}

qtrace!(
[self],
"limited={}, bytes_in_flight={}, cwnd={}, state={:?} pacing_burst_size={}",
is_app_limited,
self.bytes_in_flight,
self.congestion_window,
self.state,
MAX_DATAGRAM_SIZE * PACING_BURST_SIZE,
);

if is_app_limited {
self.cc_algorithm.on_app_limited();
return;
Expand Down Expand Up @@ -300,6 +310,13 @@ impl<T: WindowAdjustment> CongestionControl for ClassicCongestionControl<T> {
if !pkt.cc_in_flight() {
return;
}
if !self.app_limited() {
// Given the current non-app-limited condition, we're fully utilizing the congestion
// window. We can safely consider that all in-flight packets up to the current to be not
// app-limiting. However, the subsequent packets might fall under app-limited
// condition. Hence, set the `first_app_limited` to the next packet number.
self.first_app_limited = pkt.pn + 1;
}

self.bytes_in_flight += pkt.size;
qdebug!(
Expand Down Expand Up @@ -332,6 +349,7 @@ impl<T: WindowAdjustment> ClassicCongestionControl<T> {
ssthresh: usize::MAX,
recovery_start: None,
qlog: NeqoQlog::disabled(),
first_app_limited: 0,
}
}

Expand Down Expand Up @@ -501,6 +519,7 @@ mod tests {
use super::{
ClassicCongestionControl, WindowAdjustment, CWND_INITIAL, CWND_MIN, PERSISTENT_CONG_THRESH,
};
use crate::cc::classic_cc::State;
use crate::cc::cubic::{Cubic, CUBIC_BETA_USIZE_DIVIDEND, CUBIC_BETA_USIZE_DIVISOR};
use crate::cc::new_reno::NewReno;
use crate::cc::{
Expand Down Expand Up @@ -956,63 +975,47 @@ mod tests {

#[test]
fn app_limited_slow_start() {
const LESS_THAN_CWND_PKTS: usize = 4;
const BELOW_APP_LIMIT_PKTS: usize = 4;
let mut cc = ClassicCongestionControl::new(NewReno::default());

for i in 0..CWND_INITIAL_PKTS {
let sent = SentPacket::new(
PacketType::Short,
u64::try_from(i).unwrap(), // pn
now(), // time sent
true, // ack eliciting
Vec::new(), // tokens
MAX_DATAGRAM_SIZE, // size
);
cc.on_packet_sent(&sent);
}
assert_eq!(cc.bytes_in_flight(), CWND_INITIAL);

for i in 0..LESS_THAN_CWND_PKTS {
let acked = SentPacket::new(
PacketType::Short,
u64::try_from(i).unwrap(), // pn
now(), // time sent
true, // ack eliciting
Vec::new(), // tokens
MAX_DATAGRAM_SIZE, // size
);
cc.on_packets_acked(&[acked], RTT, now());

assert_eq!(
cc.bytes_in_flight(),
(CWND_INITIAL_PKTS - i - 1) * MAX_DATAGRAM_SIZE
);
assert_eq!(cc.cwnd(), (CWND_INITIAL_PKTS + i + 1) * MAX_DATAGRAM_SIZE);
}

// Now we are app limited
for i in 4..CWND_INITIAL_PKTS {
let p = [SentPacket::new(
PacketType::Short,
u64::try_from(i).unwrap(), // pn
now(), // time sent
true, // ack eliciting
Vec::new(), // tokens
MAX_DATAGRAM_SIZE, // size
)];
cc.on_packets_acked(&p, RTT, now());

let cwnd = cc.congestion_window;

// simulate 10 packet bursts below app_limit
for k in 0..10 {
// always stay below app_limit during sent.
let mut pkts = Vec::new();
for i in 0..BELOW_APP_LIMIT_PKTS {
let p = SentPacket::new(
PacketType::Short,
u64::try_from(k * BELOW_APP_LIMIT_PKTS + i + 3).unwrap(), // pn
now() + (k as u32 + 2) * RTT, // time sent
true, // ack eliciting
Vec::new(), // tokens
MAX_DATAGRAM_SIZE, // size
);
cc.on_packet_sent(&p);
pkts.push(p);
}
assert_eq!(
cc.bytes_in_flight(),
(CWND_INITIAL_PKTS - i - 1) * MAX_DATAGRAM_SIZE
BELOW_APP_LIMIT_PKTS * MAX_DATAGRAM_SIZE
);
assert_eq!(cc.cwnd(), (CWND_INITIAL_PKTS + 4) * MAX_DATAGRAM_SIZE);
for (i, pkt) in pkts.into_iter().enumerate() {
cc.on_packets_acked(&[pkt], RTT, now() + (k as u32 + 3) * RTT);

assert_eq!(
cc.bytes_in_flight(),
(BELOW_APP_LIMIT_PKTS - i - 1) * MAX_DATAGRAM_SIZE
);
assert_eq!(cwnd, cc.congestion_window);
assert_eq!(cc.acked_bytes, 0);
}
}
}

#[test]
fn app_limited_congestion_avoidance() {
const CWND_PKTS_CA: usize = CWND_INITIAL_PKTS / 2;
const BELOW_APP_LIMIT_PKTS: usize = CWND_PKTS_CA - 2;

let mut cc = ClassicCongestionControl::new(NewReno::default());

Expand All @@ -1032,7 +1035,7 @@ mod tests {
cwnd_is_halved(&cc);
let p_not_lost = SentPacket::new(
PacketType::Short,
1, // pn
2, // pn
now(), // time sent
true, // ack eliciting
Vec::new(), // tokens
Expand All @@ -1045,42 +1048,37 @@ mod tests {
assert_eq!(cc.acked_bytes, 0);

// Now we are in the congestion avoidance state.
let mut pkts = Vec::new();
for i in 0..CWND_PKTS_CA {
let p = SentPacket::new(
PacketType::Short,
u64::try_from(i + 3).unwrap(), // pn
now(), // time sent
true, // ack eliciting
Vec::new(), // tokens
MAX_DATAGRAM_SIZE, // size
);
cc.on_packet_sent(&p);
pkts.push(p);
}
assert_eq!(cc.bytes_in_flight(), CWND_INITIAL / 2);

for i in 0..CWND_PKTS_CA - 2 {
cc.on_packets_acked(&pkts[i..=i], RTT, now());

assert_eq!(
cc.bytes_in_flight(),
(CWND_PKTS_CA - i - 1) * MAX_DATAGRAM_SIZE
);
assert_eq!(cc.cwnd(), CWND_PKTS_CA * MAX_DATAGRAM_SIZE);
assert_eq!(cc.acked_bytes, MAX_DATAGRAM_SIZE * (i + 1));
}

// Now we are app limited
for i in CWND_PKTS_CA - 2..CWND_PKTS_CA {
cc.on_packets_acked(&pkts[i..=i], RTT, now());

assert_eq!(cc.state, State::CongestionAvoidance);
// simulate 10 packet bursts below app_limit
for k in 0..10 {
// always stay below app_limit during sent.
let mut pkts = Vec::new();
for i in 0..BELOW_APP_LIMIT_PKTS {
let p = SentPacket::new(
PacketType::Short,
u64::try_from(k * BELOW_APP_LIMIT_PKTS + i + 3).unwrap(), // pn
now() + (k as u32 + 2) * RTT, // time sent
true, // ack eliciting
Vec::new(), // tokens
MAX_DATAGRAM_SIZE, // size
);
cc.on_packet_sent(&p);
pkts.push(p);
}
assert_eq!(
cc.bytes_in_flight(),
(CWND_PKTS_CA - i - 1) * MAX_DATAGRAM_SIZE
BELOW_APP_LIMIT_PKTS * MAX_DATAGRAM_SIZE
);
assert_eq!(cc.cwnd(), CWND_PKTS_CA * MAX_DATAGRAM_SIZE);
assert_eq!(cc.acked_bytes, MAX_DATAGRAM_SIZE * 3);
for (i, pkt) in pkts.into_iter().enumerate() {
cc.on_packets_acked(&[pkt], RTT, now() + (k as u32 + 3) * RTT);

assert_eq!(
cc.bytes_in_flight(),
(CWND_PKTS_CA - 2 - i - 1) * MAX_DATAGRAM_SIZE
);
cwnd_is_halved(&cc);
assert_eq!(cc.acked_bytes, 0);
}
}
}
}

0 comments on commit 82f05b1

Please sign in to comment.