rt: add new api for oneshot op submission and creation #244
Conversation
This change adds a new API for creating and submitting oneshot operations. This new API is intended to obsolete the existing system, however transferring over has been left out of this PR to keep the change small. It is intended that a similar API for multishot operations be landed in a followup PR as well. This was also left out of this PR to keep the change small. This refactoring paves the way for further work on SQE linking, multishot operations, and other improvements and additions. The goal of this change is to allow us to split up the oneshot and multishot logic to allow for a cleaner and more extensible system, allow for user-provided operations, and allow users to control when and how their ops get submitted to the squeue.
| @@ -54,7 +55,7 @@ impl Socket { | |||
|
|
|||
| async fn write_all_slice<T: IoBuf>(&self, mut buf: Slice<T>) -> crate::BufResult<(), T> { | |||
| while buf.bytes_init() != 0 { | |||
| let res = self.write(buf).await; | |||
| let res = self.write(buf).submit().await; | |||
| pub struct UnsubmittedOneshot<D: 'static, T: OneshotOutputTransform<StoredData = D>> { | ||
| stable_data: D, | ||
| post_op: T, | ||
| sqe: squeue::Entry, |
There was a problem hiding this comment.
If this is made pub, does that let callers then add bits to the sqe as they see fit? Or maybe a public method that gives a mutable reference?
| self.submit_with_driver(&handle) | ||
| } | ||
|
|
||
| fn submit_with_driver(self, driver: &driver::Handle) -> InFlightOneshot<D, T> { |
There was a problem hiding this comment.
This helps set the stage for adding public handles.
| post_op: self.post_op, | ||
| }; | ||
|
|
||
| InFlightOneshot { inner: Some(inner) } |
There was a problem hiding this comment.
So is this. A specific type representing the kind of inflight operation.
There was a problem hiding this comment.
My thinking was that these could have cancellation semantics added to them in the future as well as being awaitable as they currently are.
| } | ||
| } | ||
|
|
||
| /// Submit an operation to the driver for batched entry to the kernel. |
There was a problem hiding this comment.
Maybe add that a Future is being returned by this function?
| } | ||
| } | ||
|
|
||
| impl<T: BoundedBuf> UnsubmittedWrite<T> { |
There was a problem hiding this comment.
I would prefer to see this impl that starts at line 38 which is responsible for the struct being init and the squeue Entry being created appearing right after the struct, at line 18. That would make this file better mirror the layout it used to have.
| @@ -41,7 +41,7 @@ fn main() { | |||
| break; | |||
| } | |||
|
|
|||
| let (res, b) = socket.write(b).await; | |||
| let (res, b) = socket.write(b).submit().await; | |||
There was a problem hiding this comment.
Would InfoFuture have avoided these changes being required?
|
@Noah-Kennedy I feel somewhat put out having not begin requested for input here, given the history (especially #165, #171, #120, #218). In addition, this PR appears to have had a lifespan of 8 hours, with minimal discussion, and all other Pr's on this subject have met a (deserved) wall of "let us discuss". It would appear there is a very distinct weighting of opinions here, which I think is a shame. Its certainly discouraging for future involvement. |
|
@ollie-etl It is understandable you feel that way but it is just a difference of working for Noah at the moment. Your input is very valuable. We hadn't heard from you in a while and this is just one of several PRs Noah wants to get in and he's very amenable to changing things quickly as we see fit. What's going in is by no means the final. It's just a way for him and I to make progress while we work on things in parallel for a few days. Please don't feel put out. |
|
@ollie-etl I'll add for myself, another reason I often push back harder on others than on Noah in cases like this is I feel bad if I let someone's PR go through and then Noah or I go about changing it in a few days anyway. It muddies the |
|
@ollie-etl Also, hopefully you noticed I didn't wait for Noah to address my questions with the first PR. And coincidentally, I'm getting my old multi-shot work from before your submissions to line with up at least your recent submissions, just so it compiles again and has a test case working. Knowing I will have to change it again when Noah has the multi version of the new API ready. |
|
@ollie-etl Not getting an official review request just means the next step for us isn't gated by a third party. The GitHub Notifications page shows anyone watching this when things are being proposed. When I haven't totally agreed with you or others, I have meant to step out of the way and let @Noah-Kennedy form the second opinion for approval. In a project like this; it's just not possible to wait for three or more confirmations, there are too many ways of getting something good done. That doesn't mean three or more reviews aren't welcome. Multiple reviews are very welcome. There is a lot to learn from each other - obviously. |
|
@FrankReh I get notification, but 8 hours is quite a turn around time. |
Yes. But I would have made it even shorter if I had had the energy when the PR was first put in. I realize, probably Noah does too, that later PRs are harder to fully comment on because the page doesn't show all the diffs. We'll have to work around that somehow. We can't let GitHub's javascript, as great as it is, totally dictate things for us. I suspect it's not too late to still make comments on this PR that Noah and I will see. |
|
@ollie-etl One more thing. All that being said. Is there a double standard? Well yes. Noah is the tokio core team member. Ultimately things go the way he wants. If you and I don't like it, there are other options. But you have to admit, he is one of the most welcoming people on the tokio team and when he has time, he is very thoughtful in his responses. So he's not a benevolent dictator on the project, but he could be if he wanted to be. The project is only here because he kept it alive for well over a year by himself. |
|
|
@ollie-etl That's very understandable from your side. I don't begrudge you that if you have a better option you should take it - especially because you've said you're doing this for your work and your company is paying you to make wise choices - but I hope you don't come to that decision. And I agree you've had some PRs that take a long time. And anything that had an endless discussion in it was because I was on the other end, I know that - if Noah had ever thought I was making too fine a point on something, he would have chimed in (was my feeling). In my own defense, I think if I wasn't at that end, you might not have heard anything for a while. Again, ironically, in between this thread, I am literally copying, modifying your excellent work on the MultiCQEFuture to get a MultiCQEStream working. Knowing it is better than what I had cobbled together back in Sept and also knowing when it's done, it will live in one or two branches just long enough to then go through another change. You can also take your time to decide what to do. If you care to review things in the meantime anyway, that would be great. Stepping back even further for a moment. This is a very hard open source project. It marries Linux io_uring with Rust and async Rust and tokio async Rust - which also makes a very interesting intersection to be at (and why I'm here, even without a job at the moment). There are a ton of ways to get things done in Rust and async Rust. I look at I am not trying to have the last word. Your last comment stood very well on it's own. You would be missed if you leave. |
|
Hi guys and @Noah-Kennedy. Is there any following work or further discussion to introduce link ops here based on this API? (I tried to find but nothing found on my side.) If not, we're willing to continue follow-ups for link ops on our own and want to hear what were previous design choices, idea and gaps in here. |
This change adds a new API for creating and submitting oneshot operations. This new API is intended to obsolete the existing system, however transferring over has been left out of this PR to keep the change small.
It is intended that a similar API for multishot operations be landed in a followup PR as well. This was also left out of this PR to keep the change small.
This refactoring paves the way for further work on SQE linking, multishot operations, and other improvements and additions.
The goal of this change is to allow us to split up the oneshot and multishot logic to allow for a cleaner and more extensible system, allow for user-provided operations, and allow users to control when and how their ops get submitted to the squeue.