Skip to content
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

runtime: Use a limited multi-threaded Tokio runtime in SGX #5214

Merged
merged 5 commits into from Mar 13, 2023

Conversation

kostko
Copy link
Member

@kostko kostko commented Mar 6, 2023

No description provided.

@codecov
Copy link

codecov bot commented Mar 6, 2023

Codecov Report

Merging #5214 (bb20e69) into master (2490d26) will decrease coverage by 0.09%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #5214      +/-   ##
==========================================
- Coverage   61.41%   61.33%   -0.09%     
==========================================
  Files         512      512              
  Lines       54243    54243              
==========================================
- Hits        33316    33269      -47     
- Misses      16706    16750      +44     
- Partials     4221     4224       +3     

see 19 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@kostko kostko force-pushed the kostko/feature/tokio-mt-sgx branch from 6a5f409 to 4813cb9 Compare March 6, 2023 13:29
@kostko kostko force-pushed the kostko/feature/tokio-mt-sgx branch 4 times, most recently from 894aa9e to 5e96949 Compare March 10, 2023 15:44
@kostko kostko marked this pull request as ready for review March 10, 2023 15:44
Copy link
Contributor

@peternose peternose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIce, works as expected 👍

Tested on an SGX instance and can confirm, that the ephemeral secret replication deadlock problem, which occured if all key managers were restarted at the same time, is now gone.

runtime/src/protocol.rs Show resolved Hide resolved
runtime/src/helpers.rs Outdated Show resolved Hide resolved
runtime/src/dispatcher.rs Show resolved Hide resolved
runtime/src/enclave_rpc/demux.rs Show resolved Hide resolved
runtime/src/enclave_rpc/demux.rs Outdated Show resolved Hide resolved
runtime/src/enclave_rpc/demux.rs Outdated Show resolved Hide resolved
writer: W,
) -> Result<
(
tokio::sync::OwnedMutexGuard<MultiplexedSession>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a big fan of returning a guard, especially not in public methods.

I would maybe refactor/rename demuxer to track and purge sessions only (get_or_create, remove, reset).
update multiplexed session (close) and then do all this stuff in the dispatcher.

let frame:Frame = cbor::from_slice(&request)?;
let session = self.sessions.get_or_create_session(frame.session);
session.lock_owned().await
let message = session.process_data(frame.payload, writer).await?;
...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You would still need to keep the locked session as you need to also process the response, but actual RPC dispatch can only happen in the dispatcher.

runtime/src/enclave_rpc/client.rs Show resolved Hide resolved
runtime/src/enclave_rpc/client.rs Outdated Show resolved Hide resolved
@kostko kostko force-pushed the kostko/feature/tokio-mt-sgx branch from 5e96949 to ed71909 Compare March 13, 2023 11:00
@kostko kostko force-pushed the kostko/feature/tokio-mt-sgx branch from ed71909 to bb20e69 Compare March 13, 2023 12:11
@@ -170,10 +170,13 @@ pub struct Dispatcher {
impl Dispatcher {
#[cfg(target_env = "sgx")]
fn new_tokio_runtime() -> tokio::runtime::Runtime {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the future we could move this to future.rs, call it from start_runtime and pass it to dispatcher and protocol constructors.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, that's a good idea, yeah this part needs some refactoring.

@kostko kostko merged commit 5b2d859 into master Mar 13, 2023
2 of 3 checks passed
@kostko kostko deleted the kostko/feature/tokio-mt-sgx branch March 13, 2023 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants