Replace RwLock by a futex based one on Linux

[m-ou-se]

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.

To be able to prefer writers, 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]

I'm running a stress test for this new RwLock with 200 threads randomly read locking (99.5% chance) or write locking (0.5% chance), while tracking that every thread keeps making progress. Another computer is running the same stress test with a much lower number of threads that also randomly sleep sometimes. Both have been running for 40 minutes now without any problems.

[m-ou-se]

stress test

Interestingly, the current (pthread) RwLock fails the same test, due to writer starvation.

[Amanieu]

Is there any reason why these are placed in the high bits rather than the low bits? In parking_lot I put the flags in the low bits so that counter overflows can be caught with checked_add which is slightly more efficient assembly code.

[m-ou-se]

Putting them in the high bits makes it possible for the read condition to compile down to a single comparison:

readers(state) < MAX_READERS && !readers_waiting(state) && !writers_waiting(state)

is then equal to

(state as u32) < 0x3ffffffe.

(Not sure if llvm sees that though.)

[erikdesjardins]

LLVM doesn't currently see this, although it sees a related pattern where the 0x3ffffffe mask has only one set bit.

Reported upstream: llvm/llvm-project#54856

(Note that I looked at readers(state) < MAX_READERS & !readers_waiting(state) & !writers_waiting(state) with bitwise and--LLVM has more trouble with logical and.)

[Amanieu]

It seems a bit strange to represent WRITE_LOCKED with a special reader count. I think using a separate bit for WRITE_LOCKED would make the code more efficient (e.g. using native overflow flags to detect reader count overflow, using fetch_or for locking instead of a CAS loop, etc).

[m-ou-se]

I'll try that out and see if it simplifies things. It probably does. Thanks. :)

[m-ou-se]

I tried it, but it does not seem to simplify things.

fetch_or doesn't seem very useful, because it should only set the bit if the lock isn't read-locked yet. If we unconditionally set the bit when attempting to lock, it will also be set when it's still read-locked and read-unlocking will need special handling for that situation.

Simplifying the overflow check doesn't help, because at the points where this is checked, we also need to check the waiting bits too.

I also prefer to combine the fields, to make invalid states unrepresentable. (Effectively enum { Unlocked, ReadLocked(NonZeroNonMaxU30), WriteLocked } instead of struct { read_locked: u29, write_locked: bool }.)

[Amanieu]

I think the design is sound. At the very least I couldn't find a way to break it.

[m-ou-se]

Thanks for the quick review! I'll add some comments to the code to clarify the things you pointed out.

[m-ou-se]

@Amanieu I've addressed all your comments. :)

[Amanieu]

@bors r+

[bors]

📌 Commit 8339381 has been approved by Amanieu

[Soveu]

Great job!

[m-ou-se]

@bors rollup=never

[vireshk]

This replaces the pthread-based RwLock on Linux by a futex based one.

Any chance following issue is related to this ? Started getting this since 1.62.0.

#101194

[m-ou-se]

This replaces the pthread-based RwLock on Linux by a futex based one.

Any chance following issue is related to this ? Started getting this since 1.62.0.

#101194

Yes, but that snippet never worked correctly on other operating systems, even before this change. We never guaranteed that read-locking a read-locked RwLock always succeeds. That's just a property of pthread's rwlock that your snippet accidentally relied on. E.g. the RwLock on Windows didn't have this property, and would always block reads as soon as there's a writer waiting.