BBR: Check for app-limited when entering a new iteration of the send loop

quiche/src/lib.rs

@@ -573,6 +573,7 @@ pub struct Config {
     grease: bool,
 
     cc_algorithm: CongestionControlAlgorithm,
+    enable_bbr_app_limited_fix: bool,
     custom_bbr_params: Option<BbrParams>,
     initial_congestion_window_packets: usize,
     enable_relaxed_loss_threshold: bool,
@@ -658,6 +659,7 @@ impl Config {
             application_protos: Vec::new(),
             grease: true,
             cc_algorithm: CongestionControlAlgorithm::CUBIC,
+            enable_bbr_app_limited_fix: false,
             custom_bbr_params: None,
             initial_congestion_window_packets:
                 DEFAULT_INITIAL_CONGESTION_WINDOW_PACKETS,
@@ -1086,6 +1088,15 @@ impl Config {
         self.cc_algorithm = algo;
     }
 
+    /// Enable a rework of how the BBR congestion control implementation
+    /// computes app-limited.  This config parameter will be removed after we
+    /// build confidence on the new implementation or decide to roll it back.
+    ///
+    /// Defaults to `false`.
+    pub fn set_enable_bbr_app_limited_fix(&mut self, value: bool) {
+        self.enable_bbr_app_limited_fix = value;
+    }
+
     /// Sets custom BBR settings.
     ///
     /// This API is experimental and will be removed in the future.
@@ -2509,6 +2520,32 @@ impl<F: BufFactory> Connection<F> {
         Ok(())
     }
 
+    /// Enable a rework of how the BBR congestion control implementation
+    /// computes app-limited.  This config parameter will be removed after we
+    /// build confidence on the new implementation or decide to roll it back.
+    ///
+    /// Currently this only applies if cc_algorithm is
+    /// `CongestionControlAlgorithm::Bbr2Gcongestion`.
+    ///
+    /// This function can only be called inside one of BoringSSL's handshake
+    /// callbacks, before any packet has been sent. Calling this function any
+    /// other time will have no effect.
+    ///
+    /// See [`Config::set_enable_bbr_app_limited_fix()`].
+    ///
+    /// [`Config::set_enable_bbr_app_limited_fix()`]: struct.Config.html#method.set_enable_bbr_app_limited_fix
+    #[cfg(feature = "boringssl-boring-crate")]
+    #[cfg_attr(docsrs, doc(cfg(feature = "boringssl-boring-crate")))]
+    pub fn set_enable_bbr_app_limited_fix_in_handshake(
+        ssl: &mut boring::ssl::SslRef, value: bool,
+    ) -> Result<()> {
+        let ex_data = tls::ExData::from_ssl_ref(ssl).ok_or(Error::TlsFail)?;
+
+        ex_data.recovery_config.enable_bbr_app_limited_fix = value;
+
+        Ok(())
+    }
+
     /// Sets the congestion control algorithm used by string.
     ///
     /// This function can only be called inside one of BoringSSL's handshake
@@ -2810,6 +2847,12 @@ impl<F: BufFactory> Connection<F> {
         Ok(())
     }
 
+    /// Returns true if at least 1 stream has headers or body data to
+    /// write, or there are items in the DATAGRAM send queue.
+    fn has_flushable_data(&self) -> bool {
+        self.streams.has_flushable() || !self.dgram_send_queue.is_empty()
+    }
+
     /// Processes QUIC packets received from the peer.
     ///
     /// On success the number of bytes processed from the input buffer is
@@ -2859,11 +2902,18 @@ impl<F: BufFactory> Connection<F> {
             return Err(Error::BufferTooShort);
         }
 
+        let now = Instant::now();
+
         let recv_pid = self.paths.path_id_from_addrs(&(info.to, info.from));
 
         if let Some(recv_pid) = recv_pid {
             let recv_path = self.paths.get_mut(recv_pid)?;
 
+            // Run the app limited check if the previous send loop ran out of data
+            // to send. Limit the check to the path where the packet
+            // was received.
+            recv_path.recovery.bbr_check_if_app_limited(&now);
+
             // Keep track of how many bytes we received from the client, so we
             // can limit bytes sent back before address validation, to a
             // multiple of this. The limit needs to be increased early on, so
@@ -2896,6 +2946,7 @@ impl<F: BufFactory> Connection<F> {
         // Process coalesced packets.
         while left > 0 {
             let read = match self.recv_single(
+                now,
                 &mut buf[len - left..len],
                 &info,
                 recv_pid,
@@ -2993,10 +3044,9 @@ impl<F: BufFactory> Connection<F> {
     ///
     /// [`Done`]: enum.Error.html#variant.Done
     fn recv_single(
-        &mut self, buf: &mut [u8], info: &RecvInfo, recv_pid: Option<usize>,
+        &mut self, now: Instant, buf: &mut [u8], info: &RecvInfo,
+        recv_pid: Option<usize>,
     ) -> Result<usize> {
-        let now = Instant::now();
-
         if buf.is_empty() {
             return Err(Error::Done);
         }
@@ -4035,6 +4085,11 @@ impl<F: BufFactory> Connection<F> {
 
         let send_path = self.paths.get_mut(send_pid)?;
 
+        // Run the app limited check if the previous send loop ran out of data to
+        // send. Limit the check to the path where the connection is
+        // trying to send.
+        send_path.recovery.bbr_check_if_app_limited(&now);
+
         // Update max datagram size to allow path MTU discovery probe to be sent.
         if let Some(pmtud) = send_path.pmtud.as_mut() {
             if pmtud.should_probe() {
@@ -4068,7 +4123,15 @@ impl<F: BufFactory> Connection<F> {
             ) {
                 Ok(v) => v,
 
-                Err(Error::BufferTooShort) | Err(Error::Done) => break,
+                Err(Error::BufferTooShort) => break,
+
+                Err(Error::Done) => {
+                    let has_flushable_data = self.has_flushable_data();
+                    let send_path = self.paths.get_mut(send_pid)?;
+                    send_path.recovery.send_stopped_early(has_flushable_data);
+
+                    break;
+                },
 
                 Err(e) => return Err(e),
             };
@@ -7031,6 +7094,11 @@ impl<F: BufFactory> Connection<F> {
     pub fn on_timeout(&mut self) {
         let now = Instant::now();
 
+        // Check for app limited on all paths when handling a timeout.
+        for (_, path) in self.paths.iter_mut() {
+            path.recovery.bbr_check_if_app_limited(&now);
+        }
+
         if let Some(draining_timer) = self.draining_timer {
             if draining_timer <= now {
                 trace!("{} draining timeout expired", self.trace_id);

quiche/src/recovery/congestion/recovery.rs

@@ -956,7 +956,6 @@ impl RecoveryOps for LegacyRecovery {
         self.detect_lost_packets(epoch, now, "")
     }
 
-    // FIXME only used by gcongestion
     fn on_app_limited(&mut self) {
         // Not implemented for legacy recovery, update_app_limited and
         // delivery_rate_update_app_limited used instead.
@@ -1038,6 +1037,16 @@ impl RecoveryOps for LegacyRecovery {
         false
     }
 
+    fn send_stopped_early(&mut self, _has_flushable_data: bool) {
+        // Not implemented -- only used by the BBR implementation
+        // which lives in the gcongestion directory.
+    }
+
+    fn bbr_check_if_app_limited(&mut self, _now: &Instant) {
+        // Not implemented -- only used by the BBR implementation
+        // which lives in the gcongestion directory.
+    }
+
     fn lost_count(&self) -> usize {
         self.congestion.lost_count
     }

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

@@ -517,7 +517,7 @@ impl BandwidthSampler {
         }
     }
 
-    #[allow(dead_code)]
+    #[cfg(test)]
     pub(crate) fn is_app_limited(&self) -> bool {
         self.is_app_limited
     }

quiche/src/recovery/gcongestion/bbr2.rs

@@ -749,8 +749,9 @@ impl CongestionControl for BBRv2 {
         self.last_quiescence_start.is_none()
     }
 
-    fn is_cwnd_limited(&self, bytes_in_flight: usize) -> bool {
-        bytes_in_flight >= self.get_congestion_window()
+    #[cfg(test)]
+    fn is_app_limited(&self) -> bool {
+        self.mode.network_model().is_app_limited()
     }
 
     fn pacing_rate(

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

@@ -774,6 +774,11 @@ impl BBRv2NetworkModel {
         self.bandwidth_sampler.on_app_limited()
     }
 
+    #[cfg(test)]
+    pub(super) fn is_app_limited(&self) -> bool {
+        self.bandwidth_sampler.is_app_limited()
+    }
+
     pub(super) fn loss_events_in_round(&self) -> usize {
         self.loss_events_in_round
     }

quiche/src/recovery/gcongestion/mod.rs

@@ -110,8 +110,8 @@ pub(super) trait CongestionControl: Debug {
 
     fn is_in_recovery(&self) -> bool;
 
-    #[allow(dead_code)]
-    fn is_cwnd_limited(&self, bytes_in_flight: usize) -> bool;
+    #[cfg(test)]
+    fn is_app_limited(&self) -> bool;
 
     fn pacing_rate(
         &self, bytes_in_flight: usize, rtt_stats: &RttStats,

quiche/src/recovery/gcongestion/pacer.rs

@@ -283,12 +283,7 @@ impl Pacer {
     }
 
     #[cfg(test)]
-    pub fn is_app_limited(&self, bytes_in_flight: usize) -> bool {
-        !self.is_cwnd_limited(bytes_in_flight)
-    }
-
-    #[cfg(test)]
-    fn is_cwnd_limited(&self, bytes_in_flight: usize) -> bool {
-        !self.pacing_limited && self.sender.is_cwnd_limited(bytes_in_flight)
+    pub fn is_app_limited(&self) -> bool {
+        self.sender.is_app_limited()
     }
 }