--- old/src/hotspot/os/linux/waitBarrier_linux.cpp 2018-12-18 11:15:42.032283584 +0100 +++ new/src/hotspot/os/linux/waitBarrier_linux.cpp 2018-12-18 11:15:41.709271971 +0100 @@ -38,10 +38,9 @@ #define guarantee_with_errno(cond, msg) check_with_errno(guarantee, cond, msg) -static int futex(volatile int *uaddr, int futex_op, int val, - const struct timespec *timeout, int *uaddr2, int val3) +static int futex(volatile int *addr, int futex_op, int op_arg) { - return syscall(SYS_futex, uaddr, futex_op, val, timeout, uaddr2, val3); + return syscall(SYS_futex, addr, futex_op, op_arg, NULL, NULL, 0); } void LinuxWaitBarrier::arm(int barrier_tag) { @@ -59,11 +58,8 @@ void LinuxWaitBarrier::wake() { assert(_futex_barrier == 0, "Not disarmed"); int s = futex(&_futex_barrier, - FUTEX_WAKE, - INT_MAX, /* wake a max of this many threads */ - NULL /* ignored */, - NULL /* ignored */, - 0 /* ignored */); + FUTEX_WAKE_PRIVATE, + INT_MAX /* wake a max of this many threads */); guarantee_with_errno(s > -1, "futex FUTEX_WAKE"); } @@ -76,18 +72,14 @@ } do { int s = futex(&_futex_barrier, - FUTEX_WAIT, - barrier_tag, /* should be this tag */ - NULL, /* no timeout */ - NULL, /* ignored */ - 0 /* ignored */); + FUTEX_WAIT_PRIVATE, + barrier_tag /* should be this tag */); guarantee_with_errno((s == 0) || (s == -1 && errno == EAGAIN) || (s == -1 && errno == EINTR), "futex FUTEX_WAIT"); - // Return value 0, re-check in case of spurious wake-up. - // EINTR and re-check and go back to waiting. - // EAGAIN we already are disarmed, we should pass the check, - // if not re-armed with same tag. + // Return value 0: woken up, but re-check in case of spurious wakeup + // Error EINTR: woken by signal, so re-check and re-wait if necessary. + // Error EAGAIN: we are already disarmed and so will pass the check } while (barrier_tag == _futex_barrier); } --- old/src/hotspot/share/utilities/waitBarrier.hpp 2018-12-18 11:15:43.079321229 +0100 +++ new/src/hotspot/share/utilities/waitBarrier.hpp 2018-12-18 11:15:42.759309723 +0100 @@ -26,6 +26,8 @@ #define SHARE_UTILITIES_WAITBARRIER_HPP #include "memory/allocation.hpp" +#include "runtime/thread.hpp" +#include "utilities/debug.hpp" #include "utilities/waitBarrier_generic.hpp" #if defined(LINUX) @@ -36,21 +38,38 @@ #endif // Platform independent WaitBarrier API. -// The WaitBarrier primary objective is to wake threads waiting in wait() fast. -// It can be arm with any int, the tag, that is not 0. -// - After arm() returns any thread calling wait(tag) with the correct tag will be blocked. -// - After disarm() is called any thread calling wait(...) will never block. -// - When wake() returns no threads are blocked any more, if it was disarmed before. -// - After calling disarm() and wake() it my be re-armed immediately with a different tag. -// - Re-arming with the same tag before all threads returned from previously wait -// is implementation defined. They may or may not return from the previously wait(). -// Wake thread: -// - arm(tag) -// - *work* -// - disarm() -// - wake() -// Wait thread: -// - wait(tag) +// An armed WaitBarrier prevents threads from advancing until the +// barrier is disarmed and the waiting threads woken. The barrier is +// armed by setting a non-zero value - the tag. +// +// Expected Usage: +// - Arming thread: +// tag = ...; // non-zero value +// barrier.arm(tag); +// +// +// barrier.disarm(); +// barrier.wake(); +// +// - After arm(tag) returns any thread calling wait(tag) will block. +// - After disarm() returns any subsequent calls to wait(tag) will not block. +// - After wake() returns all blocked threads are unblocked and eligible to +// execute again. +// - After calling disarm() and wake() the barrier is ready to be re-armed +// with a new tag. (may not be re-armed with last used tag) +// +// - Waiting threads +// wait(tag); // don't execute following code unless 'safe' +// +// +// - A call to wait(tag) will block if the barrier is armed with the value +// 'tag'; else it will return immediately. +// - A blocked thread is eligible to execute again once the barrier is +// disarmed and wake() has been called. +// +// A primary goal of the WaitBarrier implementation is to wake all waiting +// threads as fast, and as concurrently, as possible. +// template class WaitBarrierType : public CHeapObj { WaitBarrierImpl _impl; @@ -59,8 +78,18 @@ WaitBarrierType(const WaitBarrierDefault&); WaitBarrierType& operator=(const WaitBarrierDefault&); +#ifdef ASSERT + int _last_arm_tag; + Thread* _owner; +#endif + public: - WaitBarrierType() : _impl() {} + WaitBarrierType(Thread* owner) : _impl() { +#ifdef ASSERT + _last_arm_tag = 0; + _owner = owner; +#endif + } ~WaitBarrierType() {} // Returns implementation type. @@ -68,19 +97,35 @@ // Guarantees any thread calling wait() with same tag will be blocked. // Provides a trailing fence. - void arm(int barrier_tag) { _impl.arm(barrier_tag); } + void arm(int barrier_tag) { +#ifdef ASSERT + assert(_last_arm_tag != barrier_tag, "Re-arming with same tag"); + _last_arm_tag = barrier_tag; + assert(_owner == Thread::current(), "Not owner thread"); +#endif + _impl.arm(barrier_tag); + } // Guarantees any thread calling wait() with any tag will not be blocked. // Provides a trailing fence. - void disarm() { _impl.disarm(); } + void disarm() { + assert(_owner == Thread::current(), "Not owner thread"); + _impl.disarm(); + } // Guarantees any thread called wait() will be awake when it returns. // Provides a trailing fence. - void wake() { _impl.wake(); } + void wake() { + assert(_owner == Thread::current(), "Not owner thread"); + _impl.wake(); + } // Guarantees to return if disarm() and wake() is called. // Provides a trailing fence. - void wait(int barrier_tag) { _impl.wait(barrier_tag); } + void wait(int barrier_tag) { + assert(_owner != Thread::current(), "Trying to wait with owner thread"); + _impl.wait(barrier_tag); + } }; typedef WaitBarrierType WaitBarrier; --- old/src/hotspot/share/utilities/waitBarrier_generic.cpp 2018-12-18 11:15:44.130359016 +0100 +++ new/src/hotspot/share/utilities/waitBarrier_generic.cpp 2018-12-18 11:15:43.807347403 +0100 @@ -52,8 +52,8 @@ return 0; } assert(w > 0, "Bad counting"); - // We need an exact count and never go below 0. - // Otherwise the semaphore might contain to many posts. + // We need an exact count which never goes below zero, + // otherwise the semaphore may be signalled too many times. if (Atomic::cmpxchg(w - 1, &_waiters, w) == w) { _sem_barrier.signal(); return w - 1; @@ -71,7 +71,7 @@ // There is no thread to wake but we still have barrier threads. sp.wait(); } - // We must loop here until there is no waiters or potential waiters. + // We must loop here until there are no waiters or potential waiters. } while (left > 0 || _barrier_threads > 0); OrderAccess::fence(); } --- old/src/hotspot/share/utilities/waitBarrier_generic.hpp 2018-12-18 11:15:45.181396805 +0100 +++ new/src/hotspot/share/utilities/waitBarrier_generic.hpp 2018-12-18 11:15:44.860385263 +0100 @@ -28,14 +28,14 @@ #include "memory/allocation.hpp" #include "runtime/semaphore.hpp" -// Except for the barrier tag it self, it uses two counter to keep the semaphore -// count correct and not leave any late thread hanging. +// Except for the barrier tag itself, it uses two counters to keep the semaphore +// count correct and not leave any late thread waiting. class GenericWaitBarrier : public CHeapObj { volatile int _barrier_tag; // The number of threads waiting on or about to wait on the semaphore. volatile int _waiters; // The number of threads in the wait path, before or after the tag check. - // Which means it can become a waiter. + // These threads can become waiters. volatile int _barrier_threads; Semaphore _sem_barrier; --- old/test/hotspot/gtest/utilities/test_waitBarrier.cpp 2018-12-18 11:15:46.228434449 +0100 +++ new/test/hotspot/gtest/utilities/test_waitBarrier.cpp 2018-12-18 11:15:45.906422872 +0100 @@ -29,7 +29,7 @@ #include "utilities/waitBarrier.hpp" #include "threadHelper.inline.hpp" -static volatile int wait_tag = 1; +static volatile int wait_tag = 0; static volatile int valid_value = 0; template @@ -46,12 +46,23 @@ void main_run() { _wrt_start->signal(); int vv, tag; + // Similar to how a JavaThread would stop in a safepoint. while (!_exit) { + // Load the published tag. tag = OrderAccess::load_acquire(&wait_tag); - OrderAccess::release_store_fence(&_on_barrier, tag); + // Publish the tag this thread is going to wait for. + OrderAccess::release_store(&_on_barrier, tag); + if (_on_barrier == 0) { + SpinPause(); + continue; + } + OrderAccess::storeload(); // Loads in WB must not float up. + // Wait until we are woken. _wait_barrier->wait(tag); + // Verify that we do not see an invalid value. vv = OrderAccess::load_acquire(&valid_value); ASSERT_EQ((vv & 0x1), 0); + OrderAccess::release_store(&_on_barrier, 0); } } }; @@ -69,9 +80,7 @@ static const int NUMBER_OF_READERS = 4; Semaphore post; Semaphore wrt_start; - WaitBarrierType wb; - - wait_tag = 2; + WaitBarrierType wb(this); WBThread* reader1 = new WBThread(&post, &wb, &wrt_start); WBThread* reader2 = new WBThread(&post, &wb, &wrt_start); @@ -88,24 +97,41 @@ wrt_start.wait(); --nw; } - SpinYield sp; jlong stop_ms = os::javaTimeMillis() + 1000; // 1 seconds max test time + int next_tag = 1; + // Similar to how the VM thread would use a WaitBarrier in a safepoint. while (stop_ms > os::javaTimeMillis()) { - wb.arm(wait_tag + 1); - OrderAccess::release_store(&wait_tag, wait_tag + 1); + // Arm next tag. + wb.arm(next_tag); + // Publish tag. + OrderAccess::release_store_fence(&wait_tag, next_tag); + // Wait until threads picked up new tag. while (reader1->_on_barrier != wait_tag || reader2->_on_barrier != wait_tag || reader3->_on_barrier != wait_tag || reader4->_on_barrier != wait_tag) { - sp.wait(); + SpinPause(); } + + // Set an invalid value. OrderAccess::release_store(&valid_value, valid_value + 1); // odd - os::naked_short_sleep(1); + os::naked_yield(); + // Set a valid value. OrderAccess::release_store(&valid_value, valid_value + 1); // even - OrderAccess::storestore(); // Stores in WB must not float up. + // Publish inactive tag. + OrderAccess::release_store_fence(&wait_tag, 0); // Stores in WB must not float up. wb.disarm(); wb.wake(); + + // Wait until threads done valid_value verification. + while (reader1->_on_barrier != 0 || + reader2->_on_barrier != 0 || + reader3->_on_barrier != 0 || + reader4->_on_barrier != 0) { + SpinPause(); + } + ++next_tag; } WBThread::_exit = true; for (int i = 0; i < NUMBER_OF_READERS; i++) {