search

axboe / liburing

Library providing helpers for the Linux kernel io_uring support
MIT License
2.87k stars 402 forks source link

Question about performant use of IORING_REGISTER_BUFFERS #272

Closed goldsteinn closed 3 years ago

goldsteinn commented 3 years ago

Is there any performance issue or synchronization that would take place if multiple IORING_OP_READ_FIXED / IORING_OP_WRITE_FIXED calls use the same buffer index in IORING_REGISTER_BUFFERS assuming the address ranges ([.addr, addr + len]) never overlap (assume offset is always 0 as well).

What I am considering doing is implementing a lightweight malloc for the read / write buffers using a single 1GB register buffer. (This seems like it would be lighter weight that IORING_OP_PROVIDE_BUFFERS and IORING_BUFFER_SELECT).

I took a look at io_import_fixed and I don't see any mechanism for locking buffers by buf_index but I would appreciate if anyone who knew the code base could verify / correct my understanding.

Thank you!

isilence commented 3 years ago

Is there any performance issue or synchronization that would take place if multiple IORING_OP_READ_FIXED / IORING_OP_WRITE_FIXED calls use the same buffer index in IORING_REGISTER_BUFFERS assuming the address ranges ([.addr, addr + len]) never overlap (assume offset is always 0 as well).

There is no additional overhead for that. Note that the kernel manages buffers but not what's in them, so if user issues concurrent IO to overlaping memory chunks, it may get mixed/etc. data.

isilence commented 3 years ago

What I am considering doing is implementing a lightweight malloc for the read / write buffers using a single 1GB register buffer. (This seems like it would be lighter weight that IORING_OP_PROVIDE_BUFFERS and IORING_BUFFER_SELECT).

fwiw, registered buffers are great because memory is pinned and prepared, so if underlaying file may benefit from it, it will. E.g. direct IO will save a lot, but IIRC buffered would not do much better.

I took a look at io_import_fixed and I don't see any mechanism for locking buffers by buf_index but I would appreciate if anyone who knew the code base could verify / correct my understanding.

Not sure why would you even lock by index. Registration and unregistration are currently implemented as full quiesce of the ring.

goldsteinn commented 3 years ago

but IIRC buffered would not do much better.

registered buffers or provided buffers?

Also as a side note are you working on fd == -2 -> skip update? If not I thought I'd give it a shot as it seems simple enough a change. I have 0 experience with linux contributions so if you are already working on it probably best for you to do the patch.

goldsteinn commented 3 years ago

This patch makes it so that specify a file descriptor value of -2 will skip updating the corresponding fixed file index.

This will allow for users to reduce the number of syscalls necessary to update a sparse file range when using the fixed file option.

Answering the github thread -- it's indeed a simple change, I had it the same day you posted the issue. See below it's a bit cleaner. However, I want to first review "io_uring: buffer registration enhancements", and if it's good, for easier merging/etc I'd rather prefer to let it go first (even if partially).

Noah, want to give it a try?

Hi @isilence I would love to give it a shot (I will reply as such to kernel thread). In the interest of not polluting the development thread with too many questions I'm asking a few here.

I've just sent a prep patch

I am sure what you mean by prep patch. Do you mean the patch "[PATCH 1/1] io_uring: cleanup files_update looping"? Thats the only recent patch I see from you that touches __io_sqe_files_update

with it you can implement it cleaner with one continue.

Got it.

isilence commented 3 years ago

Hi @isilence I would love to give it a shot (I will reply as such to kernel thread). In the interest of not polluting the development thread with too many questions I'm asking a few here.

Consistency is better, those who are not interested can easily skip it, but torn threads bouncing between email and github would give hard time retrospectively collecting these pieces.

I've just sent a prep patch

I am sure what you mean by prep patch. Do you mean the patch "[PATCH 1/1] io_uring: cleanup files_update looping"? Thats the only recent patch I see from you that touches __io_sqe_files_update

Right this one. Jens took it but haven't yet pushed into a repo. See 5.12 dev branch https://git.kernel.dk/cgit/linux-block/log/?h=for-5.12/io_uring

It should appear there later, but you can always apply it yourself to not waste time

isilence commented 3 years ago

@goldsteinn, was it answered? It can be closed, I guess