From a74e9cb52e647d4644d78ce3d8bc2407160a4a80 Mon Sep 17 00:00:00 2001 From: Wesley Rosenblum <55108558+WesleyRosenblum@users.noreply.github.com> Date: Wed, 28 Aug 2024 11:03:34 -0700 Subject: [PATCH] fix(s2n-quic-transport): don't let cwnd drop below initial cwnd when MTU decreases (#2308) --- quic/s2n-quic-core/src/recovery/bbr.rs | 7 ++- ..._decrease_larger_than_initial_window.snap} | 0 ..._decrease_smaller_than_initial_window.snap | 5 ++ ...tests__events__on_mtu_update_increase.snap | 5 ++ quic/s2n-quic-core/src/recovery/bbr/tests.rs | 38 ++++++++++++- quic/s2n-quic-core/src/recovery/cubic.rs | 8 ++- ...e_decrease_larger_than_initial_window.snap | 5 ++ ..._decrease_smaller_than_initial_window.snap | 5 ++ .../s2n-quic-core/src/recovery/cubic/tests.rs | 55 ++++++++++++++++--- 9 files changed, 113 insertions(+), 15 deletions(-) rename quic/s2n-quic-core/src/recovery/bbr/snapshots/{quic__s2n-quic-core__src__recovery__bbr__tests__events__on_mtu_update.snap => quic__s2n-quic-core__src__recovery__bbr__tests__events__on_mtu_update_decrease_larger_than_initial_window.snap} (100%) create mode 100644 quic/s2n-quic-core/src/recovery/bbr/snapshots/quic__s2n-quic-core__src__recovery__bbr__tests__events__on_mtu_update_decrease_smaller_than_initial_window.snap create mode 100644 quic/s2n-quic-core/src/recovery/bbr/snapshots/quic__s2n-quic-core__src__recovery__bbr__tests__events__on_mtu_update_increase.snap create mode 100644 quic/s2n-quic-core/src/recovery/cubic/snapshots/quic__s2n-quic-core__src__recovery__cubic__tests__events__on_mtu_update_decrease_larger_than_initial_window.snap create mode 100644 quic/s2n-quic-core/src/recovery/cubic/snapshots/quic__s2n-quic-core__src__recovery__cubic__tests__events__on_mtu_update_decrease_smaller_than_initial_window.snap diff --git a/quic/s2n-quic-core/src/recovery/bbr.rs b/quic/s2n-quic-core/src/recovery/bbr.rs index 04026da5b7..e2f0656a06 100644 --- a/quic/s2n-quic-core/src/recovery/bbr.rs +++ b/quic/s2n-quic-core/src/recovery/bbr.rs @@ -532,8 +532,6 @@ impl CongestionController for BbrCongestionController { //# window measured in bytes [RFC4821]. //= https://www.rfc-editor.org/rfc/rfc9002#section-7.2 - //= type=exception - //= reason=The maximum datagram size remains at the minimum (1200 bytes) during the handshake //# If the maximum datagram size is decreased in order to complete the //# handshake, the congestion window SHOULD be set to the new initial //# congestion window. @@ -542,8 +540,11 @@ impl CongestionController for BbrCongestionController { let old_max_datagram_size = self.max_datagram_size; self.max_datagram_size = max_datagram_size; - self.cwnd = + let cwnd = ((self.cwnd as f32 / old_max_datagram_size as f32) * max_datagram_size as f32) as u32; + let initial_window = Self::initial_window(max_datagram_size, &Default::default()); + + self.cwnd = max(cwnd, initial_window); } #[inline] diff --git a/quic/s2n-quic-core/src/recovery/bbr/snapshots/quic__s2n-quic-core__src__recovery__bbr__tests__events__on_mtu_update.snap b/quic/s2n-quic-core/src/recovery/bbr/snapshots/quic__s2n-quic-core__src__recovery__bbr__tests__events__on_mtu_update_decrease_larger_than_initial_window.snap similarity index 100% rename from quic/s2n-quic-core/src/recovery/bbr/snapshots/quic__s2n-quic-core__src__recovery__bbr__tests__events__on_mtu_update.snap rename to quic/s2n-quic-core/src/recovery/bbr/snapshots/quic__s2n-quic-core__src__recovery__bbr__tests__events__on_mtu_update_decrease_larger_than_initial_window.snap diff --git a/quic/s2n-quic-core/src/recovery/bbr/snapshots/quic__s2n-quic-core__src__recovery__bbr__tests__events__on_mtu_update_decrease_smaller_than_initial_window.snap b/quic/s2n-quic-core/src/recovery/bbr/snapshots/quic__s2n-quic-core__src__recovery__bbr__tests__events__on_mtu_update_decrease_smaller_than_initial_window.snap new file mode 100644 index 0000000000..9e1f6cb14f --- /dev/null +++ b/quic/s2n-quic-core/src/recovery/bbr/snapshots/quic__s2n-quic-core__src__recovery__bbr__tests__events__on_mtu_update_decrease_smaller_than_initial_window.snap @@ -0,0 +1,5 @@ +--- +source: quic/s2n-quic-core/src/recovery/bbr/tests.rs +expression: "" +--- + diff --git a/quic/s2n-quic-core/src/recovery/bbr/snapshots/quic__s2n-quic-core__src__recovery__bbr__tests__events__on_mtu_update_increase.snap b/quic/s2n-quic-core/src/recovery/bbr/snapshots/quic__s2n-quic-core__src__recovery__bbr__tests__events__on_mtu_update_increase.snap new file mode 100644 index 0000000000..9e1f6cb14f --- /dev/null +++ b/quic/s2n-quic-core/src/recovery/bbr/snapshots/quic__s2n-quic-core__src__recovery__bbr__tests__events__on_mtu_update_increase.snap @@ -0,0 +1,5 @@ +--- +source: quic/s2n-quic-core/src/recovery/bbr/tests.rs +expression: "" +--- + diff --git a/quic/s2n-quic-core/src/recovery/bbr/tests.rs b/quic/s2n-quic-core/src/recovery/bbr/tests.rs index 4a68107e23..c3015def1d 100644 --- a/quic/s2n-quic-core/src/recovery/bbr/tests.rs +++ b/quic/s2n-quic-core/src/recovery/bbr/tests.rs @@ -1103,7 +1103,7 @@ fn control_update_required() { } #[test] -fn on_mtu_update() { +fn on_mtu_update_increase() { let mut mtu = 5000; let mut bbr = BbrCongestionController::new(mtu, Default::default()); let mut publisher = event::testing::Publisher::snapshot(); @@ -1118,6 +1118,42 @@ fn on_mtu_update() { assert_eq!(bbr.cwnd, 200_000); } +#[test] +fn on_mtu_update_decrease_smaller_than_initial_window() { + let mut mtu = 9000; + let mut bbr = BbrCongestionController::new(mtu, Default::default()); + let mut publisher = event::testing::Publisher::snapshot(); + let mut publisher = PathPublisher::new(&mut publisher, path::Id::test_id()); + + bbr.cwnd = 18_000; + + mtu = 1350; + bbr.on_mtu_update(mtu, &mut publisher); + + assert_eq!(bbr.max_datagram_size, mtu); + + // updated initial window is 10 x MTU = 10 x 1350 + assert_eq!(bbr.cwnd, 13_500); +} + +#[test] +fn on_mtu_update_decrease_larger_than_initial_window() { + let mut mtu = 9000; + let mut bbr = BbrCongestionController::new(mtu, Default::default()); + let mut publisher = event::testing::Publisher::snapshot(); + let mut publisher = PathPublisher::new(&mut publisher, path::Id::test_id()); + + bbr.cwnd = 180_000; + + mtu = 1350; + bbr.on_mtu_update(mtu, &mut publisher); + + assert_eq!(bbr.max_datagram_size, mtu); + + // Congestion window is still 20 packets based on the updated MTU + assert_eq!(bbr.cwnd, 27_000); +} + /// Helper method to move the given BBR congestion controller into the /// ProbeBW state with the given CyclePhase fn enter_probe_bw_state( diff --git a/quic/s2n-quic-core/src/recovery/cubic.rs b/quic/s2n-quic-core/src/recovery/cubic.rs index 8827f33c9d..152ca2657d 100644 --- a/quic/s2n-quic-core/src/recovery/cubic.rs +++ b/quic/s2n-quic-core/src/recovery/cubic.rs @@ -451,8 +451,6 @@ impl CongestionController for CubicCongestionController { //# limit to compensate for the size of the actual packets. //= https://www.rfc-editor.org/rfc/rfc9002#section-7.2 - //= type=exception - //= reason=The maximum datagram size remains at the minimum (1200 bytes) during the handshake //# If the maximum datagram size is decreased in order to complete the //# handshake, the congestion window SHOULD be set to the new initial //# congestion window. @@ -462,8 +460,12 @@ impl CongestionController for CubicCongestionController { self.max_datagram_size = max_datagram_size; self.cubic.max_datagram_size = max_datagram_size; - self.congestion_window = + let congestion_window = (self.congestion_window / old_max_datagram_size as f32) * max_datagram_size as f32; + let initial_window = + Self::initial_window(&self.cubic, max_datagram_size, &Default::default()); + + self.congestion_window = max(congestion_window as u32, initial_window) as f32; } //= https://www.rfc-editor.org/rfc/rfc9002#section-6.4 diff --git a/quic/s2n-quic-core/src/recovery/cubic/snapshots/quic__s2n-quic-core__src__recovery__cubic__tests__events__on_mtu_update_decrease_larger_than_initial_window.snap b/quic/s2n-quic-core/src/recovery/cubic/snapshots/quic__s2n-quic-core__src__recovery__cubic__tests__events__on_mtu_update_decrease_larger_than_initial_window.snap new file mode 100644 index 0000000000..ec75581985 --- /dev/null +++ b/quic/s2n-quic-core/src/recovery/cubic/snapshots/quic__s2n-quic-core__src__recovery__cubic__tests__events__on_mtu_update_decrease_larger_than_initial_window.snap @@ -0,0 +1,5 @@ +--- +source: quic/s2n-quic-core/src/recovery/cubic/tests.rs +expression: "" +--- + diff --git a/quic/s2n-quic-core/src/recovery/cubic/snapshots/quic__s2n-quic-core__src__recovery__cubic__tests__events__on_mtu_update_decrease_smaller_than_initial_window.snap b/quic/s2n-quic-core/src/recovery/cubic/snapshots/quic__s2n-quic-core__src__recovery__cubic__tests__events__on_mtu_update_decrease_smaller_than_initial_window.snap new file mode 100644 index 0000000000..ec75581985 --- /dev/null +++ b/quic/s2n-quic-core/src/recovery/cubic/snapshots/quic__s2n-quic-core__src__recovery__cubic__tests__events__on_mtu_update_decrease_smaller_than_initial_window.snap @@ -0,0 +1,5 @@ +--- +source: quic/s2n-quic-core/src/recovery/cubic/tests.rs +expression: "" +--- + diff --git a/quic/s2n-quic-core/src/recovery/cubic/tests.rs b/quic/s2n-quic-core/src/recovery/cubic/tests.rs index 6ee85fde42..1933cd23f2 100644 --- a/quic/s2n-quic-core/src/recovery/cubic/tests.rs +++ b/quic/s2n-quic-core/src/recovery/cubic/tests.rs @@ -790,12 +790,11 @@ fn on_packet_lost_persistent_congestion() { #[test] fn on_mtu_update_increase() { let mut mtu = 5000; - let cwnd_in_packets = 100_000f32; - let cwnd_in_bytes = cwnd_in_packets / mtu as f32; + let cwnd_in_bytes = 100_000f32; let mut cc = CubicCongestionController::new(mtu, Default::default()); let mut publisher = event::testing::Publisher::snapshot(); let mut publisher = PathPublisher::new(&mut publisher, path::Id::test_id()); - cc.congestion_window = cwnd_in_packets; + cc.congestion_window = cwnd_in_bytes; mtu = 10000; cc.on_mtu_update(mtu, &mut publisher); @@ -803,12 +802,52 @@ fn on_mtu_update_increase() { assert_eq!(cc.cubic.max_datagram_size, mtu); assert_delta!(cc.congestion_window, 200_000.0, 0.001); +} - //= https://www.rfc-editor.org/rfc/rfc8899#section-3 - //= type=test - //# An update to the PLPMTU (or MPS) MUST NOT increase the congestion - //# window measured in bytes [RFC4821]. - assert_delta!(cc.congestion_window / mtu as f32, cwnd_in_bytes, 0.001); +//= https://www.rfc-editor.org/rfc/rfc9002#section-7.2 +//= type=test +//# If the maximum datagram size changes during the connection, the +//# initial congestion window SHOULD be recalculated with the new size. + +//= https://www.rfc-editor.org/rfc/rfc9002#section-7.2 +//= type=test +//# If the maximum datagram size is decreased in order to complete the +//# handshake, the congestion window SHOULD be set to the new initial +//# congestion window. +#[test] +fn on_mtu_update_decrease_smaller_than_initial_window() { + let mut mtu = 9000; + let cwnd_in_bytes = 18_000f32; + let mut cc = CubicCongestionController::new(mtu, Default::default()); + let mut publisher = event::testing::Publisher::snapshot(); + let mut publisher = PathPublisher::new(&mut publisher, path::Id::test_id()); + cc.congestion_window = cwnd_in_bytes; + + mtu = 1350; + cc.on_mtu_update(mtu, &mut publisher); + assert_eq!(cc.max_datagram_size, mtu); + assert_eq!(cc.cubic.max_datagram_size, mtu); + + // updated initial window is 10 x MTU = 10 x 1350 + assert_delta!(cc.congestion_window, 13_500.0, 0.001); +} + +#[test] +fn on_mtu_update_decrease_larger_than_initial_window() { + let mut mtu = 9000; + let cwnd_in_bytes = 180_000f32; + let mut cc = CubicCongestionController::new(mtu, Default::default()); + let mut publisher = event::testing::Publisher::snapshot(); + let mut publisher = PathPublisher::new(&mut publisher, path::Id::test_id()); + cc.congestion_window = cwnd_in_bytes; + + mtu = 1350; + cc.on_mtu_update(mtu, &mut publisher); + assert_eq!(cc.max_datagram_size, mtu); + assert_eq!(cc.cubic.max_datagram_size, mtu); + + // Congestion window is still 20 packets based on the updated MTU + assert_delta!(cc.congestion_window, 27_000.0, 0.001); } //= https://www.rfc-editor.org/rfc/rfc9002#section-6.4