-
Notifications
You must be signed in to change notification settings - Fork 283
feat: support JIT in no_std mode #125
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
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
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.
Thank you! It's great to gain support for the JIT without std
. I'm in favour of taking this work (again, apologies for the delay!), but I've got some concerns with the current version.
-
I'm not convinced we need the second commit (helpers batch registration), it exposes the internal structure we use to hold the helper functions, for little gain in my opinion.
-
I don't fully understand the motivation for commit 3. What are the ranges trying to solve, here?
Since some maps may store a lot of data, representing the address with a
single byte may waste a lot of space. Using range can simplify this
problem.We don't support maps yet. And what do you mean by “a single byte” exactly?
-
Any chance you could reorder the commits to avoid breaking the tests between commits 3 and 4? By folding the last commit into the previous one, or swapping them if it makes sense. I don't like broken tests between commits, it's a pain when we need to bisect to find a bug.
-
We need to update the tests, too, so we can be sure that the JIT behaves as expected. The good news is, it's easy to remove the
feature = "std"
intests/ubpf_jit_x86_64.rs
andtest/misc.rs
. Bad news: we need to add macros and use the alternate version of thejit_compile()
function everywhere.
Regarding this last point: Would it make sense to maybe change your API? Instead of having a jit_compile()
function with different arguments when std
is disabled, how about adding a separate set_jit_exec_memory()
to add the memory area as an attribute to the VM, and use it in jit_compile()
without having to pass it at compilation time? Makes one more function call when we don't have std
, but it avoids making different versions of the function, and look slightly cleaner to me? We'd still have to update all the JIT tests though.
For commit 2, suppose we have an array data type as follows:
The size of this array is 512 bytes. If we follow the original method, we need to store 512 u64 types, which requires 512*8 space. Using Range, only 8*2 space is required. |
It is a bit difficult to test the JIT in no_std environment, because executable memory allocation is usually kernel-specific. I don't think the lack of this test will affect the use of the JIT in no_std mode. |
OK I looked into it again, and it makes sense now. The mention of “maps” in the commit description is a bit confusing, but fine. As for the tests:
I don't think either. But then, the number of times I thought my code would work and it didn't! 🙂 No, if we support the JIT without
What does this mean exactly? Is it too hard to put in a wrapper and make it work with the different CI runners? What do you mean by “kernel-specific”, are there more cases than Linux/macOS and Windows? |
Maybe we have different understandings of no_std mode, but it doesn’t matter. Now we allow running jit tests in --no-default-features mode. |
The API change looks good to me. |
What do you mean? (Genuine question, I may be wrong and would like to understand better.)
Thanks a lot for this! |
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.
Looks all good to me. Thanks for this!
I was about to merge but noticed the CI failures, which seem unrelated and should be addressed in #126. Would you mind rebasing so we can run the full tests with |
In order to allow the use of jit in no_std mode, it is necessary to let the user manually pass in the executable memory area. Signed-off-by: Godones <[email protected]>
…cessed Since some maps may store a lot of data, representing the address with a single byte may waste a lot of space. Using range can simplify this problem. fix: fix the no_std error for HashSet In no_std mode, BtreeMap is used instead of HashMap, but Range does not implement the `Ord` trait. In order to use HashMap in no_std mode, hashbrown crate is used here, which provides better performance than the standard library. Signed-off-by: Godones <[email protected]>
Signed-off-by: Godones <[email protected]>
Signed-off-by: Godones <[email protected]>
To run jit tests in --no-default-features mode, we rely on libc library for memory mapping. The `misc/test_jit_block_port` test is not modified because it depends on `helpers::bpf_trace_printf` internally. Signed-off-by: Godones <[email protected]>
No problem. |
I think the |
Signed-off-by: Godones <[email protected]>
We can, but if we never check that things work without the libc, how do we know that it's working? I'm confused. Anyway, thanks a lot for this work! Ready to merge, now. |
In order to allow the use of jit in no_std mode, it is necessary to let the user manually pass in the executable memory area.