Add UringCmd opcode

[albertofaria]

Add support for IORING_SETUP_SQE128 and IORING_SETUP_CQE32, and add an UringCmd opcode corresponding to IORING_OP_URING_CMD.

These changes should be backward-compatible.

[quininer]

Wow, that's pretty complicated, and frankly I don't like this.

1. it adds some opaque types
2. It makes applications running on normal sized io-uring have unnecessary overhead

I guess linus has the same reaction.
do you have a real scene that needs it? I would like to have time to consider if a better API design exists.

[albertofaria]

Wow, that's pretty complicated

Indeed.

An alternative design would be to have separate "BigEntry" types, and parameterize IoUring with the SQE and CQE types, or maybe making IoUring a trait and providing 4 impls for the 4 small/big SQE/CQE combinations.

do you have a real scene that needs it? I would like to have time to consider if a better API design exists.

We would be using this in libblkio for NVMe passthrough (https://gitlab.com/libblkio/libblkio/-/merge_requests/117), but we can use the kernel UAPI directly for now and switch to io-uring when support is added.

[quininer]

BigEntry sounds good, let's try it.

[albertofaria]

v2:

• Provide separate types for differently-sized SQEs and CQEs, and parameterize IoUring with the SQE and CQE type.
• No longer backward-compatible, but still a simple change for existing users.
• For SQEs there is an Entry trait that is implemented for structs Entry64 and Entry128, and analogously for CQEs.
• There are now separate UringCmd16 and UringCmd80 opcodes that build Entry64s and Entry128s respectively.

The two Entry traits frequently need to be used from the same module to make their methods accessible, but we can't unless we use ... as _, which is slightly awkward. Should they be given differing names?

[quininer]

Although generics make things a more complicated, but I still prefer v2 over v1.

I think the Entry trait is unnecessary, we can specify type explicitly to provide different methods.

impl SubmissionQueue<'_, Entry16> {
    pub unsafe fn push(&mut self, entry: &Entry16) -> Result<(), PushError> { ... }
}

and

impl IoUring<squeue::Entry64, cqueue::Entry16> {
    pub fn builder() -> Builder<squeue::Entry64, cqueue::Entry16> { ... }
}

maybe we can make cqueue::Entry16 keep the name of Entry, remove generics on methods, and make it still backward compatible.

there are currently only 4 combinations, and I think enumerating their builder methods is acceptable. Hopefully the kernel will not have more types of Entry in future.

[albertofaria]

Providing separate IoUring impls for each valid SQE-CQE type combination will make it difficult for users to create generic code that can work on any IoUring<_, _>. The same goes for SubmissionQueue<_> and CompletionQueue<_>.

Perhaps we could just remove all methods from squeue::Entry and cqueue::Entry and keep them as inherent methods in the EntryN types, but still have the traits?

Also, I'm unsure whether it would be best to impl From<Entry16> for Entry32 or impl From<Entry32> for Entry16. (Implementing both is probably not a good idea because the conversion is not round-trip.) What do you think?

[quininer]

This seems to work, it can be define as

pub trait EntryMarker: Copy + Sealed {}

I will consider impl From<Entry16> for Entry32, and provide a method for Entry32 like fn get(self) -> Entry16.

[albertofaria]

v3:

• Rename Entry to EntryMarker, Entry64 to Entry, and Entry16 to Entry.
• Move methods from EntryMarker traits into the EntryN structs themselves.
• impl From<Entry> for Entry32 instead of impl From<Entry32> for Entry.
• This still isn't backward compatible, but required changes to user code are minimal.

[quininer]

I think we can do backwards compatibility like giving the generics builder method a new name.

struct Bar<S = ()> {
    s: S
}

impl Bar<()> {
    pub fn new() -> Self {
        Bar { s: () }
    }
}

impl<S: Default> Bar<S> {
    pub fn new2() -> Self {
        Bar { s: S::default() }
    }
}


fn main() {
    let bar = Bar::new();
    let bar: Bar<u32> = Bar::new2();
}

[albertofaria]

SubmissionQueue::push, SubmissionQueue::push_multiple, and CompletionQueue::fill also can't become generic if we want to keep backward compatibility.

Other than that, are we sure that adding default type parameters to IoUring, SubmissionQueue, and so on will maintain compatibility for every single possible current use of those types?

[quininer]

I believe methods like push can keep generic because when we get a SubmissionQueue<S> , the type of S is deterministic.

[albertofaria]

The trouble is that the compiler can no longer use the type of those methods' arguments for type inference, see for instance the changes required to io-uring-test/src/tests/queue.rs (10f20c2#diff-d0a68ef9fac5d4f6bf1873905a7b3b83bbe153fc356af15d3c3da510de186836).

[quininer]

This is because fill takes an extra generic parameter, which I think we can remove it.

for normal cases, it should work. https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=d53b9587467696272906296a7c6c1cc6

[albertofaria]

v4:

• Rename IoUring::new() to IoUring::generic_new() and IoUring::builder() to IoUring::generic_builder().
• Add new() and builder() methods to IoUring<squeue::Entry, cqueue::Entry>.
• Drop type parameter from methods SubmissionQueue::push, SubmissionQueue::push_multiple, and CompletionQueue::fill.

I'm not knowledgeable enough to know for sure whether adding default type parameters to IoUring, SubmissionQueue, and CompletionQueue can break existing users.

[quininer]

I believe that all generics must be given default parameters, otherwise compatibility will be broken.

[albertofaria]

v5:

• Make Builder's type parameters have defaults.

[quininer]

This looks good, could you add some tests to it and put the new method behind the unstable feature gate?

[albertofaria]

Which of these should be behind unstable?

1. squeue::Entry128 and cqueue::Entry32;
2. cqueue::Entry32::big_cqe;
3. IoUring::generic_new and IoUring::generic_builder;
4. UringCmd16 and UringCmd80.

Testing the new opcodes in CI will be difficult, as uring_cmd can only currently be used for NVMe passthrough. I can make the existing tests generic over IoUring's type parameters, though, and make io-uring-test/src/main.rs run all tests on the 4 combinations of SQE and CQE types. Does that sound good?

[quininer]

I think all should be marked unstable.

We don't need to test it on ci because kernel on ci doesn't support it yet. you can use ci feature (https://github.com/tokio-rs/io-uring/blob/master/io-uring-test/Cargo.toml#L21) to skip it on ci tests.

[albertofaria]

The difficulty is that NVMe passthrough requires CAP_SYS_ADMIN and an actual NVMe device, which can also be a problem for local testing.

[albertofaria]

v6:

• Rebase.
• Make cqueue::EntryMarker extend Into<Entry> and go back to impl From<Entry32> for Entry instead of impl From<Entry> for Entry32, so that code generic over the CQE type can still use Entry::result/user_data/flags, which are inherent methods not provided by cqueue::EntryMarker.
• Conversely make squeue::EntryMarker extend From<Entry>.
• Run all tests against each of the four combinations of SQE and CQE types.
• Add #[cfg(feature = "unstable")] to IoUring::generic_new, IoUring::generic_build, squeue::Entry128, cqueue::Entry32, UringCmd16, and UringCmd80.

[quininer]

I think it also adds not(feature = "ci")

[albertofaria]

Oops, fixed.

[quininer]

This should be marked unsafe and inline.

[albertofaria]

I don't think this needs unsafe: it is just reading some bytes from a region of memory we know is valid. There is no additional contract the caller must follow to avoid undefined behavior.

Should I make it unsafe nonetheless?

[quininer]

I'm not sure it's always a valid memory region, if kernel doesn't write an entry into it, it's probably uninitialized memory.

[quininer]

This should be marked inline.

[quininer]

Thank you!