-
Notifications
You must be signed in to change notification settings - Fork 165
Add custom resource limiting for instances #650
base: main
Are you sure you want to change the base?
Conversation
This is done via a `ResourceLimiter` trait which can be implemented by the host and configured when creating an `Instance`.
I haven't dug into testing in Lucet yet, but if this PR generally seems acceptable I'm happy to add a test as well. |
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 looks great! I have a few questions about the details of the API, and how I'm maybe not thinking of it in quite the correct way. Also yes, it would be great to add some tests before proceeding.
@@ -76,6 +76,7 @@ pub fn new_instance_handle( | |||
module: Arc<dyn Module>, | |||
alloc: Alloc, | |||
embed_ctx: CtxMap, | |||
limiter: Option<Box<dyn ResourceLimiter>>, |
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 probably be an Arc
instead of a Box
to make integration with other parts of a system easier and more efficient.
/// | ||
/// * `current` gives the current size of the linear memory, in bytes. | ||
/// * `desired` gives the desired new size of the linear memory, in bytes. | ||
/// * `limit` gives the configured maximum size of the linear memory, in bytes. |
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 had to read through the call site above a couple times because this argument was kind of surprising to me. On a purely intuitive level, it feels like something called a ResourceLimiter
shouldn't have to be told what its limit is. I wonder if this is a symptom of needing a more evocative name for this trait (perhaps LimitEnforcer
?) or whether there's a different way of documenting its intended use.
current: u32, | ||
desired: u32, | ||
limit: u32, | ||
) -> Result<(), String>; |
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.
String
feels a bit weird here as an error value, I think because I have a hard time imagining we'd want a call to memory_growing
to fail with any limit description other than "linear memory". Between this and the above comment about the limit
argument I'm also wondering if I might just be thinking about this trait incorrectly 🙂
Lucet currently provides configurable limits for things like linear memory growth, but these limits are relatively "static" in that they are set at a specific value when setting up an instance.
For some use-cases, resource limits need to vary based on other conditions on the host, e.g. when attempting to limit both the WebAssembly heap and the allocations resulting from calls into the host.
This PR addresses these additional use-cases by adding an optional, more dynamic
ResourceLimiter
trait that provides hooks for "approving" requests for additional resources dynamically, as they occur. A resource limiter can be configured as part of building an instance.For now, the only check provided by
ResourceLimiter
ismemory_growing
, invoked duringInstance::grow_memory
.