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

Replace RwLock by a futex based one on Linux. #95762

Closed
wants to merge 5 commits into from

Conversation

m-ou-se
Copy link
Member

@m-ou-se m-ou-se commented Apr 7, 2022

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

@m-ou-se m-ou-se added O-linux Operating system: Linux A-concurrency Area: Concurrency T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 7, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 7, 2022
@m-ou-se
Copy link
Member Author

m-ou-se commented Apr 7, 2022

test process::tests::test_finish_twice has been running for over 60 seconds
test process::tests::test_interior_nul_in_env_key_is_error has been running for over 60 seconds
test process::tests::test_interior_nul_in_env_value_is_error has been running for over 60 seconds
test process::tests::test_override_env has been running for over 60 seconds
test sync::rwlock::tests::frob has been running for over 60 seconds

Looks like something is wrong. :)

@m-ou-se m-ou-se added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 7, 2022
@m-ou-se m-ou-se marked this pull request as draft April 7, 2022 12:10
@m-ou-se
Copy link
Member Author

m-ou-se commented Apr 7, 2022

Ah, looks like I somehow forgot to ever set the READERS_WAITING bit. Well that's an easy fix. :)

@m-ou-se
Copy link
Member Author

m-ou-se commented Apr 7, 2022

Going to do some more testing first. :)

@m-ou-se m-ou-se closed this Apr 7, 2022
#[inline]
pub unsafe fn try_write(&self) -> bool {
self.state
.fetch_update(Acquire, Relaxed, |s| (readers(s) == 0).then(|| s + WRITE_LOCKED))
Copy link
Member

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.

Copy link

@kprotty kprotty Apr 7, 2022

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))
Copy link
Member

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?

Copy link

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.

@m-ou-se
Copy link
Member Author

m-ou-se commented Apr 7, 2022

(Found a few more issues, so this is back to the drawing board for now. Will send a better tested PR soon. :) )

@m-ou-se
Copy link
Member Author

m-ou-se commented Apr 8, 2022

Sorry for the noise :)

Here's the new PR, this time properly stress tested: #95801

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-concurrency Area: Concurrency O-linux Operating system: Linux S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants