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

feat(maitake): add WaitQueue #191

Merged
merged 25 commits into from
Jun 5, 2022
Merged

feat(maitake): add WaitQueue #191

merged 25 commits into from
Jun 5, 2022

Conversation

hawkw
Copy link
Owner

@hawkw hawkw commented Jun 3, 2022

this branch adds a WaitQueue type in maitake::wait. this allows
multiple tasks to wait for an event, and be woken either one at a time
(in FIFO fashion), or all at once.

there are additional APIs we could add here, but this is a good starting
place. there probably should be docs examples but i'm lazy.

@hawkw hawkw requested a review from jamesmunns June 3, 2022 23:27
hawkw added 12 commits June 4, 2022 12:14
Signed-off-by: Eliza Weisman <[email protected]>
Signed-off-by: Eliza Weisman <[email protected]>
Signed-off-by: Eliza Weisman <[email protected]>
Signed-off-by: Eliza Weisman <[email protected]>
Signed-off-by: Eliza Weisman <[email protected]>
Signed-off-by: Eliza Weisman <[email protected]>
Signed-off-by: Eliza Weisman <[email protected]>
this fixes test failures due to unbounded branches

Signed-off-by: Eliza Weisman <[email protected]>
Signed-off-by: Eliza Weisman <[email protected]>
Signed-off-by: Eliza Weisman <[email protected]>
Copy link
Collaborator

@jamesmunns jamesmunns left a comment

Choose a reason for hiding this comment

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

Added some high level comments - I admit to not totally grokking how everything works yet, but I'm going to go try and integrate this branch into mnemos now, so I might have more meaningful feedback after that.

maitake/src/wait/queue.rs Outdated Show resolved Hide resolved
maitake/src/wait/queue.rs Outdated Show resolved Hide resolved
maitake/src/wait/queue.rs Show resolved Hide resolved
maitake/src/wait/queue.rs Outdated Show resolved Hide resolved
maitake/src/wait/queue.rs Outdated Show resolved Hide resolved
maitake/src/wait/queue.rs Show resolved Hide resolved
maitake/src/wait/queue.rs Show resolved Hide resolved
maitake/src/wait/queue.rs Outdated Show resolved Hide resolved
maitake/src/wait/queue.rs Outdated Show resolved Hide resolved
maitake/src/wait/queue/tests.rs Show resolved Hide resolved
@hawkw
Copy link
Owner Author

hawkw commented Jun 5, 2022

@jamesmunns i think i've addressed most of your review feedback; if you're still working on integrating this in mnemOS, i'd be happy to wait for that, but i have a slight preference for merging this now and addressing any additional issues that come up in a follow-up, if that's okay?

@jamesmunns
Copy link
Collaborator

Misc note (can totally be a future PR): It might be nice to be able to modify the lock behavior somehow.

For me, any lock contention is likely to be a programming bug, so .try_lock().unwrap() is likely to make a "louder"/easier to catch case for me.

@jamesmunns
Copy link
Collaborator

@jamesmunns i think i've addressed most of your review feedback; if you're still working on integrating this in mnemOS, i'd be happy to wait for that, but i have a slight preference for merging this now and addressing any additional issues that come up in a follow-up, if that's okay?

It's integrated and working, no complaints from use! I'll take a quick scan of the recent changes and r+ it.

@hawkw
Copy link
Owner Author

hawkw commented Jun 5, 2022

Misc note (can totally be a future PR): It might be nice to be able to modify the lock behavior somehow.

For me, any lock contention is likely to be a programming bug, so .try_lock().unwrap() is likely to make a "louder"/easier to catch case for me.

hmm, this isn't a bad idea, but it might be kind of painful to figure out a good interface for overriding lock behavior...could be worth opening a separate issue for?


/// Wake the next task in the queue.
///
/// If the queue is empty, a wakeup is stored in the `WaitQueue`, and the
Copy link
Collaborator

Choose a reason for hiding this comment

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

Two things I don't grok:

What happens if I call "wake" a bunch with an empty queue?

How come this stores a "wakeup" in the queue, rather than keeping it at the "empty" state?

Copy link
Owner Author

Choose a reason for hiding this comment

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

it's necessary to store a single wakeup in the queue because a task may be attempting to enqueue itself while wake is called, which would result in a "lost" wake. multiple calls to wake on an empty queue will not store additional wakeups; storing a count of wakeups in the queue seems like a job for a semaphore, which i'd also like to add to maitake.

Copy link
Collaborator

@jamesmunns jamesmunns left a comment

Choose a reason for hiding this comment

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

Overall LGTM! Added one "I don't understand" question, but that's mostly on me, unless you want to update the docs to explain.

@hawkw hawkw merged commit 85d5b00 into main Jun 5, 2022
@hawkw hawkw deleted the eliza/waitq branch June 5, 2022 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants