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

io_uring: implement automatic buffer selection #10197

Merged
merged 7 commits into from May 26, 2022

Conversation

vrischmann
Copy link
Contributor

This implements automatic buffer selection for io_uring.

The current api of read and recv is not great when using automatic buffer selection, we have to write relatively ugly code like this:

var slice: []u8 = "";
slice.len = 128;

var sqe = try ring.read(0xdfdfdfdf, fd, slice[0..], 0);
sqe.flags |= linux.IOSQE_BUFFER_SELECT;
sqe.buf_index = group_id;

because read takes a slice and gives its ptr and len to the kernel via these fields.

This is the reason why I added read_raw and recv_raw which take a ?[*]u8 and a separate length, so we can write code like this instead:

var sqe = try ring.read_raw(0xdfdfdfdf, fd, null, buffer_len, 0);
sqe.flags |= linux.IOSQE_BUFFER_SELECT;
sqe.buf_index = group_id;

which is better imo.

I'm not 100% happy with this API either, because:

  • it feels redundant given the super small code difference between the two methods
  • future operations which take a buffer as input and support automatic buffer selection will also need to implement two different methods.

There are other solutions but they require breaking existing code using read and recv.

  1. define read the same as read_raw
pub fn read(self: *IO_Uring, user_data: u64, fd: os.fd_t, buffer: ?[*]u8, len: usize, offset: u64) !*io_uring_sqe

and let the caller give the correct pointer/len.

  1. we could add a buffer type, something like this:
pub const ReadBuffer = union(enum) {
    slice: []u8,
    buffer_select: usize,
};

pub fn read(self: *IO_Uring, user_data: u64, fd: os.fd_t, buffer: ReadBuffer, offset: u64) !*io_uring_sqe

var buffer: [128]u8 = undefined;
var sqe = try ring.read(0, fd, .{ .slice = &buffer }, 0);

var sqe = try ring.read(0, fd, .{ .buffer_select = buffer_len }, 0);

Any input on this is appreciated, I might have missed something obvious.

@andrewrk
Copy link
Member

andrewrk commented May 11, 2022

This PR had an Azure CI failure and the log was deleted since it is so old, so I rebased this against master.

lib/std/os/linux/io_uring.zig Outdated Show resolved Hide resolved
lib/std/os/linux/io_uring.zig Outdated Show resolved Hide resolved
lib/std/os/linux/io_uring.zig Show resolved Hide resolved
lib/std/os/linux/io_uring.zig Outdated Show resolved Hide resolved
@andrewrk andrewrk added enhancement Solving this issue will likely involve adding new logic or components to the codebase. standard library This issue involves writing Zig code for the standard library. labels May 11, 2022
@vrischmann vrischmann force-pushed the io_uring-provide_buffers branch 2 times, most recently from f074285 to ee484dd Compare May 14, 2022 18:46
@vrischmann
Copy link
Contributor Author

I changed the PR, now IO_Uring.read() takes a ReadBuffer which can be used for simple reads or reads using buffer selection.

Both IO_Uring.readv() and IO_Uring.read_fixed() could be handled in the same way but it should be maybe go in another PR ?

Copy link
Sponsor Contributor

@jorangreef jorangreef left a comment

Choose a reason for hiding this comment

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

Perhaps it makes sense to do the breaking changes in one go? Would be really nice to get recv() into this PR since it's so similar to read().

lib/std/os/linux/io_uring.zig Outdated Show resolved Hide resolved
lib/std/os/linux/io_uring.zig Outdated Show resolved Hide resolved
lib/std/os/linux/io_uring.zig Outdated Show resolved Hide resolved
@vrischmann
Copy link
Contributor Author

Perhaps it makes sense to do the breaking changes in one go? Would be really nice to get recv() into this PR since it's so similar to read().

Ok, I can do that.

@vrischmann vrischmann force-pushed the io_uring-provide_buffers branch 3 times, most recently from e06dcc9 to c8bb3a9 Compare May 16, 2022 20:05
@vrischmann
Copy link
Contributor Author

I've added support for buffer selection for recv.
I've also removed the readv method; read can now call io_uring_prep_readv for ReadBuffer.iovecs.

This means that I have to handle the iovecs case in recv and there's no API to use a iovec for recv so I simply make a slice from the first buffer in the iovec.
Another option might be to just return a "not supported" error ?

@jorangreef
Copy link
Sponsor Contributor

I've added support for buffer selection for recv.
I've also removed the readv method; read can now call io_uring_prep_readv for ReadBuffer.iovecs.

Thanks for doing this!

This means that I have to handle the iovecs case in recv and there's no API to use a iovec for recv
so I simply make a slice from the first buffer in the iovec.
Another option might be to just return a "not supported" error ?

Rather than using ReadBuffer also for recv(), I think it would be better to use a separate, more restricted RecvBuffer type to eliminate this case completely.

@vrischmann
Copy link
Contributor Author

vrischmann commented May 20, 2022

Rather than using ReadBuffer also for recv(), I think it would be better to use a separate, more restricted RecvBuffer type to eliminate this case completely.

Ok makes sense, I'll change it

@vrischmann
Copy link
Contributor Author

I've pushed an update which uses the RecvBuffer type for recv().

These functions are needed to implement automatic buffer selection.

This maps to the IORING_OP_PROVIDE_BUFFERS and IORING_OP_PROVIDE_BUFFERS ops.
Reads can be done in two ways with io_uring:
* using a simple buffer
* using a automatic buffer selection which requires the user to have
provided a number of buffers before

ReadBuffer let's the caller choose where the data should be read.
RecvBuffer is equivalent to ReadBuffer but tailored for recv only.
readv() is essentially identical to read() except for the buffer type,
this simplifies the API for the caller at the cost of not clearly mapping to the liburing C API.
@andrewrk andrewrk merged commit ba426f0 into ziglang:master May 26, 2022
@andrewrk andrewrk added the breaking Implementing this issue could cause existing code to no longer compile or have different behavior. label May 26, 2022
@andrewrk
Copy link
Member

Thanks @vrischmann and reviewers!

@vrischmann vrischmann deleted the io_uring-provide_buffers branch May 26, 2022 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Implementing this issue could cause existing code to no longer compile or have different behavior. enhancement Solving this issue will likely involve adding new logic or components to the codebase. standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants