Avoid pinning notified future #3098

zaharidichev · 2020-11-05T15:02:39Z

Motivation

Previously in order for the consumer to
register interest, in a notification triggered by
notify_waiters, the Notified future had to be
polled. This introduced friction when using the api
as the future had to be pinned before polled.

Solution

This change introduces a counter that tracks how many
times notified_waiters has been called. Upon creation of
the future the number of times is loaded. When first
polled the future compares this number with the count
state of the Notify type. This avoids the need for
registering the waiter upfront.

Fixes: #3066

Signed-off-by: Zahari Dichev zaharidichev@gmail.com

This PR changes the way `notify_waiters`, is used. Previously in order for the consumer to register interest, in a notification triggered by `notify_waiters`, the `Notified` future had to be polled. This introduced friction when using the api as the future had to be pinned before polled. This change introduces a counter that tracks how many times `notified_waiters` has been called. Upon creation of the future the number of times is loaded. When first polled the future compares this number with the count state of the `Notify` type. This avoids the need for registering the waiter upfront. Fixes: tokio-rs#3066 Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

carllerche

Looks good to me 👍 I have an inline comment. We could punt this to another PR.

I would like another review before merging. Maybe @hawkw ?

tokio/src/sync/notify.rs

hawkw

Overall, this seems pretty straightforward. The potential deadlock @carllerche pointed out in #3098 (comment) is the only blocking issue I could find.

Other than that, you might be able to simplify some of the bit-packing code a little bit using the existing utils, but that's more of a style thing.

tokio/src/sync/notify.rs

hawkw · 2020-11-10T17:59:09Z

tokio/src/sync/notify.rs

+fn inc_num_notify_waiters_calls(data: usize) -> usize {
+    let new = get_num_notify_waiters_calls(data) + 1;
+    (data & STATE_MASK) | (new << NOTIFY_WAITERS_SHIFT)
+}


it seems like this can just be data + (1 << NOTIFY_WAITERS_SHIFT), right?

hawkw · 2020-11-10T18:00:11Z

tokio/src/sync/notify.rs

            // There are no waiting tasks. In this case, no synchronization is
            // established between `notify` and `notified().await`.
+            // All we need to do is increment the number of times this
+            // method was called.
+            self.state.store(inc_num_notify_waiters_calls(curr), SeqCst);


nit: this seems like it could just be a fetch_add?

"just" would actually be more expensive :) A store is usually cheaper than RMW. SeqCst changes that a bit, but the SeqCst isn't strictly necessary here.

@carllerche would Release suffice here ?

@carllerche sure, i was thinking about the SeqCst load-store pair with instructions in between, which is two synchronization points rather than one, right? but if SeqCst is not load bearing here that may not be the case.

My guess is AcqRel is sufficient for Notify, but I have not looked into it much.

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

hawkw

Looks good to me overall! I had some minor suggestions.

hawkw · 2020-11-13T18:16:01Z

tokio/src/sync/notify.rs

@@ -109,7 +109,11 @@ type WaitList = LinkedList<Waiter, <Waiter as linked_list::Link>::Target>;
 /// [`Semaphore`]: crate::sync::Semaphore
 #[derive(Debug)]
 pub struct Notify {
-    state: AtomicU8,
+    // this uses 2 bits to store one of `EMPTY`,


very, very minor nit: other sentences in this comment start with capital letters; let's be consistent:

Suggested change

// this uses 2 bits to store one of `EMPTY`,

// This uses 2 bits to store one of `EMPTY`,

hawkw · 2020-11-13T18:16:24Z

tokio/src/sync/notify.rs

@@ -109,7 +109,11 @@ type WaitList = LinkedList<Waiter, <Waiter as linked_list::Link>::Target>;
 /// [`Semaphore`]: crate::sync::Semaphore
 #[derive(Debug)]
 pub struct Notify {
-    state: AtomicU8,
+    // this uses 2 bits to store one of `EMPTY`,
+    // `WAITING` or "NOTIFIED". The rest of the bits


nit:

Suggested change

// `WAITING` or "NOTIFIED". The rest of the bits

// `WAITING` or `NOTIFIED`. The rest of the bits

hawkw · 2020-11-13T18:19:57Z

tokio/src/sync/notify.rs

+        const NUM_WAKERS: usize = 32;
+
+        let mut wakers: [Option<Waker>; NUM_WAKERS] = Default::default();
+        let mut curr_waker = 0;


nit: i might move this until after the if let on line 394, since we might return early there. we could skip initializing the stack array in that case, since it won't be used. this probably doesn't have a huge performance impact but it might be worth doing.

I would avoid complicating code for micro-optimizations unless it is measurable.

i thought it also reads nicer if these variables are declared closer to where they're actually used, but 🤷

hawkw · 2020-11-13T18:25:48Z

tokio/src/sync/notify.rs

+            while curr_waker < NUM_WAKERS {
+                match waiters.pop_back() {
+                    Some(mut waiter) => {
+                        // Safety: `waiters` lock is still held.
+                        let waiter = unsafe { waiter.as_mut() };

-            assert!(waiter.notified.is_none());
+                        assert!(waiter.notified.is_none());

-            waiter.notified = Some(NotificationType::AllWaiters);
+                        waiter.notified = Some(NotificationType::AllWaiters);

-            if let Some(waker) = waiter.waker.take() {
-                waker.wake();
+                        if let Some(waker) = waiter.waker.take() {
+                            wakers[curr_waker] = Some(waker);
+                            curr_waker += 1;
+                        }


TIOLI: this could be a for loop:

for slot in &mut wakers { match waiters.pop_back() { Some(mut waiter) => { // Safety: `waiters` lock is still held. let waiter = unsafe { waiter.as_mut() }; assert!(waiter.notified.is_none()); waiter.notified = Some(NotificationType::AllWaiters); if let Some(waker) = waiter.waker.take() { *slot = Some(waker); } } None => break, } }

carllerche · 2020-11-13T22:41:41Z

I added a test to cover the potential deadlock when waking in the mutex. I verified it locked in the earlier version and passes now.

carllerche

LGTM. Feel free to merge once @hawkw is happy 👍 thanks.

hawkw

all my remaining feedback is just not-picking, feel free to ship whenever 👍

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

zaharidichev added 2 commits November 5, 2020 14:51

fix comment

4d1e606

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

Darksonn added A-tokio Area: The main tokio crate C-enhancement Category: A PR with an enhancement or bugfix. M-sync Module: tokio/sync labels Nov 5, 2020

zaharidichev added 5 commits November 5, 2020 15:23

fix visibility

dc59a9b

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

add loom test

af54dfc

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

one more waiter

acf6c12

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

add docs

8683a6d

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

add missing imports

3d31e98

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

carllerche reviewed Nov 10, 2020

View reviewed changes

tokio/src/sync/notify.rs Show resolved Hide resolved

hawkw reviewed Nov 10, 2020

View reviewed changes

wake outside of lock

e76755f

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

zaharidichev requested review from carllerche and hawkw November 13, 2020 17:52

hawkw approved these changes Nov 13, 2020

View reviewed changes

test no deadlock in notify on drop

0ddf422

fmt

fa937f8

carllerche approved these changes Nov 13, 2020

View reviewed changes

hawkw approved these changes Nov 14, 2020

View reviewed changes

feedback

8f3d853

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

carllerche merged commit d0ebb41 into tokio-rs:master Nov 16, 2020

This was referenced Feb 5, 2023

sync: fix notify_waiters notifying sequential awaits #5404

Closed

Possible deadlock when a waker is replaced #5429

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid pinning notified future #3098

Avoid pinning notified future #3098

zaharidichev commented Nov 5, 2020

carllerche left a comment

hawkw left a comment

hawkw Nov 10, 2020

hawkw Nov 10, 2020

carllerche Nov 10, 2020

zaharidichev Nov 10, 2020

hawkw Nov 10, 2020

carllerche Nov 13, 2020

hawkw left a comment

hawkw Nov 13, 2020

hawkw Nov 13, 2020

hawkw Nov 13, 2020

carllerche Nov 13, 2020

hawkw Nov 13, 2020

hawkw Nov 13, 2020

carllerche commented Nov 13, 2020

carllerche left a comment

hawkw left a comment

	// this uses 2 bits to store one of `EMPTY`,
	// This uses 2 bits to store one of `EMPTY`,

	// `WAITING` or "NOTIFIED". The rest of the bits
	// `WAITING` or `NOTIFIED`. The rest of the bits

Avoid pinning notified future #3098

Avoid pinning notified future #3098

Conversation

zaharidichev commented Nov 5, 2020

Motivation

Solution

carllerche left a 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

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

Choose a reason for hiding this comment

carllerche commented Nov 13, 2020

carllerche left a comment

Choose a reason for hiding this comment

hawkw left a comment

Choose a reason for hiding this comment