From f29fdfec9332d4c35afec219ac91da309198beb3 Mon Sep 17 00:00:00 2001 From: Kershaw Chang Date: Tue, 23 Jan 2024 13:24:03 +0100 Subject: [PATCH 1/9] Log RttEstimate --- neqo-transport/src/ackrate.rs | 2 +- neqo-transport/src/cc/classic_cc.rs | 22 +++++++++++++--------- neqo-transport/src/cc/mod.rs | 4 ++-- neqo-transport/src/cc/tests/cubic.rs | 4 +++- neqo-transport/src/cc/tests/new_reno.rs | 21 ++++++++++++++------- neqo-transport/src/path.rs | 3 +-- neqo-transport/src/rtt.rs | 12 ++++++++++++ neqo-transport/src/sender.rs | 10 ++++++++-- 8 files changed, 54 insertions(+), 24 deletions(-) diff --git a/neqo-transport/src/ackrate.rs b/neqo-transport/src/ackrate.rs index 6c4ae44f86..b5b207baa8 100644 --- a/neqo-transport/src/ackrate.rs +++ b/neqo-transport/src/ackrate.rs @@ -149,7 +149,7 @@ pub enum PeerAckDelay { } impl PeerAckDelay { - pub fn fixed(max_ack_delay: Duration) -> Self { + pub const fn fixed(max_ack_delay: Duration) -> Self { Self::Fixed(max_ack_delay) } diff --git a/neqo-transport/src/cc/classic_cc.rs b/neqo-transport/src/cc/classic_cc.rs index 000d9bf4d5..99b2dec290 100644 --- a/neqo-transport/src/cc/classic_cc.rs +++ b/neqo-transport/src/cc/classic_cc.rs @@ -19,6 +19,7 @@ use crate::{ cc::MAX_DATAGRAM_SIZE, packet::PacketNumber, qlog::{self, QlogMetric}, + rtt::RttEstimate, sender::PACING_BURST_SIZE, tracking::SentPacket, }; @@ -161,17 +162,18 @@ impl CongestionControl for ClassicCongestionControl { } // Multi-packet version of OnPacketAckedCC - fn on_packets_acked(&mut self, acked_pkts: &[SentPacket], min_rtt: Duration, now: Instant) { + fn on_packets_acked(&mut self, acked_pkts: &[SentPacket], rtts: &RttEstimate, now: Instant) { let mut is_app_limited = true; let mut new_acked = 0; for pkt in acked_pkts { qinfo!( - "packet_acked this={:p}, pn={}, ps={}, ignored={}, lost={}", + "packet_acked this={:p}, pn={}, ps={}, ignored={}, lost={}, rtts={:?}", self, pkt.pn, pkt.size, i32::from(!pkt.cc_outstanding()), - i32::from(pkt.lost()) + i32::from(pkt.lost()), + rtts, ); if !pkt.cc_outstanding() { continue; @@ -222,7 +224,7 @@ impl CongestionControl for ClassicCongestionControl { let bytes_for_increase = self.cc_algorithm.bytes_for_cwnd_increase( self.congestion_window, new_acked, - min_rtt, + rtts.minimum(), now, ); debug_assert!(bytes_for_increase > 0); @@ -546,6 +548,7 @@ mod tests { CongestionControl, CongestionControlAlgorithm, CWND_INITIAL_PKTS, MAX_DATAGRAM_SIZE, }, packet::{PacketNumber, PacketType}, + rtt::RttEstimate, tracking::SentPacket, }; use neqo_common::qinfo; @@ -557,6 +560,7 @@ mod tests { const PTO: Duration = Duration::from_millis(100); const RTT: Duration = Duration::from_millis(98); + const RTT_ESTIMATE: RttEstimate = RttEstimate::from_duration(Duration::from_millis(98)); const ZERO: Duration = Duration::from_secs(0); const EPSILON: Duration = Duration::from_nanos(1); const GAP: Duration = Duration::from_secs(1); @@ -1025,7 +1029,7 @@ mod tests { } assert_eq!(cc.bytes_in_flight(), packet_burst_size * MAX_DATAGRAM_SIZE); now += RTT; - cc.on_packets_acked(&pkts, RTT, now); + cc.on_packets_acked(&pkts, &RTT_ESTIMATE, now); assert_eq!(cc.bytes_in_flight(), 0); assert_eq!(cc.acked_bytes, 0); assert_eq!(cwnd, cc.congestion_window); // CWND doesn't grow because we're app limited @@ -1054,7 +1058,7 @@ mod tests { now += RTT; // Check if congestion window gets increased for all packets currently in flight for (i, pkt) in pkts.into_iter().enumerate() { - cc.on_packets_acked(&[pkt], RTT, now); + cc.on_packets_acked(&[pkt], &RTT_ESTIMATE, now); assert_eq!( cc.bytes_in_flight(), @@ -1101,7 +1105,7 @@ mod tests { ); cc.on_packet_sent(&p_not_lost); now += RTT; - cc.on_packets_acked(&[p_not_lost], RTT, now); + cc.on_packets_acked(&[p_not_lost], &RTT_ESTIMATE, now); cwnd_is_halved(&cc); // cc is app limited therefore cwnd in not increased. assert_eq!(cc.acked_bytes, 0); @@ -1129,7 +1133,7 @@ mod tests { assert_eq!(cc.bytes_in_flight(), packet_burst_size * MAX_DATAGRAM_SIZE); now += RTT; for (i, pkt) in pkts.into_iter().enumerate() { - cc.on_packets_acked(&[pkt], RTT, now); + cc.on_packets_acked(&[pkt], &RTT_ESTIMATE, now); assert_eq!( cc.bytes_in_flight(), @@ -1164,7 +1168,7 @@ mod tests { let mut last_acked_bytes = 0; // Check if congestion window gets increased for all packets currently in flight for (i, pkt) in pkts.into_iter().enumerate() { - cc.on_packets_acked(&[pkt], RTT, now); + cc.on_packets_acked(&[pkt], &RTT_ESTIMATE, now); assert_eq!( cc.bytes_in_flight(), diff --git a/neqo-transport/src/cc/mod.rs b/neqo-transport/src/cc/mod.rs index 675168367a..dcbce5e44e 100644 --- a/neqo-transport/src/cc/mod.rs +++ b/neqo-transport/src/cc/mod.rs @@ -7,7 +7,7 @@ // Congestion control #![deny(clippy::pedantic)] -use crate::{path::PATH_MTU_V6, tracking::SentPacket, Error}; +use crate::{path::PATH_MTU_V6, rtt::RttEstimate, tracking::SentPacket, Error}; use neqo_common::qlog::NeqoQlog; use std::{ @@ -42,7 +42,7 @@ pub trait CongestionControl: Display + Debug { #[must_use] fn cwnd_avail(&self) -> usize; - fn on_packets_acked(&mut self, acked_pkts: &[SentPacket], min_rtt: Duration, now: Instant); + fn on_packets_acked(&mut self, acked_pkts: &[SentPacket], rtts: &RttEstimate, now: Instant); /// Returns true if the congestion window was reduced. fn on_packets_lost( diff --git a/neqo-transport/src/cc/tests/cubic.rs b/neqo-transport/src/cc/tests/cubic.rs index 1229e6307f..b24f1fc118 100644 --- a/neqo-transport/src/cc/tests/cubic.rs +++ b/neqo-transport/src/cc/tests/cubic.rs @@ -17,6 +17,7 @@ use crate::{ CongestionControl, MAX_DATAGRAM_SIZE, MAX_DATAGRAM_SIZE_F64, }, packet::PacketType, + rtt::RttEstimate, tracking::SentPacket, }; use std::{ @@ -27,6 +28,7 @@ use std::{ use test_fixture::now; const RTT: Duration = Duration::from_millis(100); +const RTT_ESTIMATE: RttEstimate = RttEstimate::from_duration(Duration::from_millis(100)); const CWND_INITIAL_F64: f64 = 10.0 * MAX_DATAGRAM_SIZE_F64; const CWND_INITIAL_10_F64: f64 = 10.0 * CWND_INITIAL_F64; const CWND_INITIAL_10: usize = 10 * CWND_INITIAL; @@ -59,7 +61,7 @@ fn ack_packet(cc: &mut ClassicCongestionControl, pn: u64, now: Instant) { Vec::new(), // tokens MAX_DATAGRAM_SIZE, // size ); - cc.on_packets_acked(&[acked], RTT, now); + cc.on_packets_acked(&[acked], &RTT_ESTIMATE, now); } fn packet_lost(cc: &mut ClassicCongestionControl, pn: u64) { diff --git a/neqo-transport/src/cc/tests/new_reno.rs b/neqo-transport/src/cc/tests/new_reno.rs index 0e4322c08c..f86e87b953 100644 --- a/neqo-transport/src/cc/tests/new_reno.rs +++ b/neqo-transport/src/cc/tests/new_reno.rs @@ -7,15 +7,22 @@ // Congestion control #![deny(clippy::pedantic)] -use crate::cc::new_reno::NewReno; -use crate::cc::{ClassicCongestionControl, CongestionControl, CWND_INITIAL, MAX_DATAGRAM_SIZE}; -use crate::packet::PacketType; -use crate::tracking::SentPacket; +use crate::{ + cc::{ + new_reno::NewReno, ClassicCongestionControl, CongestionControl, CWND_INITIAL, + MAX_DATAGRAM_SIZE, + }, + packet::PacketType, + rtt::RttEstimate, + tracking::SentPacket, +}; + use std::time::Duration; use test_fixture::now; const PTO: Duration = Duration::from_millis(100); const RTT: Duration = Duration::from_millis(98); +const RTT_ESTIMATE: RttEstimate = RttEstimate::from_duration(Duration::from_millis(98)); fn cwnd_is_default(cc: &ClassicCongestionControl) { assert_eq!(cc.cwnd(), CWND_INITIAL); @@ -117,7 +124,7 @@ fn issue_876() { assert_eq!(cc.bytes_in_flight(), 6 * MAX_DATAGRAM_SIZE - 5); // and ack it. cwnd increases slightly - cc.on_packets_acked(&sent_packets[6..], RTT, time_now); + cc.on_packets_acked(&sent_packets[6..], &RTT_ESTIMATE, time_now); assert_eq!(cc.acked_bytes(), sent_packets[6].size); cwnd_is_halved(&cc); assert_eq!(cc.bytes_in_flight(), 5 * MAX_DATAGRAM_SIZE - 2); @@ -181,7 +188,7 @@ fn issue_1465() { // the acked packets before on_packet_sent were the cause of // https://github.com/mozilla/neqo/pull/1465 - cc.on_packets_acked(&[p2], RTT, now); + cc.on_packets_acked(&[p2], &RTT_ESTIMATE, now); assert_eq!(cc.bytes_in_flight(), 0); @@ -189,7 +196,7 @@ fn issue_1465() { let p4 = send_next(&mut cc, now); cc.on_packet_sent(&p4); now += RTT; - cc.on_packets_acked(&[p4], RTT, now); + cc.on_packets_acked(&[p4], &RTT_ESTIMATE, now); // do the same as in the first rtt but now the bug appears let p5 = send_next(&mut cc, now); diff --git a/neqo-transport/src/path.rs b/neqo-transport/src/path.rs index 40014c73a1..0ec04b86bd 100644 --- a/neqo-transport/src/path.rs +++ b/neqo-transport/src/path.rs @@ -967,8 +967,7 @@ impl Path { /// Record packets as acknowledged with the sender. pub fn on_packets_acked(&mut self, acked_pkts: &[SentPacket], now: Instant) { debug_assert!(self.is_primary()); - self.sender - .on_packets_acked(acked_pkts, self.rtt.minimum(), now); + self.sender.on_packets_acked(acked_pkts, &self.rtt, now); } /// Record packets as lost with the sender. diff --git a/neqo-transport/src/rtt.rs b/neqo-transport/src/rtt.rs index 3d6d0e70f8..a5ceb37da2 100644 --- a/neqo-transport/src/rtt.rs +++ b/neqo-transport/src/rtt.rs @@ -47,6 +47,18 @@ impl RttEstimate { self.rttvar = rtt / 2; } + #[cfg(test)] + pub const fn from_duration(rtt: Duration) -> Self { + Self { + first_sample_time: None, + latest_rtt: rtt, + smoothed_rtt: rtt, + rttvar: Duration::from_millis(0), + min_rtt: rtt, + ack_delay: PeerAckDelay::Fixed(Duration::from_millis(25)), + } + } + pub fn set_initial(&mut self, rtt: Duration) { qtrace!("initial RTT={:?}", rtt); if rtt >= GRANULARITY { diff --git a/neqo-transport/src/sender.rs b/neqo-transport/src/sender.rs index 3d8302369c..10fa6e6629 100644 --- a/neqo-transport/src/sender.rs +++ b/neqo-transport/src/sender.rs @@ -12,6 +12,7 @@ use crate::cc::{ ClassicCongestionControl, CongestionControl, CongestionControlAlgorithm, Cubic, NewReno, }; use crate::pace::Pacer; +use crate::rtt::RttEstimate; use crate::tracking::SentPacket; use neqo_common::qlog::NeqoQlog; @@ -68,8 +69,13 @@ impl PacketSender { self.cc.cwnd_avail() } - pub fn on_packets_acked(&mut self, acked_pkts: &[SentPacket], min_rtt: Duration, now: Instant) { - self.cc.on_packets_acked(acked_pkts, min_rtt, now); + pub fn on_packets_acked( + &mut self, + acked_pkts: &[SentPacket], + rtts: &RttEstimate, + now: Instant, + ) { + self.cc.on_packets_acked(acked_pkts, rtts, now); } /// Called when packets are lost. Returns true if the congestion window was reduced. From 0b91c44d6a34571012b0ff27ae52df08d64ee59b Mon Sep 17 00:00:00 2001 From: Kershaw Chang Date: Wed, 24 Jan 2024 13:49:36 +0100 Subject: [PATCH 2/9] address comment --- neqo-transport/src/ackrate.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/neqo-transport/src/ackrate.rs b/neqo-transport/src/ackrate.rs index b5b207baa8..6c4ae44f86 100644 --- a/neqo-transport/src/ackrate.rs +++ b/neqo-transport/src/ackrate.rs @@ -149,7 +149,7 @@ pub enum PeerAckDelay { } impl PeerAckDelay { - pub const fn fixed(max_ack_delay: Duration) -> Self { + pub fn fixed(max_ack_delay: Duration) -> Self { Self::Fixed(max_ack_delay) } From 72926e07d6c63a7cbd442fa3f6bb4cc100a45a7d Mon Sep 17 00:00:00 2001 From: Kershaw Date: Thu, 25 Jan 2024 09:50:24 +0100 Subject: [PATCH 3/9] Update neqo-transport/src/cc/classic_cc.rs Co-authored-by: Lars Eggert --- neqo-transport/src/cc/classic_cc.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/neqo-transport/src/cc/classic_cc.rs b/neqo-transport/src/cc/classic_cc.rs index 99b2dec290..9f6f9702d2 100644 --- a/neqo-transport/src/cc/classic_cc.rs +++ b/neqo-transport/src/cc/classic_cc.rs @@ -162,7 +162,7 @@ impl CongestionControl for ClassicCongestionControl { } // Multi-packet version of OnPacketAckedCC - fn on_packets_acked(&mut self, acked_pkts: &[SentPacket], rtts: &RttEstimate, now: Instant) { + fn on_packets_acked(&mut self, acked_pkts: &[SentPacket], rtt_est: &RttEstimate, now: Instant) { let mut is_app_limited = true; let mut new_acked = 0; for pkt in acked_pkts { From d1faf1b52f3af32335f80cad67a7ef3a59f16010 Mon Sep 17 00:00:00 2001 From: Kershaw Date: Thu, 25 Jan 2024 09:50:43 +0100 Subject: [PATCH 4/9] Update neqo-transport/src/cc/classic_cc.rs Co-authored-by: Lars Eggert --- neqo-transport/src/cc/classic_cc.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/neqo-transport/src/cc/classic_cc.rs b/neqo-transport/src/cc/classic_cc.rs index 9f6f9702d2..0a2e593568 100644 --- a/neqo-transport/src/cc/classic_cc.rs +++ b/neqo-transport/src/cc/classic_cc.rs @@ -167,7 +167,7 @@ impl CongestionControl for ClassicCongestionControl { let mut new_acked = 0; for pkt in acked_pkts { qinfo!( - "packet_acked this={:p}, pn={}, ps={}, ignored={}, lost={}, rtts={:?}", + "packet_acked this={:p}, pn={}, ps={}, ignored={}, lost={}, rtt_est={:?}", self, pkt.pn, pkt.size, From d7c27cd281ea34eee0b5424debbef69589acc05e Mon Sep 17 00:00:00 2001 From: Kershaw Date: Thu, 25 Jan 2024 09:50:53 +0100 Subject: [PATCH 5/9] Update neqo-transport/src/sender.rs Co-authored-by: Lars Eggert --- neqo-transport/src/sender.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/neqo-transport/src/sender.rs b/neqo-transport/src/sender.rs index 10fa6e6629..515d2e860b 100644 --- a/neqo-transport/src/sender.rs +++ b/neqo-transport/src/sender.rs @@ -75,7 +75,7 @@ impl PacketSender { rtts: &RttEstimate, now: Instant, ) { - self.cc.on_packets_acked(acked_pkts, rtts, now); + self.cc.on_packets_acked(acked_pkts, rtt_est, now); } /// Called when packets are lost. Returns true if the congestion window was reduced. From 51796766eaa2daefce2e90f644247d46e1da1387 Mon Sep 17 00:00:00 2001 From: Kershaw Date: Thu, 25 Jan 2024 09:51:01 +0100 Subject: [PATCH 6/9] Update neqo-transport/src/sender.rs Co-authored-by: Lars Eggert --- neqo-transport/src/sender.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/neqo-transport/src/sender.rs b/neqo-transport/src/sender.rs index 515d2e860b..0c1e66ff9a 100644 --- a/neqo-transport/src/sender.rs +++ b/neqo-transport/src/sender.rs @@ -72,7 +72,7 @@ impl PacketSender { pub fn on_packets_acked( &mut self, acked_pkts: &[SentPacket], - rtts: &RttEstimate, + rtt_est: &RttEstimate, now: Instant, ) { self.cc.on_packets_acked(acked_pkts, rtt_est, now); From 06c87425030a0b4e83cfd82dd153f7bd743a1550 Mon Sep 17 00:00:00 2001 From: Kershaw Date: Thu, 25 Jan 2024 09:51:13 +0100 Subject: [PATCH 7/9] Update neqo-transport/src/cc/mod.rs Co-authored-by: Lars Eggert --- neqo-transport/src/cc/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/neqo-transport/src/cc/mod.rs b/neqo-transport/src/cc/mod.rs index dcbce5e44e..0321ab1de5 100644 --- a/neqo-transport/src/cc/mod.rs +++ b/neqo-transport/src/cc/mod.rs @@ -42,7 +42,7 @@ pub trait CongestionControl: Display + Debug { #[must_use] fn cwnd_avail(&self) -> usize; - fn on_packets_acked(&mut self, acked_pkts: &[SentPacket], rtts: &RttEstimate, now: Instant); + fn on_packets_acked(&mut self, acked_pkts: &[SentPacket], rtt_est: &RttEstimate, now: Instant); /// Returns true if the congestion window was reduced. fn on_packets_lost( From 1971b05132a626672da0904617f2711fc9095940 Mon Sep 17 00:00:00 2001 From: Kershaw Date: Thu, 25 Jan 2024 09:51:23 +0100 Subject: [PATCH 8/9] Update neqo-transport/src/cc/classic_cc.rs Co-authored-by: Lars Eggert --- neqo-transport/src/cc/classic_cc.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/neqo-transport/src/cc/classic_cc.rs b/neqo-transport/src/cc/classic_cc.rs index 0a2e593568..cfe466e363 100644 --- a/neqo-transport/src/cc/classic_cc.rs +++ b/neqo-transport/src/cc/classic_cc.rs @@ -224,7 +224,7 @@ impl CongestionControl for ClassicCongestionControl { let bytes_for_increase = self.cc_algorithm.bytes_for_cwnd_increase( self.congestion_window, new_acked, - rtts.minimum(), + rtt_est.minimum(), now, ); debug_assert!(bytes_for_increase > 0); From 1cd1543178e965d27444a5ea912ed8280e0f2872 Mon Sep 17 00:00:00 2001 From: Kershaw Date: Thu, 25 Jan 2024 09:51:31 +0100 Subject: [PATCH 9/9] Update neqo-transport/src/cc/classic_cc.rs Co-authored-by: Lars Eggert --- neqo-transport/src/cc/classic_cc.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/neqo-transport/src/cc/classic_cc.rs b/neqo-transport/src/cc/classic_cc.rs index cfe466e363..c1d8fd08a6 100644 --- a/neqo-transport/src/cc/classic_cc.rs +++ b/neqo-transport/src/cc/classic_cc.rs @@ -173,7 +173,7 @@ impl CongestionControl for ClassicCongestionControl { pkt.size, i32::from(!pkt.cc_outstanding()), i32::from(pkt.lost()), - rtts, + rtt_est, ); if !pkt.cc_outstanding() { continue;