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

Sqpoll support #8

Closed
wants to merge 5 commits into from
Closed

Conversation

Licenser
Copy link
Contributor

@Licenser Licenser commented Feb 4, 2020

heavy WIP, trying to add support for SQPOLL not currently working, will cleanup & squash at the end.

@spacejam
Copy link
Owner

spacejam commented Feb 8, 2020

Hey @Licenser, thanks for taking this on! One thing I was wondering about was maybe allowing various file and buffer-accepting operations to accept generic types that implement traits that trigger the flags for registered files / buffers automatically, and adding types RegisteredBuffer and RegisteredFile that know about their registered offset. I haven't sketched out a prototype of this, but that general approach has been on my mind as a way to minimize API surface area and try to minimize misuse. Something like that wouldn't block this work here, but maybe it's something to wonder about. What do you think about that approach in the long run?

By the way, I would quickly accept a PR that just adds the ability to register files / buffers / both, before any methods that use them exist, so we can just iterate on an API like that quickly.

@Licenser
Copy link
Contributor Author

Licenser commented Feb 8, 2020

I like the idea! I'm still fighting with getting a file to register to start with once I get over that bump I think the rest will fall in place easily. I'm in no hurry to merge, there is nothing immediate I would use it for other than to see the effect in regards to #5 so I'd be happy with both flashing out a nice API or getting it merged and iterating :)

@Licenser
Copy link
Contributor Author

Licenser commented Feb 8, 2020

I rebased and added an example, but I'm somewhat stuck with this error:

thread '<unnamed>' panicked at 'error in cqe reaper: Os { code: 6, kind: Other, message: "No such device or address" }', src/io_uring/cq.rs:96:17

The file registers successfully. The manpage states:

.TP
.B IORING_REGISTER_FILES
Register files for I/O.
.I arg
contains a pointer to an array of
.I nr_args
file descriptors (signed 32 bit integers).

To make use of the registered files, the
.B IOSQE_FIXED_FILE
flag must be set in the
.I flags
member of the
.IR "struct io_uring_sqe" ,
and the
.I fd
member is set to the index of the file in the file descriptor array.

registered_file_prep_rw sets IOSQE_FIXED_FILE before committing the operation, and the FD passed in in the example is 0. I think I'm missing something but I'm not sure what.

@Licenser
Copy link
Contributor Author

Traced it a bit further, seems the error above was because once a ring is created with sqpoll it can't be used with normal files. I wonder if it should error when it's attempted to be used that way?

Anyway moving things forward it seems the completion for sqpoll requests hangs here

inner = self.cv.wait(inner).unwrap();

I suspect there could be a deadlock somewhere? Is there something obvious that comes to mind?

@Licenser
Copy link
Contributor Author

With some further digging - it seems that the filler is never executed (Filler::fill) doesn't seem to get called.

@spacejam
Copy link
Owner

I hit something similar when running this from multiple requesting threads in its usage in sled. Will dive into this in the next few days

@Licenser
Copy link
Contributor Author

Thank you! I'll keep digging on my side but I think you'll have more luck ;)

@Licenser Licenser mentioned this pull request Feb 28, 2020
@Licenser
Copy link
Contributor Author

Licenser commented Mar 4, 2020

Interestingly I now get the error:

thread '<unnamed>' panicked at 'error in cqe reaper: Os { code: 6, kind: Other, message: "No such device or address" }', src/io_uring/cq.rs:96:17

on creating the ring when sq_poll is true.

@Licenser Licenser closed this Aug 24, 2024
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.

2 participants