Import mutex, semaphore, and condition variable from LWSP #8455

bugaevc · 2021-07-05T12:59:26Z

This is a rewrite of mutexes, condition variables, and semaphores based on code from Let's write synchronization primitives 🎉 We've already had a trial run with importing the once primitive in #4158, and it's been a success, so let's do more.

The mutex was a spinlock, and is now a proper sleeping lock with a pure-atomic fastpath. (And there's also some less UB.)
Condition variables should now behave properly (fixes LibPthread: Implement condition variables properly #8432), and also be very fast in the uncontended case (no thread waiting)
Semaphores should be much faster

The rest of this paper is organized as follows... err, I mean, let's talk about some details and caveats though.

The semaphore implementation is complex, and hard to understand. The good things are:

The code is commented.
It's been heavily tested in LWSP; moreover, the tests are known to have easily detected some misbehaving implementations.

The condition variable is simpler, and the code is also commented. However,

It has not been tested extensively; there may very well be bugs. Coming up with a way to test the correctness of condition variable (namely, that it doesn't lose wakeups) under heavy load is still a TODO, because unlike with other synchronization primitives, for condition variables heavy load tends to sweep bugs under the rug.
This implementation does not support using one condition variable with multiple different mutexes, which POSIX allows. In my humble opinion, this is insane, and no program should actually do that (and I don't think anybody does do that). We've previously discussed this with @tomuta here. I believe it would be possible to make this implementation support changing the mutex mid-flight be adding an internal mutex (but only in the slow path)... But honestly, why would we want to even support that.

I did not replace the rwlocks implementation with the one from LWSP. The existing one, which @alimpfard says he has written after having taken a look at LWSP, seems sophisticated enough. He says it "is pretty damn dumb, and probably has more bugs than features. But for the time being, it seems to work. however, we should definitely replace it with a good implementation sometime very soon". But I don't know, I don't feel like ripping it out and replacing with my own version. I'd at least need to carefully read it & understand how it works first.

cc:

@awesomekling because you filed LibPthread: Implement condition variables properly #8432 and seemed to like the idea of importing stuff from LWSP
@tomuta because you worked on futexes and other synchronization primitives
@gunnarbeutner because you worked on LibPthread
@alimpfard because you wrote rwlocks
@gmta because this touches some of the same code as your LibPthread: Use realtime clock in futex_wait() #6664 & LibPthread: Correct error check in sem_post and sem_wait #7645

BuggieBot · 2021-07-05T12:59:55Z

One or more of the commits in this PR do not match the code submission policy, please check the lint_commits CI job for more details.

alimpfard

reviewing locking/syncing code is kinda nerve-wrecking for me, but here's a bunch of comments regardless :P

Userland/Libraries/LibC/pthread_integration.cpp

Userland/Libraries/LibPthread/pthread_cond.cpp

These are convinient wrappers over the most used futex operations. futex_wait() also does some smarts for timeout and clock handling. Use the new futex_wait() instead of a similar private helper in LibPthread.

pthread_mutex is now an actual "sleeping" mutex, and not just a spinlock! It still has a fast path that only uses atomics and (in the successful case) returns immediately without sleeping. In case of contention, it calls futex_wait(), which lets the kernel scheduler put this thread to sleep, *and* lets it know exactly when to consider scheduling it again.

This is a private function that locks the lock much like the regular pthread_mutex_lock(), but causes the corresponding unlock operation to always assume there may be other waiters. This is useful in case some waiters are made to wait on the mutex's futex directly, without going through pthread_mutex_lock(). This is going to be used by the condition variable implementation in the next commit.

This implementation features a fast path for pthread_cond_signal() and pthread_cond_broadcast() for the case there's no thread waiting, and does not exhibit the "thundering herd" issue in pthread_cond_broadcast(). Fixes SerenityOS#8432

This implementation does not use locking or condition variables internally; it's purely based on atomics and futexes. Notably, concurrent sem_wait() and sem_post() calls can run *completely in parallel* without slowing each other down, as long as there are empty slots for them all to succeed without blocking. Additionally, sem_wait() never executes an atomic operation with release ordering, and sem_post() never executes an atomic operation with acquire ordering (unless you count the syscall). This means the compiler and the hardware are free to reorder code *into* the critical section.

awesomekling · 2021-07-05T18:25:58Z

This all looks better than what we have right now, so let's get it merged.
Thank you for working on this Sergey! :^)

bugaevc · 2021-07-05T18:37:09Z

Userland/Libraries/LibPthread/semaphore.cpp

+    if (!(value & POST_WAKES)) [[likely]]
+        return 0;
+    int rc = futex_wake(&sem->value, 1);
+    VERIFY(rc == 0);


Ok, this is stupid, and gives away that I haven't actually tested the Serenity version beside seeing that it boots 😀

Will submit a fix soon.

supercomputer7 · 2021-07-05T19:27:20Z

Seems like after reverting these changes, booting x86_64 mode works OK for me. I tried to figure out why, but to be honest, I really don't know what is going wrong with this now.

bugaevc force-pushed the import-from-lwsp branch from 5389a94 to 51d5f56 Compare July 5, 2021 13:01

alimpfard reviewed Jul 5, 2021

View reviewed changes

Userland/Libraries/LibC/pthread_integration.cpp Show resolved Hide resolved

Userland/Libraries/LibPthread/pthread_cond.cpp Show resolved Hide resolved

bugaevc added 5 commits July 5, 2021 18:29

LibC: Add futex_wait() and futex_wake() helpers

a395d1e

These are convinient wrappers over the most used futex operations. futex_wait() also does some smarts for timeout and clock handling. Use the new futex_wait() instead of a similar private helper in LibPthread.

LibPthread: Reimplement condition variables

49d8014

This implementation features a fast path for pthread_cond_signal() and pthread_cond_broadcast() for the case there's no thread waiting, and does not exhibit the "thundering herd" issue in pthread_cond_broadcast(). Fixes SerenityOS#8432

bugaevc force-pushed the import-from-lwsp branch from 51d5f56 to 8c1d10a Compare July 5, 2021 15:33

awesomekling merged commit 690141f into SerenityOS:master Jul 5, 2021

bugaevc commented Jul 5, 2021

View reviewed changes

This was referenced Jul 6, 2021

Follow-up to 8455 #8495

Merged

Follow-up to 8495 #8504

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Import mutex, semaphore, and condition variable from LWSP #8455

Import mutex, semaphore, and condition variable from LWSP #8455

bugaevc commented Jul 5, 2021

BuggieBot commented Jul 5, 2021

alimpfard left a comment

awesomekling commented Jul 5, 2021

bugaevc Jul 5, 2021

supercomputer7 commented Jul 5, 2021

Import mutex, semaphore, and condition variable from LWSP #8455

Import mutex, semaphore, and condition variable from LWSP #8455

Conversation

bugaevc commented Jul 5, 2021

BuggieBot commented Jul 5, 2021

alimpfard left a comment

Choose a reason for hiding this comment

awesomekling commented Jul 5, 2021

bugaevc Jul 5, 2021

Choose a reason for hiding this comment

supercomputer7 commented Jul 5, 2021