smoltcp-rs / smoltcp

a smol tcp/ip stack
BSD Zero Clause License
3.63k stars 402 forks source link

feat(congestion control): add CongestionController trait and example impl #907

Closed ytakano closed 3 months ago

ytakano commented 4 months ago

Add CongestionController trait for congestion controllers of TCP.

By default, this PR uses NoControl, which do not control congestion, and the PR does not affect conventional behavior.

To use Cubic or Reno congestion controllers, socket-tcp-cubic or socket-tcp-reno features must be set or tcp::Socket::new_with_cc() must be used.

Users can implement custom congestion controllers by using CongestionController trait and tcp::Socket::new_with_cc().

Cubic or Reno can be tested as follows.

$ cargo test --features socket-tcp-cubic
$ cargo test --features socket-tcp-reno
codecov[bot] commented 4 months ago

Codecov Report

Attention: Patch coverage is 98.41270% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 79.96%. Comparing base (f0d1fd9) to head (833399f). Report is 4 commits behind head on main.

Files Patch % Lines
src/socket/tcp/congestion.rs 97.67% 1 Missing :warning:
src/socket/tcp/congestion/no_control.rs 75.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #907 +/- ## ========================================== + Coverage 79.92% 79.96% +0.04% ========================================== Files 80 82 +2 Lines 28234 28378 +144 ========================================== + Hits 22566 22693 +127 - Misses 5668 5685 +17 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ytakano commented 4 months ago

Avoid cbrt() and powi() for no_std.

pothos commented 4 months ago

Just for your reference, there was also https://github.com/smoltcp-rs/smoltcp/pull/450 and https://github.com/cbranch/smoltcp/commits/cbranch/cc (other non-upstreamed things are also public: https://github.com/cbranch/smoltcp/branches/stale, and if you are looking into BBR there is https://github.com/quinn-rs/quinn/tree/0.8.0)

ytakano commented 4 months ago

When I was testing smoltcp, it occupied traffic bandwidth, and other TCP connections starved due to the lack of congestion control.

By the way, I think delay based congestion control mechanisms like BBR should be also implemented by using CongestionController, because it can know time and bytes of sending/receiving data and RTT by using following methods.

pub trait CongestionController: Default {
    #[allow(unused_variables)]
    fn on_ack(&mut self, now: Instant, len: usize, rtt: &RttEstimator) {}

    #[allow(unused_variables)]
    fn post_transmit(&mut self, now: Instant, len: usize) {}
}

This is similar to quinn. https://github.com/quinn-rs/quinn/blob/0.8.0/quinn-proto/src/congestion.rs

I know theory of BBR, but I cannot evaluate it sufficiently because it requires emulation of delays.

ytakano commented 4 months ago

I think we shouldn't add this with non-additive features. I think it's OK to have an enum of all possible congestion control methods, with some sensible default selection, but still allowing two crates that both use smoltcp to compile without needing to coordinate their feature sets.

Right. A general enum type of congestion control can be prepared. I implemented it before, but discarded. I will fix it later.

Thank you for your review.

whitequark commented 4 months ago

To expand a bit, I'm thinking that the enum could include all of the necessary state, therefore making tcp::Socket as much bigger as the biggest possible to choose congestion controller (0 bytes if none are enabled).

The way to create it would probably be methods on the CongestionController enum, whose fields are all private. Then a Default implementation would pick the "best" one available if several are enabled. (The decision on which congestion controller is the "best" one is probably contentious but I don't expect it to matter that much since most people will pick one.)

Once this PR is in, we should strongly recommend picking a congestion controller in the README, to be a good netizen.

ytakano commented 4 months ago

I have defined set_congestion_control() and get_congestion_control() methods for tcp::Socket as follows.

pub enum CongestionControl {
    None,
    Cubic,
    Reno,
}

impl tcp::Scoket {
    /// Set an algorithm for congestion control.
    ///
    /// The default setting is `CongestionControl::None`, indicating that no congestion control is applied.
    /// Options `CongestionControl::Cubic` and `CongestionControl::Reno` are also available.
    ///
    /// `CongestionControl::Cubic` represents a modern congestion control algorithm designed to
    /// be more efficient and fair compared to `CongestionControl::Reno`.
    /// It is the default choice for Linux, Windows, and macOS.
    ///
    /// `CongestionControl::Reno` is a classic congestion control algorithm valued for its simplicity.
    /// Despite having a lower algorithmic complexity than `Cubic`,
    /// it is less efficient in terms of bandwidth usage.
    pub fn set_congestion_control(&mut self, congestion_control: CongestionControl) { /* omitted */ }

    /// Return the current congestion control algorithm.
    pub fn get_congestion_control(&self) -> CongestionControl { /* omitted */ }
}

CongestionController enum, whose fields are all private.

Variants of a public enum cannot be private. So, I have defined these 2 methods and CongestionControl enum to tcp::Socket.

If the best performance is required, I think it should be a type parameter of tcp::Socket.

pub struct Socket<'a, CC: CongestionController> {
    congestion_controller: CC
}

However, it is less compatible. See https://github.com/smoltcp-rs/smoltcp/pull/907#discussion_r1493859449

If it is a type parameter, the required bytes of the empty congestion controller must be 0 as follows.

let socket = tcp::Socket<EmptyCongestionController>();
whitequark commented 4 months ago

Yeah it's not actually possible to use a type parameter here. A enum is perfectly fine.

ytakano commented 4 months ago

Now we can test Cubic and Reno by specifying the features as follows.

$ cargo test --features=socket-tcp-reno
$ cargo test --features=socket-tcp-cubic
whitequark commented 3 months ago

Can you squash everything into one commit please?

ytakano commented 3 months ago

Squashed.

whitequark commented 3 months ago

I think this includes some unrelated commits?

ytakano commented 3 months ago

Sorry. How about this?

whitequark commented 3 months ago

Thanks!

whitequark commented 3 months ago

@Dirbaio Once this is merged I think we should strongly recommend that Reno be selected unless there is an extremely good reason not to use any congestion control.