[libcxx-commits] [PATCH] D114119: [libcxx] Fix potential lost wake-up in counting semaphore

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

In D114119#3147218 <https://reviews.llvm.org/D114119#3147218>, @jiixyj wrote:

> Wouldn't it also be possible to just unconditionally notify the atomic? Something like this:
>
>   __a.fetch_add(__update, memory_order_release);
>   if(__update > 1)
>       __a.notify_all();
>   else
>       __a.notify_one();
>
> One downside is that there might be some unneeded calls to the platform wake function (`futex`, etc.), but it would have the advantage of avoiding thundering herd problems if there are many consumers and a single producer repeatedly calling `notify_one()`.

IMO it makes more sense to try and diminish spurious wakeups, but I'm far from an expert in that domain.

The current fix LGTM -- I did my due diligence and I understand the problem and how this fixes it. I'd simply like to make sure the test is not flaky.



================
Comment at: libcxx/include/semaphore:88-92
         if(0 < __a.fetch_add(__update, memory_order_release))
             ;
-        else if(__update > 1)
-            __a.notify_all();
         else
-            __a.notify_one();
+            // Always notify all, regardless of the value of __update (see PR47013)
+            __a.notify_all();
----------------
We might want to reformulate this as

```
if(__a.fetch_add(__update, memory_order_release) == 0)
  __a.notify_all();
```

This seems easier to understand.


================
Comment at: libcxx/test/std/thread/thread.semaphore/lost_wakeup.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
Do you think this test is the reason why the CI nodes timed out after running for 2 hours? It seems like it's not consistently slow (I just restarted the macOS CI and it finished quickly, but before that it froze for 2 hours, which I've never seen). https://buildkite.com/llvm-project/libcxx-ci/builds/6835#4278d152-fa4d-482f-a12e-e9697caa99e3


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114119/new/

https://reviews.llvm.org/D114119