rt: don't notify when pushing task to empty LIFO slot

[hawkw]

See #8065

This change un-does the increase in notifications described in #8065. Running the repro from #8065 (comment), I see voluntary context switches back to 1.50.0 counts (note the cargo build output saying that this is a local Tokio checkout):

eliza@hekate ~/Code/tokio-8065 $ cargo build --release
   Compiling tokio v1.52.1 (/home/eliza/Code/tokio/tokio)
   Compiling tokio-8065 v0.1.0 (/home/eliza/Code/tokio-8065)
    Finished `release` profile [optimized] target(s) in 2.76s

eliza@hekate ~/Code/tokio-8065 $ nix run nixpkgs#time -- -v target/release/tokio-8065
	Command being timed: "target/release/tokio-8065"
	User time (seconds): 0.01
	System time (seconds): 0.00
	Percent of CPU this job got: 0%
	Elapsed (wall clock) time (h:mm:ss or m:ss): 0:10.00
	Average shared text size (kbytes): 0
	Average unshared data size (kbytes): 0
	Average stack size (kbytes): 0
	Average total size (kbytes): 0
	Maximum resident set size (kbytes): 1984
	Average resident set size (kbytes): 0
	Major (requiring I/O) page faults: 0
	Minor (reclaiming a frame) page faults: 146
	Voluntary context switches: 6563
	Involuntary context switches: 1
	Swaps: 0
	File system inputs: 0
	File system outputs: 0
	Socket messages sent: 0
	Socket messages received: 0
	Signals delivered: 0
	Page size (bytes): 4096
	Exit status: 0
``

[hawkw]

However, I'm not 100% sure that this change is correct, given #8065 (comment) (which is what originally prompted me to remove the should_notify boolean and only notify a parked worker if the LIFO slot was not empty. In that comment, @Darksonn points out that the previous intent behind having should_notify was that we should be always notifying a parked worker in any case where a task is available to steal, and that we only skipped doing so when pushing to an empty LIFO slot because the LIFO task was not stealable. Now, we are introducing a third state, in which a task is available to steal, but we haven't notified another worker. I'm not sure if this is correct off the top of my head.

I worry that it would be possible to re-introduce the deadlock that can occur when a task is placed in the LIFO slot and the worker thread is immediately blocked by the task that notified it, because the blocked worker won't have any further reasons to unpark another worker while it is blocked. However, the rt_threaded::lifo_stealable test I added to reproduce such deadlocks does still pass after this change. I think that's because the test involves another task whose job is to keep the non-blocked worker from parking:

tokio/tokio/tests/rt_threaded.rs

Lines 733 to 743 in b010b5d

	// Keep the runtime busy so that the workers that might steal the
	// blocked task don't all park themselves forever.
	//
	// Since this task will always be woken by whichever worker is holding
	// the time driver, rather than a worker that's executing tasks, it
	// shouldn't ever kick the victim task out of its worker's LIFO slot.
	let churn = tokio::spawn(async move {
	loop {
	tokio::time::sleep(Duration::from_millis(4)).await;
	}
	});

I wonder if removing that task from the test makes it pass on 1.52.1 but hang on this branch. Let's find out..

[hawkw]

I wonder if removing that task from the test makes it pass on 1.52.1 but hang on this branch. Let's find out..

Yup. If we remove churn from the test, like this:

diff --git a/tokio/tests/rt_threaded.rs b/tokio/tests/rt_threaded.rs
index 133ba4c3..ad00abaa 100644
--- a/tokio/tests/rt_threaded.rs
+++ b/tokio/tests/rt_threaded.rs
@@ -730,18 +730,6 @@ fn lifo_stealable() {
         .unwrap();

     rt.block_on(async {
-        // Keep the runtime busy so that the workers that might steal the
-        // blocked task don't all park themselves forever.
-        //
-        // Since this task will always be woken by whichever worker is holding
-        // the time driver, rather than a worker that's executing tasks, it
-        // shouldn't ever kick the victim task out of its worker's LIFO slot.
-        let churn = tokio::spawn(async move {
-            loop {
-                tokio::time::sleep(Duration::from_millis(4)).await;
-            }
-        });
-
         let victim_task_joined = tokio::spawn(async move {
             println!("[victim] task started");
             task_started_tx.send(()).unwrap();
@@ -791,7 +779,6 @@ fn lifo_stealable() {
         // Before possibly panicking, make sure that we wake up the blocker task
         // so that it doesn't stop the runtime from shutting down.
         block_thread_tx.send(()).unwrap();
-        churn.abort();
         result
             .expect("task in LIFO slot should complete within 30 seconds")
             .expect("task in LIFO slot should not panic");

...then the test still passes on master (b010b5d), but fails on this branch (b010b5d):

eliza@hekate ~/Code/tokio $ git co master
Switched to branch 'master'
Your branch is up to date with 'origin/master'.

eliza@hekate ~/Code/tokio $ git rev-parse HEAD
b010b5ddafb316e5d540de1736f5f7bfde42ec39

eliza@hekate ~/Code/tokio $ cargo nextest run lifo_stealable
   Compiling examples v0.0.0 (/home/eliza/Code/tokio/examples)
   Compiling tokio-util v0.7.18 (/home/eliza/Code/tokio/tokio-util)
   Compiling tokio v1.52.1 (/home/eliza/Code/tokio/tokio)
    Finished `test` profile [unoptimized + debuginfo] target(s) in 2.62s
────────────
 Nextest run ID 004702f1-2de3-42e8-b18c-69606f942b2f with nextest profile: default
    Starting 1 test across 235 binaries (1717 tests skipped)
        PASS [   0.003s] tokio::rt_threaded lifo_stealable
────────────
     Summary [   0.005s] 1 test run: 1 passed, 1717 skipped

eliza@hekate ~/Code/tokio $ git checkout eliza/should-notify-again
M	tokio/tests/rt_threaded.rs
Switched to branch 'eliza/should-notify-again'
Your branch is up to date with 'origin/eliza/should-notify-again'.

eliza@hekate ~/Code/tokio $ git rev-parse HEAD
bc62e2d94cb84d556af7d6a707405baa1058e832

eliza@hekate ~/Code/tokio $ cargo nextest run lifo_stealable
   Compiling tokio v1.52.1 (/home/eliza/Code/tokio/tokio)
   Compiling tokio-stream v0.1.18 (/home/eliza/Code/tokio/tokio-stream)
   Compiling tokio-util v0.7.18 (/home/eliza/Code/tokio/tokio-util)
   Compiling tests-integration v0.1.0 (/home/eliza/Code/tokio/tests-integration)
   Compiling tokio-test v0.4.5 (/home/eliza/Code/tokio/tokio-test)
   Compiling tokio-macros v2.7.0 (/home/eliza/Code/tokio/tokio-macros)
   Compiling stress-test v0.1.0 (/home/eliza/Code/tokio/stress-test)
   Compiling examples v0.0.0 (/home/eliza/Code/tokio/examples)
    Finished `test` profile [unoptimized + debuginfo] target(s) in 3.77s
────────────
 Nextest run ID 67fc6e9f-99cd-48d7-bddd-582cce200425 with nextest profile: default
    Starting 1 test across 235 binaries (1717 tests skipped)
        FAIL [  30.020s] tokio::rt_threaded lifo_stealable
  stdout ───

    running 1 test
    [victim] task started
    [victim] task waiting for wakeup...
    [main] victim slot task start acked!
    [main] blocker task spawned
    [blocker] sending wakeup
    [blocker] blocking the worker thread...
    [main] result: Err(Elapsed(()))
    [blocker] done
    [victim] task running after wakeup
    test lifo_stealable ... FAILED

    failures:

    failures:
        lifo_stealable

    test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 28 filtered out; finished in 30.02s

  stderr ───

    thread 'lifo_stealable' (3775832) panicked at tokio/tests/rt_threaded.rs:783:14:
    task in LIFO slot should complete within 30 seconds: Elapsed(())
    stack backtrace:
       0: __rustc::rust_begin_unwind
                 at /rustc/59807616e1fa2540724bfbac14d7976d7e4a3860/library/std/src/panicking.rs:689:5
       1: core::panicking::panic_fmt
                 at /rustc/59807616e1fa2540724bfbac14d7976d7e4a3860/library/core/src/panicking.rs:80:14
       2: core::result::unwrap_failed
                 at /rustc/59807616e1fa2540724bfbac14d7976d7e4a3860/library/core/src/result.rs:1867:5
       3: core::result::Result<T,E>::expect
                 at /home/eliza/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/result.rs:1185:23
       4: rt_threaded::lifo_stealable::{{closure}}
                 at ./tests/rt_threaded.rs:783:14
       5: tokio::runtime::park::CachedParkThread::block_on::{{closure}}
                 at ./src/runtime/park.rs:284:71
       6: tokio::task::coop::with_budget
                 at ./src/task/coop/mod.rs:167:5
       7: tokio::task::coop::budget
                 at ./src/task/coop/mod.rs:133:5
       8: tokio::runtime::park::CachedParkThread::block_on
                 at ./src/runtime/park.rs:284:31
       9: tokio::runtime::context::blocking::BlockingRegionGuard::block_on
                 at ./src/runtime/context/blocking.rs:66:14
      10: tokio::runtime::scheduler::multi_thread::MultiThread::block_on::{{closure}}
                 at ./src/runtime/scheduler/multi_thread/mod.rs:92:22
      11: tokio::runtime::context::runtime::enter_runtime
                 at ./src/runtime/context/runtime.rs:65:16
      12: tokio::runtime::scheduler::multi_thread::MultiThread::block_on
                 at ./src/runtime/scheduler/multi_thread/mod.rs:91:9
      13: tokio::runtime::runtime::Runtime::block_on_inner
                 at ./src/runtime/runtime.rs:373:50
      14: tokio::runtime::runtime::Runtime::block_on
                 at ./src/runtime/runtime.rs:345:18
      15: rt_threaded::lifo_stealable
                 at ./tests/rt_threaded.rs:732:8
      16: rt_threaded::lifo_stealable::{{closure}}
                 at ./tests/rt_threaded.rs:701:20
      17: core::ops::function::FnOnce::call_once
                 at /home/eliza/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5
      18: <fn() -> core::result::Result<(), alloc::string::String> as core::ops::function::FnOnce<()>>::call_once
                 at /rustc/59807616e1fa2540724bfbac14d7976d7e4a3860/library/core/src/ops/function.rs:250:5
    note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

  Cancelling due to test failure:
────────────
     Summary [  30.023s] 1 test run: 0 passed, 1 failed, 1717 skipped
        FAIL [  30.020s] tokio::rt_threaded lifo_stealable
error: test run failed

eliza@hekate ~/Code/tokio $ 

Unfortunately, I think that this means we should not move forwards with putting back a should_notify boolean to skip notifying another worker when only the LIFO slot is occupied, because that results in us failing to prevent the deadlock in cases where there isn't another very active task to keep other workers from parking. Instead, I think we should remove churn() from the test so that it doesn't rely on another task stopping workers from parking.

Sadly, this means we have to accept the performance impact of additional notifications as part of the cost of preventing deadlocks here. Personally, I feel that this is appropriate, because I don't think we should consider a performance decrease from code that behaved incorrectly to be a "regression".

In the long term, I think we might be able to reduce the amount of cross-thread notifications here by changing the logic for notifying a worker when pushing tasks to the queue so that it is less eager about waking workers in general. At present we always notify another worker if there are no workers currently in searching and any workers are parked:

tokio/tokio/src/runtime/scheduler/multi_thread/idle.rs

Line 153 in b010b5d

fn notify_should_wakeup(&self) -> bool {

We might consider changing the "lifo slot only" case to notify only if all other workers are parked, or something, so that we will notify here if it is necessary to prevent a deadlock, but not if there are no other currently searching workers. But I think we'll want to think carefully about that.

@Darksonn, what do you think?

[Darksonn]

I think it is basically correct that you must issue these wakeups to avoid issues where a blocking task prevents execution of the task in the lifo slot.

We could perhaps consider entirely different solutions where a second thread wakes up periodically to check if the running worker thread is making progress. Of course that's a bigger runtime change.