Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(s2n-quic-events): add search_completed boolean to mtu updated event #2322

Merged
merged 2 commits into from
Sep 19, 2024
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
25 changes: 22 additions & 3 deletions quic/s2n-quic-core/src/event/generated.rs
Original file line number Diff line number Diff line change
Expand Up @@ -581,6 +581,12 @@ pub mod api {
#[non_exhaustive]
#[doc = " An early packet using the configured InitialMtu was lost"]
InitialMtuPacketLost {},
#[non_exhaustive]
#[doc = " An early packet using the configured InitialMtu was acknowledged by the peer"]
InitialMtuPacketAcknowledged {},
#[non_exhaustive]
#[doc = " MTU probes larger than the current MTU were not acknowledged"]
LargerProbesLost {},
}
#[derive(Clone, Debug)]
#[non_exhaustive]
Expand Down Expand Up @@ -998,12 +1004,14 @@ pub mod api {
}
#[derive(Clone, Debug)]
#[non_exhaustive]
#[doc = " The maximum transmission unit (MTU) for the path has changed"]
#[doc = " The maximum transmission unit (MTU) and/or MTU probing status for the path has changed"]
pub struct MtuUpdated {
pub path_id: u64,
#[doc = " The maximum QUIC datagram size, not including UDP and IP headers"]
pub mtu: u16,
pub cause: MtuUpdatedCause,
#[doc = " The search for the maximum MTU has completed for now"]
pub search_complete: bool,
}
impl Event for MtuUpdated {
const NAME: &'static str = "connectivity:mtu_updated";
Expand Down Expand Up @@ -2217,8 +2225,9 @@ pub mod tracing {
path_id,
mtu,
cause,
search_complete,
} = event;
tracing :: event ! (target : "mtu_updated" , parent : id , tracing :: Level :: DEBUG , path_id = tracing :: field :: debug (path_id) , mtu = tracing :: field :: debug (mtu) , cause = tracing :: field :: debug (cause));
tracing :: event ! (target : "mtu_updated" , parent : id , tracing :: Level :: DEBUG , path_id = tracing :: field :: debug (path_id) , mtu = tracing :: field :: debug (mtu) , cause = tracing :: field :: debug (cause) , search_complete = tracing :: field :: debug (search_complete));
}
#[inline]
fn on_slow_start_exited(
Expand Down Expand Up @@ -3521,6 +3530,10 @@ pub mod builder {
Blackhole,
#[doc = " An early packet using the configured InitialMtu was lost"]
InitialMtuPacketLost,
#[doc = " An early packet using the configured InitialMtu was acknowledged by the peer"]
InitialMtuPacketAcknowledged,
#[doc = " MTU probes larger than the current MTU were not acknowledged"]
LargerProbesLost,
}
impl IntoEvent<api::MtuUpdatedCause> for MtuUpdatedCause {
#[inline]
Expand All @@ -3531,6 +3544,8 @@ pub mod builder {
Self::ProbeAcknowledged => ProbeAcknowledged {},
Self::Blackhole => Blackhole {},
Self::InitialMtuPacketLost => InitialMtuPacketLost {},
Self::InitialMtuPacketAcknowledged => InitialMtuPacketAcknowledged {},
Self::LargerProbesLost => LargerProbesLost {},
}
}
}
Expand Down Expand Up @@ -4253,12 +4268,14 @@ pub mod builder {
}
}
#[derive(Clone, Debug)]
#[doc = " The maximum transmission unit (MTU) for the path has changed"]
#[doc = " The maximum transmission unit (MTU) and/or MTU probing status for the path has changed"]
pub struct MtuUpdated {
pub path_id: u64,
#[doc = " The maximum QUIC datagram size, not including UDP and IP headers"]
pub mtu: u16,
pub cause: MtuUpdatedCause,
#[doc = " The search for the maximum MTU has completed for now"]
pub search_complete: bool,
}
impl IntoEvent<api::MtuUpdated> for MtuUpdated {
#[inline]
Expand All @@ -4267,11 +4284,13 @@ pub mod builder {
path_id,
mtu,
cause,
search_complete,
} = self;
api::MtuUpdated {
path_id: path_id.into_event(),
mtu: mtu.into_event(),
cause: cause.into_event(),
search_complete: search_complete.into_event(),
}
}
}
Expand Down
136 changes: 107 additions & 29 deletions quic/s2n-quic-core/src/path/mtu.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,10 @@ pub mod testing {

#[derive(Clone, Debug, PartialEq, Eq)]
enum State {
/// EARLY_SEARCH_REQUESTED indicates the initial MTU was configured higher
/// than the base MTU, to allow for quick confirmation or rejection of the
/// initial MTU
EarlySearchRequested,
//= https://www.rfc-editor.org/rfc/rfc8899#section-5.2
//# The DISABLED state is the initial state before probing has started.
Disabled,
Expand All @@ -67,10 +71,20 @@ enum State {
}

impl State {
/// Returns true if the MTU controller is in the early search requested state
fn is_early_search_requested(&self) -> bool {
matches!(self, State::EarlySearchRequested)
}

/// Returns true if the MTU controller is in the disabled state
fn is_disabled(&self) -> bool {
matches!(self, State::Disabled)
}

/// Returns true if the MTU controller is in the search complete state
fn is_search_complete(&self) -> bool {
matches!(self, State::SearchComplete)
}
}

//= https://www.rfc-editor.org/rfc/rfc8899#section-5.1.2
Expand Down Expand Up @@ -536,8 +550,20 @@ impl Controller {
}
.min(max_udp_payload);

let state = if plpmtu > base_plpmtu {
// The initial MTU has been configured higher than the base MTU
State::EarlySearchRequested
} else if initial_probed_size - base_plpmtu < PROBE_THRESHOLD {
// The next probe size is within the probe threshold of the
// base MTU, so no probing will occur and the search is complete
State::SearchComplete
} else {
// Otherwise wait for regular MTU probing to be enabled
State::Disabled
};

Self {
state: State::Disabled,
state,
base_plpmtu,
plpmtu,
probed_size: initial_probed_size,
Expand All @@ -554,7 +580,7 @@ impl Controller {
#[inline]
pub fn enable(&mut self) {
// ensure we haven't already enabled the controller
ensure!(self.state == State::Disabled);
ensure!(self.state.is_disabled() || self.state.is_early_search_requested());

// TODO: Look up current MTU in a cache. If there is a cache hit
// move directly to SearchComplete and arm the PMTU raise timer.
Expand Down Expand Up @@ -583,6 +609,25 @@ impl Controller {
path_id: path::Id,
publisher: &mut Pub,
) {
if self.state.is_early_search_requested() && sent_bytes > self.base_plpmtu {
if self.is_next_probe_size_above_threshold() {
// Early probing has succeeded, but the max MTU is higher still so
// wait for regular MTU probing to be enabled to attempt higher MTUs
self.state = State::Disabled;
} else {
self.state = State::SearchComplete;
}

// Publish an `on_mtu_updated` event since the cause
// and possibly search_complete status have changed
publisher.on_mtu_updated(event::builder::MtuUpdated {
path_id: path_id.into_event(),
mtu: self.plpmtu,
cause: MtuUpdatedCause::InitialMtuPacketAcknowledged,
search_complete: self.state.is_search_complete(),
});
}

// no need to process anything in the disabled state
ensure!(self.state != State::Disabled);

Expand All @@ -609,12 +654,6 @@ impl Controller {
&mut congestion_controller::PathPublisher::new(publisher, path_id),
);

publisher.on_mtu_updated(event::builder::MtuUpdated {
path_id: path_id.into_event(),
mtu: self.plpmtu,
cause: MtuUpdatedCause::ProbeAcknowledged,
});

self.update_probed_size();

//= https://www.rfc-editor.org/rfc/rfc8899#section-8
Expand All @@ -625,6 +664,13 @@ impl Controller {
// Subsequent probe packets are sent based on the round trip transmission and
// acknowledgement/loss of a packet, so the interval will be at least 1 RTT.
self.request_new_search(Some(transmit_time));

publisher.on_mtu_updated(event::builder::MtuUpdated {
path_id: path_id.into_event(),
mtu: self.plpmtu,
cause: MtuUpdatedCause::ProbeAcknowledged,
search_complete: self.state.is_search_complete(),
});
}
}
}
Expand All @@ -647,28 +693,39 @@ impl Controller {
) {
// MTU probes are only sent in the application data space, but since early packet
// spaces will use the `InitialMtu` prior to MTU probing being enabled, we need
// to check for potentially MTU-related packet loss even when MTU probing is disabled
ensure!(self.state.is_disabled() || packet_number.space().is_application_data());
// to check for potentially MTU-related packet loss if an early search has been requested
ensure!(
self.state.is_early_search_requested() || packet_number.space().is_application_data()
);

match &self.state {
State::Disabled => {
if lost_bytes > self.base_plpmtu && self.plpmtu > self.base_plpmtu {
// MTU probing hasn't been enabled yet, but since the initial MTU was configured
// higher than the base PLPMTU and this setting resulted in a lost packet
// we drop back down to the base PLPMTU.
self.plpmtu = self.base_plpmtu;

congestion_controller.on_mtu_update(
self.plpmtu,
&mut congestion_controller::PathPublisher::new(publisher, path_id),
);

publisher.on_mtu_updated(event::builder::MtuUpdated {
path_id: path_id.into_event(),
mtu: self.plpmtu,
cause: MtuUpdatedCause::InitialMtuPacketLost,
})
State::Disabled => {}
State::EarlySearchRequested => {
// MTU probing hasn't been enabled yet, but since the initial MTU was configured
// higher than the base PLPMTU and this setting resulted in a lost packet
// we drop back down to the base PLPMTU.
self.plpmtu = self.base_plpmtu;

congestion_controller.on_mtu_update(
self.plpmtu,
&mut congestion_controller::PathPublisher::new(publisher, path_id),
);

if self.is_next_probe_size_above_threshold() {
// Resume regular probing when the MTU controller is enabled
self.state = State::Disabled;
} else {
// The next probe is within the threshold, so move directly
// to the SearchComplete state
self.state = State::SearchComplete;
}

publisher.on_mtu_updated(event::builder::MtuUpdated {
path_id: path_id.into_event(),
mtu: self.plpmtu,
cause: MtuUpdatedCause::InitialMtuPacketLost,
search_complete: self.state.is_search_complete(),
})
}
State::Searching(probe_pn, _) if *probe_pn == packet_number => {
// The MTU probe was lost
Expand All @@ -678,6 +735,16 @@ impl Controller {
self.max_probe_size = self.probed_size;
self.update_probed_size();
self.request_new_search(None);

if self.is_search_completed() {
// Emit an on_mtu_updated event as the search has now completed
publisher.on_mtu_updated(event::builder::MtuUpdated {
path_id: path_id.into_event(),
mtu: self.plpmtu,
cause: MtuUpdatedCause::LargerProbesLost,
search_complete: true,
})
}
} else {
// Try the same probe size again
self.state = State::SearchRequested
Expand Down Expand Up @@ -716,6 +783,11 @@ impl Controller {
self.probed_size as usize
}

/// Returns true if probing for the MTU has completed
pub fn is_search_completed(&self) -> bool {
self.state.is_search_complete()
}

/// Sets `probed_size` to the next MTU size to probe for based on a binary search
#[inline]
fn update_probed_size(&mut self) {
Expand All @@ -731,14 +803,19 @@ impl Controller {
current + ((max - current) / 2)
}

#[inline]
fn is_next_probe_size_above_threshold(&self) -> bool {
self.probed_size - self.plpmtu >= PROBE_THRESHOLD
}

/// Requests a new search to be initiated
///
/// If `last_probe_time` is supplied, the PMTU Raise Timer will be armed as
/// necessary if the probed_size is already within the PROBE_THRESHOLD
/// of the current PLPMTU
#[inline]
fn request_new_search(&mut self, last_probe_time: Option<Timestamp>) {
if self.probed_size - self.plpmtu >= PROBE_THRESHOLD {
if self.is_next_probe_size_above_threshold() {
self.probe_count = 0;
self.state = State::SearchRequested;
} else {
Expand Down Expand Up @@ -778,6 +855,7 @@ impl Controller {
path_id: path_id.into_event(),
mtu: self.plpmtu,
cause: MtuUpdatedCause::Blackhole,
search_complete: self.state.is_search_complete(),
})
}

Expand All @@ -789,7 +867,7 @@ impl Controller {
self.max_probe_size = self.max_udp_payload;
self.update_probed_size();

if self.probed_size - self.plpmtu >= PROBE_THRESHOLD {
if self.is_next_probe_size_above_threshold() {
// There is still some room to try a larger MTU again,
// so arm the pmtu raise timer
self.pmtu_raise_timer.set(timestamp);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,4 @@
source: quic/s2n-quic-core/src/path/mtu/tests.rs
expression: ""
---
MtuUpdated { path_id: 0, mtu: 1472, cause: ProbeAcknowledged }
MtuUpdated { path_id: 0, mtu: 1472, cause: ProbeAcknowledged, search_complete: false }
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,4 @@
source: quic/s2n-quic-core/src/path/mtu/tests.rs
expression: ""
---
MtuUpdated { path_id: 0, mtu: 1200, cause: ProbeAcknowledged }
MtuUpdated { path_id: 0, mtu: 1200, cause: ProbeAcknowledged, search_complete: true }
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,4 @@
source: quic/s2n-quic-core/src/path/mtu/tests.rs
expression: ""
---
MtuUpdated { path_id: 0, mtu: 1472, cause: ProbeAcknowledged }
MtuUpdated { path_id: 0, mtu: 1472, cause: ProbeAcknowledged, search_complete: true }
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,4 @@
source: quic/s2n-quic-core/src/path/mtu/tests.rs
expression: ""
---
MtuUpdated { path_id: 0, mtu: 1200, cause: Blackhole }
MtuUpdated { path_id: 0, mtu: 1200, cause: Blackhole, search_complete: true }
Loading
Loading