fix(maitake): handle spurious WaitCell polls
#453
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This branch changes the `Wait` future for `maitake::sync::WaitCell` to handle spurious polls correctly. Currently, a `wait_cell::Wait` future assumes that if it's ever polled a second time, that means its waker was woken. However, there might be other reasons that a stack of futures containing a `Wait` is polled again, and the `Wait` future will incorrectly complete immediately in that case. This branch fixes this by replacing the `bool` field in `Wait` that's set on first poll with an "event count" stored in the remaining `WaitCell` state bits. Now, when a `Wait` is created, it loads the current event count, and calls to `wake()` and `close()` increment the event count. The `Wait` future then checks if the event count has gone up when it's polled, rather than just checking if it's ever been polled before. This allows the `Wait` future to determine if it is being polled because the `WaitCell` woke it up, or if it's being polled because some other future decided to poll it. This *also* has the side benefit of fixing racy scenarios where the `WaitCell` is woken between when the `Wait` future is created and when it's polled for the first time. Fixes #449
|
@jamesmunns it would be nice to confirm that this actually fixes the weird behavior you were seeing in mnemOS... |
hawkw
added a commit
that referenced
this pull request
Jul 24, 2023
This branch rewrites the `maitake::sync::WaitCell` type a bit to fix potential lost wakeups accidentally introduced in #453. In addition, it changes the API a bit: - Replace `register_wait` with `poll_wait`, which I think is a bit nicer, - Removed the `RegisterError::Waking` variant since `poll_wait` returns `Ready` if the cell was woken, and renamed `RegisterError` to just `Error`, - Introduced a `WaitCell::subscribe` method to eagerly pre-register the task's `Waker` in the `WaitCell`. This is intended to be used to register the task to be woken *before* performing an operation that results in a wakeup, such as enabling an interrupt that will wake the task. Closes #455
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This branch changes the
Waitfuture formaitake::sync::WaitCelltohandle spurious polls correctly. Currently, a
wait_cell::Waitfutureassumes that if it's ever polled a second time, that means its waker was
woken. However, there might be other reasons that a stack of futures
containing a
Waitis polled again, and theWaitfuture willincorrectly complete immediately in that case.
This branch fixes this by replacing the
boolfield inWaitthat'sset on first poll with an "event count" stored in the remaining
WaitCellstate bits. Now, when aWaitis created, it loads thecurrent event count, and calls to
wake()andclose()increment theevent count. The
Waitfuture then checks if the event count has goneup when it's polled, rather than just checking if it's ever been polled
before. This allows the
Waitfuture to determine if it is being polledbecause the
WaitCellwoke it up, or if it's being polled because someother future decided to poll it. This also has the side benefit of
fixing racy scenarios where the
WaitCellis woken between when theWaitfuture is created and when it's polled for the first time.Fixes #449