-
Notifications
You must be signed in to change notification settings - Fork 150
Account for CPU time spent on handshakes in bach sims #2717
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
base: main
Are you sure you want to change the base?
Changes from all commits
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 |
|---|---|---|
|
|
@@ -360,6 +360,16 @@ where | |
| fn on_read(&mut self, data: &mut [u8]) -> usize { | ||
| let max_len = Some(data.len()); | ||
|
|
||
| // This is a semi-random number. However, it happens to work out OK to approximate | ||
| // spend during handshakes. On one side of a connection a handshake typically costs about | ||
| // 0.5-1ms of CPU. Most of that is driven by the peer sending information that the local | ||
| // endpoint acts on (e.g., verifying signatures). | ||
| // | ||
| // In practice this was chosen to make s2n-quic-sim simulate an uncontended mTLS handshake | ||
| // as taking 2ms (in combination with the transition edge adding some extra cost), which is | ||
| // fairly close to what we see in one scenario with real handshakes. | ||
| s2n_quic_core::io::event_loop::attribute_cpu(core::time::Duration::from_micros(100)); | ||
|
Contributor
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 fine with this for now. I do wonder if @maddeleine's TLS offload work would be a bit less intrusive, though, since you could essentially intercept the task and inject this delay. But I'm fine with unblocking you for now.
Collaborator
Author
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 could in principle put this in a wrapper around s2n-quic-tls's Provider (similar to slow_tls), it's just fairly painful to write that. I think @maddeleine's work doesn't change that -- it's not offloading just the crypto (what we'd probably ideally do here), so there's no obvious attach point. Every poll is too much.
Contributor
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. That's a good point, yeah.
Contributor
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. A note; I don't know if attributing some micros per on_read call will lead to the most accurate depiction of TLS handshake times. s2n-tls will calls on_read twice for every TLS record(once to retrieve the record header and then once to retrieve the full record). Additionally it can be called even if there is no TLS data to provide to s2n-tls. |
||
|
|
||
| let chunk = match self.state.rx_phase { | ||
| HandshakePhase::Initial => self.context.receive_initial(max_len), | ||
| HandshakePhase::Handshake => self.context.receive_handshake(max_len), | ||
|
|
@@ -407,6 +417,9 @@ enum HandshakePhase { | |
|
|
||
| impl HandshakePhase { | ||
| fn transition(&mut self) { | ||
| // See comment in `on_read` for value and why this exists. | ||
| s2n_quic_core::io::event_loop::attribute_cpu(core::time::Duration::from_micros(100)); | ||
|
|
||
| *self = match self { | ||
| Self::Initial => Self::Handshake, | ||
| _ => Self::Application, | ||
|
|
||
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.
Another thought I kind of wanted to jot down: This breaks with offloading; it messes with the assumptions being made in this PR. Because if the TLS task is now async then you could be attributing CPU while the event loop task is sleeping.
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.
I actually think we should build on the changes we made for the async TLS task to accomplish this. Basically we could use that runtime trait to spawn a wrapped TLS task that adds delays. We may want to also add support for delaying responses? Not sure... Anyway that area of the code seems like the right place to hook in.
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.
Hmmm yeah, that does sound doable. It wouldn't help if you wanted to simulate a non-offload handshake though.
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.
That's true, yeah