-
Notifications
You must be signed in to change notification settings - Fork 417
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
[GIT PULL] Add Rust implementation of liburing.h #1339
base: master
Are you sure you want to change the base?
Conversation
Update the code to function as a standalone crate that provides native Rust implementations of all the `static inline` functions that live in liburing.h. GHA is updated to build on as many architectures as Rust supports. The crate statically links in liburing.a and the liburing.h header is updated to duplicate a declaration per `static inline` function definition. This is done so the Rust code can reuse all of the existing tests. If a new definition is added to liburing.h and the Rust implementation lags, a linking error is generated when the liburing_rs_test is built. Signed-off-by: Christian Mazakas <[email protected]>
if: ${{matrix.sanitizer == '0'}} | ||
if: ${{matrix.sanitize == '0'}} | ||
run: | | ||
./configure --cc=${{matrix.cc}} --cxx=${{matrix.cxx}}; | ||
make -j$(nproc) V=1 CPPFLAGS="-Werror" CFLAGS="$FLAGS" CXXFLAGS="$FLAGS"; | ||
|
||
- name: Build | ||
if: ${{matrix.sanitizer == '1'}} | ||
if: ${{matrix.sanitize == '1'}} |
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.
Good catch, this variable name typo was introduced in PR #1215
But this fix is not related to Rust support and should be a separate commit.
if [[ "${{matrix.cc_pkg}}" == "clang" ]]; then \ | ||
wget https://apt.llvm.org/llvm.sh -O /tmp/llvm.sh; \ | ||
sudo apt-get purge --auto-remove llvm python3-lldb-14 llvm-14 -y; \ | ||
sudo bash /tmp/llvm.sh 17; \ | ||
sudo update-alternatives --install /usr/bin/clang++ clang++ /usr/bin/clang++-17 400; \ | ||
sudo update-alternatives --install /usr/bin/clang clang /usr/bin/clang-17 400; \ | ||
else \ | ||
sudo apt-get update -y; \ | ||
sudo apt-get install -y ${{matrix.cc_pkg}} ${{matrix.cxx_pkg}}; \ | ||
fi; | ||
sudo apt-get update -y; | ||
sudo apt-get install -y ${{matrix.cc_pkg}} ${{matrix.cxx_pkg}}; |
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 change is not related to Rust support and should be a separate commit.
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 think we should keep the llvm.sh
, but bump the version to the latest available LLVM version.
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 probably should be a separate commit, yes.
I made the change here because of a deficiency in bindgen. A user here has reported an exact issue @axboe himself had run into.
I was able to replicate it here: https://github.com/cmazakas/liburing/actions/runs/12932599582/job/36069219385
This only seems to happen with the clang installed via the llvm.sh script. I'm not exactly sure why it trips bindgen up so much, but it does. I figured, unless there was a super compelling reason to use a super modern clang, whichever one comes with Ubuntu by default was Good Enough.
liburing-rs/.gitignore
Outdated
@@ -0,0 +1 @@ | |||
/target/ |
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 .gitignore
is probably not needed (?)
Doesn't /liburing-rs/target
in the top-level project directory's .gitignore
already do the job?
.gitignore
Outdated
/target | ||
/liburing-rs/target | ||
|
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.
Reference for a previous comment. This is what I meant by top-level project directory's .gitignore
.
Also, it's better to have /target/
, /liburing-rs/target/
(slash at the end) to clearly tell the reader it's a directory.
[[package]] | ||
name = "windows-targets" | ||
version = "0.52.6" | ||
source = "registry+https://github.com/rust-lang/crates.io-index" | ||
checksum = "9b724f72796e036ab90c1021d4780d4d3d648aca59e491e6b98e725b84e99973" | ||
dependencies = [ | ||
"windows_aarch64_gnullvm", | ||
"windows_aarch64_msvc", | ||
"windows_i686_gnu", | ||
"windows_i686_gnullvm", | ||
"windows_i686_msvc", | ||
"windows_x86_64_gnu", | ||
"windows_x86_64_gnullvm", | ||
"windows_x86_64_msvc", | ||
] |
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.
liburing obviously only works on Linux. I don't fully understand Cargo, but why do we have Windows targets?
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.
That's actually a really good question. I can do a little digging and see what's going on there.
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.
Alright, did some digging. It's just how Cargo generates the lockfiles. We have a dependency on bindgen (necessary) which has a dependency on clang-sys which depends on libloading which has conditional dependencies for Windows targets.
The good news is, Cargo doesn't actually download the Windows dependencies from what I can tell if you're not actually on the target platform (or compiling for it, at least).
However, it seems like for the sake of sealed auditability, it will be present in the generated Cargo.lock. I don't think there's anything we can do about that.
I wonder if liburing will support rust officially? There is already some crates like io-uring, liburing-rs. |
This what the PR is aiming to add. There's existing Rust crates but they're incomplete in comparison to this. They also force a lot of opinionated designs and try to offer safe interfaces, which I think isn't as ideal as an unopinionated low-level vocabulary like what liburing offers. |
Why that is better than a separate library? There are many languages, all of them keep bindings separately. |
This is fair, and it's been something I've been considering doing. In this case, it's because this isn't a set of bindings but a native transliteration of the C code. The crate builds and links in liburing.a but everything else provided via liburing.h has real impls in Rust. This means it's wise to link against and reuse all of the same tests and it also ensures that if liburing tests new functionality, the Rust build fails to link so everything is caught. |
I am sure everyone here has thought of "What if Liburing was written with Rust rather than C?" at least once! For me personally wrapping Liburing to Python has been a pain. It was suppose to be a weekend project that turned into months and months of hunting for bugs, using multiple FFI interfaces and finally there was no real solution. Critical functions still not working as intended, to batching Rust on the other hand provides much better bridge + tooling + testing between C & Python. Only reason I haven't switched to using Rust is because Liburing is still using C. If this Pull request gets merged I would seriously consider moving to Rust to do the wrapping. I can see many projects using Rust's Liburing interface vs C in the future. Thank You |
I have a little about python. But if there is some hard things to wrap liburing to python with FFI, you can make uring python package without liburing. There is also rust io-uring crate without liburing. The crate implements uring crate with primitive syscalls itself. |
@SidongYang Its suppose to be Python version of this C Liburing library, it would be weird if I used some other library. Also its not hard to wrap a C library. I even had FFI library core developers look at the issue and they couldn't figure out what was going on either. Its rare but stuff like this happens, programing language are not really designed to work with other languages. With Rust since they are new kid on the block they have accounted for better way to work with both C and Python. |
I have no idea about the problem that couldn't be solved with FFI. (Is there any reference like github issue?) |
I don't think it changes anything? An external project still can link to liburing.a and do the same thing.
Not sure what you mean. Do you want to write kernel regression tests in rust (as most of test/ is)? I didn't understand in what circumstance you expect rust build to fail. |
No, not really.
I don't think there is anything that would prevent you from switching if it's a part of an separate project, the result would essentially be the same. I believe you that binding a language could be tricky, but that's exactly the reason to keep it in a separate project, i.e. to avoid the mess in liburing inevitably coming from multi language project. |
Sure.
Ha ha, of course not. An explicit point of this PR was to not touch the tests at all. Instead, I've modified the liburing.h header to have a dual-mode where it only exports declarations for all the For some philosophical clarification, I thought about just making an external lib. But I wanted to ship the liburing source alongside it so liburing.a can get built and so that bindgen can generate the correct bindings for all Rust toolchains. Then I wanted to test the transliteration using the exact same tests that liburing uses now. I expect the build to fail when Jens or you or anyone else adds a new function to liburing.h and they write a unit test for it. In the Rust build, The code kind of straddles that line with how invasive and how much a fork interacts with the base code. I figured, I came up with a mostly non-noticeable series of changes and everything was nicely tucked away so I might as well just upstream it. Plus, I don't really think Rust has any good io_uring crates like liburing. All the Rust crates focus on the wrong things, imo. They focus on making the basic submit-reap-CQE loop safe. They don't focus on building a rich vocabulary of functions to help users build SQEs. They also force a lot of Rust idioms where they don't necessarily fit. By having a low-level set of This would be the most in-depth, well-tested and comprehensive way of using io_uring from Rust. liburing-ffi has a lot of overhead in Rust that can be avoided, and users don't have to try and do any LTO tricks either. Plus, and I hate appealing to authority here, it'd be good to have an "official" io_uring crate in Rust. The tokio team seems to have some stuff but it's not like this. |
@isilence I see what @cmazakas is adding to Liburing as what you already have with Liburing is such an awesome project yet its highly underutilized right now, such features will help boost its popularity. |
Right, which is why from functionality perspective it can also be a separate project.
And, again, nothing would change, as far as I can see, if it's done as a separate project. It's not going to be distributed together, i.e. .so wouldn't have the rust bits and it'd still need to be packaged into the rust package manager, and I expect people would be pulling it from there. And I these bindings would serve right for an average user because it probably(?) doesn't do good with the rust "safety" model. In which case it'd make more sense as a dependency to higher level libraries / frameworks or for experiments. I do believe it'll be easier to keep it as a separate project, but if we take sth like that, we should open liburing to bindings for all interested languages, and keep the folder structure accordingly, like
But ultimately the question is whether Jens wants maintain rust code, and whether people involved in the project understand rust well enough, and other logistic problems. Questions that wouldn't even exist if it's made as a separate project. |
I can't say I'm excited about that. I'm personally don't plan adjusting bindings, while having broken builds is always nasty. I'd say we either shouldn't do these tricks it at all, or it can be an external project by default testing like that some known liburing hash, which may need an additional makefile target / flag in liburing.
Sure, and that's beyond (at least my) point. Neither I can really comment on the abstractions and what's right or not. |
Fwiw, packaging into a Rust crate includes all the source (C included) and is technically already done. The top-level Cargo.toml takes care of that. It includes the C code and Makefiles required to build liburing.a (or .so, in the future) and the headers so bindgen can generate sensical definitions. This crate is more low-level than what most would hope for in the Rust community but that's the secret to its success, imo. Making an interface sound can be non-trivial and touching io_uring and low-system systems APIs like this is already an unsafe endeavor. I'm not opposed to this being an external project but there's certain logistical issues I'd like to tackle. I like re-using all the liburing tests. Is there a way to get gcc or clang to ignore the It's like I mentioned slightly earlier, to make the Rust code good and actually well-tested, I had to touch a lot of stuff inside liburing. I'm not opposed to just maintaining it as a single patch that I'll continuously reapply against master but that's kind of where the motivation to just merge it all to master came from. Btw, the Rust testing targets in the Makefile aren't part of Maybe a good compromise is that Jens creates a new repo and then we move all this Rust stuff over to that. I hate to appeal to authority here but it'd be significant to the community to have an "official" io_uring Rust crate. |
IIUC your changes to liburing.h were trying to achieve that? There should be no problem with merging sth like that into liburing, the ffi might also benefit from it. By the way, you likely don't want to mirror the C api in full, the library has some legacy parts, at the very least I wouldn't include them.
That's appreciated. However, in my opinion, if there is a target, it should build. Someone will try to build it, and we'll get a constant stream of complains about that (even though that would be only testing problem and not rust api itself). |
True. If it was my project and someone said lets use ... language on the project and I had no clue how to use/want that language I wouldn't bother with it either.
I also agree with this as well but come down to Jens again. |
To elaborate a bit, that statement goes for targets that are supposed to be run by many, and/or end user, and/or automation, like build all, build rust, run normal rust tests and so on. That's in the opposite to some highly specific dev target. |
That actually got me a bit confused. I don't understand why this should be "official", why it's better than other rust approaches to be "official"? I'm pretty sure other binding project have their own reasons, and I don't know it to judge. While it can be published by anyone, and then the merits of the approach will decide if it's used, and not it's "official-ness". |
@isilence What about Zig? |
Update the code to function as a standalone crate that provides native Rust implementations of all the
static inline
functions that live in liburing.h.GHA is updated to build on as many architectures as Rust supports. The crate statically links in liburing.a and the liburing.h header is updated to duplicate a declaration per
static inline
function definition. This is done so the Rust code can reuse all of the existing tests. If a new definition is added to liburing.h and the Rust implementation lags, a linking error is generated when the liburing_rs_test is built.I've never really made a PR this large before and I'm sure I haven't explained nearly enough. This change adds a Rust-native impl of liburing which makes it the most comprehensive/well-tested set of io_uring bindings because it ensures feature parity with liburing.h and also is as well-tested.
git request-pull output:
By submitting this pull request, I acknowledge that: