Refactor IO registration using intrusive linked list #2828

seanmonstar · 2020-09-10T21:46:02Z

This refactors IO registration in a few ways:

Cleans up the cached readiness in PollEvented. This cache used to be helpful when readiness was a linked list of *mut Nodes in Registration. Previous refactors have turned Registration into just an AtomicUsize holding the current readiness, so the cache is just extra work and complexity. Gone.
Polling the Registration for readiness now gives a ReadyEvent, which includes the driver tick. This event must be passed back into clear_readiness, so that the readiness is only cleared from Registration if the tick hasn't changed. Previously, it was possible to clear the readiness even though another thread had just polled the driver and found the socket ready again.
Registration now also contains an async fn readiness, which stores wakers in an instrusive linked list. This allows an unbounded number of tasks to register for readiness (previously, only 1 per direction (read and write)). By using the intrusive linked list, there is no concern of leaking the storage of the wakers, since they are stored inside the async fn and released when the future is dropped.
Registration retains a poll_readiness(Direction) method, to support AsyncRead and AsyncWrite. They aren't able to use async fns, and so there are 2 reserved slots for those methods.
IO types where it makes sense to have multiple tasks waiting on them now take advantage of this new async fn readiness, such as UdpSocket and UnixDatagram.

Additionally, this makes the io-driver "feature" internal-only (no longer documented, not part of public API), and adds a second internal-only feature, io-readiness, to group together linked list part of registration that is only used by some of the IO types.

After a bit of discussion, changing stream-based transports (like TcpStream) to have async fn read(&self) is punted, since that is likely too easy of a footgun to activate.

cc #2779, #2728

carllerche

Looks great to me 👍 Great job. I appreciate keeping the PR as minimal as possible as well.

I had some super minor nits I posted inline, nothing is a blocker.

I would like to get another reviewer, especially on the unsafe bits. I tagged a few others on the PR.

tokio/src/io/driver/scheduled_io.rs

carllerche · 2020-09-17T22:49:20Z

.github/workflows/ci.yml

@@ -150,11 +150,11 @@ jobs:
        run: cargo install cargo-hack

      - name: check --each-feature
-        run: cargo hack check --all --each-feature -Z avoid-dev-deps
+        run: cargo hack check --all --each-feature --skip io-driver,io-readiness -Z avoid-dev-deps


Why is this needed?

If only io-driver or io-readiness are enabled, it results in a bunch of unused warnings. The other option is to remove the features and tag all the related code with the cfg(any(..)) matrix of things that need the io-driver stuff.

tokio/src/net/udp/socket.rs

carllerche · 2020-09-17T23:10:08Z

tokio/src/sync/task/atomic_waker.rs

    #[cfg(feature = "io-driver")]
+    #[allow(unused)]


You probably can delete it then.

hawkw · 2020-09-21T20:53:39Z

It would definitely be nice to see a performance comparison --- I imagine this could make some cases faster but it would be nice to have some measurement rather than guesses.

Darksonn

I have not looked at the entire thing, but focused on the parts with unsafe.

tokio/src/net/tcp/stream.rs

tokio/src/net/unix/stream.rs

Darksonn · 2020-09-21T21:13:51Z

tokio/src/io/driver/scheduled_io.rs

+                            return Poll::Pending;
+                        }
+
+                        // Explicit drop of the lock to indicate the scope that the
+                        // lock is held. Because holding the lock is required to
+                        // ensure safe access to fields not held within the lock, it
+                        // is helpful to visualize the scope of the critical
+                        // section.
+                        drop(waiters);


This code is correct, but the return seems a bit confusing considering the comment.

Which return is confusing?

Probably the return Poll::Pending? I think it is ok as return clearly indicates the mutex goes away.

Darksonn · 2020-09-21T21:20:22Z

tokio/src/io/driver/scheduled_io.rs

+                        // Safety: State::Done means it is no longer shared
+                        let w = unsafe { &mut *waiter.get() };


I tried to verify this by following the code around, but found it non-obvious. It is still in the intrusive linked list as far as I can see?

Inside ScheduledIo::wake, at line 216, the waiters are drained out of the list when notified.

tokio/src/io/driver/scheduled_io.rs

hawkw

This looks good to me overall! I didn't see any obvious issues with the new implementation beyond a concern about leaving a pointer dangling if the mutex is poisoned.

tokio/src/io/driver/scheduled_io.rs

tokio/Cargo.toml

tokio/src/io/driver/scheduled_io.rs

hawkw · 2020-09-21T21:14:52Z

tokio/src/io/driver/scheduled_io.rs

+
+    impl Drop for Readiness<'_> {
+        fn drop(&mut self) {
+            let mut waiters = self.scheduled_io.waiters.lock().unwrap();


since removing the waiter from the list is necessary to maintain safety invariants, i think we should change this to use LockResult::into_inner to extract a mutex guard even if the lock was poisoned. if we don't ensure it's removed from the list, we'll leave the list with a potentially dangling pointer that is assumed to be valid.

The idea of relying on Drop for safety is scary and probably wrong...

Also, I borrowed a lot of this from notify, like:

tokio/tokio/src/sync/notify.rs

Line 560 in e7091fd

let mut waiters = notify.waiters.lock().unwrap();

The idea of relying on Drop for safety is scary and probably wrong...

Pin guarantees that the memory location remains valid until it is dropped. If it is not dropped, then the memory must be leaked and remain valid forever.

The idea of relying on Drop for safety is scary and probably wrong...

We're relying on the drop impl to maintain safety regardless of whether or not we ignore mutex poisoning here, it's inherent to the intrusive list that nodes must be unlinked before they are deallocated.

I think Notify also needs to needs to ignore poisoning for the same reason. It might actually matter more in that case since there are probably going to be higher-level users of Notify that send a notification when dropped, potentially including while unwinding...

I think this change should be done in a dedicated PR :) I tracked it here: #2867

For now, unwrap() is fine.

tokio/src/io/driver/scheduled_io.rs

Co-authored-by: Eliza Weisman <eliza@buoyant.io> Co-authored-by: Alice Ryhl <alice@ryhl.io>

hawkw · 2020-09-22T22:49:10Z

Looks like rustfmt and loom CI jobs are sad (the loom one looks like an unnecessary reexport or something?)

Uses the infrastructure added by #2828 to enable switching `TcpListener::accept` to use `&self`. This also switches `poll_accept` to use `&self`. While doing introduces a hazard, `poll_*` style functions are considered low-level. Most users will use the `async fn` variants which are more misuse-resistant. TcpListener::incoming() is temporarily removed as it has the same problem as `TcpSocket::by_ref()` and will be implemented later.

seanmonstar requested a review from carllerche September 10, 2020 21:50

seanmonstar force-pushed the io-driver-intrusive branch 4 times, most recently from 3dffee1 to b26ed76 Compare September 10, 2020 22:19

seanmonstar mentioned this pull request Sep 10, 2020

Figure out plans for UdpFramed #2830

Closed

seanmonstar force-pushed the io-driver-intrusive branch 11 times, most recently from ee8a4b6 to 9ce4e61 Compare September 11, 2020 01:19

Refactor IO registration using intrusive linked list

9d92742

seanmonstar force-pushed the io-driver-intrusive branch 5 times, most recently from 2c4f851 to 1faa353 Compare September 12, 2020 02:31

Put readiness stuff in io-readiness feature

7a43d9c

seanmonstar force-pushed the io-driver-intrusive branch from 1faa353 to 7a43d9c Compare September 14, 2020 16:45

Darksonn added A-tokio Area: The main tokio crate M-io Module: tokio/io M-net Module: tokio/net labels Sep 14, 2020

seanmonstar force-pushed the io-driver-intrusive branch from 5533179 to b321df3 Compare September 14, 2020 18:06

fix unused mut in udp example

e1c9192

carllerche approved these changes Sep 17, 2020

View reviewed changes

carllerche requested review from Matthias247 and udoprog September 17, 2020 23:12

Darksonn reviewed Sep 21, 2020

View reviewed changes

hawkw approved these changes Sep 21, 2020

View reviewed changes

Apply suggestions from code review

c92db21

Co-authored-by: Eliza Weisman <eliza@buoyant.io> Co-authored-by: Alice Ryhl <alice@ryhl.io>

seanmonstar added 3 commits September 22, 2020 16:10

Move do_io method to PollEvented

a2ffe53

remove unused code, improve set_readiness comments

f64928b

Merge remote-tracking branch 'origin/master' into io-driver-intrusive

2ba0c30

seanmonstar force-pushed the io-driver-intrusive branch 6 times, most recently from ac42251 to f823cf5 Compare September 23, 2020 16:36

update signal driver

dbb99f3

seanmonstar force-pushed the io-driver-intrusive branch from f823cf5 to dbb99f3 Compare September 23, 2020 16:40

fix loom unused warning

7293369

This was referenced Sep 23, 2020

WIP: Add try methods to SendHalf and RecvHalf. #2021

Closed

chore: update internal Mutex shim #2867

Closed

seanmonstar mentioned this pull request Sep 23, 2020

net: restore TcpStream::{poll_read_ready, poll_write_ready} #2743

Merged

Merge remote-tracking branch 'origin/master' into io-driver-intrusive

2e9a1b0

carllerche merged commit a055784 into master Sep 23, 2020

carllerche deleted the io-driver-intrusive branch September 23, 2020 20:02

carllerche mentioned this pull request Sep 23, 2020

io: investigate removing io-driver & io-readiness feature flags from Cargo.toml #2869

Closed

carllerche mentioned this pull request Oct 6, 2020

net: use &self with TcpListener::accept #2919

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor IO registration using intrusive linked list #2828

Refactor IO registration using intrusive linked list #2828

seanmonstar commented Sep 10, 2020 •

edited

carllerche left a comment

carllerche Sep 17, 2020

seanmonstar Sep 22, 2020

carllerche Sep 17, 2020

hawkw commented Sep 21, 2020

Darksonn left a comment

Darksonn Sep 21, 2020

seanmonstar Sep 22, 2020

carllerche Sep 23, 2020

Darksonn Sep 21, 2020

seanmonstar Sep 22, 2020

hawkw left a comment

hawkw Sep 21, 2020

seanmonstar Sep 22, 2020

Darksonn Sep 22, 2020

hawkw Sep 23, 2020

carllerche Sep 23, 2020

hawkw commented Sep 22, 2020

		// Safety: State::Done means it is no longer shared
		let w = unsafe { &mut *waiter.get() };

Refactor IO registration using intrusive linked list #2828

Refactor IO registration using intrusive linked list #2828

Conversation

seanmonstar commented Sep 10, 2020 • edited

carllerche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkw commented Sep 21, 2020

Darksonn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkw commented Sep 22, 2020

seanmonstar commented Sep 10, 2020 •

edited