-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Replace RwLock by a futex based one on Linux. #95762
Conversation
Looks like something is wrong. :) |
Ah, looks like I somehow forgot to ever set the READERS_WAITING bit. Well that's an easy fix. :) |
Going to do some more testing first. :) |
#[inline] | ||
pub unsafe fn try_write(&self) -> bool { | ||
self.state | ||
.fetch_update(Acquire, Relaxed, |s| (readers(s) == 0).then(|| s + WRITE_LOCKED)) |
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.
I don't think this correctly handles the case where there is an existing write lock but no waiting threads or readers.
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.
The readers()
function looks like it extracts the readers count, meaning the value doesn't consider existing waiting threads. Having a writer locked means the readers count would be the maximum value so the .then()
clause would short-circuit early and return None
. The use of +
over |
is a bit misleading, but that just transitions from 0 readers to max readers (a.k.a writer).
#[inline] | ||
pub unsafe fn try_read(&self) -> bool { | ||
self.state | ||
.fetch_update(Acquire, Relaxed, |s| read_lockable(s).then(|| s + READ_LOCKED)) |
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.
What happens if the reader count overflows?
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.
read_lockable()
checks for MAX_READERS
which looks like its writer value minus one. If the reader count would overflow into a writer, it looks like that would be detected by read_lockable()
and short circuit to None
.
(Found a few more issues, so this is back to the drawing board for now. Will send a better tested PR soon. :) ) |
Sorry for the noise :) Here's the new PR, this time properly stress tested: #95801 |
This replaces the pthread-based RwLock on Linux by a futex based one.
This implementation is similar to the algorithm suggested by @kprotty, but modified to prefer writers and spin before sleeping. It uses two futexes: One for the readers to wait on, and one for the writers to wait on. The readers futex contains the state of the RwLock: The number of readers, a bit indicating whether writers are waiting, and a bit indicating whether readers are waiting. The writers futex is used as a simple condition variable and its contents are meaningless; it just needs to be changed on every notification.
Using two futexes rather than one has the obvious advantage of allowing a separate queue for readers and writers, but it also means we avoid the problem a single-futex RwLock would have of making it hard for a writer to go to sleep while the number of readers is rapidly changing up and down, as the writers futex is only changed when we actually want to wake up a writer.
It always prefers writers, as we decided here.
It relies on futex_wake to return the number of awoken threads to be able to handle write-unlocking while both the readers-waiting and writers-waiting bits are set. Instead of waking both and letting them race, it first wakes writers and only continues to wake the readers too if futex_wake reported there were no writers to wake up.
r? @Amanieu