driver: refactor the driver to be stored in a shared set of handles #196

Noah-Kennedy · 2022-12-04T02:33:08Z

This opens up a lot of further cleanup opportunities, especially around shutdown logic. It also opens up the opportunity for us to eventually provide a public Handle API.

I am trying to make it so that we don't need to orchestrate our entire shutdown logic around the thread locals rather than purely the CONTEXT thread local, which is slightly broken to begin with (I believe that there are currently issues around creating two runtimes at the same time).

Now the CONTEXT behaves more like in tokio. When you enter a runtime context via block_on, we set the context. When we leave that call, we unset the context.

This also allows us to have a bit more flexibility with things like sending ops between multiple runtimes as well, so you can now in theory await ops created from another runtime.

Part of the goal of this is to add new options around things like fixed or provided buffers. This potentially allows us to work towards things like transferring buffers between uring runtimes.

More importantly however, it is a crucial first step towards having a handle to a uring runtime.

More refactoring to come.

Noah-Kennedy · 2022-12-04T02:34:01Z

I still need to fix the tests.

Noah-Kennedy · 2022-12-04T02:35:13Z

Might be a good idea to hide whitespace when reviewing.

Noah-Kennedy · 2022-12-04T03:03:37Z

src/io/accept.rs

-                .build()
-            },
-        )
+        CONTEXT.with(|x| {


For many of these op constructors, I plan to have the weak handle in the socket object.

Noah-Kennedy · 2022-12-04T03:07:40Z

src/runtime/driver/handle.rs

+        self.inner.borrow_mut().uring.submit()
+    }
+
+    pub(crate) fn register_buffers(


I'm going to move a bunch of these methods into the Driver and do delegated calls here, but in another PR.

FrankReh · 2022-12-04T03:44:25Z

src/runtime/driver/mod.rs

    // only used in tests rn
    #[allow(unused)]
-    fn num_operations(&self) -> usize {
+    pub(super) fn num_operations(&self) -> usize {


Would be nice if things like this were exposed to the tracing tool, tokio console.

I think that this is a good idea to expose eventually.

src/runtime/driver/handle.rs

This opens up a lot of further cleanup opportunities, especially around shutdown logic. It also opens up the opportunity for us to eventually provide a public Handle API. More refactoring to come.

Noah-Kennedy · 2022-12-04T05:13:51Z

I've fixed the tests and marked this and removed the draft label.

FrankReh · 2022-12-04T14:35:52Z

Not as part of this PR, but perhaps a quick follow-on. What would you think about moving the definitions from all the mod.rs files into their own files? So the mod files are just lines of use and mod themselves? Like most of runtime/mod.rs goes into runtime/runtime.rs and most of runtime/driver/mod.rs goes in runtime/driver/driver.rs and so on.

FrankReh · 2022-12-04T14:49:08Z

Please remind me, why does the tokio_uring::spawn call tokio::task::spawn_local and not use the Runtime's local?

tokio-uring/src/runtime/mod.rs

Lines 55 to 57 in ca8887c

    
           pub fn spawn<T: Future + 'static>(task: T) -> tokio::task::JoinHandle<T::Output> { 
        
               tokio::task::spawn_local(task) 
        
           }

Noah-Kennedy · 2022-12-04T16:31:17Z

Not as part of this PR, but perhaps a quick follow-on. What would you think about moving the definitions from all the mod.rs files into their own files? So the mod files are just lines of use and mod themselves? Like most of runtime/mod.rs goes into runtime/runtime.rs and most of runtime/driver/mod.rs goes in runtime/driver/driver.rs and so on.

This is a really good idea.

Noah-Kennedy · 2022-12-04T16:32:53Z

Please remind me, why does the tokio_uring::spawn call tokio::task::spawn_local and not use the Runtime's local?

tokio-uring/src/runtime/mod.rs

Lines 55 to 57 in ca8887c

pub fn spawn<T: Future + 'static>(task: T) -> tokio::task::JoinHandle<T::Output> {

tokio::task::spawn_local(task)

}

Because this spawn method is static and thus happens via the thread local context in tokio.

FrankReh · 2022-12-04T16:46:27Z

Because this spawn method is static and thus happens via the thread local context in tokio.

But Tokio still adds it to this crate's runtime's localset because that localset is in some sense active, because it all happens in the context of calling rt.block_on(self.local.run_until(..))?

Noah-Kennedy · 2022-12-04T17:10:17Z

Entering a LocalSet context (e.g run_until) sets thread locals within tokio to denote the "current" runtime, kinda like we now are with the drivers.

Noah-Kennedy · 2022-12-04T17:17:31Z

Regarding the goal of this PR, I am trying to make it so that we don't need to orchestrate our entire shutdown logic around the thread locals rather than purely the CONTEXT thread local, which is slightly broken to begin with (I believe that there are currently issues around creating two runtimes at the same time).

Now the CONTEXT behaves more like in tokio. When you enter a runtime context via block_on, we set the context. When we leave that call, we unset the context.

This also allows us to have a bit more flexibility with things like sending ops between multiple runtimes as well, so you can now await ops created from another runtime.

Part of the goal of this is to add new options around things like fixed or provided buffers. This potentially allows us to work towards things like transferring buffers between uring runtimes.

More importantly however, it is a crucial first step towards having a handle to a uring runtime.

src/runtime/driver/op/mod.rs

src/runtime/driver/handle.rs

src/runtime/mod.rs

src/runtime/driver/handle.rs

src/runtime/context.rs

src/io/writev.rs

src/io/recv_from.rs

This cleans up a few different pieces and is a followup change from #196.

#196 broke the ability to populate a collection of fixed buffers off-runtime and then have it registered inside a runtime. There is no need for the driver when a fixed buffer collection is populated. The driver comes in when registering and unregistering the buffers, and then as well, we don't currently need the collection to be affine to the driver. The driver always compares the pointer passed in to prevent unregistering the wrong buffers, so we can just grab the driver from the context. Change the doc examples to show and exercise off-runtime creation of `FixedBufPool` and `FixedBufRegistry`.

Noah-Kennedy commented Dec 4, 2022

View reviewed changes

FrankReh reviewed Dec 4, 2022

View reviewed changes

src/runtime/driver/handle.rs Outdated Show resolved Hide resolved

FrankReh reviewed Dec 4, 2022

View reviewed changes

src/runtime/driver/handle.rs Outdated Show resolved Hide resolved

FrankReh reviewed Dec 4, 2022

View reviewed changes

src/runtime/driver/handle.rs Show resolved Hide resolved

Noah-Kennedy added 3 commits December 3, 2022 22:55

driver: refactor the driver to be stored in a shared set of handles

cb9b8f0

This opens up a lot of further cleanup opportunities, especially around shutdown logic. It also opens up the opportunity for us to eventually provide a public Handle API. More refactoring to come.

fix compilation in tests

1a38706

fix tests

140b505

Noah-Kennedy force-pushed the noah/handle branch from 050282b to 140b505 Compare December 4, 2022 04:55

Noah-Kennedy added 3 commits December 3, 2022 22:59

fix block_on

cc6d9d4

add comment to document the handle schema

b2c0eb8

cleanup cqueue in handle

ca8887c

Noah-Kennedy marked this pull request as ready for review December 4, 2022 05:13

Noah-Kennedy requested a review from FrankReh December 4, 2022 05:32

FrankReh approved these changes Dec 4, 2022

View reviewed changes

Noah-Kennedy merged commit e1b2f1a into master Dec 4, 2022

Noah-Kennedy deleted the noah/handle branch December 4, 2022 17:59

Noah-Kennedy added a commit that referenced this pull request Dec 4, 2022

rt: refactor driver, runtime, and op

7afdc52

This cleans up a few different pieces and is a followup change from #196.

Noah-Kennedy mentioned this pull request Dec 4, 2022

rt: refactor driver, runtime, and op #197

Merged

Noah-Kennedy added a commit that referenced this pull request Dec 6, 2022

rt: refactor driver, runtime, and op (#197)

cdedbfc

This cleans up a few different pieces and is a followup change from #196.

mzabaluev mentioned this pull request Dec 10, 2022

Restore off-runtime creation of fixed buffer collections #200

Merged

driver: refactor the driver to be stored in a shared set of handles #196

driver: refactor the driver to be stored in a shared set of handles #196

Uh oh!

Conversation

Noah-Kennedy commented Dec 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Noah-Kennedy commented Dec 4, 2022

Uh oh!

Noah-Kennedy commented Dec 4, 2022

Uh oh!

Noah-Kennedy Dec 4, 2022

Choose a reason for hiding this comment

Uh oh!

Noah-Kennedy Dec 4, 2022

Choose a reason for hiding this comment

Uh oh!

FrankReh Dec 4, 2022

Choose a reason for hiding this comment

Uh oh!

Noah-Kennedy Dec 4, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Noah-Kennedy commented Dec 4, 2022

Uh oh!

FrankReh commented Dec 4, 2022

Uh oh!

FrankReh commented Dec 4, 2022

Uh oh!

Noah-Kennedy commented Dec 4, 2022

Uh oh!

Noah-Kennedy commented Dec 4, 2022

Uh oh!

FrankReh commented Dec 4, 2022

Uh oh!

Noah-Kennedy commented Dec 4, 2022

Uh oh!

Noah-Kennedy commented Dec 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Noah-Kennedy commented Dec 4, 2022 •

edited

Loading

Noah-Kennedy commented Dec 4, 2022 •

edited

Loading