-
Notifications
You must be signed in to change notification settings - Fork 120
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-dc): update MTU on dc path when MTU is updated #2327
base: main
Are you sure you want to change the base?
Changes from 2 commits
4a9033f
40d927d
24c1b94
104c90d
d1c2f7f
1ed14d1
ea1322a
ac3b2e5
5c4125f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not convinced this is right. If we detect a new MTU with a new handshake, this is updating a ~random path secret's MTU -- not the one associated with that handshake. IMO we should treat the ApplicationParams as immutable after insertion into the map as much as possible. (In part, that's because it seems likely that we'll want to have a separate storage area for them and de-duplicate, since they're rather large, and this sort of thing will make that harder since the indirection/pointer then becomes mutable). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
it would be a random path secret, but its not a random path. The intention of this is to allow for higher MTUs even when there was some trouble confirming an MTU during the handshake. The MTU probing mechanism we have for the during the handshake is very conservative (to ensure the handshake can complete), if there is a single lost packet before the configured MTU can be confirmed, we drop down to the base MTU. This mechanism allows for us to continue with the more traditional MTU probing after we've already allowed the handshake to complete.
would it be feasible/better to re-insert into the map then? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Re-inserting is going to be problematic too -- at least currently that would panic! since you're inserting a duplicate path secret ID. I think we shoudl stick with this PR as-is, probably, but long-term we might want to have a separate(?) map for MTUs -- in particular I think we'd end up clamping to ~1200 byte MTUs or w/e for any in-progress handshakes, which feels a bit unfortunate. It feels true that we probably don't have any good testing in place for dynamically changing MTUs. OTOH, right now MTU is not yet used for anything I think, since it is only needed for dcQUIC streams over UDP, which aren't yet supported. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We use the MTU value to control TCP packet sizes, so there's a little less efficiency there. But we don't update MTUs mid-stream so only after the MTU probing is complete the flows would inherit the update. |
||
entry | ||
.parameters | ||
.max_datagram_size | ||
.store(mtu, Ordering::Relaxed); | ||
} | ||
} | ||
} | ||
|
||
#[cfg(test)] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,4 +44,8 @@ impl Path for () { | |
) { | ||
unimplemented!() | ||
} | ||
|
||
fn on_mtu_updated(&mut self, _mtu: u16) { | ||
unimplemented!() | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should use
fetch_min
: https://doc.rust-lang.org/std/sync/atomic/struct.AtomicU64.html#method.fetch_minThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good call, I could see Clippy catching this someday