tokio-rs / io-uring

The `io_uring` library for Rust
Apache License 2.0
1.22k stars 134 forks source link

missing multishot commands #187

Open FrankReh opened 1 year ago

FrankReh commented 1 year ago

I'm going to be adding more multishot commands, some or all in separate PRs. But I wanted a place where we could discuss the naming of these opcodes and options. I'm going to list the names that come from the liburing API as a starting point.

The multishot version of recv should not take a buffer pointer or length so a separate opcode seems warranted to me, maybe RecvMulti as a convention I think we're using is to place modifiers in a name after the primary part of the name? If we wanted to use the same opcode, we could add an optional buf_group field, but a buf_group id value of 0 is allowed so we might have to wrap that optional type in an Option in that case or default it to a max value.

The multishot version of recvmsg I'm still confused about. I think parts of the msghdr are still relevant to the function even though the buffer it is designed to point to would not be. I would be tempted to tackle this one last because it's not clear to me from even the liburing documentation how it works or what it does. It might require looking through the kernel source or asking on the liburing repo.

The poll_multishot ends up taking the same arguments as the single-shot poll_add operation, so maybe that doesn't deserve its own opcode here, maybe an optional multishot boolean? But if you want a separate opcode, how about the name PollMulti to go with the PollAdd that is there now?

@quininer @lucab

lucab commented 1 year ago

The multishot version of recvmsg I'm still confused about. I think parts of the msghdr are still relevant to the function even though the buffer it is designed to point to would not be. I would be tempted to tackle this one last because it's not clear to me from even the liburing documentation how it works or what it does. It might require looking through the kernel source or asking on the liburing repo.

I was playing with this one and I have a locally working branch (on kernel 6.1), but I still need to polish and write tests before submitting.

What I've found out so far is that:

FrankReh commented 1 year ago

I was playing with this one and I have a locally working branch (on kernel 6.1), but I still need to polish and write tests before submitting.

That is great. Anytime in the next week or two is probably not a problem. I was hoping to get a tokio-uring release out when all of these PRs were reviewed and merged but there are a number that haven't been reviewed in that repo either so a new release is likely a week or two out anyway. I feel your pain, getting the test in is often harder than adding the binding.

FrankReh commented 1 year ago

They are all supported now. See the unit test named test_udp_recvmsg_multishot for how to use the existing opcode::RecvMsg but with proper flags for multishot and see the same test for how to use the new types::RecvMsgOut::parse function to create a RecvMsgOut with methods to access various parts of the data that was received.

lucab commented 1 year ago

There is still one UX question open about how to expose this: https://github.com/tokio-rs/io-uring/blob/6785e666869993f81414a5cfad501ef714c742e9/io-uring-test/src/tests/net.rs#L1295-L1296

On the same topic, it is also relevant to note that multishot recvmsg also needs to have the BUFFER_SELECT flag set: https://github.com/tokio-rs/io-uring/blob/6785e666869993f81414a5cfad501ef714c742e9/io-uring-test/src/tests/net.rs#L1298-L1304

At this point, it looks like all other multishot operations have been placed behind their own FooMulti types, right? If so it may make sense to also introduce a RecvMsgMulti for this, taking care of setting all the relevant flags internally.

FrankReh commented 1 year ago

@lucab I agree. See what @quininer says or just create a PR and see if @quininer accepts it?

quininer commented 1 year ago

We already have RecvMulti, it's nice to have RecvMsgMulti.

FrankReh commented 1 year ago

@lucab ~Do you want to finish this? Do you have time?~ I'll give it a shot now.

FrankReh commented 1 year ago

@lucab Are you aware of anything else, after 201, that should go in before we ask @quininer for a new release to be tagged?

I would suggest updating the sys module to make the next round of work potentially easier on people, but it's not necessary for the currently supported opcodes of course.

lucab commented 1 year ago

Sorry my availability is a bit spotty as I'm busy taking care of other stuff offline. No I don't have other things to merge before the next release.

daniel-trnk commented 3 weeks ago

Possibly dumb question: we ran into an issue with code that used io_uring::probe.is_supported(opcode::RecvMulti::CODE)...
As far as I understand, that does not actually work here because RecvMulti is translating to IORING_OP_RECV plus the flag IORING_RECV_MULTISHOT. Should there be a way to probe for flags? If that is even possible?