From 613d23d68d05754fb5e010ff59033df9a3b8d4df Mon Sep 17 00:00:00 2001 From: Wesley Rosenblum Date: Wed, 5 Apr 2023 16:53:47 -0700 Subject: [PATCH] fix(s2n-quic-core): Advance max bw filter only once per probe bw cycle --- .../src/recovery/bbr/data_rate.rs | 5 ++ .../src/recovery/bbr/probe_bw.rs | 77 ++++++++++++++++++- ...r__probe_bw__events__update_ack_phase.snap | 5 ++ 3 files changed, 85 insertions(+), 2 deletions(-) create mode 100644 quic/s2n-quic-core/src/recovery/bbr/snapshots/quic__s2n-quic-core__src__recovery__bbr__probe_bw__events__update_ack_phase.snap diff --git a/quic/s2n-quic-core/src/recovery/bbr/data_rate.rs b/quic/s2n-quic-core/src/recovery/bbr/data_rate.rs index c06e288631..882a7b3610 100644 --- a/quic/s2n-quic-core/src/recovery/bbr/data_rate.rs +++ b/quic/s2n-quic-core/src/recovery/bbr/data_rate.rs @@ -147,6 +147,11 @@ impl Model { //# BBR.bw = min(BBR.max_bw, BBR.bw_lo, BBR.bw_hi) self.bw = self.max_bw().min(self.bw_lo()).min(self.bw_hi()) } + + #[cfg(test)] + pub fn cycle_count(&self) -> u8 { + self.cycle_count.0 + } } #[cfg(test)] diff --git a/quic/s2n-quic-core/src/recovery/bbr/probe_bw.rs b/quic/s2n-quic-core/src/recovery/bbr/probe_bw.rs index 8c5f5770a1..bee369f5d9 100644 --- a/quic/s2n-quic-core/src/recovery/bbr/probe_bw.rs +++ b/quic/s2n-quic-core/src/recovery/bbr/probe_bw.rs @@ -142,10 +142,12 @@ impl AckPhase { || *self == AckPhase::ProbeFeedback ) } - AckPhase::Refilling => assert_eq!(*self, AckPhase::ProbeStopping), + AckPhase::Refilling => { + assert!(*self == AckPhase::Init || *self == AckPhase::ProbeStopping) + } AckPhase::ProbeStarting => assert_eq!(*self, AckPhase::Refilling), AckPhase::ProbeFeedback => assert_eq!(*self, AckPhase::ProbeStarting), - AckPhase::Init => unreachable!("cannot transition to AckPhase::Init"), + AckPhase::Init => assert_eq!(*self, AckPhase::ProbeStopping), } } @@ -709,6 +711,7 @@ impl BbrCongestionController { AckPhase::ProbeStopping => { // end of samples from bw probing phase self.bw_probe_samples = false; + probe_bw_state.ack_phase.transition_to(AckPhase::Init); if !rate_sample.is_app_limited { self.data_rate_model.advance_max_bw_filter(); } @@ -1102,6 +1105,10 @@ mod tests { assert_eq!(ack_phase, AckPhase::ProbeStarting); ack_phase.transition_to(AckPhase::ProbeStopping); assert_eq!(ack_phase, AckPhase::ProbeStopping); + ack_phase.transition_to(AckPhase::Init); + assert_eq!(ack_phase, AckPhase::Init); + ack_phase.transition_to(AckPhase::Refilling); + assert_eq!(ack_phase, AckPhase::Refilling); } #[test] @@ -1128,4 +1135,70 @@ mod tests { bbr.enter_probe_bw(true, &mut rng, now, &mut publisher); assert!(bbr.state.is_probing_bw_cruise()); } + + #[test] + fn update_ack_phase() { + let mut bbr = BbrCongestionController::new(MINIMUM_MTU); + let mut rng = random::testing::Generator::default(); + let mut publisher = event::testing::Publisher::snapshot(); + let mut publisher = PathPublisher::new(&mut publisher, path::Id::test_id()); + let now = NoopClock.get_time(); + bbr.state = bbr::State::Drain; + + // cruise_immediately = false + bbr.enter_probe_bw(false, &mut rng, now, &mut publisher); + + // Start a new round + let packet_info = PacketInfo { + delivered_bytes: 0, + delivered_time: now, + lost_bytes: 0, + ecn_ce_count: 0, + first_sent_time: now, + bytes_in_flight: 3000, + is_app_limited: false, + }; + bbr.round_counter.on_ack(packet_info, 5000); + + bbr.bw_probe_samples = true; + + assert!(bbr.state.is_probing_bw()); + if let bbr::State::ProbeBw(ref mut probe_bw_state) = bbr.state { + assert_eq!(probe_bw_state.ack_phase, AckPhase::ProbeStopping); + assert_eq!(bbr.data_rate_model.cycle_count(), 0); + } + + let rate_sample = RateSample { + is_app_limited: false, + ..Default::default() + }; + bbr.update_ack_phase(rate_sample); + + // Moving from ProbeStopping to Init increments the cycle count + if let bbr::State::ProbeBw(ref mut probe_bw_state) = bbr.state { + assert_eq!(probe_bw_state.ack_phase, AckPhase::Init); + assert_eq!(bbr.data_rate_model.cycle_count(), 1); + assert!(!bbr.bw_probe_samples); + } + + bbr.update_ack_phase(rate_sample); + + // Updating the ack phase again does not increment the cycle count + if let bbr::State::ProbeBw(ref mut probe_bw_state) = bbr.state { + assert_eq!(probe_bw_state.ack_phase, AckPhase::Init); + assert_eq!(bbr.data_rate_model.cycle_count(), 1); + + // set ack phase for the next test + probe_bw_state.ack_phase = AckPhase::ProbeStarting; + } + + bbr.bw_probe_samples = true; + bbr.update_ack_phase(rate_sample); + + if let bbr::State::ProbeBw(ref mut probe_bw_state) = bbr.state { + assert_eq!(probe_bw_state.ack_phase, AckPhase::ProbeFeedback); + assert_eq!(bbr.data_rate_model.cycle_count(), 1); + assert!(bbr.bw_probe_samples); + } + } } diff --git a/quic/s2n-quic-core/src/recovery/bbr/snapshots/quic__s2n-quic-core__src__recovery__bbr__probe_bw__events__update_ack_phase.snap b/quic/s2n-quic-core/src/recovery/bbr/snapshots/quic__s2n-quic-core__src__recovery__bbr__probe_bw__events__update_ack_phase.snap new file mode 100644 index 0000000000..aeaa5285bb --- /dev/null +++ b/quic/s2n-quic-core/src/recovery/bbr/snapshots/quic__s2n-quic-core__src__recovery__bbr__probe_bw__events__update_ack_phase.snap @@ -0,0 +1,5 @@ +--- +source: quic/s2n-quic-core/src/recovery/bbr/probe_bw.rs +expression: "" +--- +BbrStateChanged { path_id: 0, state: ProbeBwDown }