Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion noq-proto/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ identity-hash = { workspace = true }
lru-slab = { workspace = true }
qlog = { workspace = true, optional = true }
rand = { workspace = true }
rand_pcg = "0.10"
ring = { workspace = true, optional = true }
rustc-hash = { workspace = true }
rustls = { workspace = true, optional = true }
Expand All @@ -86,7 +87,6 @@ web-time = { workspace = true }
assert_matches = { workspace = true }
hex-literal = { workspace = true }
proptest = { workspace = true }
rand_pcg = "0.10"
rcgen = { workspace = true }
test-strategy = { workspace = true }
testresult = "0.4.1"
Expand Down
50 changes: 43 additions & 7 deletions noq-proto/src/congestion.rs
Original file line number Diff line number Diff line change
@@ -1,23 +1,27 @@
//! Logic for controlling the rate at which data is sent

use crate::Instant;
use crate::connection::RttEstimator;
use crate::{Duration, Instant};
use std::any::Any;
use std::sync::Arc;

mod bbr;
mod bbr3;
mod cubic;
mod new_reno;

pub use bbr::{Bbr, BbrConfig};
pub use bbr3::{Bbr3, Bbr3Config};
pub use cubic::{Cubic, CubicConfig};
pub use new_reno::{NewReno, NewRenoConfig};

/// Common interface for different congestion controllers
pub trait Controller: Send + Sync + std::fmt::Debug {
/// One or more packets were just sent
#[allow(unused_variables)]
fn on_sent(&mut self, now: Instant, bytes: u64, last_packet_number: u64) {}
fn on_sent(&mut self, now: Instant, bytes: u64, largest_pn: u64) {}

/// One packet was just sent
#[allow(unused_variables)]
fn on_packet_sent(&mut self, now: Instant, bytes: u16, pn: u64) {}

/// Packet deliveries were confirmed
///
Expand All @@ -29,6 +33,7 @@ pub trait Controller: Send + Sync + std::fmt::Debug {
now: Instant,
sent: Instant,
bytes: u64,
pn: u64,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum, I realise this is probably upstream... but let's improve naming because our experience so far has shown that these names do get confusing and consistency helps. I think @matheus23 advocated for pn before over packet_number and that seems like it would be the right compromise for the variables here. So:

  • on_sent(..., largest_pn: u64)
  • on_packet_sent(..., pn: u64)
  • on_ack(..., pn: u64)
  • on_congestion_event(..., largest_lost_pn: u64)
  • on_packet_lost(..., pn: u64, ...)

And also use the same variable names in all the impls.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

app_limited: bool,
rtt: &RttEstimator,
) {
Expand All @@ -51,15 +56,22 @@ pub trait Controller: Send + Sync + std::fmt::Debug {
/// congestion threshold period ending when the most recent packet in this batch was sent were
/// lost.
/// `lost_bytes` indicates how many bytes were lost. This value will be 0 for ECN triggers.
/// `largest_lost_pn` indicates the packet number of the packet with the highest packet number
/// in the congestion event.
fn on_congestion_event(
&mut self,
now: Instant,
sent: Instant,
is_persistent_congestion: bool,
is_ecn: bool,
lost_bytes: u64,
largest_lost_pn: u64,
);

/// One packet was just lost
#[allow(unused_variables)]
fn on_packet_lost(&mut self, lost_bytes: u16, pn: u64, now: Instant) {}

/// Packets were incorrectly deemed lost
///
/// This function is called when all packets that were deemed lost (for instance because
Expand All @@ -69,15 +81,35 @@ pub trait Controller: Send + Sync + std::fmt::Debug {
/// The known MTU for the current network path has been updated
fn on_mtu_update(&mut self, new_mtu: u16);

/// The peer's ACK-frequency parameters have changed
///
/// `ack_eliciting_threshold` is the number of ack-eliciting packets the peer may receive
/// before being required to send an immediate ACK (per the QUIC ACK frequency extension).
/// `requested_max_ack_delay` is the maximum delay we asked the peer to wait before sending
/// an ACK when the threshold hasn't been reached.
///
/// Controllers can use this to refine estimates that depend on peer ACK behavior (e.g.
/// BBR's offload budget).
#[allow(unused_variables)]
fn on_ack_frequency_update(
&mut self,
ack_eliciting_threshold: u64,
requested_max_ack_delay: Duration,
) {
}

/// Number of ack-eliciting bytes that may be in flight
fn window(&self) -> u64;

/// Retrieve implementation-specific metrics used to populate `qlog` traces when they are enabled
/// This is also used to alter the pacing of the connection with
/// `pacing_rate` and `send_quantum`
fn metrics(&self) -> ControllerMetrics {
ControllerMetrics {
congestion_window: self.window(),
ssthresh: None,
pacing_rate: None,
send_quantum: None,
}
}

Expand All @@ -91,16 +123,20 @@ pub trait Controller: Send + Sync + std::fmt::Debug {
fn into_any(self: Box<Self>) -> Box<dyn Any>;
}

/// Common congestion controller metrics
#[derive(Default, Debug)]
/// Common congestion controller metrics used both for logging purposes
/// but also to alter the pacing of the connection with
/// `pacing_rate` and `send_quantum`
#[derive(Default)]
#[non_exhaustive]
pub struct ControllerMetrics {
/// Congestion window (bytes)
pub congestion_window: u64,
/// Slow start threshold (bytes)
pub ssthresh: Option<u64>,
/// Pacing rate (bits/s)
/// Pacing rate (bytes/s)
pub pacing_rate: Option<u64>,
/// Send Quantum (bytes) used to control the size of packet bursts
pub send_quantum: Option<u64>,
}

/// Constructs controllers on demand
Expand Down
100 changes: 0 additions & 100 deletions noq-proto/src/congestion/bbr/bw_estimation.rs

This file was deleted.

152 changes: 0 additions & 152 deletions noq-proto/src/congestion/bbr/min_max.rs

This file was deleted.

Loading
Loading