Update delivery_rate units for gcongestion (#2043)

* Update delivery_rate units for gcongestion

* congestion test

* expose typed bandwidth for delivery rate

---------

Co-authored-by: Apoorv Kothari <[email protected]>

Author: toidiu

quiche/src/lib.rs

@@ -9046,6 +9046,9 @@ impl TransportParams {
 #[doc(hidden)]
 pub mod testing {
     use super::*;
+    use crate::recovery::Sent;
+    use smallvec::smallvec;
+    use std::time::Instant;
 
     pub struct Pipe {
         pub client: Connection,
@@ -9555,6 +9558,27 @@ pub mod testing {
 
         (cid, reset_token)
     }
+
+    pub fn helper_packet_sent(pkt_num: u64, now: Instant, size: usize) -> Sent {
+        Sent {
+            pkt_num,
+            frames: smallvec![],
+            time_sent: now,
+            time_acked: None,
+            time_lost: None,
+            size,
+            ack_eliciting: true,
+            in_flight: true,
+            delivered: 0,
+            delivered_time: now,
+            first_sent_time: now,
+            is_app_limited: false,
+            tx_in_flight: 0,
+            lost: 0,
+            has_data: false,
+            pmtud: false,
+        }
+    }
 }
 
 #[cfg(test)]

quiche/src/path.rs

@@ -509,7 +509,7 @@ impl Path {
             lost_bytes: self.recovery.bytes_lost(),
             stream_retrans_bytes: self.stream_retrans_bytes,
             pmtu: self.recovery.max_datagram_size(),
-            delivery_rate: self.recovery.delivery_rate(),
+            delivery_rate: self.recovery.delivery_rate().to_bytes_per_second(),
         }
     }
 }

quiche/src/recovery/gcongestion/bandwidth.rs renamed to quiche/src/recovery/bandwidth.rs

@@ -119,6 +119,10 @@ impl Bandwidth {
         self.bits_per_second
     }
 
+    pub const fn to_bytes_per_second(self) -> u64 {
+        self.bits_per_second / 8
+    }
+
     pub const fn from_kbits_per_second(k_bits_per_second: u64) -> Self {
         Bandwidth {
             bits_per_second: k_bits_per_second * 1_000,
@@ -177,10 +181,9 @@ mod tests {
     fn constructors() {
         // Internal representation is bits per second.
         assert_eq!(Bandwidth::from_bytes_per_second(100).bits_per_second, 800);
-        assert_eq!(
-            Bandwidth::from_bytes_per_second(100).to_bits_per_second(),
-            800
-        );
+        let bw = Bandwidth::from_bytes_per_second(100);
+        assert_eq!(bw.to_bits_per_second(), 800);
+        assert_eq!(bw.to_bytes_per_second(), 100);
 
         // kbits == 1000 bits
         assert_eq!(

quiche/src/recovery/congestion/bbr/mod.rs

@@ -405,10 +405,13 @@ mod tests {
         assert_eq!(sender.congestion_window, cwnd_prev + mss * 5);
         assert_eq!(sender.bytes_in_flight, 0);
         assert_eq!(
-            sender.delivery_rate(),
+            sender.delivery_rate().to_bytes_per_second(),
             ((mss * 5) as f64 / rtt.as_secs_f64()) as u64
         );
-        assert_eq!(sender.bbr_state.btlbw, sender.delivery_rate());
+        assert_eq!(
+            sender.bbr_state.btlbw,
+            sender.delivery_rate().to_bytes_per_second()
+        );
     }
 
     #[test]

quiche/src/recovery/congestion/bbr/per_ack.rs

@@ -75,15 +75,15 @@ pub fn bbr_update_control_parameters(
 fn bbr_update_btlbw(r: &mut Congestion, packet: &Acked, _bytes_in_flight: usize) {
     bbr_update_round(r, packet);
 
-    if r.delivery_rate() >= r.bbr_state.btlbw ||
+    if r.delivery_rate().to_bytes_per_second() >= r.bbr_state.btlbw ||
         !r.delivery_rate.sample_is_app_limited()
     {
         // Since minmax filter is based on time,
         // start_time + (round_count as seconds) is used instead.
         r.bbr_state.btlbw = r.bbr_state.btlbwfilter.running_max(
             BTLBW_FILTER_LEN,
             r.bbr_state.start_time + Duration::from_secs(r.bbr_state.round_count),
-            r.delivery_rate(),
+            r.delivery_rate().to_bytes_per_second(),
         );
     }
 }

quiche/src/recovery/congestion/bbr2/mod.rs

@@ -757,10 +757,13 @@ mod tests {
         assert_eq!(r.cwnd(), cwnd_prev + mss * 5);
         assert_eq!(r.bytes_in_flight, 0);
         assert_eq!(
-            r.delivery_rate(),
+            r.delivery_rate().to_bytes_per_second(),
             ((mss * 5) as f64 / rtt.as_secs_f64()) as u64
         );
-        assert_eq!(r.congestion.bbr2_state.full_bw, r.delivery_rate());
+        assert_eq!(
+            r.congestion.bbr2_state.full_bw,
+            r.delivery_rate().to_bytes_per_second()
+        );
     }
 
     #[test]

quiche/src/recovery/congestion/bbr2/per_ack.rs

@@ -428,8 +428,9 @@ fn bbr2_adapt_upper_bounds(r: &mut Congestion, now: Instant) {
         }
 
         // TODO: what's rs.bw???
-        if r.delivery_rate() > r.bbr2_state.bw_hi {
-            r.bbr2_state.bw_hi = r.delivery_rate();
+        let delivery_rate = r.delivery_rate().to_bytes_per_second();
+        if delivery_rate > r.bbr2_state.bw_hi {
+            r.bbr2_state.bw_hi = delivery_rate;
         }
 
         if r.bbr2_state.state == BBR2StateMachine::ProbeBWUP {
@@ -569,7 +570,7 @@ fn bbr2_start_round(r: &mut Congestion) {
 pub fn bbr2_update_max_bw(r: &mut Congestion, packet: &Acked) {
     bbr2_update_round(r, packet);
 
-    if r.delivery_rate() >= r.bbr2_state.max_bw ||
+    if r.delivery_rate().to_bytes_per_second() >= r.bbr2_state.max_bw ||
         !r.delivery_rate.sample_is_app_limited()
     {
         let max_bw_filter_len = r
@@ -581,7 +582,7 @@ pub fn bbr2_update_max_bw(r: &mut Congestion, packet: &Acked) {
             max_bw_filter_len,
             r.bbr2_state.start_time +
                 Duration::from_secs(r.bbr2_state.cycle_count),
-            r.delivery_rate(),
+            r.delivery_rate().to_bytes_per_second(),
         );
     }
 }

quiche/src/recovery/congestion/bbr2/per_loss.rs

@@ -106,7 +106,9 @@ pub fn bbr2_update_latest_delivery_signals(r: &mut Congestion) {
 
     // Near start of ACK processing.
     bbr.loss_round_start = false;
-    bbr.bw_latest = bbr.bw_latest.max(r.delivery_rate.sample_delivery_rate());
+    bbr.bw_latest = bbr
+        .bw_latest
+        .max(r.delivery_rate.sample_delivery_rate().to_bytes_per_second());
     bbr.inflight_latest =
         bbr.inflight_latest.max(r.delivery_rate.sample_delivered());
 
@@ -121,7 +123,8 @@ pub fn bbr2_advance_latest_delivery_signals(r: &mut Congestion) {
 
     // Near end of ACK processing.
     if bbr.loss_round_start {
-        bbr.bw_latest = r.delivery_rate.sample_delivery_rate();
+        bbr.bw_latest =
+            r.delivery_rate.sample_delivery_rate().to_bytes_per_second();
         bbr.inflight_latest = r.delivery_rate.sample_delivered();
     }
 }

quiche/src/recovery/congestion/delivery_rate.rs

@@ -32,6 +32,8 @@
 use std::time::Duration;
 use std::time::Instant;
 
+use crate::recovery::bandwidth::Bandwidth;
+
 use super::Acked;
 use super::Sent;
 
@@ -73,7 +75,7 @@ impl Default for Rate {
 
             largest_acked: 0,
 
-            rate_sample: RateSample::default(),
+            rate_sample: RateSample::new(),
         }
     }
 }
@@ -149,9 +151,11 @@ impl Rate {
 
             if !interval.is_zero() {
                 // Fill in rate_sample with a rate sample.
-                self.rate_sample.delivery_rate =
-                    (self.rate_sample.delivered as f64 / interval.as_secs_f64())
-                        as u64;
+                let bytes_per_second = (self.rate_sample.delivered as f64 /
+                    interval.as_secs_f64())
+                    as u64;
+                self.rate_sample.bandwidth =
+                    Bandwidth::from_bytes_per_second(bytes_per_second);
             }
         }
     }
@@ -168,8 +172,8 @@ impl Rate {
         self.delivered
     }
 
-    pub fn sample_delivery_rate(&self) -> u64 {
-        self.rate_sample.delivery_rate
+    pub fn sample_delivery_rate(&self) -> Bandwidth {
+        self.rate_sample.bandwidth
     }
 
     pub fn sample_rtt(&self) -> Duration {
@@ -189,9 +193,10 @@ impl Rate {
     }
 }
 
-#[derive(Default, Debug)]
+#[derive(Debug)]
 struct RateSample {
-    delivery_rate: u64,
+    // The sample delivery_rate in bytes/sec
+    bandwidth: Bandwidth,
 
     is_app_limited: bool,
 
@@ -210,6 +215,22 @@ struct RateSample {
     rtt: Duration,
 }
 
+impl RateSample {
+    const fn new() -> Self {
+        RateSample {
+            bandwidth: Bandwidth::zero(),
+            is_app_limited: false,
+            interval: Duration::ZERO,
+            delivered: 0,
+            prior_delivered: 0,
+            prior_time: None,
+            send_elapsed: Duration::ZERO,
+            ack_elapsed: Duration::ZERO,
+            rtt: Duration::ZERO,
+        }
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -288,7 +309,7 @@ mod tests {
         assert_eq!(r.congestion.delivery_rate.delivered(), 2400);
 
         // Estimated delivery rate = (1200 x 2) / 0.05s = 48000.
-        assert_eq!(r.delivery_rate(), 48000);
+        assert_eq!(r.delivery_rate().to_bytes_per_second(), 48000);
     }
 
     #[test]

quiche/src/recovery/congestion/mod.rs

@@ -28,6 +28,7 @@ use debug_panic::debug_panic;
 use std::time::Instant;
 
 use self::recovery::Acked;
+use super::bandwidth::Bandwidth;
 use super::RecoveryConfig;
 use super::Sent;
 use crate::recovery::rtt;
@@ -146,7 +147,8 @@ impl Congestion {
         }
     }
 
-    pub(crate) fn delivery_rate(&self) -> u64 {
+    /// The most recent data delivery rate estimate.
+    pub(crate) fn delivery_rate(&self) -> Bandwidth {
         self.delivery_rate.sample_delivery_rate()
     }
 

quiche/src/recovery/congestion/recovery.rs

@@ -34,11 +34,11 @@ use std::collections::VecDeque;
 use super::RecoveryConfig;
 use super::Sent;
 
-use crate::recovery::HandshakeStatus;
-use crate::recovery::RecoveryOps;
-
 use crate::packet::Epoch;
 use crate::ranges::RangeSet;
+use crate::recovery::Bandwidth;
+use crate::recovery::HandshakeStatus;
+use crate::recovery::RecoveryOps;
 
 #[cfg(feature = "qlog")]
 use crate::recovery::QlogMetrics;
@@ -817,7 +817,8 @@ impl RecoveryOps for LegacyRecovery {
         self.rtt() + cmp::max(self.rtt_stats.rttvar * 4, GRANULARITY)
     }
 
-    fn delivery_rate(&self) -> u64 {
+    /// The most recent data delivery rate estimate.
+    fn delivery_rate(&self) -> Bandwidth {
         self.congestion.delivery_rate()
     }
 

quiche/src/recovery/gcongestion/bbr/bandwidth_sampler.rs

@@ -33,7 +33,7 @@ use std::time::Duration;
 use std::time::Instant;
 
 use super::Acked;
-use crate::recovery::gcongestion::bandwidth::Bandwidth;
+use crate::recovery::gcongestion::Bandwidth;
 use crate::recovery::gcongestion::Lost;
 
 use super::windowed_filter::WindowedFilter;

quiche/src/recovery/gcongestion/bbr2.rs

@@ -40,10 +40,11 @@ use std::time::Instant;
 
 use network_model::BBRv2NetworkModel;
 
+use crate::recovery::gcongestion::Bandwidth;
+
 use self::mode::Mode;
 use self::mode::ModeImpl;
 
-use super::bandwidth::Bandwidth;
 use super::bbr::SendTimeState;
 use super::Acked;
 use super::BbrBwLoReductionStrategy;

quiche/src/recovery/gcongestion/bbr2/network_model.rs

@@ -32,9 +32,9 @@ use std::ops::Add;
 use std::time::Duration;
 use std::time::Instant;
 
-use crate::recovery::gcongestion::bandwidth::Bandwidth;
 use crate::recovery::gcongestion::bbr::BandwidthSampler;
 use crate::recovery::gcongestion::bbr2::Params;
+use crate::recovery::gcongestion::Bandwidth;
 use crate::recovery::gcongestion::Lost;
 use crate::recovery::rtt::RttStats;
 use crate::recovery::rtt::INITIAL_RTT;

quiche/src/recovery/gcongestion/mod.rs

@@ -24,7 +24,6 @@
 // NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
 // SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
-pub mod bandwidth;
 mod bbr;
 mod bbr2;
 pub mod pacer;
@@ -34,8 +33,8 @@ use std::fmt::Debug;
 use std::str::FromStr;
 use std::time::Instant;
 
-use self::bandwidth::Bandwidth;
 pub use self::recovery::GRecovery;
+use crate::recovery::bandwidth::Bandwidth;
 
 use crate::recovery::rtt::RttStats;
 use crate::recovery::rtt::INITIAL_RTT;

quiche/src/recovery/gcongestion/pacer.rs

@@ -30,11 +30,11 @@
 
 use std::time::Instant;
 
+use crate::recovery::gcongestion::Bandwidth;
 use crate::recovery::rtt::RttStats;
 use crate::recovery::ReleaseDecision;
 use crate::recovery::ReleaseTime;
 
-use super::bandwidth::Bandwidth;
 use super::Acked;
 use super::Congestion;
 use super::CongestionControl;