-
-
Notifications
You must be signed in to change notification settings - Fork 20
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
Conversation
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]>
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]>
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]>
There was a problem hiding this 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.
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]>
@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? |
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 |
It's integrated and working, no complaints from use! I'll take a quick scan of the recent changes and r+ it. |
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
.
There was a problem hiding this 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.
this branch adds a
WaitQueue
type inmaitake::wait
. this allowsmultiple 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.