Skip to content
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

Internalize uring #285

Merged
merged 2 commits into from
Mar 3, 2021
Merged

Internalize uring #285

merged 2 commits into from
Mar 3, 2021

Conversation

glommer
Copy link
Collaborator

@glommer glommer commented Mar 2, 2021

What does this PR do?

Internalizes uring-sys and iou, hopefully in a temporary manner, until all the code we need makes into their crates release

Motivation

Those crates are moving too slowly, and blocking our progress in a significant manner

Related issues

#283
#243

This prepares us for the release of a new version of liburing.
iou and uring-sys changed a bit, in particular iou. Most of the
changes stem from the fact that we now can (should) use the nix
versions of things like MsgFlags. The registrar and sqe apis
are also marginally different and are updated

Note that this patch, on itself, will not compile as it needs
matching iou and uring-sys. The next patch will fix that as it brings
those dependencies inside glommio.
Copy link
Contributor

@mxxo mxxo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good. There are a lot of u32 as usize casts, is the long term idea to use u32s everywhere?

I raised a couple of open iou issues that might impact us. There is a leftover swap file glommio/src/iou/.submission_queue.rs.swp.

unsigned int nbytes,
unsigned int splice_flags)
{
io_uring_prep_splice(sqe, fd_in, fd_out, off_in, off_out, nbytes, splice_flags);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a bug with the argument order (fd_in, off_in, not fd_in, fd_out) (ringbahn/uring-sys#25), maybe it's worth addressing it here right away.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review!

I would prefer not to touch this for now because we don't use splice. That said, if uring-sys keeps being unmaintained then maybe we will. Let's leave that on hold for a while.

) -> Option<SQEs<'a>> {
atomic::fence(Ordering::Acquire);

let head: u32 = *sq.khead;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a possible issue with prepare_sqes and thread safety here that should be addressed: ringbahn/iou#61

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue indicates that the issue only happens with sqpool mode, right?
We don't use that, so I'll take the same approach

@glommer
Copy link
Collaborator Author

glommer commented Mar 2, 2021

Thanks a lot!

The .swp file was on purpose, just so I could tell the world how I love vim.
(J/k, I finished this late, thanks for catching!)

I prefer usizes for higher layer returns (like wait) ,but iou, which used usizes, change most of it (but not all, I have a consistency issue opened with them for the buffer ids) to u32. My idea is to translate from u32 to usize before returning from wait

@mxxo
Copy link
Contributor

mxxo commented Mar 2, 2021

That makes sense, usize seems like the right choice for high-level stuff.

@bryandmc
Copy link
Collaborator

bryandmc commented Mar 2, 2021

what is this file: glommio/src/iou/.submission_queue.rs.swp ?? Looks like just an editor or otherwise build artifact type file that accidentally got submitted... still going through the rest but since it's largely a mechanical operation I don't foresee finding too much.

EDIT: just saw that this was commented on already..

Copy link
Collaborator

@bryandmc bryandmc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have nothing to add.. I was also gonna comment on the u32->usize thing but it's been discussed.

Those crates are moving in a fairly slow pace, while uring moves fast.
As much as I fully understand that the maintainers have other
commitments, this is harming us a bit.

Importing this inside glommio is, actually a way to avoid a fork in my
view: I consider this temporary and would encourage anyone that is
sending code that touches iou and uring-sys to make a bona fide effort
to upstream them. But by having them as a buffer here, we can detach
our lifetimes.
@glommer
Copy link
Collaborator Author

glommer commented Mar 3, 2021

Thank you both for the review

@glommer glommer merged commit eaf4290 into DataDog:master Mar 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants