feat(s2n-quic-dc): update MTU on dc path when MTU is updated #2327
Conversation
|
|
||
| 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.
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.
this is updating a ~random path secret's MTU
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.
IMO we should treat the ApplicationParams as immutable after insertion into the map
would it be feasible/better to re-insert into the map then?
There was a problem hiding this comment.
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.
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.
…senblum/mtuupdate # Conflicts: # quic/s2n-quic-core/src/dc.rs
| self.map.state.rehandshake_period, | ||
| ); | ||
| let entry = Arc::new(entry); | ||
| self.map.insert(entry); | ||
| } | ||
|
|
||
| fn on_mtu_updated(&mut self, mtu: u16) { | ||
| if let Some(entry) = self.map.state.peers.get_by_key(&self.peer) { |
There was a problem hiding this comment.
I wonder if we should store an Option<Arc<Entry>> on self for this... The way it is right now may have issues if we have some other handshake that are racing each other.
There was a problem hiding this comment.
I think we'd probably need that for #2314 too?
There was a problem hiding this comment.
possibly, though i haven't looked into that issue too much at this point
Description of changes:
This change will update the MTU in the ApplicationParams on a dc Path when the MTU is updated as part of MTU probing.
Testing:
Added unit tests
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.