Allow specifying number of worker threads used to run an enclave #625
Conversation
s-arash
requested review from
mzohreva,
nshyrei,
Taowyoo,
aditijannu,
raoulstrackx and
arai-fortanix
| } | ||
|
|
||
| /// Run the enclave with the provided number of worker threads. | ||
| /// # Panics |
There was a problem hiding this comment.
Why is it a panic if the number of worker threads is 0 and not an error?
There was a problem hiding this comment.
I can make it an error. But my thinking is that worker_threads == 0 is a user error/precondition failure that the user must fix and not an error that comes out of running the enclave.
Another option would be to use NonZeroUsize, but it's very rarely used in Rust libraries.
There was a problem hiding this comment.
Libraries should try really hard to avoid panics. Panics in libraries should really only be for fatal conditions that the library has no way to recover from, like internal data structure inconsistencies that can't be reconciled somehow. An incorrect number of worker threads is absolutely not fatal in this way. Either the API should make it impossible to pass 0 via NonZeroUsize, or the call should return an error. The caller could decide it wants to panic if it gets this error, or it might try to make the call again with a different number of threads.
I think having an error for an invalid number of threads makes sense, as we probably want to have some upper bound on the number of threads as well. Does it really make sense for the application to request for example trillions of threads?
There was a problem hiding this comment.
I'm now returning an error instead of panicking when num_worker_threads is set to 0.
s-arash
force-pushed
the
arash/num-worker-threads
branch
from
b9af71e
to
cc998ae
Compare
|
I had been working the CI fixes here which you will face in your CI job too - #624 |
s-arash
force-pushed
the
arash/num-worker-threads
branch
from
da2817c
to
cc998ae
Compare
s-arash
force-pushed
the
arash/num-worker-threads
branch
from
cc998ae
to
ea171ab
Compare
s-arash
changed the title
Thanks @aditijannu. I'll rebase this PR once your PR lands. |
s-arash
force-pushed
the
arash/num-worker-threads
branch
from
ea171ab
to
cbe6600
Compare
| } | ||
|
|
||
| /// Panics if you have previously called [`arg`] or [`args`]. | ||
| /// Panics if you have previously called [`arg`], [`args`], or [`num_worker_threads`]. |
There was a problem hiding this comment.
With so many parameters that aren't shared between Command and Library enclaves, does it maybe make sense to have separate builders for Command and Library enclaves? That way, each type of builder could only have the parameters that make sense for the type of enclave it builds.
There was a problem hiding this comment.
I agree. But that would be a breaking change. I think it's better to release a sem-ver compatible version with this change, and leave this kind of refactoring to a future sem-ver breaking version.
| @@ -332,21 +342,25 @@ impl<'a> EnclaveBuilder<'a> { | |||
| } | |||
|
|
|||
| pub fn build<T: Load>(mut self, loader: &mut T) -> Result<Command, anyhow::Error> { | |||
| let num_worker_threads = self.num_worker_threads.unwrap_or_else(num_cpus::get); | |||
| anyhow::ensure!(num_worker_threads > 0, "`num_worker_threads` cannot be zero"); | |||
| anyhow::ensure!(num_worker_threads <= 512, "`num_worker_threads` cannot exceed 512"); | |||
There was a problem hiding this comment.
Is this needed at all? It might be unlikely but I don't think we should prevent people from spawning more threads than there are cores. It's just inefficient, but not wrong.
There was a problem hiding this comment.
Right. That's a good point. But maybe we should enforce a reasonable maximum? It doesn't have to be 512, but maybe we should somehow prevent specifying something like 1 << 60 as num_worker_threads.
- fixed upper bound for num_worker_threads (64k) - typo
s-arash
force-pushed
the
arash/num-worker-threads
branch
from
4fd27d4
to
2fa7a24
Compare
This change allows users of this library to specify the number of worker threads. This lets users limit the number of cores used by an enclave.
This is a backwards-compatible change.