Skip to content

Commit

Permalink
refactor: rename mtu to max_datagram_size (#2181)
Browse files Browse the repository at this point in the history
  • Loading branch information
WesleyRosenblum authored Apr 8, 2024
1 parent 5be4b37 commit 5f88e54
Show file tree
Hide file tree
Showing 9 changed files with 85 additions and 54 deletions.
2 changes: 2 additions & 0 deletions quic/s2n-quic-core/src/event/generated.rs
Original file line number Diff line number Diff line change
Expand Up @@ -973,6 +973,7 @@ pub mod api {
#[doc = " The maximum transmission unit (MTU) 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,
}
Expand Down Expand Up @@ -4134,6 +4135,7 @@ pub mod builder {
#[doc = " The maximum transmission unit (MTU) 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,
}
Expand Down
8 changes: 5 additions & 3 deletions quic/s2n-quic-core/src/path/mtu.rs
Original file line number Diff line number Diff line change
Expand Up @@ -581,9 +581,11 @@ impl Controller {
}
}

/// Gets the currently validated maximum transmission unit, not including IP or UDP header len
/// Gets the currently validated maximum QUIC datagram size
///
/// This does not include the size of UDP and IP headers.
#[inline]
pub fn mtu(&self) -> usize {
pub fn max_datagram_size(&self) -> usize {
self.plpmtu as usize
}

Expand All @@ -593,7 +595,7 @@ impl Controller {
self.max_mtu
}

/// Gets the MTU currently being probed for
/// Gets the max datagram size currently being probed for
#[inline]
pub fn probed_sized(&self) -> usize {
self.probed_size as usize
Expand Down
12 changes: 9 additions & 3 deletions quic/s2n-quic-core/src/path/mtu/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,10 @@ fn new_ipv4() {
1600 - UDP_HEADER_LEN - IPV4_MIN_HEADER_LEN,
controller.max_probe_size
);
assert_eq!(MINIMUM_MAX_DATAGRAM_SIZE as usize, controller.mtu());
assert_eq!(
MINIMUM_MAX_DATAGRAM_SIZE as usize,
controller.max_datagram_size()
);
assert_eq!(0, controller.probe_count);
assert_eq!(State::Disabled, controller.state);
assert!(!controller.pmtu_raise_timer.is_armed());
Expand Down Expand Up @@ -215,7 +218,10 @@ fn new_ipv6() {
2000 - UDP_HEADER_LEN - IPV6_MIN_HEADER_LEN,
controller.max_probe_size
);
assert_eq!(MINIMUM_MAX_DATAGRAM_SIZE as usize, controller.mtu());
assert_eq!(
MINIMUM_MAX_DATAGRAM_SIZE as usize,
controller.max_datagram_size()
);
assert_eq!(0, controller.probe_count);
assert_eq!(State::Disabled, controller.state);
assert!(!controller.pmtu_raise_timer.is_armed());
Expand Down Expand Up @@ -259,7 +265,7 @@ fn new_initial_and_base_mtu() {
);
assert_eq!(
(2500 - UDP_HEADER_LEN - IPV4_MIN_HEADER_LEN) as usize,
controller.mtu()
controller.max_datagram_size()
);
assert_eq!(0, controller.probe_count);
assert_eq!(State::Disabled, controller.state);
Expand Down
1 change: 1 addition & 0 deletions quic/s2n-quic-events/events/connection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,7 @@ pub struct KeepAliveTimerExpired {
/// The maximum transmission unit (MTU) for the path has changed
struct MtuUpdated {
path_id: u64,
/// The maximum QUIC datagram size, not including UDP and IP headers
mtu: u16,
cause: MtuUpdatedCause,
}
Expand Down
5 changes: 4 additions & 1 deletion quic/s2n-quic-transport/src/connection/connection_impl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -615,7 +615,10 @@ impl<Config: endpoint::Config> connection::Trait for ConnectionImpl<Config> {

publisher.on_mtu_updated(event::builder::MtuUpdated {
path_id: path_manager.active_path_id().into_event(),
mtu: path_manager.active_path().mtu_controller.mtu() as u16,
mtu: path_manager
.active_path()
.mtu_controller
.max_datagram_size() as u16,
cause: MtuUpdatedCause::NewPath,
});

Expand Down
16 changes: 10 additions & 6 deletions quic/s2n-quic-transport/src/connection/transmission.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,12 @@ impl<'a, 'sub, Config: endpoint::Config> tx::Message for ConnectionTransmission<

// If a packet can be GSO'd it means it's limited to the previously written packet
// size. We want to avoid sending several small packets and artificially clamping packets to
// less than an MTU.
segment_len >= self.context.path().mtu(self.context.transmission_mode)
// less than the max datagram size.
segment_len
>= self
.context
.path()
.max_datagram_size(self.context.transmission_mode)
}

#[inline]
Expand Down Expand Up @@ -135,18 +139,18 @@ impl<'a, 'sub, Config: endpoint::Config> tx::Message for ConnectionTransmission<
self.context.path().transmission_constraint()
};

let mtu = self
let max_datagram_size = self
.context
.path()
.clamp_mtu(buffer.len(), self.context.transmission_mode);
.clamp_datagram_size(buffer.len(), self.context.transmission_mode);
debug_assert_ne!(
mtu, 0,
max_datagram_size, 0,
"the amplification limit should be checked before trying to transmit"
);

// limit the number of retries to the MAX_BURST_PACKETS
for _ in 0..MAX_BURST_PACKETS {
let encoder = EncoderBuffer::new(&mut buffer[..mtu]);
let encoder = EncoderBuffer::new(&mut buffer[..max_datagram_size]);
let initial_capacity = encoder.capacity();

//= https://www.rfc-editor.org/rfc/rfc9002#section-6.2.4
Expand Down
2 changes: 1 addition & 1 deletion quic/s2n-quic-transport/src/path/manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -462,7 +462,7 @@ impl<Config: endpoint::Config> Manager<Config> {

publisher.on_mtu_updated(event::builder::MtuUpdated {
path_id: new_path_id.into_event(),
mtu: path.mtu_controller.mtu() as u16,
mtu: path.mtu_controller.max_datagram_size() as u16,
cause: MtuUpdatedCause::NewPath,
});

Expand Down
85 changes: 49 additions & 36 deletions quic/s2n-quic-transport/src/path/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ impl<Config: endpoint::Config> Path<Config> {
}

debug_assert_ne!(
self.clamp_mtu(bytes, transmission::Mode::Normal),
self.clamp_datagram_size(bytes, transmission::Mode::Normal),
0,
"path should not transmit when amplification limited; tried to transmit {bytes}"
);
Expand Down Expand Up @@ -409,20 +409,20 @@ impl<Config: endpoint::Config> Path<Config> {
}

#[inline]
pub fn mtu(&self, transmission_mode: transmission::Mode) -> usize {
pub fn max_datagram_size(&self, transmission_mode: transmission::Mode) -> usize {
match transmission_mode {
// Use the minimum MTU for loss recovery probes to allow detection of packets
// lost when the previously confirmed path MTU is no longer supported.
// Use the minimum max datagram size for loss recovery probes to allow detection of
// packets lost when the previously confirmed path MTU is no longer supported.
//
// The priority during PathValidationOnly is to validate the path, so the
// minimum max datagram size is used to avoid packet loss due to MTU limits.
Mode::LossRecoveryProbing | Mode::PathValidationOnly => {
MINIMUM_MAX_DATAGRAM_SIZE as usize
}
// When MTU Probing, clamp to the size of the MTU we are attempting to validate
// When MTU Probing, clamp to the max datagram size we are attempting to validate
Mode::MtuProbing => self.mtu_controller.probed_sized(),
// Otherwise use the confirmed MTU
Mode::Normal => self.mtu_controller.mtu(),
// Otherwise use the confirmed max datagram size
Mode::Normal => self.mtu_controller.max_datagram_size(),
}
}

Expand All @@ -441,19 +441,23 @@ impl<Config: endpoint::Config> Path<Config> {
//# packet) with a size at the PL that is larger than the current
//# PLPMTU.

/// Clamps payload sizes to the current MTU for the path
/// Clamps payload sizes to the current max datagram size for the path
///
/// # Panics
///
/// Panics if this is called when the path is amplification limited
#[inline]
pub fn clamp_mtu(&self, requested_size: usize, transmission_mode: transmission::Mode) -> usize {
pub fn clamp_datagram_size(
&self,
requested_size: usize,
transmission_mode: transmission::Mode,
) -> usize {
debug_assert!(
!self.at_amplification_limit(),
"amplification limits should be checked before clamping MTU values"
"amplification limits should be checked before clamping datagram size values"
);

requested_size.min(self.mtu(transmission_mode))
requested_size.min(self.max_datagram_size(transmission_mode))
}

#[inline]
Expand Down Expand Up @@ -522,18 +526,18 @@ impl<Config: endpoint::Config> Path<Config> {
self.mtu_controller.max_mtu()
}

/// Returns `true` if the congestion window does not have sufficient space for a packet of
/// size `mtu` considering the current bytes in flight and the additional `bytes_sent` provided
/// Returns `true` if the congestion window does not have sufficient space for a packet of the maximum
/// datagram size considering the current bytes in flight and the additional `bytes_sent` provided
#[inline]
pub fn is_congestion_limited(&self, bytes_sent: usize) -> bool {
let cwnd = self.congestion_controller.congestion_window();
let bytes_in_flight = self
.congestion_controller
.bytes_in_flight()
.saturating_add(bytes_sent as u32);
let mtu = self.mtu(transmission::Mode::Normal) as u32;
let max_datagram_size = self.max_datagram_size(transmission::Mode::Normal) as u32;

cwnd.saturating_sub(bytes_in_flight) < mtu
cwnd.saturating_sub(bytes_in_flight) < max_datagram_size
}

/// Compare a Path based on its PathHandle.
Expand Down Expand Up @@ -997,15 +1001,18 @@ mod tests {
] {
let mut path = testing::helper_path_server();
// Verify we can transmit up to the mtu
let mtu = path.mtu(transmission_mode);
let max_datagram_size = path.max_datagram_size(transmission_mode);

let amplification_outcome = path.on_bytes_received(3);
path.on_bytes_transmitted(8);

assert!(amplification_outcome.is_inactivate_path_unblocked());
assert_eq!(path.clamp_mtu(1, transmission_mode), 1);
assert_eq!(path.clamp_mtu(10, transmission_mode), 10);
assert_eq!(path.clamp_mtu(1800, transmission_mode), mtu);
assert_eq!(path.clamp_datagram_size(1, transmission_mode), 1);
assert_eq!(path.clamp_datagram_size(10, transmission_mode), 10);
assert_eq!(
path.clamp_datagram_size(1800, transmission_mode),
max_datagram_size
);

path.on_bytes_transmitted(1);
// Verify we can't transmit any more bytes
Expand All @@ -1014,13 +1021,16 @@ mod tests {
let amplification_outcome = path.on_bytes_received(1);
// Verify we can transmit up to 3 more bytes
assert!(amplification_outcome.is_inactivate_path_unblocked());
assert_eq!(path.clamp_mtu(1, transmission_mode), 1);
assert_eq!(path.clamp_mtu(10, transmission_mode), 10);
assert_eq!(path.clamp_mtu(1800, transmission_mode), mtu);
assert_eq!(path.clamp_datagram_size(1, transmission_mode), 1);
assert_eq!(path.clamp_datagram_size(10, transmission_mode), 10);
assert_eq!(
path.clamp_datagram_size(1800, transmission_mode),
max_datagram_size
);

path.on_validated();
// Validated paths should always be able to transmit
assert_eq!(path.clamp_mtu(4, transmission_mode), 4);
assert_eq!(path.clamp_datagram_size(4, transmission_mode), 4);
}
}

Expand All @@ -1033,20 +1043,20 @@ mod tests {
path.mtu_controller = mtu::testing::test_controller(mtu, probed_size);

assert_eq!(
path.mtu_controller.mtu(),
path.clamp_mtu(10000, transmission::Mode::Normal)
path.mtu_controller.max_datagram_size(),
path.clamp_datagram_size(10000, transmission::Mode::Normal)
);
assert_eq!(
MINIMUM_MAX_DATAGRAM_SIZE as usize,
path.clamp_mtu(10000, transmission::Mode::PathValidationOnly)
path.clamp_datagram_size(10000, transmission::Mode::PathValidationOnly)
);
assert_eq!(
MINIMUM_MAX_DATAGRAM_SIZE as usize,
path.clamp_mtu(10000, transmission::Mode::LossRecoveryProbing)
path.clamp_datagram_size(10000, transmission::Mode::LossRecoveryProbing)
);
assert_eq!(
path.mtu_controller.probed_sized(),
path.clamp_mtu(10000, transmission::Mode::MtuProbing)
path.clamp_datagram_size(10000, transmission::Mode::MtuProbing)
);
}

Expand All @@ -1061,20 +1071,20 @@ mod tests {
path.mtu_controller = mtu::testing::test_controller(mtu, probed_size);

assert_eq!(
path.mtu_controller.mtu(),
path.mtu(transmission::Mode::Normal)
path.mtu_controller.max_datagram_size(),
path.max_datagram_size(transmission::Mode::Normal)
);
assert_eq!(
MINIMUM_MAX_DATAGRAM_SIZE as usize,
path.mtu(transmission::Mode::PathValidationOnly)
path.max_datagram_size(transmission::Mode::PathValidationOnly)
);
assert_eq!(
MINIMUM_MAX_DATAGRAM_SIZE as usize,
path.mtu(transmission::Mode::LossRecoveryProbing)
path.max_datagram_size(transmission::Mode::LossRecoveryProbing)
);
assert_eq!(
path.mtu_controller.probed_sized(),
path.mtu(transmission::Mode::MtuProbing)
path.max_datagram_size(transmission::Mode::MtuProbing)
);
}

Expand All @@ -1086,7 +1096,10 @@ mod tests {
let probed_size = 1500;
path.mtu_controller = mtu::testing::test_controller(mtu, probed_size);

assert_eq!(0, path.clamp_mtu(10000, transmission::Mode::Normal));
assert_eq!(
0,
path.clamp_datagram_size(10000, transmission::Mode::Normal)
);
}

#[test]
Expand Down Expand Up @@ -1176,10 +1189,10 @@ mod tests {
#[test]
fn is_congestion_limited() {
let mut path = testing::helper_path_client();
let mtu = path.mtu_controller.mtu() as u32;
let max_datagram_size = path.mtu_controller.max_datagram_size() as u32;

path.congestion_controller.congestion_window = 12000;
path.congestion_controller.bytes_in_flight = 12000 - 500 - mtu;
path.congestion_controller.bytes_in_flight = 12000 - 500 - max_datagram_size;

// There is room for an MTU sized packet after including the 500 bytes, so the path is not congestion limited
assert!(!path.is_congestion_limited(500));
Expand Down
8 changes: 4 additions & 4 deletions quic/s2n-quic-transport/src/space/application.rs
Original file line number Diff line number Diff line change
Expand Up @@ -524,7 +524,7 @@ impl<Config: endpoint::Config> ApplicationSpace<Config> {

// bound the random value
let pkt_per_cwnd = path.congestion_controller.congestion_window()
/ path.mtu(transmission::Mode::Normal) as u32;
/ path.max_datagram_size(transmission::Mode::Normal) as u32;
let lower = pkt_per_cwnd / 2;
let upper = pkt_per_cwnd.saturating_mul(2);
let cardinality = upper - lower + 1;
Expand Down Expand Up @@ -1168,14 +1168,14 @@ mod tests {
let mut skip_counter = None;

let mut path = helper_path_server();
assert_eq!(path.mtu(transmission::Mode::Normal), 1200);
let mtu = path.mtu(transmission::Mode::Normal) as u32;
assert_eq!(path.max_datagram_size(transmission::Mode::Normal), 1200);
let max_datagram_size = path.max_datagram_size(transmission::Mode::Normal) as u32;

check!().with_type().cloned().for_each(|(seed, cwnd)| {
let random = &mut random::testing::Generator(seed);
path.congestion_controller.congestion_window = cwnd;
// calculate the bounds
let pkt_per_cwnd = cwnd / mtu;
let pkt_per_cwnd = cwnd / max_datagram_size;
let lower = pkt_per_cwnd / 2;
let upper = pkt_per_cwnd * 2;
ApplicationSpace::arm_skip_counter(&mut skip_counter, &path, random);
Expand Down

0 comments on commit 5f88e54

Please sign in to comment.