From 4a9033fba203c01ecf53990d2477153deb3492d0 Mon Sep 17 00:00:00 2001 From: Wesley Rosenblum Date: Fri, 20 Sep 2024 17:15:26 -0700 Subject: [PATCH 1/7] feat(s2n-quic-dc): update MTU on dc path when MTU is updated --- dc/s2n-quic-dc/src/path/secret/map.rs | 30 +++++++--- dc/s2n-quic-dc/src/stream/endpoint.rs | 7 ++- dc/s2n-quic-dc/src/stream/send/state.rs | 7 ++- quic/s2n-quic-core/src/dc.rs | 20 ++++++- quic/s2n-quic-core/src/dc/disabled.rs | 4 ++ quic/s2n-quic-core/src/dc/testing.rs | 16 ++++- quic/s2n-quic-core/src/dc/traits.rs | 10 ++++ quic/s2n-quic-core/src/path/mtu.rs | 43 ++++++++++--- quic/s2n-quic-core/src/path/mtu/tests.rs | 60 +++++++++++++------ quic/s2n-quic-transport/src/dc/manager.rs | 5 ++ .../src/recovery/manager.rs | 46 ++++++++------ .../src/recovery/manager/tests.rs | 6 ++ .../src/space/application.rs | 4 ++ .../s2n-quic-transport/src/space/handshake.rs | 2 + quic/s2n-quic-transport/src/space/initial.rs | 2 + 15 files changed, 198 insertions(+), 64 deletions(-) diff --git a/dc/s2n-quic-dc/src/path/secret/map.rs b/dc/s2n-quic-dc/src/path/secret/map.rs index 792bbacad8..63fd48cbed 100644 --- a/dc/s2n-quic-dc/src/path/secret/map.rs +++ b/dc/s2n-quic-dc/src/path/secret/map.rs @@ -338,7 +338,7 @@ impl Map { state.mark_live(self.state.cleaner.epoch()); let (sealer, credentials) = state.uni_sealer(); - Some((sealer, credentials, state.parameters)) + Some((sealer, credentials, state.parameters.clone())) } pub fn open_once( @@ -362,7 +362,7 @@ impl Map { let keys = state.bidi_local(features); - Some((keys, state.parameters)) + Some((keys, state.parameters.clone())) } pub fn pair_for_credentials( @@ -373,7 +373,7 @@ impl Map { ) -> Option<(Bidirectional, ApplicationParams)> { let state = self.pre_authentication(credentials, control_out)?; - let params = state.parameters; + let params = state.parameters.clone(); let keys = state.bidi_remote(self.clone(), credentials, features); Some((keys, params)) @@ -696,13 +696,15 @@ impl Entry { secret: schedule::Secret, sender: sender::State, receiver: receiver::State, - mut parameters: ApplicationParams, + parameters: ApplicationParams, rehandshake_time: Duration, ) -> Self { // clamp max datagram size to a well-known value - parameters.max_datagram_size = parameters - .max_datagram_size - .min(crate::stream::MAX_DATAGRAM_SIZE as _); + let max_datagram_size = parameters.max_datagram_size.load(Ordering::Relaxed); + parameters.max_datagram_size.store( + max_datagram_size.min(crate::stream::MAX_DATAGRAM_SIZE as _), + Ordering::Relaxed, + ); assert!(rehandshake_time.as_secs() <= u32::MAX as u64); Self { @@ -941,7 +943,7 @@ impl HandshakingPath { Self { peer: connection_info.remote_address.clone().into(), dc_version: connection_info.dc_version, - parameters: connection_info.application_params, + parameters: connection_info.application_params.clone(), endpoint_type, secret: None, map, @@ -1035,12 +1037,22 @@ impl dc::Path for HandshakingPath { .expect("peer tokens are only received after secrets are ready"), sender, receiver, - self.parameters, + self.parameters.clone(), self.map.state.rehandshake_period, ); let entry = Arc::new(entry); self.map.insert(entry); } + + fn on_mtu_updated(&mut self, mtu: u16) { + let peers_guard = self.map.state.peers.guard(); + if let Some(entry) = self.map.state.peers.get(&self.peer, &peers_guard) { + entry + .parameters + .max_datagram_size + .store(mtu, Ordering::Relaxed); + } + } } #[cfg(test)] diff --git a/dc/s2n-quic-dc/src/stream/endpoint.rs b/dc/s2n-quic-dc/src/stream/endpoint.rs index 8d617b458c..df5e44ad4c 100644 --- a/dc/s2n-quic-dc/src/stream/endpoint.rs +++ b/dc/s2n-quic-dc/src/stream/endpoint.rs @@ -19,7 +19,10 @@ use s2n_quic_core::{ inet::{ExplicitCongestionNotification, SocketAddress}, varint::VarInt, }; -use std::{io, sync::Arc}; +use std::{ + io, + sync::{atomic::Ordering, Arc}, +}; use tracing::{debug_span, Instrument as _}; type Result = core::result::Result; @@ -193,7 +196,7 @@ where let flow = flow::non_blocking::State::new(flow_offset); let path = send::path::Info { - max_datagram_size: parameters.max_datagram_size, + max_datagram_size: parameters.max_datagram_size.load(Ordering::Relaxed), send_quantum, ecn: ExplicitCongestionNotification::Ect0, next_expected_control_packet: VarInt::ZERO, diff --git a/dc/s2n-quic-dc/src/stream/send/state.rs b/dc/s2n-quic-dc/src/stream/send/state.rs index 166a10af19..5b109098f7 100644 --- a/dc/s2n-quic-dc/src/stream/send/state.rs +++ b/dc/s2n-quic-dc/src/stream/send/state.rs @@ -42,7 +42,10 @@ use s2n_quic_core::{ varint::VarInt, }; use slotmap::SlotMap; -use std::collections::{BinaryHeap, VecDeque}; +use std::{ + collections::{BinaryHeap, VecDeque}, + sync::atomic::Ordering, +}; use tracing::{debug, trace}; pub mod probe; @@ -118,7 +121,7 @@ pub struct PeerActivity { impl State { #[inline] pub fn new(stream_id: stream::Id, params: &ApplicationParams) -> Self { - let max_datagram_size = params.max_datagram_size; + let max_datagram_size = params.max_datagram_size.load(Ordering::Relaxed); let initial_max_data = params.remote_max_data; let local_max_data = params.local_send_max_data; diff --git a/quic/s2n-quic-core/src/dc.rs b/quic/s2n-quic-core/src/dc.rs index 14a88c5884..df6a967610 100644 --- a/quic/s2n-quic-core/src/dc.rs +++ b/quic/s2n-quic-core/src/dc.rs @@ -12,6 +12,7 @@ use crate::{ varint::VarInt, }; use core::time::Duration; +use std::sync::atomic::{AtomicU16, Ordering}; mod disabled; mod traits; @@ -91,10 +92,10 @@ impl<'a> DatagramInfo<'a> { } /// Various settings relevant to the dc path -#[derive(Clone, Copy, Debug)] +#[derive(Debug)] #[non_exhaustive] pub struct ApplicationParams { - pub max_datagram_size: u16, + pub max_datagram_size: AtomicU16, pub remote_max_data: VarInt, pub local_send_max_data: VarInt, pub local_recv_max_data: VarInt, @@ -102,6 +103,19 @@ pub struct ApplicationParams { pub max_ack_delay: Duration, } +impl Clone for ApplicationParams { + fn clone(&self) -> Self { + Self { + max_datagram_size: AtomicU16::new(self.max_datagram_size.load(Ordering::Relaxed)), + remote_max_data: Default::default(), + local_send_max_data: Default::default(), + local_recv_max_data: Default::default(), + max_idle_timeout: None, + max_ack_delay: Default::default(), + } + } +} + impl ApplicationParams { pub fn new( max_datagram_size: u16, @@ -109,7 +123,7 @@ impl ApplicationParams { limits: &Limits, ) -> Self { Self { - max_datagram_size, + max_datagram_size: AtomicU16::new(max_datagram_size), remote_max_data: peer_flow_control_limits.max_data, local_send_max_data: limits.initial_stream_limits().max_data_bidi_local, local_recv_max_data: limits.initial_stream_limits().max_data_bidi_remote, diff --git a/quic/s2n-quic-core/src/dc/disabled.rs b/quic/s2n-quic-core/src/dc/disabled.rs index f5a132f777..e3650b1281 100644 --- a/quic/s2n-quic-core/src/dc/disabled.rs +++ b/quic/s2n-quic-core/src/dc/disabled.rs @@ -44,4 +44,8 @@ impl Path for () { ) { unimplemented!() } + + fn on_mtu_updated(&mut self, _mtu: u16) { + unimplemented!() + } } diff --git a/quic/s2n-quic-core/src/dc/testing.rs b/quic/s2n-quic-core/src/dc/testing.rs index 2fea792b56..64204afb5e 100644 --- a/quic/s2n-quic-core/src/dc/testing.rs +++ b/quic/s2n-quic-core/src/dc/testing.rs @@ -10,7 +10,7 @@ use crate::{ }; use core::time::Duration; use std::sync::{ - atomic::{AtomicU8, Ordering}, + atomic::{AtomicU16, AtomicU8, Ordering}, Arc, }; @@ -36,14 +36,19 @@ pub struct MockDcPath { pub on_peer_stateless_reset_tokens_count: u8, pub stateless_reset_tokens: Vec, pub peer_stateless_reset_tokens: Vec, + pub mtu: u16, } impl dc::Endpoint for MockDcEndpoint { type Path = MockDcPath; - fn new_path(&mut self, _connection_info: &ConnectionInfo) -> Option { + fn new_path(&mut self, connection_info: &ConnectionInfo) -> Option { Some(MockDcPath { stateless_reset_tokens: self.stateless_reset_tokens.clone(), + mtu: connection_info + .application_params + .max_datagram_size + .load(Ordering::Relaxed), ..Default::default() }) } @@ -76,10 +81,15 @@ impl dc::Path for MockDcPath { self.peer_stateless_reset_tokens .extend(stateless_reset_tokens); } + + fn on_mtu_updated(&mut self, mtu: u16) { + self.mtu = mtu + } } +#[allow(clippy::declare_interior_mutable_const)] pub const TEST_APPLICATION_PARAMS: ApplicationParams = ApplicationParams { - max_datagram_size: 1472, + max_datagram_size: AtomicU16::new(1472), remote_max_data: VarInt::from_u32(1u32 << 25), local_send_max_data: VarInt::from_u32(1u32 << 25), local_recv_max_data: VarInt::from_u32(1u32 << 25), diff --git a/quic/s2n-quic-core/src/dc/traits.rs b/quic/s2n-quic-core/src/dc/traits.rs index 8dab48b969..198c723017 100644 --- a/quic/s2n-quic-core/src/dc/traits.rs +++ b/quic/s2n-quic-core/src/dc/traits.rs @@ -45,6 +45,9 @@ pub trait Path: 'static + Send { &mut self, stateless_reset_tokens: impl Iterator, ); + + /// Called when the MTU has been updated for the path + fn on_mtu_updated(&mut self, mtu: u16); } impl Path for Option

{ @@ -69,4 +72,11 @@ impl Path for Option

{ path.on_peer_stateless_reset_tokens(stateless_reset_tokens) } } + + #[inline] + fn on_mtu_updated(&mut self, max_datagram_size: u16) { + if let Some(path) = self { + path.on_mtu_updated(max_datagram_size) + } + } } diff --git a/quic/s2n-quic-core/src/path/mtu.rs b/quic/s2n-quic-core/src/path/mtu.rs index c77f43af44..171e01c1a2 100644 --- a/quic/s2n-quic-core/src/path/mtu.rs +++ b/quic/s2n-quic-core/src/path/mtu.rs @@ -472,6 +472,12 @@ impl Builder { } } +#[derive(Eq, PartialEq, Debug)] +pub enum MtuResult { + NoChange, + MtuUpdated(u16), +} + #[derive(Clone, Debug)] pub struct Controller { state: State, @@ -608,7 +614,7 @@ impl Controller { congestion_controller: &mut CC, path_id: path::Id, publisher: &mut Pub, - ) { + ) -> MtuResult { 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 @@ -629,10 +635,13 @@ impl Controller { } // no need to process anything in the disabled state - ensure!(self.state != State::Disabled); + ensure!(self.state != State::Disabled, MtuResult::NoChange); // MTU probes are only sent in application data space - ensure!(packet_number.space().is_application_data()); + ensure!( + packet_number.space().is_application_data(), + MtuResult::NoChange + ); if sent_bytes >= self.plpmtu && self @@ -671,8 +680,12 @@ impl Controller { cause: MtuUpdatedCause::ProbeAcknowledged, search_complete: self.state.is_search_complete(), }); + + return MtuResult::MtuUpdated(self.plpmtu); } } + + MtuResult::NoChange } //= https://www.rfc-editor.org/rfc/rfc8899#section-3 @@ -690,12 +703,13 @@ impl Controller { congestion_controller: &mut CC, path_id: path::Id, publisher: &mut Pub, - ) { + ) -> MtuResult { // 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 if an early search has been requested ensure!( - self.state.is_early_search_requested() || packet_number.space().is_application_data() + self.state.is_early_search_requested() || packet_number.space().is_application_data(), + MtuResult::NoChange ); match &self.state { @@ -725,7 +739,9 @@ impl Controller { mtu: self.plpmtu, cause: MtuUpdatedCause::InitialMtuPacketLost, search_complete: self.state.is_search_complete(), - }) + }); + + return MtuResult::MtuUpdated(self.plpmtu); } State::Searching(probe_pn, _) if *probe_pn == packet_number => { // The MTU probe was lost @@ -763,10 +779,17 @@ impl Controller { } if self.black_hole_counter > BLACK_HOLE_THRESHOLD { - self.on_black_hole_detected(now, congestion_controller, path_id, publisher); + return self.on_black_hole_detected( + now, + congestion_controller, + path_id, + publisher, + ); } } } + + MtuResult::NoChange } /// Gets the currently validated maximum QUIC datagram size @@ -837,7 +860,7 @@ impl Controller { congestion_controller: &mut CC, path_id: path::Id, publisher: &mut Pub, - ) { + ) -> MtuResult { self.black_hole_counter = Default::default(); self.largest_acked_mtu_sized_packet = None; // Reset the plpmtu back to the base_plpmtu and notify the congestion controller @@ -856,7 +879,9 @@ impl Controller { mtu: self.plpmtu, cause: MtuUpdatedCause::Blackhole, search_complete: self.state.is_search_complete(), - }) + }); + + MtuResult::MtuUpdated(self.plpmtu) } /// Arm the PMTU Raise Timer if there is still room to increase the diff --git a/quic/s2n-quic-core/src/path/mtu/tests.rs b/quic/s2n-quic-core/src/path/mtu/tests.rs index ce34b593cc..886f28a0a0 100644 --- a/quic/s2n-quic-core/src/path/mtu/tests.rs +++ b/quic/s2n-quic-core/src/path/mtu/tests.rs @@ -394,13 +394,14 @@ fn on_packet_ack_within_threshold() { controller.probed_size = MINIMUM_MAX_DATAGRAM_SIZE; controller.max_probe_size = MINIMUM_MAX_DATAGRAM_SIZE + PROBE_THRESHOLD * 2 - 1; - controller.on_packet_ack( + let result = controller.on_packet_ack( pn, MINIMUM_MAX_DATAGRAM_SIZE, &mut cc, path::Id::test_id(), &mut publisher, ); + assert_eq!(MtuResult::MtuUpdated(MINIMUM_MAX_DATAGRAM_SIZE), result); assert_eq!( MINIMUM_MAX_DATAGRAM_SIZE + (max_udp_payload - MINIMUM_MAX_DATAGRAM_SIZE) / 2, @@ -437,14 +438,16 @@ fn on_packet_ack_within_threshold_of_max_plpmtu() { let mut publisher = Publisher::snapshot(); controller.state = State::Searching(pn, now); - controller.on_packet_ack( + let probed_sized = controller.probed_size; + let result = controller.on_packet_ack( pn, - controller.probed_size, + probed_sized, &mut cc, path::Id::test_id(), &mut publisher, ); + assert_eq!(MtuResult::MtuUpdated(probed_sized), result); assert_eq!(1472 + (max_udp_payload - 1472) / 2, controller.probed_size); assert_eq!(1, cc.on_mtu_update); assert_eq!(State::SearchComplete, controller.state); @@ -472,14 +475,16 @@ fn on_packet_ack_search_requested() { let mut publisher = Publisher::snapshot(); controller.state = State::Searching(pn, now); - controller.on_packet_ack( + let probed_size = controller.probed_size; + let result = controller.on_packet_ack( pn, - controller.probed_size, + probed_size, &mut cc, path::Id::test_id(), &mut publisher, ); + assert_eq!(MtuResult::MtuUpdated(probed_size), result); assert_eq!(1472 + (max_udp_payload - 1472) / 2, controller.probed_size); assert_eq!(1, cc.on_mtu_update); assert_eq!(State::SearchRequested, controller.state); @@ -497,24 +502,26 @@ fn on_packet_ack_resets_black_hole_counter() { controller.black_hole_counter += 1; // ack a packet smaller than the plpmtu - controller.on_packet_ack( + let result = controller.on_packet_ack( pnum, controller.plpmtu - 1, &mut cc, path::Id::test_id(), &mut publisher, ); + assert_eq!(MtuResult::NoChange, result); assert_eq!(controller.black_hole_counter, 1); assert_eq!(None, controller.largest_acked_mtu_sized_packet); // ack a packet the size of the plpmtu - controller.on_packet_ack( + let result = controller.on_packet_ack( pnum, controller.plpmtu, &mut cc, path::Id::test_id(), &mut publisher, ); + assert_eq!(MtuResult::NoChange, result); assert_eq!(controller.black_hole_counter, 0); assert_eq!(Some(pnum), controller.largest_acked_mtu_sized_packet); @@ -522,13 +529,14 @@ fn on_packet_ack_resets_black_hole_counter() { // ack an older packet let pnum_2 = pn(2); - controller.on_packet_ack( + let result = controller.on_packet_ack( pnum_2, controller.plpmtu, &mut cc, path::Id::test_id(), &mut publisher, ); + assert_eq!(MtuResult::NoChange, result); assert_eq!(controller.black_hole_counter, 1); assert_eq!(Some(pnum), controller.largest_acked_mtu_sized_packet); } @@ -544,7 +552,7 @@ fn on_packet_ack_disabled_controller() { controller.largest_acked_mtu_sized_packet = Some(pnum); let pn = pn(10); - controller.on_packet_ack( + let result = controller.on_packet_ack( pn, controller.plpmtu, &mut cc, @@ -552,6 +560,7 @@ fn on_packet_ack_disabled_controller() { &mut publisher, ); + assert_eq!(MtuResult::NoChange, result); assert_eq!(State::Disabled, controller.state); assert_eq!(controller.black_hole_counter, 1); assert_eq!(Some(pnum), controller.largest_acked_mtu_sized_packet); @@ -571,13 +580,14 @@ fn on_packet_ack_not_application_space() { // on_packet_ack will be called with packet numbers from Initial and Handshake space, // so it should not fail in this scenario. let pn = PacketNumberSpace::Handshake.new_packet_number(VarInt::from_u8(10)); - controller.on_packet_ack( + let result = controller.on_packet_ack( pn, controller.plpmtu, &mut cc, path::Id::test_id(), &mut publisher, ); + assert_eq!(MtuResult::NoChange, result); assert_eq!(controller.black_hole_counter, 1); assert_eq!(Some(pnum), controller.largest_acked_mtu_sized_packet); } @@ -598,7 +608,7 @@ fn on_packet_loss() { controller.state = State::Searching(pn, now); let probed_size = controller.probed_size; - controller.on_packet_loss( + let result = controller.on_packet_loss( pn, controller.probed_size, false, @@ -608,6 +618,7 @@ fn on_packet_loss() { &mut publisher, ); + assert_eq!(MtuResult::NoChange, result); assert_eq!(0, cc.on_mtu_update); assert_eq!(max_udp_payload, controller.max_probe_size); assert_eq!(probed_size, controller.probed_size); @@ -626,7 +637,7 @@ fn on_packet_loss_max_probes() { controller.probe_count = MAX_PROBES; assert_eq!(max_udp_payload, controller.max_probe_size); - controller.on_packet_loss( + let result = controller.on_packet_loss( pn, controller.probed_size, false, @@ -636,6 +647,7 @@ fn on_packet_loss_max_probes() { &mut publisher, ); + assert_eq!(MtuResult::NoChange, result); assert_eq!(0, cc.on_mtu_update); assert_eq!(1472, controller.max_probe_size); assert_eq!( @@ -659,7 +671,7 @@ fn on_packet_loss_black_hole() { let pn = pn(i as usize); // Losing a packet the size of the BASE_PLPMTU should not increase the black_hole_counter - controller.on_packet_loss( + let result = controller.on_packet_loss( pn, base_plpmtu, true, @@ -668,10 +680,11 @@ fn on_packet_loss_black_hole() { path::Id::test_id(), &mut publisher, ); + assert_eq!(MtuResult::NoChange, result); assert_eq!(controller.black_hole_counter, i); // Losing a packet larger than the PLPMTU should not increase the black_hole_counter - controller.on_packet_loss( + let result = controller.on_packet_loss( pn, controller.plpmtu + 1, true, @@ -680,10 +693,11 @@ fn on_packet_loss_black_hole() { path::Id::test_id(), &mut publisher, ); + assert_eq!(MtuResult::NoChange, result); assert_eq!(controller.black_hole_counter, i); // Losing a packet that does not start a new loss burst should not increase the black_hole_counter - controller.on_packet_loss( + let result = controller.on_packet_loss( pn, base_plpmtu + 1, false, @@ -692,9 +706,10 @@ fn on_packet_loss_black_hole() { path::Id::test_id(), &mut publisher, ); + assert_eq!(MtuResult::NoChange, result); assert_eq!(controller.black_hole_counter, i); - controller.on_packet_loss( + let result = controller.on_packet_loss( pn, base_plpmtu + 1, true, @@ -704,7 +719,10 @@ fn on_packet_loss_black_hole() { &mut publisher, ); if i < BLACK_HOLE_THRESHOLD { + assert_eq!(MtuResult::NoChange, result); assert_eq!(controller.black_hole_counter, i + 1); + } else { + assert_eq!(MtuResult::MtuUpdated(MINIMUM_MAX_DATAGRAM_SIZE), result); } } @@ -731,7 +749,7 @@ fn on_packet_loss_disabled_controller() { for i in 0..BLACK_HOLE_THRESHOLD + 1 { let pn = pn(i as usize); assert_eq!(controller.black_hole_counter, 0); - controller.on_packet_loss( + let result = controller.on_packet_loss( pn, base_plpmtu + 1, false, @@ -740,6 +758,7 @@ fn on_packet_loss_disabled_controller() { path::Id::test_id(), &mut publisher, ); + assert_eq!(MtuResult::NoChange, result); } assert_eq!(State::Disabled, controller.state); @@ -766,7 +785,7 @@ fn on_packet_loss_not_application_space() { // on_packet_loss may be called with packet numbers from Initial and Handshake space // so it should not fail in this scenario. let pn = PacketNumberSpace::Initial.new_packet_number(VarInt::from_u8(i)); - controller.on_packet_loss( + let result = controller.on_packet_loss( pn, base_plpmtu + 1, false, @@ -775,6 +794,7 @@ fn on_packet_loss_not_application_space() { path::Id::test_id(), &mut publisher, ); + assert_eq!(MtuResult::NoChange, result); assert_eq!(controller.black_hole_counter, 0); assert_eq!(0, cc.on_mtu_update); } @@ -832,7 +852,7 @@ fn on_packet_loss_initial_mtu_configured() { let mut cc = CongestionController::default(); let now = now(); - controller.on_packet_loss( + let result = controller.on_packet_loss( pn, original_plpmtu, false, @@ -844,6 +864,7 @@ fn on_packet_loss_initial_mtu_configured() { if original_plpmtu > base_plpmtu { // the MTU was updated + assert_eq!(MtuResult::MtuUpdated(base_plpmtu), result); assert_eq!( 1, cc.on_mtu_update, "base {} init {} max {} original_plpmtu {}, base_plpmtu {}", @@ -852,6 +873,7 @@ fn on_packet_loss_initial_mtu_configured() { assert_eq!(base_plpmtu, controller.plpmtu); } else { // everything remains the same since we are operating at the base plpmtu + assert_eq!(MtuResult::NoChange, result); assert_eq!(0, cc.on_mtu_update); assert_eq!(original_plpmtu, controller.plpmtu); } diff --git a/quic/s2n-quic-transport/src/dc/manager.rs b/quic/s2n-quic-transport/src/dc/manager.rs index 3ff55219a7..2384cc879c 100644 --- a/quic/s2n-quic-transport/src/dc/manager.rs +++ b/quic/s2n-quic-transport/src/dc/manager.rs @@ -186,6 +186,11 @@ impl Manager { self.stateless_reset_token_sync.on_packet_loss(ack_set); } + /// Called when the MTU of the path has changed + pub fn on_mtu_updated(&mut self, max_datagram_size: u16) { + self.path.on_mtu_updated(max_datagram_size) + } + #[cfg(any(test, feature = "testing"))] pub fn path(&self) -> &<::DcEndpoint as Endpoint>::Path { self.path.as_ref().expect("path should be specified") diff --git a/quic/s2n-quic-transport/src/recovery/manager.rs b/quic/s2n-quic-transport/src/recovery/manager.rs index acf698f737..84cda84d32 100644 --- a/quic/s2n-quic-transport/src/recovery/manager.rs +++ b/quic/s2n-quic-transport/src/recovery/manager.rs @@ -98,7 +98,7 @@ macro_rules! recovery_event { } pub(crate) use recovery_event; -use s2n_quic_core::recovery::loss; +use s2n_quic_core::{path::mtu::MtuResult, recovery::loss}; // Since `SentPacketInfo` is generic over a type supplied by the Congestion Controller implementation, // the type definition is particularly lengthy, especially since rust requires the fully-qualified @@ -537,15 +537,20 @@ impl Manager { includes_ack_eliciting |= acked_packet_info.ack_elicitation.is_ack_eliciting(); let path = context.path_mut_by_id(acked_packet_info.path_id); - path.mtu_controller.on_packet_ack( + path.ecn_controller + .on_packet_ack(acked_packet_info.time_sent, acked_packet_info.ecn); + match path.mtu_controller.on_packet_ack( packet_number, acked_packet_info.sent_bytes, &mut path.congestion_controller, acked_packet_info.path_id, publisher, - ); - path.ecn_controller - .on_packet_ack(acked_packet_info.time_sent, acked_packet_info.ecn); + ) { + MtuResult::NoChange => {} + MtuResult::MtuUpdated(max_datagram_size) => { + context.on_mtu_update(max_datagram_size) + } + } } if let Some((start, end)) = newly_acked_range { @@ -998,18 +1003,6 @@ impl Manager { is_mtu_probe: sent_info.transmission_mode.is_mtu_probing(), }); - // Notify the MTU controller of packet loss even if it wasn't a probe since it uses - // that information for blackhole detection. - path.mtu_controller.on_packet_loss( - packet_number, - sent_info.sent_bytes, - new_loss_burst, - now, - &mut path.congestion_controller, - sent_info.path_id, - publisher, - ); - let path_id = sent_info.path_id; // Notify the ECN controller of packet loss for blackhole detection. @@ -1028,6 +1021,23 @@ impl Manager { path.rtt_estimator.on_persistent_congestion(); } + // Notify the MTU controller of packet loss even if it wasn't a probe since it uses + // that information for blackhole detection. + match path.mtu_controller.on_packet_loss( + packet_number, + sent_info.sent_bytes, + new_loss_burst, + now, + &mut path.congestion_controller, + sent_info.path_id, + publisher, + ) { + MtuResult::NoChange => {} + MtuResult::MtuUpdated(max_datagram_size) => { + context.on_mtu_update(max_datagram_size) + } + } + prev_lost_packet_number = Some(packet_number); } @@ -1136,6 +1146,8 @@ pub trait Context { publisher: &mut Pub, ); fn on_rtt_update(&mut self, now: Timestamp); + + fn on_mtu_update(&mut self, max_datagram_size: u16); } impl transmission::interest::Provider for Manager { diff --git a/quic/s2n-quic-transport/src/recovery/manager/tests.rs b/quic/s2n-quic-transport/src/recovery/manager/tests.rs index 5d70c7b2cc..66ccd8d996 100644 --- a/quic/s2n-quic-transport/src/recovery/manager/tests.rs +++ b/quic/s2n-quic-transport/src/recovery/manager/tests.rs @@ -3471,6 +3471,7 @@ struct MockContext<'a, Config: endpoint::Config> { on_packet_ack_count: u8, on_packet_loss_count: u8, on_rtt_update_count: u8, + on_mtu_update_count: u8, path_id: path::Id, lost_packets: HashSet, path_manager: &'a mut path::Manager, @@ -3485,6 +3486,7 @@ impl<'a, Config: endpoint::Config> MockContext<'a, Config> { on_packet_ack_count: 0, on_packet_loss_count: 0, on_rtt_update_count: 0, + on_mtu_update_count: 0, path_id: path_manager.active_path_id(), lost_packets: HashSet::default(), path_manager, @@ -3571,4 +3573,8 @@ impl<'a, Config: endpoint::Config> recovery::Context for MockContext<'a, fn on_rtt_update(&mut self, _now: Timestamp) { self.on_rtt_update_count += 1; } + + fn on_mtu_update(&mut self, _max_datagram_size: u16) { + self.on_mtu_update_count += 1; + } } diff --git a/quic/s2n-quic-transport/src/space/application.rs b/quic/s2n-quic-transport/src/space/application.rs index 64c853d471..4fbe3fbb54 100644 --- a/quic/s2n-quic-transport/src/space/application.rs +++ b/quic/s2n-quic-transport/src/space/application.rs @@ -811,6 +811,10 @@ impl<'a, Config: endpoint::Config> recovery::Context for RecoveryContext .on_rtt_update(&self.path_manager.active_path().rtt_estimator, now) } } + + fn on_mtu_update(&mut self, max_datagram_size: u16) { + self.dc_manager.on_mtu_updated(max_datagram_size) + } } impl PacketSpace for ApplicationSpace { diff --git a/quic/s2n-quic-transport/src/space/handshake.rs b/quic/s2n-quic-transport/src/space/handshake.rs index 3d8534fd52..1a2772bc95 100644 --- a/quic/s2n-quic-transport/src/space/handshake.rs +++ b/quic/s2n-quic-transport/src/space/handshake.rs @@ -500,6 +500,8 @@ impl<'a, Config: endpoint::Config> recovery::Context for RecoveryContext } fn on_rtt_update(&mut self, _now: Timestamp) {} + + fn on_mtu_update(&mut self, _mtu: u16) {} } //= https://www.rfc-editor.org/rfc/rfc9000#section-17.2.4 diff --git a/quic/s2n-quic-transport/src/space/initial.rs b/quic/s2n-quic-transport/src/space/initial.rs index de451729c1..67b5649e2a 100644 --- a/quic/s2n-quic-transport/src/space/initial.rs +++ b/quic/s2n-quic-transport/src/space/initial.rs @@ -624,6 +624,8 @@ impl<'a, Config: endpoint::Config> recovery::Context for RecoveryContext } fn on_rtt_update(&mut self, _now: Timestamp) {} + + fn on_mtu_update(&mut self, _mtu: u16) {} } //= https://www.rfc-editor.org/rfc/rfc9000#section-17.2.2 From 40d927d90120fc400f2ab00e86eeb56c7c56c313 Mon Sep 17 00:00:00 2001 From: Wesley Rosenblum Date: Fri, 20 Sep 2024 17:21:23 -0700 Subject: [PATCH 2/7] remove std --- quic/s2n-quic-core/src/dc.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/quic/s2n-quic-core/src/dc.rs b/quic/s2n-quic-core/src/dc.rs index df6a967610..675c161c92 100644 --- a/quic/s2n-quic-core/src/dc.rs +++ b/quic/s2n-quic-core/src/dc.rs @@ -11,8 +11,10 @@ use crate::{ transport::parameters::{DcSupportedVersions, InitialFlowControlLimits}, varint::VarInt, }; -use core::time::Duration; -use std::sync::atomic::{AtomicU16, Ordering}; +use core::{ + sync::atomic::{AtomicU16, Ordering}, + time::Duration, +}; mod disabled; mod traits; From 104c90d362bcec7f19ea797d05d2f61a91e727a5 Mon Sep 17 00:00:00 2001 From: Wesley Rosenblum Date: Mon, 30 Sep 2024 18:51:51 -0700 Subject: [PATCH 3/7] add tests and update map --- dc/s2n-quic-dc/src/path/secret/map.rs | 3 +- .../src/recovery/manager.rs | 4 +- ...d_remove_lost_packets_early_mtu_probe.snap | 6 + ..._new_acked_packets_call_on_mtu_update.snap | 7 + .../src/recovery/manager/tests.rs | 145 +++++++++++++++++- 5 files changed, 160 insertions(+), 5 deletions(-) create mode 100644 quic/s2n-quic-transport/src/recovery/manager/snapshots/quic__s2n-quic-transport__src__recovery__manager__tests__events__detect_and_remove_lost_packets_early_mtu_probe.snap create mode 100644 quic/s2n-quic-transport/src/recovery/manager/snapshots/quic__s2n-quic-transport__src__recovery__manager__tests__events__process_new_acked_packets_call_on_mtu_update.snap diff --git a/dc/s2n-quic-dc/src/path/secret/map.rs b/dc/s2n-quic-dc/src/path/secret/map.rs index ec39c746da..79b95f0c8c 100644 --- a/dc/s2n-quic-dc/src/path/secret/map.rs +++ b/dc/s2n-quic-dc/src/path/secret/map.rs @@ -1048,8 +1048,7 @@ impl dc::Path for HandshakingPath { } fn on_mtu_updated(&mut self, mtu: u16) { - let peers_guard = self.map.state.peers.guard(); - if let Some(entry) = self.map.state.peers.get(&self.peer, &peers_guard) { + if let Some(entry) = self.map.state.peers.get_by_key(&self.peer) { entry .parameters .max_datagram_size diff --git a/quic/s2n-quic-transport/src/recovery/manager.rs b/quic/s2n-quic-transport/src/recovery/manager.rs index 84cda84d32..ee29595884 100644 --- a/quic/s2n-quic-transport/src/recovery/manager.rs +++ b/quic/s2n-quic-transport/src/recovery/manager.rs @@ -546,10 +546,10 @@ impl Manager { acked_packet_info.path_id, publisher, ) { - MtuResult::NoChange => {} MtuResult::MtuUpdated(max_datagram_size) => { context.on_mtu_update(max_datagram_size) } + MtuResult::NoChange => {} } } @@ -1032,10 +1032,10 @@ impl Manager { sent_info.path_id, publisher, ) { - MtuResult::NoChange => {} MtuResult::MtuUpdated(max_datagram_size) => { context.on_mtu_update(max_datagram_size) } + MtuResult::NoChange => {} } prev_lost_packet_number = Some(packet_number); diff --git a/quic/s2n-quic-transport/src/recovery/manager/snapshots/quic__s2n-quic-transport__src__recovery__manager__tests__events__detect_and_remove_lost_packets_early_mtu_probe.snap b/quic/s2n-quic-transport/src/recovery/manager/snapshots/quic__s2n-quic-transport__src__recovery__manager__tests__events__detect_and_remove_lost_packets_early_mtu_probe.snap new file mode 100644 index 0000000000..010d0d5746 --- /dev/null +++ b/quic/s2n-quic-transport/src/recovery/manager/snapshots/quic__s2n-quic-transport__src__recovery__manager__tests__events__detect_and_remove_lost_packets_early_mtu_probe.snap @@ -0,0 +1,6 @@ +--- +source: quic/s2n-quic-transport/src/recovery/manager/tests.rs +expression: "" +--- +PacketLost { packet_header: OneRtt { number: 2 }, path: Path { local_addr: 0.0.0.0:0, local_cid: 0x4c6f63616c4900000000000000004c6f63616c49, remote_addr: 0.0.0.0:0, remote_cid: 0x5065657249640000000000000000506565724964, id: 0, is_active: true }, bytes_lost: 1472, is_mtu_probe: true } +MtuUpdated { path_id: 0, mtu: 1200, cause: InitialMtuPacketLost, search_complete: false } diff --git a/quic/s2n-quic-transport/src/recovery/manager/snapshots/quic__s2n-quic-transport__src__recovery__manager__tests__events__process_new_acked_packets_call_on_mtu_update.snap b/quic/s2n-quic-transport/src/recovery/manager/snapshots/quic__s2n-quic-transport__src__recovery__manager__tests__events__process_new_acked_packets_call_on_mtu_update.snap new file mode 100644 index 0000000000..fd5c4ac067 --- /dev/null +++ b/quic/s2n-quic-transport/src/recovery/manager/snapshots/quic__s2n-quic-transport__src__recovery__manager__tests__events__process_new_acked_packets_call_on_mtu_update.snap @@ -0,0 +1,7 @@ +--- +source: quic/s2n-quic-transport/src/recovery/manager/tests.rs +expression: "" +--- +AckRangeReceived { packet_header: OneRtt { number: 0 }, path: Path { local_addr: 0.0.0.0:0, local_cid: 0x4c6f63616c4900000000000000004c6f63616c49, remote_addr: 0.0.0.0:0, remote_cid: 0x5065657249640000000000000000506565724964, id: 0, is_active: true }, ack_range: 0..=0 } +MtuUpdated { path_id: 0, mtu: 1472, cause: ProbeAcknowledged, search_complete: true } +RecoveryMetrics { path: Path { local_addr: 0.0.0.0:0, local_cid: 0x4c6f63616c4900000000000000004c6f63616c49, remote_addr: 0.0.0.0:0, remote_cid: 0x5065657249640000000000000000506565724964, id: 0, is_active: true }, min_rtt: 10ms, smoothed_rtt: 10ms, latest_rtt: 10ms, rtt_variance: 5ms, max_ack_delay: 10ms, pto_count: 0, congestion_window: 15000, bytes_in_flight: 1472, congestion_limited: false } diff --git a/quic/s2n-quic-transport/src/recovery/manager/tests.rs b/quic/s2n-quic-transport/src/recovery/manager/tests.rs index 66ccd8d996..adddbb33c8 100644 --- a/quic/s2n-quic-transport/src/recovery/manager/tests.rs +++ b/quic/s2n-quic-transport/src/recovery/manager/tests.rs @@ -6,6 +6,7 @@ use crate::{ connection::{ limits::ANTI_AMPLIFICATION_MULTIPLIER, ConnectionIdMapper, InternalConnectionIdGenerator, }, + contexts::testing::MockWriteContext, endpoint::{ self, testing::{Client as ClientConfig, Server as ServerConfig}, @@ -31,7 +32,7 @@ use s2n_quic_core::{ RttEstimator, DEFAULT_INITIAL_RTT, K_GRANULARITY, }, time::{clock::testing as time, testing::now, Clock, NoopClock}, - transmission::Outcome, + transmission::{writer::testing::OutgoingFrameBuffer, Outcome}, varint::VarInt, }; use std::{collections::HashSet, net::SocketAddr}; @@ -370,6 +371,7 @@ fn on_ack_frame() { Duration::from_millis(500) ); assert_eq!(1, context.on_rtt_update_count); + assert_eq!(0, context.on_mtu_update_count); // Reset the pto backoff to 2 so we can tell if it was reset context.path_mut().pto_backoff = 2; @@ -403,6 +405,7 @@ fn on_ack_frame() { Duration::from_millis(500) ); assert_eq!(1, context.on_rtt_update_count); + assert_eq!(0, context.on_mtu_update_count); // Ack packets 7 to 9 (4 - 6 will be considered lost) let ack_receive_time = ack_receive_time + Duration::from_secs(1); @@ -429,6 +432,7 @@ fn on_ack_frame() { Duration::from_millis(2500) ); assert_eq!(2, context.on_rtt_update_count); + assert_eq!(0, context.on_mtu_update_count); // Ack packet 10, but with a path that is not peer validated let path_id = unsafe { path::Id::new(0) }; @@ -464,6 +468,7 @@ fn on_ack_frame() { Duration::from_millis(3000) ); assert_eq!(3, context.on_rtt_update_count); + assert_eq!(0, context.on_mtu_update_count); // Send and ack a non ack eliciting packet manager.on_packet_sent( @@ -502,6 +507,7 @@ fn on_ack_frame() { Duration::from_millis(3000) ); assert_eq!(3, context.on_rtt_update_count); + assert_eq!(0, context.on_mtu_update_count); } // Test that receiving an invalid ack frame still allows for `on_timeout` @@ -975,6 +981,68 @@ fn process_new_acked_packets_pto_timer() { assert!(manager.pto.is_armed()); } +// a newly acked packet that updates the MTU should result in the +// on_mtu_update method on the recovery context being invoked +#[test] +fn process_new_acked_packets_call_on_mtu_update() { + let space = PacketNumberSpace::ApplicationData; + let mut manager = ServerManager::new(space); + let mut path_manager = helper_generate_path_manager(Duration::from_millis(10)); + let mut context = MockContext::new(&mut path_manager); + let mut publisher = Publisher::snapshot(); + + let time_sent = time::now() + Duration::from_secs(10); + + context.path_mut().mtu_controller.enable(); + let probed_size = context.path().mtu_controller.probed_sized(); + let mut frame_buffer = OutgoingFrameBuffer::new(); + // Set max packet size so the PING+PADDING that the mtu controller writes creates one packet + frame_buffer.set_max_packet_size(Some(1500)); + let mut write_context = MockWriteContext::new( + now(), + &mut frame_buffer, + s2n_quic_core::transmission::Constraint::None, + s2n_quic_core::transmission::Mode::MtuProbing, + endpoint::Type::Client, + ); + let packet_number = write_context.packet_number(); + context + .path_mut() + .mtu_controller + .on_transmit(&mut write_context); + + // Send the MTU probe + manager.on_packet_sent( + packet_number, + transmission::Outcome { + ack_elicitation: AckElicitation::Eliciting, + is_congestion_controlled: true, + bytes_sent: probed_size, + bytes_progressed: 0, + }, + time_sent, + ExplicitCongestionNotification::default(), + transmission::Mode::MtuProbing, + None, + &mut context, + &mut publisher, + ); + + assert_eq!(0, context.on_mtu_update_count); + + // Ack the packet + let ack_receive_time = time_sent + Duration::from_millis(10); + ack_packets( + packet_number.as_u64()..=packet_number.as_u64(), + ack_receive_time, + &mut context, + &mut manager, + None, + &mut publisher, + ); + assert_eq!(1, context.on_mtu_update_count); +} + // Test that the PTO timer is armed after a non-congestion controlled // packet is acked #[test] @@ -1984,6 +2052,7 @@ fn detect_and_remove_lost_packets_nothing_lost() { assert_eq!(context.lost_packets.len(), 0); assert_eq!(context.path().congestion_controller.lost_bytes, 0); assert_eq!(context.path().congestion_controller.on_packets_lost, 0); + assert_eq!(context.on_mtu_update_count, 0); } //= https://www.rfc-editor.org/rfc/rfc9000#section-14.4 @@ -2042,6 +2111,80 @@ fn detect_and_remove_lost_packets_mtu_probe() { assert_eq!(context.path().congestion_controller.lost_bytes, 0); assert_eq!(context.path().congestion_controller.on_packets_lost, 0); assert_eq!(context.path().congestion_controller.bytes_in_flight, 0); + assert_eq!(context.on_mtu_update_count, 0); +} + +// when the MTU is updated due to an early MTU probe being lost +// the on_mtu_update method on the recovery context is invoked +#[test] +fn detect_and_remove_lost_packets_early_mtu_probe() { + let space = PacketNumberSpace::ApplicationData; + let mut manager = ServerManager::new(space); + let mut random_generator = random::testing::Generator(123); + let registry = ConnectionIdMapper::new(&mut random_generator, endpoint::Type::Server) + .create_server_peer_id_registry( + InternalConnectionIdGenerator::new().generate_id(), + connection::PeerId::TEST_ID, + true, + ); + let mut rtt_estimator = RttEstimator::default(); + rtt_estimator.on_max_ack_delay(Duration::from_millis(10).try_into().unwrap()); + + // Configure the mtu controller with an initial MTU + let path = Path::new( + Default::default(), + connection::PeerId::TEST_ID, + connection::LocalId::TEST_ID, + rtt_estimator, + MockCongestionController::new(Default::default()), + true, + mtu::Config::builder() + .with_initial_mtu(1500) + .unwrap() + .with_max_mtu(1500) + .unwrap() + .build() + .unwrap(), + ANTI_AMPLIFICATION_MULTIPLIER, + ); + + let mut path_manager = path::Manager::new(path, registry); + + let ecn = ExplicitCongestionNotification::default(); + let mut context = MockContext::new(&mut path_manager); + manager.largest_acked_packet = Some(space.new_packet_number(VarInt::from_u8(10))); + let mut publisher = Publisher::snapshot(); + let random = &mut random::testing::Generator::default(); + let probed_size = context.path().mtu_controller.max_datagram_size(); + + let time_sent = time::now(); + let outcome = transmission::Outcome { + ack_elicitation: AckElicitation::Eliciting, + is_congestion_controlled: true, + bytes_sent: probed_size, + bytes_progressed: 0, + }; + + // Send the packet + let lost_packet = space.new_packet_number(VarInt::from_u8(2)); + manager.on_packet_sent( + lost_packet, + outcome, + time_sent, + ecn, + transmission::Mode::MtuProbing, + None, + &mut context, + &mut publisher, + ); + + assert_eq!(context.on_mtu_update_count, 0); + + manager.detect_and_remove_lost_packets(time_sent, random, &mut context, &mut publisher); + + // Verify the MTU update count increased + assert_eq!(context.lost_packets.len(), 1); + assert_eq!(context.on_mtu_update_count, 1); } #[test] From d1c2f7fb8cae66341f38f89bfa9bfc1463ce8d85 Mon Sep 17 00:00:00 2001 From: Wesley Rosenblum Date: Mon, 30 Sep 2024 19:40:52 -0700 Subject: [PATCH 4/7] typo --- tools/xdp/s2n-quic-xdp/src/ring.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/xdp/s2n-quic-xdp/src/ring.rs b/tools/xdp/s2n-quic-xdp/src/ring.rs index 1780e504c5..4197a4ee39 100644 --- a/tools/xdp/s2n-quic-xdp/src/ring.rs +++ b/tools/xdp/s2n-quic-xdp/src/ring.rs @@ -15,7 +15,7 @@ use std::{io, os::unix::io::AsRawFd}; struct Ring { cursor: Cursor, flags: NonNull, - // make the area clonable in test mode + // make the area cloneable in test mode #[cfg(test)] area: std::sync::Arc, #[cfg(not(test))] From 1ed14d1c161616a4b5fab075a14714874c54bfc0 Mon Sep 17 00:00:00 2001 From: Wesley Rosenblum Date: Tue, 1 Oct 2024 09:15:07 -0700 Subject: [PATCH 5/7] add test --- ...rc__dc__manager__tests__events__on_mtu_updated.snap | 5 +++++ quic/s2n-quic-transport/src/dc/manager/tests.rs | 10 ++++++++++ 2 files changed, 15 insertions(+) create mode 100644 quic/s2n-quic-transport/src/dc/manager/snapshots/quic__s2n-quic-transport__src__dc__manager__tests__events__on_mtu_updated.snap diff --git a/quic/s2n-quic-transport/src/dc/manager/snapshots/quic__s2n-quic-transport__src__dc__manager__tests__events__on_mtu_updated.snap b/quic/s2n-quic-transport/src/dc/manager/snapshots/quic__s2n-quic-transport__src__dc__manager__tests__events__on_mtu_updated.snap new file mode 100644 index 0000000000..b89174da85 --- /dev/null +++ b/quic/s2n-quic-transport/src/dc/manager/snapshots/quic__s2n-quic-transport__src__dc__manager__tests__events__on_mtu_updated.snap @@ -0,0 +1,5 @@ +--- +source: quic/s2n-quic-transport/src/dc/manager/tests.rs +expression: "" +--- +DcStateChanged { state: VersionNegotiated { version: 1 } } diff --git a/quic/s2n-quic-transport/src/dc/manager/tests.rs b/quic/s2n-quic-transport/src/dc/manager/tests.rs index 203e5cb0b4..90a6544f4b 100644 --- a/quic/s2n-quic-transport/src/dc/manager/tests.rs +++ b/quic/s2n-quic-transport/src/dc/manager/tests.rs @@ -268,6 +268,16 @@ fn on_packet_loss() { assert!(manager.has_transmission_interest()); } +#[test] +fn on_mtu_updated() { + let mut publisher = Publisher::snapshot(); + let path = MockDcPath::default(); + let mut manager: Manager = Manager::new(Some(path), 1, &mut publisher); + manager.on_mtu_updated(1500); + + assert_eq!(1500, manager.path().mtu); +} + #[test] #[cfg_attr(miri, ignore)] fn snapshots() { From ac3b2e52b491e8f8b2a3bb25e991ec4eaa44c84b Mon Sep 17 00:00:00 2001 From: Wesley Rosenblum Date: Tue, 1 Oct 2024 16:10:53 -0700 Subject: [PATCH 6/7] add clone test --- quic/s2n-quic-core/src/dc.rs | 52 +++++++++++++++++++++++++++++++++++- 1 file changed, 51 insertions(+), 1 deletion(-) diff --git a/quic/s2n-quic-core/src/dc.rs b/quic/s2n-quic-core/src/dc.rs index 153551d492..70c83d5b75 100644 --- a/quic/s2n-quic-core/src/dc.rs +++ b/quic/s2n-quic-core/src/dc.rs @@ -12,8 +12,9 @@ use crate::{ varint::VarInt, }; use core::{ - num::NonZeroU32, time::Duration, + num::NonZeroU32, sync::atomic::{AtomicU16, Ordering}, + time::Duration, }; mod disabled; @@ -140,3 +141,52 @@ impl ApplicationParams { Some(Duration::from_millis(self.max_idle_timeout?.get() as u64)) } } + +#[cfg(test)] +mod tests { + use crate::{ + connection::Limits, dc::ApplicationParams, transport::parameters::InitialFlowControlLimits, + varint::VarInt, + }; + use std::{sync::atomic::Ordering, time::Duration}; + + #[test] + fn clone() { + let initial_flow_control_limits = InitialFlowControlLimits { + max_data: VarInt::from_u32(2222), + ..Default::default() + }; + + let limits = Limits { + bidirectional_local_data_window: 1234.try_into().unwrap(), + bidirectional_remote_data_window: 6789.try_into().unwrap(), + max_idle_timeout: Duration::from_millis(999).try_into().unwrap(), + ..Default::default() + }; + + let params = ApplicationParams::new(9000, &initial_flow_control_limits, &limits); + + assert_eq!(9000, params.max_datagram_size.load(Ordering::Relaxed)); + assert_eq!(limits.max_idle_timeout(), params.max_idle_timeout()); + assert_eq!(1234, params.local_send_max_data.as_u64()); + assert_eq!(6789, params.local_recv_max_data.as_u64()); + assert_eq!(2222, params.remote_max_data.as_u64()); + + let cloned_params = params.clone(); + + assert_eq!( + params.max_datagram_size.load(Ordering::Relaxed), + cloned_params.max_datagram_size.load(Ordering::Relaxed) + ); + assert_eq!(params.max_idle_timeout, cloned_params.max_idle_timeout); + assert_eq!( + params.local_send_max_data, + cloned_params.local_send_max_data + ); + assert_eq!( + params.local_recv_max_data, + cloned_params.local_recv_max_data + ); + assert_eq!(params.remote_max_data, cloned_params.remote_max_data); + } +} From 5c4125fb6721159020440df73d3bb2b0a269b678 Mon Sep 17 00:00:00 2001 From: Wesley Rosenblum Date: Wed, 2 Oct 2024 14:08:40 -0700 Subject: [PATCH 7/7] use fetch_min --- dc/s2n-quic-dc/src/path/secret/map.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/dc/s2n-quic-dc/src/path/secret/map.rs b/dc/s2n-quic-dc/src/path/secret/map.rs index 06e5bf31f8..ed51f190c8 100644 --- a/dc/s2n-quic-dc/src/path/secret/map.rs +++ b/dc/s2n-quic-dc/src/path/secret/map.rs @@ -688,11 +688,9 @@ impl Entry { rehandshake_time: Duration, ) -> Self { // clamp max datagram size to a well-known value - let max_datagram_size = parameters.max_datagram_size.load(Ordering::Relaxed); - parameters.max_datagram_size.store( - max_datagram_size.min(crate::stream::MAX_DATAGRAM_SIZE as _), - Ordering::Relaxed, - ); + parameters + .max_datagram_size + .fetch_min(crate::stream::MAX_DATAGRAM_SIZE as _, Ordering::Relaxed); assert!(rehandshake_time.as_secs() <= u32::MAX as u64); Self {