Skip to content

Commit

Permalink
feat: DPLPMTUD (#1903)
Browse files Browse the repository at this point in the history
* WIP

* Fixes

* Minimize diff

* Progress

* Fix clippy

* Reduce diff to main

* Use RefCell

* Make Pacer use PmtudState

* Renamings

* Fix tests broken by changing PATH_MTU_V6

* WIP

* Finalize

* Update neqo-transport/src/path.rs

Co-authored-by: Martin Thomson <mt@lowentropy.net>
Signed-off-by: Lars Eggert <lars@eggert.org>

* Address comments

* Update neqo-transport/src/pmtud.rs

Co-authored-by: Martin Thomson <mt@lowentropy.net>
Signed-off-by: Lars Eggert <lars@eggert.org>

* Remove PmtudRef from ClassicCongestionControl

* Disable PMTUD by default, except for demo client and server, and simulator

* Fix clippy

* Make Pmtud part of Pacer only

* These are now `const_assert`s

* Cleanups

* Address more comments

* Cleanups

* Add some initial tests

* Fix clippy

* Minimize diff

* Probe with non-padding data

* Search table based on TMA paper

* Deal with PMTU reductions

* Fix crypto invocation limits

* Fix comment

* More comments

* Add PMTU_RAISE_TIMER

* Lost PMTUD probes do not elicit a congestion control reaction

* Update pacer when MTU changes

* Better way to update pacer

* Fix last commit

* Potential fix for bench

* Update neqo-transport/src/pmtud.rs

Co-authored-by: Max Inden <mail@max-inden.de>
Signed-off-by: Lars Eggert <lars@eggert.org>

* Update neqo-transport/src/pmtud.rs

Co-authored-by: Max Inden <mail@max-inden.de>
Signed-off-by: Lars Eggert <lars@eggert.org>

* Update neqo-transport/src/pmtud.rs

Co-authored-by: Max Inden <mail@max-inden.de>
Signed-off-by: Lars Eggert <lars@eggert.org>

* Update neqo-transport/src/cc/classic_cc.rs

Co-authored-by: Max Inden <mail@max-inden.de>
Signed-off-by: Lars Eggert <lars@eggert.org>

* Undo

* Simplifications

* rustfmt

* Disarm raise timer when it fired

* test script that triggers the bug

* Revert test.sh (modulo bug fix)

* Increase coverage

* Better test

* Another test

* let Some(...) instead of testing

Co-authored-by: Max Inden <mail@max-inden.de>
Signed-off-by: Martin Thomson <mt@lowentropy.net>

* Fix

* static_assertions to dev-dependencies

* Panic on failure

* Fixes after merge

* Make test-only plpmtu() panic on error

* set_confirmed

* invocations_base -> largest_packet_len

* Simplify

* Set builder limit in output_path()

* fmt

* A bunch of changes based on Martin's review

* Avoid spurious PMTUD restarts better. Possible perf. optimizations.

* Improve coverage

* Enable PMTUD for simulator

* Update neqo-transport/src/connection/mod.rs

Co-authored-by: Max Inden <mail@max-inden.de>
Signed-off-by: Lars Eggert <lars@eggert.org>

* Update neqo-transport/src/connection/mod.rs

Co-authored-by: Max Inden <mail@max-inden.de>
Signed-off-by: Lars Eggert <lars@eggert.org>

* Update neqo-transport/src/connection/mod.rs

Co-authored-by: Max Inden <mail@max-inden.de>
Signed-off-by: Lars Eggert <lars@eggert.org>

* Update neqo-transport/src/packet/mod.rs

Co-authored-by: Max Inden <mail@max-inden.de>
Signed-off-by: Lars Eggert <lars@eggert.org>

* Update neqo-transport/src/stats.rs

Co-authored-by: Max Inden <mail@max-inden.de>
Signed-off-by: Lars Eggert <lars@eggert.org>

* Suggestions from Max

* Update neqo-transport/src/pmtud.rs

Co-authored-by: Martin Thomson <mt@lowentropy.net>
Signed-off-by: Lars Eggert <lars@eggert.org>

* Suggestions from Martin

* More suggestions

* Add TODO

* Use filter fn

Follow-up on #1903 (comment)

* clippy

* Fixes

* doc fix

* refactor(pmtud): implement Copy for Probe

`Probe` is a small simple enum on the stack, thus convention is to implement
`Copy` instead of only `Clone` with a call to `clone()`.

The following helped me in the past:

> When should my type be Copy?
>
> Generally speaking, if your type can implement Copy, it should. Keep in mind,
> though, that implementing Copy is part of the public API of your type. If the
> type might become non-Copy in the future, it could be prudent to omit the Copy
> implementation now, to avoid a breaking API change.

https://doc.rust-lang.org/std/marker/trait.Copy.html#when-should-my-type-be-copy

* Make search_tables identical length, and deal with the fallout

* More

---------

Signed-off-by: Lars Eggert <lars@eggert.org>
Signed-off-by: Martin Thomson <mt@lowentropy.net>
Co-authored-by: Martin Thomson <mt@lowentropy.net>
Co-authored-by: Max Inden <mail@max-inden.de>
  • Loading branch information
3 people authored Jul 10, 2024
1 parent 7d610ed commit 4852dc6
Show file tree
Hide file tree
Showing 32 changed files with 1,382 additions and 354 deletions.
8 changes: 7 additions & 1 deletion neqo-bin/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,10 @@ pub struct QuicParameters {
/// Whether to disable pacing.
pub no_pacing: bool,

#[arg(long)]
/// Whether to disable path MTU discovery.
pub no_pmtud: bool,

#[arg(name = "preferred-address-v4", long)]
/// An IPv4 address for the server preferred address.
pub preferred_address_v4: Option<String>,
Expand All @@ -137,6 +141,7 @@ impl Default for QuicParameters {
idle_timeout: 30,
congestion_control: CongestionControlAlgorithm::NewReno,
no_pacing: false,
no_pmtud: false,
preferred_address_v4: None,
preferred_address_v6: None,
}
Expand Down Expand Up @@ -203,7 +208,8 @@ impl QuicParameters {
.max_streams(StreamType::UniDi, self.max_streams_uni)
.idle_timeout(Duration::from_secs(self.idle_timeout))
.cc_algorithm(self.congestion_control)
.pacing(!self.no_pacing);
.pacing(!self.no_pacing)
.pmtud(!self.no_pmtud);

if let Some(&first) = self.quic_version.first() {
let all = if self.quic_version[1..].contains(&first) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use std::{cell::RefCell, rc::Rc, time::Duration};

use neqo_common::event::Provider;
use neqo_crypto::AuthenticationStatus;
use neqo_transport::{ConnectionParameters, StreamId, StreamType, MIN_INITIAL_PACKET_SIZE};
use neqo_transport::{ConnectionParameters, Pmtud, StreamId, StreamType};
use test_fixture::{
anti_replay, fixture_init, now, CountingConnectionIdGenerator, DEFAULT_ADDR, DEFAULT_ALPN_H3,
DEFAULT_KEYS, DEFAULT_SERVER_NAME,
Expand All @@ -25,7 +25,8 @@ use crate::{
WebTransportServerEvent, WebTransportSessionAcceptAction,
};

const DATAGRAM_SIZE: u64 = MIN_INITIAL_PACKET_SIZE as u64;
// Leave space for large QUIC header.
const DATAGRAM_SIZE: u64 = Pmtud::default_plpmtu(DEFAULT_ADDR.ip()) as u64 - 40;

pub fn wt_default_parameters() -> Http3Parameters {
Http3Parameters::default()
Expand Down
1 change: 1 addition & 0 deletions neqo-transport/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ neqo-common = { path = "../neqo-common" }
neqo-crypto = { path = "../neqo-crypto" }
qlog = { workspace = true }
smallvec = { version = "1.11", default-features = false }
static_assertions = { version = "1.1", default-features = false }

[dev-dependencies]
criterion = { version = "0.5", default-features = false, features = ["html_reports"] }
Expand Down
Loading

0 comments on commit 4852dc6

Please sign in to comment.