Add a JIT based on Cranelift #86

afonso360 · 2023-07-02T18:52:14Z

👋 Hey,

I've been playing around with translating eBPF and running it with Cranelift as a weekend project. Cranelift is a Rust based optimizing compiler that includes a JIT.

This is a prototype and it is still a bit rough around the edges, but maybe you'd be interested in having this upstream?

This can also help with #21 and #48 since Cranelift supports x86_64, AArch64, s390x and RISC-V, as well as Windows, Linux, MacOS and some BSD's.

(I accidentally ran rustfmt on src/lib.rs, but If you'd like me to polish this PR and upstream it, I'll revert that change)

qmonnet · 2023-07-07T19:51:37Z

Thanks and apologies for the delay, I'll try to find some time to look into this soon 🙏

qmonnet · 2023-07-23T19:09:55Z

Personal business has kept me mostly away from GitHub last week... I'll do my best this week, apologies again

qmonnet · 2023-07-25T20:56:07Z

So this is a lot of work, thanks for this PR!

To be honest with you, I'm not really sure what to do of it. We already have a JIT-compiler in rbpf, and I'm not looking forwards to maintain a separate implementation. This being said, support for the various architectures and platforms you mentioned is definitely something interesting.

I'm thinking of accepting the PR, especially if you use it on your side, but I'd have the feature opt-in rather than opt-out (my understanding is that default = ["cranelift"] in the manifest currently enables it by default, right?). Would it be OK for you? I hope I haven't killed your motivation with the review delay, apologies once again.

I've got a few questions:

You mentioned a "weekend project". Do you have a concrete use case for this PR, or was it simply an experiment?
How does it compare with the existing JIT, in particular, is it at feature parity? (I can check as well, but haven't been through all the code and tests yet).
What platform/arch have you tested so far?

Some observations:

I can build locally and all tests pass, but I see a few warnings when building, we'll need to address these.
We'll want the CI checks to pass, too (note the Windows builds are flaky).
I don't mind the formatting on src/lib.rs, as long as it comes as a separate commit (although it would probably work just as well as a separate PR). There's some code where I prefer to keep the current formatting, but I've moved it all to src/interpreter.rs.

afonso360 · 2023-07-26T10:35:05Z

Hey, Thanks for reviewing this!

We already have a JIT-compiler in rbpf, and I'm not looking forwards to maintain a separate implementation.

That is completely reasonable, and mostly the reason I posted such an early draft (with build warnings!).

This really is a weekend project, and I used it to learn a bit more about how eBPF works. I don't currently use it anywhere or have any real plans for using it.

I'm thinking of accepting the PR, especially if you use it on your side, but I'd have the feature opt-in rather than opt-out (my understanding is that default = ["cranelift"] in the manifest currently enables it by default, right?). Would it be OK for you?

Yeah, that makes sense.

You mentioned a "weekend project". Do you have a concrete use case for this PR, or was it simply an experiment?

Not really, it really was just an experiment into learning how eBPF works.

How does it compare with the existing JIT, in particular, is it at feature parity? (I can check as well, but haven't been through all the code and tests yet).

I don't think we are at feature parity.

The test file for cranelift is pretty much a copy from the existing JIT's tests and we pass all of those, however the cranelift JIT doesn't yet implement the mbuf load/store instructions, that being said, it shouldn't be very difficult to add them.

From reading the existing JIT I think the only extra feature we implement is that we do bounds checks on all loads and stores. Additionally we use some of the existing cranelift features such as constant propagation and some other optimizations.

This mostly means that some of the test cases end up being optimized down to a couple of instructions. (As an aside this ended up revealing a missing optimization in cranelift!)

What platform/arch have you tested so far?

Natively I've only tested on x86_64 Linux. I've also ran the testsuite for AArch64 and RISC-V under QEMU.

RISC-V failed due to an instruction not yet being implemented on cranelift, but that has since been fixed in the latest version and the testsuite passes with that version.

I haven't tested s390x, or any other OS. That being said, they should be supported.

Some observations:

Those make sense! This really is a sort of early draft which I was planning on fixing up if you decide you'd like this upstreamed. Let me know!

And I completely understand that this might be too much of a maintenance burden! Especially for something that already pretty much exists.

qmonnet · 2023-07-26T19:44:44Z

Thanks for the details! Nice to have validation on several architectures already 💪.

OK let's go for an opt-in feature then. I've also thought of keeping it in a separate branch, but the project is not active enough and the feature would likely end up dying there. At least in main it can be discovered and tested by users.

Would be nice to get the load/stores with the mbuf too in that case. And good to have some additional optimisations, I must admit I haven't spent time looking into that.

One other question, I remember seeing that you build the CFG for the program at some point in the code. What is it for?

As an aside this ended up bytecodealliance/wasmtime#6683!

Pretty neat!

afonso360 · 2023-07-26T21:42:35Z

👍 Sounds good, I'll clean it up a bit and add those missing features.

One other question, I remember seeing that you build the CFG for the program at some point in the code. What is it for?

Cranelift's IR builder does not allow us to go back and edit a block after the branch / terminator instruction has been inserted (Called a "filled block" in cranelift's terminology). So what happens is that when we have a backwards branch into the middle of a previous block, we no longer can go back, split that previous block in two and emit a jump into it.

So I've built a two pass solution, first we scan and mark all offsets where we either land from another instruction, or we jump into another instruction, and emit the IR blocks according to that. When translating the instructions into IR, we switch blocks according to that previous scan.

afonso360 · 2023-07-27T19:18:24Z

I've cleaned up this PR a little bit. I've also made the following changes:

Added a --cranelift flag to the bpf_conformance plugin
- This now runs in CI
- CI seems to need some sort of approval to run, now that I've changed it
Implemented the mbuf load instructions
Tested this on Windows
- I've also edited the appveyor CI to run tests with --all-features so that cranelift tests run as well
I've changed the public API to one that is similar to the current JIT one.
- We now expose compile_cranelift and execute_program_cranelift

I'd appreciate a review especially on the mbuf handling part, which I'm not entirely sure is correct. We store either the mbuf or the mem pointer in r1 based on the length argument, which is what seems to be done in the interpreter. But the existing JIT does something slightly more complicated which I'm not sure I fully understand.

Edit: There are also quite a few WIP commits on this branch, should I squash this into a single commit?

qmonnet · 2023-08-10T23:48:32Z

Apologies for the delay, again... This time I must admit that I completely forgot to come back to this during the week 😬 but I've started, now.

There are also quite a few WIP commits on this branch, should I squash this into a single commit?

Not necessarily a single one, ideally logical commits (New JIT addition, adding the feature, enabling in CI, ...).

CI is happy, this is great!

qmonnet

Looks good on high-level code organisation. I'd like to take a closer look, at least for the changes in src/lib.rs, but it's already late here so this will take a few more days.

qmonnet · 2023-08-10T23:56:51Z

tests/cranelift.rs

+
+
+#[test]
+fn test_cranelift_ldabsb() {


I see that this file contains all the same tests as in tests/ubpf_jit_x86_64.rs, which is great! It also has the following additional tests:

ldabsb

ldabsdw

ldabsh

ldabsw

ldindb

ldinddw

ldindh

ldindw

... for which the existing JIT has equivalent tests under test/misc.rs. The reason why they're not in the same file is that tests/ubpf_jit_x86_64.rs only contains those tests that were copied from uBPF, and ldabs*/ldind were not supported there, at least at the time. It would probably make sense to move related tests together though.

This comment is mostly for myself. Looks good, and OK to have all your tests in tests/cranelift.rs 👍.

👍 It'd be nice to eventually have a unified test suite that would run in cranelift, the JIT and the interpreter!

qmonnet · 2023-08-10T23:57:53Z

.github/workflows/test.yaml

+          "${{ env.CONFORMANCE_IMAGE }}" \
+          bin/bpf_conformance_runner --test_file_directory tests \
+          --plugin_path /rbpf/target/release/examples/rbpf_plugin \
+          --exclude_regex "lock"  \


Isn't that possible to reuse "${{ env.KNOWN_FAILURES }}" here? Or is the mem-len test passing?

mem-len is passing, so that's why I didn't reuse it. Getting lock to work might also not be too much work, but It might be best as a follow up, and then we could remove this entirely.

afonso360 · 2023-08-13T13:09:02Z

Apologies for the delay, again... This time I must admit that I completely forgot to come back to this during the week 😬 but I've started, now.

No worries! I'm not in a hurry with this, so take as long as you'd like.

Not necessarily a single one, ideally logical commits (New JIT addition, adding the feature, enabling in CI, ...).

Cool, I'll rework those commits into something slightly more reasonable.

Run rustfmt on src/lib.rs to improve formatting. Suggested-by: Afonso Bordado <[email protected]> Signed-off-by: Quentin Monnet <[email protected]>

Signed-off-by: Afonso Bordado <[email protected]> Signed-off-by: Quentin Monnet <[email protected]>

qmonnet · 2023-09-05T00:28:10Z

No worries! I'm not in a hurry with this, so take as long as you'd like.

I'm taking you at your word 😅. This time I started to look more into it though.

My main comment would be that we could clean up the commits a bit more - I started to comment on the PR, but I realised I was maybe being too picky and imposing too much work on you. So instead I cleaned it up on my side and created another branch (I didn't want to overwrite yours).

So would you mind checking whether https://github.com/qmonnet/rbpf/tree/pr/cranelift-jit-reworked is OK with you, and if so, updating the current PR? Changes include:

Rebasing on current main
Reworking some commits: taking out all the commented tests or code portions, and only adding them in the relevant commits
Similarly, squashing the fixes from later commits into the relevant earlier commits
Keeping the formatting changes from rustfmt for src/lib.rs, but as a separate commit
Applying rustfmt to src/lib.rs (including after your changes) and src/cranelift.rs
Un-commenting ebpf::ST_W_XADD => unimplemented!(), ebpf::ST_DW_XADD => unimplemented!()
Adding the SPDX tags for the new files
Reworded/formatted commit titles slightly, although I didn't touch the descriptions
Added my sign-off to the commits I changed

I think that's about it. I checked that the tests are still passing at each commit, so hopefully I didn't break your PR too much while reorganising the contents between the different commits 🙂.

I note there's a test which is commented out in tests/cranelift.rs, is there a particular reason for that?

Signed-off-by: Afonso Bordado <[email protected]> Signed-off-by: Quentin Monnet <[email protected]>

This tests the Cranelift JIT in CI Signed-off-by: Afonso Bordado <[email protected]>

Signed-off-by: Afonso Bordado <[email protected]> Signed-off-by: Quentin Monnet <[email protected]>

Signed-off-by: Afonso Bordado <[email protected]>

Signed-off-by: Afonso Bordado <[email protected]> Signed-off-by: Quentin Monnet <[email protected]>

Signed-off-by: Afonso Bordado <[email protected]>

Some operations may not work in some older CPU's and may need to be lowered to a libcall. Signed-off-by: Afonso Bordado <[email protected]>

afonso360 · 2023-09-05T11:44:51Z

My main comment would be that we could clean up the commits a bit more - I started to comment on the PR, but I realised I was maybe being too picky and imposing too much work on you. So instead I cleaned it up on my side and created another branch (I didn't want to overwrite yours).

That's awesome! Thanks! Your branch looks great to me!

I note there's a test which is commented out in tests/cranelift.rs, is there a particular reason for that?

Not really, I just kinda forgot about it 🙃 . I've now updated commit 83146eb to include that test, and also some better error messages when we don't find the required helper.

qmonnet

OK, I've been through the changes one more time and I think we're good 🎉. I'm not familiar with Cranelift to provide constructive feedback on the new JIT implementation, but it's an opt-in feature, it's self-contained, the changes to src/lib.rs is limited to the new related API functions, and we have CI tests that pass nearly all the existing tests. We can always refine or improve as a follow-up if necessary. The small loss of CI coverage reported is acceptable.

Do you have any more changes to bring to the PR, or shall I merge it?

afonso360 · 2023-09-06T19:53:06Z

OK, I've been through the changes one more time and I think we're good 🎉

🎉

Do you have any more changes to bring to the PR, or shall I merge it?

Not right now, I might send a follow up PR soon to update the cranelift version when the new one is out. But It's probably not worth waiting for a release.

qmonnet · 2023-09-06T20:02:00Z

I might send a follow up PR soon to update the cranelift version when the new one is out. But It's probably not worth waiting for a release.

OK. No need to update for the sake of it though, no need to update if there's no benefit in it (perf improvement, relevant security fixes, etc.).

We should also update the REAMDE.md at some point to mention the feature and document the API functions. I can look into it when I find the time, if you don't beat me to it.

afonso360 force-pushed the cranelift-jit branch 3 times, most recently from 185b5e0 to 7074ec9 Compare July 27, 2023 19:10

afonso360 marked this pull request as ready for review July 27, 2023 19:10

qmonnet reviewed Aug 11, 2023

View reviewed changes

afonso360 force-pushed the cranelift-jit branch from 459d39c to 1bb71ae Compare August 13, 2023 13:29

qmonnet and others added 3 commits September 4, 2023 21:57

src/lib.rs: Apply rustfmt

6f84981

Run rustfmt on src/lib.rs to improve formatting. Suggested-by: Afonso Bordado <[email protected]> Signed-off-by: Quentin Monnet <[email protected]>

cranelift: Add initial Cranelift JIT

4f5041b

Signed-off-by: Afonso Bordado <[email protected]> Signed-off-by: Quentin Monnet <[email protected]>

cranelift: Implement loads and byteswaps

eccba85

Signed-off-by: Afonso Bordado <[email protected]> Signed-off-by: Quentin Monnet <[email protected]>

afonso360 added 11 commits September 5, 2023 12:21

cranelift: Implement helper calls

83146eb

Signed-off-by: Afonso Bordado <[email protected]> Signed-off-by: Quentin Monnet <[email protected]>

cranelift: Add more Cranelift JIT test cases

0afb240

Signed-off-by: Afonso Bordado <[email protected]> Signed-off-by: Quentin Monnet <[email protected]>

cranelift: Support stores

986477f

Signed-off-by: Afonso Bordado <[email protected]> Signed-off-by: Quentin Monnet <[email protected]>

cranelift: Checked stack accesses

c02f26b

Signed-off-by: Afonso Bordado <[email protected]> Signed-off-by: Quentin Monnet <[email protected]>

cranelift: Add more Cranelift tests

5c79e56

Signed-off-by: Afonso Bordado <[email protected]> Signed-off-by: Quentin Monnet <[email protected]>

cranelift: Implement Cranelift jumps

2a2c5af

Signed-off-by: Afonso Bordado <[email protected]> Signed-off-by: Quentin Monnet <[email protected]>

cranelift: Fix early exit tests

9e06209

Signed-off-by: Afonso Bordado <[email protected]> Signed-off-by: Quentin Monnet <[email protected]>

cranelift: Update CFG algorithm

7fe1465

Signed-off-by: Afonso Bordado <[email protected]> Signed-off-by: Quentin Monnet <[email protected]>

conformance: Add Cranelift to BPF conformance tests

30e5c32

Signed-off-by: Afonso Bordado <[email protected]> Signed-off-by: Quentin Monnet <[email protected]>

cranelift: Ensure memory is freed after dropping the Cranelift module

935b2fe

Signed-off-by: Afonso Bordado <[email protected]> Signed-off-by: Quentin Monnet <[email protected]>

ci: Add --all-features to CI

42c431a

This tests the Cranelift JIT in CI Signed-off-by: Afonso Bordado <[email protected]>

afonso360 added 6 commits September 5, 2023 12:28

cranelift: Add complete Cranelift API

da873d2

Signed-off-by: Afonso Bordado <[email protected]> Signed-off-by: Quentin Monnet <[email protected]>

cranelift: Test all features on Windows, too

ac25305

Signed-off-by: Afonso Bordado <[email protected]>

cranelift: Correctly pass mbuff to Cranelift JIT programs

8abbbc2

Signed-off-by: Afonso Bordado <[email protected]>

cranelift: Implement IND and ABS load instructions

4b160a5

Signed-off-by: Afonso Bordado <[email protected]> Signed-off-by: Quentin Monnet <[email protected]>

cranelift: Enable stack probes on the Cranelift JIT

5bd32f1

Signed-off-by: Afonso Bordado <[email protected]>

cranelift: Use the default libcall names

37a6284

Some operations may not work in some older CPU's and may need to be lowered to a libcall. Signed-off-by: Afonso Bordado <[email protected]>

afonso360 force-pushed the cranelift-jit branch from a85b1e4 to 37a6284 Compare September 5, 2023 11:44

qmonnet approved these changes Sep 6, 2023

View reviewed changes

qmonnet mentioned this pull request Sep 6, 2023

Unified test suite for Cranelift, x86 JIT and interpreter #90

Open

qmonnet merged commit 4812c52 into qmonnet:main Sep 6, 2023

This was referenced Sep 6, 2023

Make JIT usable on Windows #21

Open

Is only support x86_64? #48

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a JIT based on Cranelift #86

Add a JIT based on Cranelift #86

afonso360 commented Jul 2, 2023 •

edited

Loading

qmonnet commented Jul 7, 2023

qmonnet commented Jul 23, 2023 •

edited

Loading

qmonnet commented Jul 25, 2023

afonso360 commented Jul 26, 2023

qmonnet commented Jul 26, 2023

afonso360 commented Jul 26, 2023 •

edited

Loading

afonso360 commented Jul 27, 2023 •

edited

Loading

qmonnet commented Aug 10, 2023

qmonnet left a comment

qmonnet Aug 10, 2023

afonso360 Aug 13, 2023

qmonnet Aug 10, 2023

afonso360 Aug 13, 2023

afonso360 commented Aug 13, 2023

qmonnet commented Sep 5, 2023 •

edited

Loading

afonso360 commented Sep 5, 2023

qmonnet left a comment

afonso360 commented Sep 6, 2023

qmonnet commented Sep 6, 2023



		#[test]
		fn test_cranelift_ldabsb() {

Add a JIT based on Cranelift #86

Add a JIT based on Cranelift #86

Conversation

afonso360 commented Jul 2, 2023 • edited Loading

qmonnet commented Jul 7, 2023

qmonnet commented Jul 23, 2023 • edited Loading

qmonnet commented Jul 25, 2023

afonso360 commented Jul 26, 2023

qmonnet commented Jul 26, 2023

afonso360 commented Jul 26, 2023 • edited Loading

afonso360 commented Jul 27, 2023 • edited Loading

qmonnet commented Aug 10, 2023

qmonnet left a comment

Choose a reason for hiding this comment

qmonnet Aug 10, 2023

Choose a reason for hiding this comment

afonso360 Aug 13, 2023

Choose a reason for hiding this comment

qmonnet Aug 10, 2023

Choose a reason for hiding this comment

afonso360 Aug 13, 2023

Choose a reason for hiding this comment

afonso360 commented Aug 13, 2023

qmonnet commented Sep 5, 2023 • edited Loading

afonso360 commented Sep 5, 2023

qmonnet left a comment

Choose a reason for hiding this comment

afonso360 commented Sep 6, 2023

qmonnet commented Sep 6, 2023

afonso360 commented Jul 2, 2023 •

edited

Loading

qmonnet commented Jul 23, 2023 •

edited

Loading

afonso360 commented Jul 26, 2023 •

edited

Loading

afonso360 commented Jul 27, 2023 •

edited

Loading

qmonnet commented Sep 5, 2023 •

edited

Loading