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

[Fabian Wolff via Phabricator via libcxx-commits]

fwolff marked 2 inline comments as done.
fwolff added inline comments.


================
Comment at: libcxx/test/std/thread/thread.semaphore/lost_wakeup.pass.cpp:33-36
+  for (auto i = 0; i < 100'000; ++i) {
+    for (auto j = 0; j < 4; ++j)
+      s.release(1);
+    b.arrive_and_wait();
----------------
Quuxplusone wrote:
> Throughout: please `int [ij] = 0` instead of `auto [ij] = 0`.
> Also, I find this nested-loop version much harder to grok at first sight (I mean, maybe just because I wrote the other version myself ;)). IMO it would help to unroll the `j` loop.
> I guess the real question is, does the change from "2 acquiring threads, 2 releases in 1 releasing thread" to "8 acquiring threads, 8 releases in 2 releasing threads" make the buggy situation more or less likely to occur? From the description in https://bugs.llvm.org/show_bug.cgi?id=47013#c1 , I //think// that increasing the number of waiting acquirers is good (because each adjacent pair of acquirers has a chance to race with each other, so we have 7x the chances to reproduce in each loop iteration), but I don't see how increasing the number of releasers helps. If you think it'd be harmless to go back to just one releaser (keeping all 8 acquirers, and ideally still unrolling the loop), that would simplify this code IMHO.
It's hard to say. From my understanding, the releases have to happen in a quick enough succession that none of the acquirer threads get around to decrementing the variable in between. This might be more likely with two releaser threads to happen, but maybe not because it increases the contention on the atomic variable. I really don't know either, so I've implemented your suggestion for now.


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

https://reviews.llvm.org/D114119