Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

task: wake local tasks to the local queue when woken by the same thread #5095

Merged
merged 22 commits into from
Oct 13, 2022
Merged

task: wake local tasks to the local queue when woken by the same thread #5095

merged 22 commits into from
Oct 13, 2022

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Oct 11, 2022 •
edited
Loading

Motivation

Currently, when a task spawned on a LocalSet is woken by an I/O driver
or time driver running on the same thread as the LocalSet, the task is
pushed to the LocalSet's locked remote run queue, rather than to its
unsynchronized local run queue. This is somewhat unfortunate, as it
negates some of the performance benefit of having an unsynchronized
local run queue. Instead, tasks are only woken to the local queue when
they are woken by other tasks also running on the local set.

This occurs because the local queue is only used when the CONTEXT
thread-local contains a Context that's the same as the task's
Schedule instance (an Arc<Shared>)'s Context. When the LocalSet
is not being polled, the thread-local is unset, and the local run queue
cannot be accessed by the Schedule implementation for Arc<Shared>.

Solution

This branch fixes this by moving the local run queue into Shared along
with the remote run queue. When an Arc<Shared>'s Schedule impl wakes
a task and the CONTEXT thread-local is None (indicating we are not
currently polling the LocalSet on this thread), we now check if the
current thread's ThreadId matches that of the thread the LocalSet
was created on, and push the woken task to the local queue if it was.

Moving the local run queue into Shared is somewhat unfortunate, as it
means we now have a single field on the Shared type which must not be
accessed from other threads, and must add an unsafe impl Sync for Shared.
owever, it's the only viable way to wake to the local queue
from the Schedule impl for Arc<Shared>, so I figured it was worth
the additional unsafe code. I added a debug assertion to check that the
local queue is only accessed from the thread that owns the LocalSet.

@hawkw hawkw requested a review from carllerche October 11, 2022 22:20
@hawkw
Copy link
Member Author

hawkw commented Oct 11, 2022

uhhh...looks like the assertion is failing on freebsd (https://cirrus-ci.com/task/5075280174776320?logs=test#L1661)?

that's very surprising given that LocalSet being !Send means that the localset will always be dropped on the thread that created it and the assertion failure appears to be in a Drop (given that the process gets SIGABRT'd)...and it works fine for me (on Linux). can...can thread IDs change under freebsd? are they supposed to ?

@github-actions github-actions bot added the R-loom Run loom tests on this PR label Oct 11, 2022
hawkw added a commit that referenced this pull request Oct 12, 2022
@hawkw
runtime: fix Stacked Borrows violation in LocalOwnedTasks
346b7c0 
## Motivation

It turns out that `LocalSet` has...never actually passed Miri. This is
because all of the tests for `LocalSet` are defined as integration tests
in `tokio/tests/task_local_set.rs`, rather than lib tests in
`tokio/src`, and we never run Miri for integration tests.

PR #5095 added a new test reproducing an unrelated bug in `LocalSet`,
which had to be implemented as a lib test, as it must make internal
assertions about the state of the `LocalSet` that cannot be tested with
just the public API. This test failed under Miri, and it turns out that
the reason for this is unrelated to the change in #5095 --- `LocalSet`
uses the `LocalOwnedTasks` type, which turns out to contain a stacked
borrows violation due to converting an `&Header` into an
`NonNull<Header>` when removing a task from the `LocalOwnedTasks` list.

## Solution

Fortunately, this was actually quite easy to fix. The non-`Local`
version of `OwnedTasks` already uses `Task::header_ptr()` to get a
`NonNull<Header>` in its version of `remove`, which avoids this issue.
Therefore, the fix was as simple as updating `LocalOwnedTasks` to do the
same.

I've also added a very simple lib test in `src/task/local_set.rs` that
just creates a `LocalSet` and spawns a single task on it under a
current-thread runtime. This test fails under Miri without the fix in
this PR: https://github.com/tokio-rs/tokio/actions/runs/3237072694/jobs/5303682530

We should probably keep the test so that we're always testing at least
the most trivial `LocalSet` usage under Miri...
@hawkw hawkw mentioned this pull request Oct 12, 2022
Merged
@github-actions github-actions bot removed the R-loom Run loom tests on this PR label Oct 12, 2022
hawkw added a commit that referenced this pull request Oct 12, 2022
@hawkw
runtime: fix Stacked Borrows violation in LocalOwnedTasks (#5099)
ca9dd72 
## Motivation

It turns out that `LocalSet` has...never actually passed Miri. This is
because all of the tests for `LocalSet` are defined as integration tests
in `tokio/tests/task_local_set.rs`, rather than lib tests in
`tokio/src`, and we never run Miri for integration tests.

PR #5095 added a new test reproducing an unrelated bug in `LocalSet`,
which had to be implemented as a lib test, as it must make internal
assertions about the state of the `LocalSet` that cannot be tested with
just the public API. This test failed under Miri, and it turns out that
the reason for this is unrelated to the change in #5095 --- `LocalSet`
uses the `LocalOwnedTasks` type, which turns out to contain a stacked
borrows violation due to converting an `&Header` into an
`NonNull<Header>` when removing a task from the `LocalOwnedTasks` list.

## Solution

Fortunately, this was actually quite easy to fix. The non-`Local`
version of `OwnedTasks` already uses `Task::header_ptr()` to get a
`NonNull<Header>` in its version of `remove`, which avoids this issue.
Therefore, the fix was as simple as updating `LocalOwnedTasks` to do the
same.

I've also added a very simple lib test in `src/task/local_set.rs` that
just creates a `LocalSet` and spawns a single task on it under a
current-thread runtime. This test fails under Miri without the fix in
this PR: https://github.com/tokio-rs/tokio/actions/runs/3237072694/jobs/5303682530

We should probably keep the test so that we're always testing at least
the most trivial `LocalSet` usage under Miri...
hawkw and others added 15 commits October 12, 2022 12:25
@hawkw
(messy) test that tasks are woken to local queue
9983f4a
@hawkw
wake tasks locally if woken by the same thread
a0b7574
@hawkw
avoid an Arc clone every time we get a thread id
51b4a9f 
this also handles the annoying issue that the thread id's value is not
guaranteed to be correct while dropping a thread-local...
@hawkw
whoops, fix unstable
26703d6
@carllerche @hawkw
debug_assert still compiles code
eadd363
@carllerche @hawkw
see if CI passes
b9df4d1
@carllerche @hawkw
try again
453deb6
@carllerche @hawkw
fix clippy
1dc9d4d
@hawkw
rewrite test to avoid io
a2cf00f
@hawkw
use loom thread IDs (whoops!)
51a3b66 
i forgot to import `Thread`/`ThreadId` from `loom` to use
the mocked thread IDs during loom runs. this should debreak the loom
tests.
@hawkw
rm unused test code
80a501a
@hawkw
try to get a panic message out of macos ci
4e97d74
@hawkw
ladies and gentlemen, we got him
c4e0cc1
@hawkw
fix possible double panic while dropping
f2564c1
@hawkw
+docs
1aa7a39
@github-actions github-actions bot added the R-loom Run loom tests on this PR label Oct 12, 2022
@github-actions github-actions bot removed the R-loom Run loom tests on this PR label Oct 12, 2022
carllerche
carllerche approved these changes Oct 13, 2022
View reviewed changes
Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks.

@carllerche carllerche merged commit 23a1ccf into master Oct 13, 2022
@carllerche carllerche deleted the eliza/localset-local-wake branch October 13, 2022 18:27
@Darksonn Darksonn added A-tokio Area: The main tokio crate M-task Module: tokio/task labels Oct 14, 2022
@Darksonn Darksonn mentioned this pull request Nov 17, 2022
Merged
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Nov 22, 2022
Update Rust crate tokio to 1.22.0 (#1651)
20d459b 
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [tokio](https://tokio.rs) ([source](https://github.com/tokio-rs/tokio)) | dependencies | minor | `1.21.2` -> `1.22.0` |
| [tokio](https://tokio.rs) ([source](https://github.com/tokio-rs/tokio)) | dev-dependencies | minor | `1.21.2` -> `1.22.0` |

---

### Release Notes

<details>
<summary>tokio-rs/tokio</summary>

### [`v1.22.0`](https://github.com/tokio-rs/tokio/releases/tag/tokio-1.22.0): Tokio v1.22.0

[Compare Source](tokio-rs/tokio@tokio-1.21.2...tokio-1.22.0)

##### Added

-   runtime: add `Handle::runtime_flavor` ([#&#8203;5138])
-   sync: add `Mutex::blocking_lock_owned` ([#&#8203;5130])
-   sync: add `Semaphore::MAX_PERMITS` ([#&#8203;5144])
-   sync: add `merge()` to semaphore permits ([#&#8203;4948])
-   sync: add `mpsc::WeakUnboundedSender` ([#&#8203;5189])

##### Added (unstable)

-   process: add `Command::process_group` ([#&#8203;5114])
-   runtime: export metrics about the blocking thread pool ([#&#8203;5161])
-   task: add `task::id()` and `task::try_id()` ([#&#8203;5171])

##### Fixed

-   macros: don't take ownership of futures in macros ([#&#8203;5087])
-   runtime: fix Stacked Borrows violation in `LocalOwnedTasks` ([#&#8203;5099])
-   runtime: mitigate ABA with 32-bit queue indices when possible ([#&#8203;5042])
-   task: wake local tasks to the local queue when woken by the same thread ([#&#8203;5095])
-   time: panic in release mode when `mark_pending` called illegally ([#&#8203;5093])
-   runtime: fix typo in expect message ([#&#8203;5169])
-   runtime: fix `unsync_load` on atomic types ([#&#8203;5175])
-   task: elaborate safety comments in task deallocation ([#&#8203;5172])
-   runtime: fix `LocalSet` drop in thread local ([#&#8203;5179])
-   net: remove libc type leakage in a public API ([#&#8203;5191])
-   runtime: update the alignment of `CachePadded` ([#&#8203;5106])

##### Changed

-   io: make `tokio::io::copy` continue filling the buffer when writer stalls ([#&#8203;5066])
-   runtime: remove `coop::budget` from `LocalSet::run_until` ([#&#8203;5155])
-   sync: make `Notify` panic safe ([#&#8203;5154])

##### Documented

-   io: fix doc for `write_i8` to use signed integers ([#&#8203;5040])
-   net: fix doc typos for TCP and UDP `set_tos` methods ([#&#8203;5073])
-   net: fix function name in `UdpSocket::recv` documentation ([#&#8203;5150])
-   sync: typo in `TryLockError` for `RwLock::try_write` ([#&#8203;5160])
-   task: document that spawned tasks execute immediately ([#&#8203;5117])
-   time: document return type of `timeout` ([#&#8203;5118])
-   time: document that `timeout` checks only before poll ([#&#8203;5126])
-   sync: specify return type of `oneshot::Receiver` in docs ([#&#8203;5198])

##### Internal changes

-   runtime: use const `Mutex::new` for globals ([#&#8203;5061])
-   runtime: remove `Option` around `mio::Events` in io driver ([#&#8203;5078])
-   runtime: remove a conditional compilation clause ([#&#8203;5104])
-   runtime: remove a reference to internal time handle ([#&#8203;5107])
-   runtime: misc time driver cleanup ([#&#8203;5120])
-   runtime: move signal driver to runtime module ([#&#8203;5121])
-   runtime: signal driver now uses I/O driver directly ([#&#8203;5125])
-   runtime: start decoupling I/O driver and I/O handle ([#&#8203;5127])
-   runtime: switch `io::handle` refs with scheduler:Handle ([#&#8203;5128])
-   runtime: remove Arc from I/O driver ([#&#8203;5134])
-   runtime: use signal driver handle via `scheduler::Handle` ([#&#8203;5135])
-   runtime: move internal clock fns out of context ([#&#8203;5139])
-   runtime: remove `runtime::context` module ([#&#8203;5140])
-   runtime: keep driver cfgs in `driver.rs` ([#&#8203;5141])
-   runtime: add `runtime::context` to unify thread-locals ([#&#8203;5143])
-   runtime: rename some confusing internal variables/fns ([#&#8203;5151])
-   runtime: move `coop` mod into `runtime` ([#&#8203;5152])
-   runtime: move budget state to context thread-local ([#&#8203;5157])
-   runtime: move park logic into runtime module ([#&#8203;5158])
-   runtime: move `Runtime` into its own file ([#&#8203;5159])
-   runtime: unify entering a runtime with `Handle::enter` ([#&#8203;5163])
-   runtime: remove handle reference from each scheduler ([#&#8203;5166])
-   runtime: move `enter` into `context` ([#&#8203;5167])
-   runtime: combine context and entered thread-locals ([#&#8203;5168])
-   runtime: fix accidental unsetting of current handle ([#&#8203;5178])
-   runtime: move `CoreStage` methods to `Core` ([#&#8203;5182])
-   sync: name mpsc semaphore types ([#&#8203;5146])

[#&#8203;4948]: tokio-rs/tokio#4948

[#&#8203;5040]: tokio-rs/tokio#5040

[#&#8203;5042]: tokio-rs/tokio#5042

[#&#8203;5061]: tokio-rs/tokio#5061

[#&#8203;5066]: tokio-rs/tokio#5066

[#&#8203;5073]: tokio-rs/tokio#5073

[#&#8203;5078]: tokio-rs/tokio#5078

[#&#8203;5087]: tokio-rs/tokio#5087

[#&#8203;5093]: tokio-rs/tokio#5093

[#&#8203;5095]: tokio-rs/tokio#5095

[#&#8203;5099]: tokio-rs/tokio#5099

[#&#8203;5104]: tokio-rs/tokio#5104

[#&#8203;5106]: tokio-rs/tokio#5106

[#&#8203;5107]: tokio-rs/tokio#5107

[#&#8203;5114]: tokio-rs/tokio#5114

[#&#8203;5117]: tokio-rs/tokio#5117

[#&#8203;5118]: tokio-rs/tokio#5118

[#&#8203;5120]: tokio-rs/tokio#5120

[#&#8203;5121]: tokio-rs/tokio#5121

[#&#8203;5125]: tokio-rs/tokio#5125

[#&#8203;5126]: tokio-rs/tokio#5126

[#&#8203;5127]: tokio-rs/tokio#5127

[#&#8203;5128]: tokio-rs/tokio#5128

[#&#8203;5130]: tokio-rs/tokio#5130

[#&#8203;5134]: tokio-rs/tokio#5134

[#&#8203;5135]: tokio-rs/tokio#5135

[#&#8203;5138]: tokio-rs/tokio#5138

[#&#8203;5138]: tokio-rs/tokio#5138

[#&#8203;5139]: tokio-rs/tokio#5139

[#&#8203;5140]: tokio-rs/tokio#5140

[#&#8203;5141]: tokio-rs/tokio#5141

[#&#8203;5143]: tokio-rs/tokio#5143

[#&#8203;5144]: tokio-rs/tokio#5144

[#&#8203;5144]: tokio-rs/tokio#5144

[#&#8203;5146]: tokio-rs/tokio#5146

[#&#8203;5150]: tokio-rs/tokio#5150

[#&#8203;5151]: tokio-rs/tokio#5151

[#&#8203;5152]: tokio-rs/tokio#5152

[#&#8203;5154]: tokio-rs/tokio#5154

[#&#8203;5155]: tokio-rs/tokio#5155

[#&#8203;5157]: tokio-rs/tokio#5157

[#&#8203;5158]: tokio-rs/tokio#5158

[#&#8203;5159]: tokio-rs/tokio#5159

[#&#8203;5160]: tokio-rs/tokio#5160

[#&#8203;5161]: tokio-rs/tokio#5161

[#&#8203;5163]: tokio-rs/tokio#5163

[#&#8203;5166]: tokio-rs/tokio#5166

[#&#8203;5167]: tokio-rs/tokio#5167

[#&#8203;5168]: tokio-rs/tokio#5168

[#&#8203;5169]: tokio-rs/tokio#5169

[#&#8203;5171]: tokio-rs/tokio#5171

[#&#8203;5172]: tokio-rs/tokio#5172

[#&#8203;5175]: tokio-rs/tokio#5175

[#&#8203;5178]: tokio-rs/tokio#5178

[#&#8203;5179]: tokio-rs/tokio#5179

[#&#8203;5182]: tokio-rs/tokio#5182

[#&#8203;5189]: tokio-rs/tokio#5189

[#&#8203;5191]: tokio-rs/tokio#5191

[#&#8203;5198]: tokio-rs/tokio#5198

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about these updates again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4yNy4xIiwidXBkYXRlZEluVmVyIjoiMzQuMjkuMiJ9-->

Co-authored-by: cabr2-bot <[email protected]>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1651
Reviewed-by: crapStone <[email protected]>
Co-authored-by: Calciumdibromid Bot <[email protected]>
Co-committed-by: Calciumdibromid Bot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carllerche carllerche carllerche approved these changes

Assignees
No one assigned
Labels
A-tokio Area: The main tokio crate M-task Module: tokio/task
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hawkw @carllerche @Darksonn