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.

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

rt: yield_now defers task until after driver poll #5223

Merged
merged 15 commits into from Nov 30, 2022
Merged

Conversation

carllerche
Copy link
Member

@carllerche carllerche commented Nov 22, 2022

Previously, calling task::yield_now().await would yield the current task to the scheduler, but the scheduler would poll it again before polling the resource drivers. This behavior can result in starving the resource drivers.

This patch creates a queue tracking yielded tasks. The scheduler notifies those tasks after polling the resource drivers.

Previously, calling `task::yield_now().await` would yield the current
task to the scheduler, but the scheduler would poll it again before
polling the resource drivers. This behavior can result in starving the
resource drivers.

This patch creates a queue tracking yielded tasks. The scheduler
notifies those tasks **after** polling the resource drivers.
@github-actions github-actions bot added the R-loom Run loom tests on this PR label Nov 22, 2022
@luben
Copy link
Contributor

luben commented Nov 22, 2022

This would be amazing when merged!

@Darksonn Darksonn added A-tokio Area: The main tokio crate M-runtime Module: tokio/runtime labels Nov 23, 2022
@carllerche carllerche marked this pull request as ready for review November 24, 2022 00:27
@carllerche
Copy link
Member Author

Ok, this PR should be ready for review. I will fix CI errors as they show up.

@carllerche
Copy link
Member Author

Ok, it seems like the multi-threaded test is still not correct. It is proving quite challenging to test it reliably with the multi-threaded work stealing.

cnt += 1;
}

assert!(cnt < 3, "actual={}", cnt);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this assertion complete after one iteration?

Copy link
Member Author

Choose a reason for hiding this comment

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

False positives and all that are possible...

Copy link
Member Author

Choose a reason for hiding this comment

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

I"m also seeing cases where the OS appears to delay the I/O notification. I don't think there is any reliable way to test this here. I added a loom test, though.

@carllerche
Copy link
Member Author

Ah, it looks like the hang is on windows. I will investigate that next.

@carllerche
Copy link
Member Author

The windows fix was simple.

carllerche added a commit that referenced this pull request Dec 1, 2022
PR #5223 changed the behavior of `yield_now()` to store yielded tasks
and notify them *after* polling the resource drivers. This PR fixes a
couple of bugs with this new behavior when combined with
`block_in_place()`.

First, we need to avoid freeing the deferred task queue when exiting a
runtime if it is *not* the root runtime. Because `block_in_place()`
allows a user to start a new runtime from within an existing task, this
check is necessary.

Second, when a worker core is stolen from a thread during a
`block_in_place()` call, we need to ensure that deferred tasks are
notified anyway.
carllerche added a commit that referenced this pull request Dec 2, 2022
PR #5223 changed the behavior of `yield_now()` to store yielded tasks
and notify them *after* polling the resource drivers. This PR fixes a
couple of bugs with this new behavior when combined with
`block_in_place()`.

First, we need to avoid freeing the deferred task queue when exiting a
runtime if it is *not* the root runtime. Because `block_in_place()`
allows a user to start a new runtime from within an existing task, this
check is necessary.

Second, when a worker core is stolen from a thread during a
`block_in_place()` call, we need to ensure that deferred tasks are
notified anyway.
carllerche added a commit that referenced this pull request Dec 5, 2022
 - net: fix Windows named pipe connect ([#5208])
 - io: support vectored writes for `ChildStdin` ([#5216])
 - io: fix `async fn ready()` false positive for OS-specific events ([#5231])

 ### Changed
 - runtime: `yield_now` defers task until after driver poll ([#5223])
 - runtime: reduce amount of codegen needed per spawned task ([#5213])
 - windows: replace `winapi` dependency with `windows-sys` ([#5204])

 [#5208]: #5208
 [#5216]: #5216
 [#5213]: #5213
 [#5204]: #5204
 [#5223]: #5223
 [#5231]: #5231
carllerche added a commit that referenced this pull request Dec 5, 2022
### Fixed
 - net: fix Windows named pipe connect ([#5208])
 - io: support vectored writes for `ChildStdin` ([#5216])
 - io: fix `async fn ready()` false positive for OS-specific events ([#5231])

 ### Changed
 - runtime: `yield_now` defers task until after driver poll ([#5223])
 - runtime: reduce amount of codegen needed per spawned task ([#5213])
 - windows: replace `winapi` dependency with `windows-sys` ([#5204])

 [#5208]: #5208
 [#5216]: #5216
 [#5213]: #5213
 [#5204]: #5204
 [#5223]: #5223
 [#5231]: #5231
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Dec 23, 2022
This PR contains the following updates:

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

---

### Release Notes

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

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

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

##### Fixed

-   net: fix Windows named pipe connect ([#&#8203;5208])
-   io: support vectored writes for `ChildStdin` ([#&#8203;5216])
-   io: fix `async fn ready()` false positive for OS-specific events ([#&#8203;5231])

##### Changed

-   runtime: `yield_now` defers task until after driver poll ([#&#8203;5223])
-   runtime: reduce amount of codegen needed per spawned task ([#&#8203;5213])
-   windows: replace `winapi` dependency with `windows-sys` ([#&#8203;5204])

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

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

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

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

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

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

</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:eyJjcmVhdGVkSW5WZXIiOiIzNC43MC4wIiwidXBkYXRlZEluVmVyIjoiMzQuNzAuMCJ9-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1687
Reviewed-by: crapStone <crapstone@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
hds added a commit to tokio-rs/console that referenced this pull request Feb 16, 2024
Part of the testing performed in the `console-subscriber` integration
tests is detecting self wakes. This relied upon the `yield_now()` from
Tokio.

However, the behavior of this function was changed in
tokio-rs/tokio#5223 and since Tokio 1.23 the wake doesn't occur in the
task that `yield_now()` is called from. This breaks the test when using
a newer version of Tokio.

This change replaces the use of `yield_now()` with a custom
`self_wake()` function that returns a future which does perform a self
wake (wakes the task from within itself before returning
`Poll::Pending`).

The same custom `self_wake()` is also included in the `app` example so
that it shows self wakes correctly.
hds added a commit to tokio-rs/console that referenced this pull request Feb 16, 2024
Part of the testing performed in the `console-subscriber` integration
tests is detecting self wakes. This relied upon the `yield_now()` from
Tokio.

However, the behavior of this function was changed in
tokio-rs/tokio#5223 and since Tokio 1.23 the wake doesn't occur in the
task that `yield_now()` is called from. This breaks the test when using
a newer version of Tokio.

This change replaces the use of `yield_now()` with a custom
`self_wake()` function that returns a future which does perform a self
wake (wakes the task from within itself before returning
`Poll::Pending`).

The same custom `self_wake()` is also included in the `app` example so
that it shows self wakes correctly.

Tokio has been updated to 1.28.2 in the lock file (the last with
compatible MSRV) so that this fix is tested.

Ref #512
hds added a commit to tokio-rs/console that referenced this pull request Mar 8, 2024
Part of the testing performed in the `console-subscriber` integration
tests is detecting self wakes. This relied upon the `yield_now()` from
Tokio.

However, the behavior of this function was changed in
tokio-rs/tokio#5223 and since Tokio 1.23 the wake doesn't occur in the
task that `yield_now()` is called from. This breaks the test when using
a newer version of Tokio.

This change replaces the use of `yield_now()` with a custom
`self_wake()` function that returns a future which does perform a self
wake (wakes the task from within itself before returning
`Poll::Pending`).

The same custom `self_wake()` is also included in the `app` example so
that it shows self wakes correctly.

Tokio has been updated to 1.28.2 in the lock file (the last with
compatible MSRV) so that this fix is tested.

Ref #512
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate M-runtime Module: tokio/runtime R-loom Run loom tests on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants