diff --git a/platforms/unix/libpandabase/futex/fmutex.cpp b/platforms/unix/libpandabase/futex/fmutex.cpp index d89060405a28474da4cdfbac3acc8acb514d7a8d..7a586f5eea5c81a65b9199dd117747832df1dd4a 100644 --- a/platforms/unix/libpandabase/futex/fmutex.cpp +++ b/platforms/unix/libpandabase/futex/fmutex.cpp @@ -37,8 +37,9 @@ static ATOMIC(bool) DEADLOCK_FLAG = false; #ifdef MC_ON // GenMC does not support syscalls(futex) static ATOMIC_INT FUTEX_SIGNAL; +static ATOMIC_INT SIGNAL; -static inline void FutexWait(ATOMIC_INT *m, int v) +static inline void FutexWait(ATOMIC(THREAD_ID) * m, int v) { // Atomic with acquire order reason: mutex synchronization int s = ATOMIC_LOAD(&FUTEX_SIGNAL, memory_order_acquire); @@ -56,6 +57,26 @@ static inline void FutexWake(void) // Atomic with release order reason: mutex synchronization ATOMIC_FETCH_ADD(&FUTEX_SIGNAL, 1, memory_order_release); } + +// For signals +static inline void SignalWait(ATOMIC_INT *m, int v) +{ + // Atomic with acquire order reason: mutex synchronization + int s = ATOMIC_LOAD(&SIGNAL, memory_order_acquire); + // Atomic with acquire order reason: mutex synchronization + if (ATOMIC_LOAD(m, memory_order_acquire) != v) { + return; + } + // Atomic with acquire order reason: mutex synchronization + while (ATOMIC_LOAD(&SIGNAL, memory_order_acquire) == s) { + } +} + +static inline void SignalWake(void) +{ + // Atomic with release order reason: mutex synchronization + ATOMIC_FETCH_ADD(&SIGNAL, 1, memory_order_release); +} #else // futex() is defined in header, as it is still included in different places #endif @@ -72,25 +93,7 @@ void MutexIgnoreChecksOnTerminationLoop() int *GetStateAddr(struct fmutex *const m) { - return reinterpret_cast(&m->state_and_waiters); -} - -void IncrementWaiters(struct fmutex *const m) -{ - // Atomic with relaxed order reason: mutex synchronization - ATOMIC_FETCH_ADD(&m->state_and_waiters, WAITER_INCREMENT, memory_order_relaxed); -} -void DecrementWaiters(struct fmutex *const m) -{ - // Atomic with relaxed order reason: mutex synchronization - ATOMIC_FETCH_SUB(&m->state_and_waiters, WAITER_INCREMENT, memory_order_relaxed); -} - -int32_t GetWaiters(struct fmutex *const m) -{ - // Atomic with relaxed order reason: mutex synchronization - return static_cast(static_cast(ATOMIC_LOAD(&m->state_and_waiters, memory_order_relaxed)) >> - static_cast(WAITER_SHIFT)); + return reinterpret_cast(&m->exclusive_owner); } bool IsHeld(struct fmutex *const m, THREAD_ID thread) @@ -118,7 +121,7 @@ static void BackOff(uint32_t i) // Wait until pred is true, or until timeout is reached. // Return true if the predicate test succeeded, false if we timed out. -static inline bool WaitBrieflyFor(ATOMIC_INT *addr) +static inline bool WaitBrieflyFor(ATOMIC(THREAD_ID) * addr) { // We probably don't want to do syscall (switch context) when we use WaitBrieflyFor static constexpr uint32_t MAX_BACK_OFF = 10; @@ -126,8 +129,7 @@ static inline bool WaitBrieflyFor(ATOMIC_INT *addr) for (uint32_t i = 1; i <= MAX_ITER; i++) { BackOff(MIN(i, MAX_BACK_OFF)); // Atomic with relaxed order reason: mutex synchronization - int state = ATOMIC_LOAD(addr, memory_order_relaxed); - if ((HELPERS_TO_UNSIGNED(state) & HELPERS_TO_UNSIGNED(HELD_MASK)) == 0) { + if (ATOMIC_LOAD(addr, memory_order_relaxed) == 0) { return true; } } @@ -140,8 +142,6 @@ void MutexInit(struct fmutex *const m) ATOMIC_STORE(&m->exclusive_owner, 0, memory_order_relaxed); m->recursive_count = 0; m->recursive_mutex = false; - // Atomic with release order reason: mutex synchronization - ATOMIC_STORE(&m->state_and_waiters, 0, memory_order_release); } void MutexDestroy(struct fmutex *const m) @@ -151,10 +151,7 @@ void MutexDestroy(struct fmutex *const m) if (!MutexDoNotCheckOnTerminationLoop()) { #endif // PANDA_TARGET_MOBILE // Atomic with relaxed order reason: mutex synchronization - if (ATOMIC_LOAD(&m->state_and_waiters, memory_order_relaxed) != 0) { - FAIL_WITH_MESSAGE("Mutex destruction failed; state_and_waiters is non zero!"); - // Atomic with relaxed order reason: mutex synchronization - } else if (ATOMIC_LOAD(&m->exclusive_owner, memory_order_relaxed) != 0) { + if (ATOMIC_LOAD(&m->exclusive_owner, memory_order_relaxed) != 0) { FAIL_WITH_MESSAGE("Mutex destruction failed; mutex has an owner!"); } #ifndef PANDA_TARGET_MOBILE @@ -180,30 +177,26 @@ bool MutexLock(struct fmutex *const m, bool trylock) bool done = false; while (!done) { // Atomic with relaxed order reason: mutex synchronization - auto cur_state = ATOMIC_LOAD(&m->state_and_waiters, memory_order_relaxed); - if (LIKELY((HELPERS_TO_UNSIGNED(cur_state) & HELPERS_TO_UNSIGNED(HELD_MASK)) == 0)) { + auto cur_state = ATOMIC_LOAD(&m->exclusive_owner, memory_order_relaxed); + if (LIKELY(cur_state == 0)) { // Lock not held, try acquiring it. - auto new_state = HELPERS_TO_UNSIGNED(cur_state) | HELPERS_TO_UNSIGNED(HELD_MASK); - done = ATOMIC_CAS_WEAK(&m->state_and_waiters, cur_state, new_state, memory_order_acquire, - memory_order_relaxed); + auto new_state = current_tid; + done = + ATOMIC_CAS_WEAK(&m->exclusive_owner, cur_state, new_state, memory_order_acquire, memory_order_relaxed); } else { if (trylock) { return false; } // Failed to acquire, wait for unlock // NOLINTNEXTLINE(hicpp-signed-bitwise) - if (!WaitBrieflyFor(&m->state_and_waiters)) { + if (!WaitBrieflyFor(&m->exclusive_owner)) { // WaitBrieflyFor failed, go to futex wait - // Increment waiters count. - IncrementWaiters(m); - // Update cur_state to be equal to current expected state_and_waiters. - cur_state += WAITER_INCREMENT; - // Retry wait until lock not held. In heavy contention situations cur_state check can fail + // Retry wait until lock not held. In heavy contention situations curState check can fail // immediately due to repeatedly decrementing and incrementing waiters. // NOLINTNEXTLINE(C_RULE_ID_FUNCTION_NESTING_LEVEL) - while ((HELPERS_TO_UNSIGNED(cur_state) & HELPERS_TO_UNSIGNED(HELD_MASK)) != 0) { + while (cur_state != 0) { #ifdef MC_ON - FutexWait(&m->state_and_waiters, cur_state); + FutexWait(&m->exclusive_owner, cur_state); #else // NOLINTNEXTLINE(hicpp-signed-bitwise), NOLINTNEXTLINE(C_RULE_ID_FUNCTION_NESTING_LEVEL) if (futex(GetStateAddr(m), FUTEX_WAIT_PRIVATE, cur_state, nullptr, nullptr, 0) != 0) { @@ -214,20 +207,14 @@ bool MutexLock(struct fmutex *const m, bool trylock) } #endif // Atomic with relaxed order reason: mutex synchronization - cur_state = ATOMIC_LOAD(&m->state_and_waiters, memory_order_relaxed); + cur_state = ATOMIC_LOAD(&m->exclusive_owner, memory_order_relaxed); } - DecrementWaiters(m); } } } // Mutex is held now // Atomic with relaxed order reason: mutex synchronization - ASSERT((HELPERS_TO_UNSIGNED(ATOMIC_LOAD(&m->state_and_waiters, memory_order_relaxed)) & - HELPERS_TO_UNSIGNED(HELD_MASK)) != 0); - // Atomic with relaxed order reason: mutex synchronization - ASSERT(ATOMIC_LOAD(&m->exclusive_owner, memory_order_relaxed) == 0); - // Atomic with relaxed order reason: mutex synchronization - ATOMIC_STORE(&m->exclusive_owner, current_tid, memory_order_relaxed); + ASSERT(ATOMIC_LOAD(&m->exclusive_owner, memory_order_relaxed) != 0); m->recursive_count++; ASSERT(m->recursive_count == 1); // should be 1 here, there's a separate path for recursive mutex above return true; @@ -241,7 +228,7 @@ bool MutexTryLockWithSpinning(struct fmutex *const m) return true; } // NOLINTNEXTLINE(hicpp-signed-bitwise) - if (!WaitBrieflyFor(&m->state_and_waiters)) { + if (!WaitBrieflyFor(&m->exclusive_owner)) { // WaitBrieflyFor failed, means lock is held return false; } @@ -251,9 +238,8 @@ bool MutexTryLockWithSpinning(struct fmutex *const m) void MutexUnlock(struct fmutex *const m) { - if (current_tid == 0) { - current_tid = GET_CURRENT_THREAD; - } + // should be set in MutexLock + ASSERT(current_tid != 0); if (!IsHeld(m, current_tid)) { FAIL_WITH_MESSAGE("Trying to unlock mutex which is not held by current thread"); } @@ -267,38 +253,26 @@ void MutexUnlock(struct fmutex *const m) ASSERT(m->recursive_count == 0); // should be 0 here, there's a separate path for recursive mutex above bool done = false; // Atomic with relaxed order reason: mutex synchronization - auto cur_state = ATOMIC_LOAD(&m->state_and_waiters, memory_order_relaxed); + auto cur_state = ATOMIC_LOAD(&m->exclusive_owner, memory_order_relaxed); + auto new_state = 0; // Retry CAS until succeess - while (!done) { - auto new_state = HELPERS_TO_UNSIGNED(cur_state) & ~HELPERS_TO_UNSIGNED(HELD_MASK); // State without holding bit - if ((HELPERS_TO_UNSIGNED(cur_state) & HELPERS_TO_UNSIGNED(HELD_MASK)) == 0) { + do { + if (cur_state == 0) { FAIL_WITH_MESSAGE("Mutex unlock got unexpected state, mutex is unlocked?"); } - // Reset exclusive owner before changing state to avoid check failures if other thread sees UNLOCKED - // Atomic with relaxed order reason: mutex synchronization - ATOMIC_STORE(&m->exclusive_owner, 0, memory_order_relaxed); // cur_state should be updated with fetched value on fail - done = ATOMIC_CAS_WEAK(&m->state_and_waiters, cur_state, new_state, memory_order_release, memory_order_relaxed); - if (LIKELY(done)) { - // If we had waiters, we need to do futex call - if (UNLIKELY(new_state != 0)) { + done = ATOMIC_CAS_WEAK(&m->exclusive_owner, cur_state, new_state, memory_order_release, memory_order_relaxed); + } while (!done); #ifdef MC_ON - FutexWake(); + FutexWake(); #else - // NOLINTNEXTLINE(hicpp-signed-bitwise) - futex(GetStateAddr(m), FUTEX_WAKE_PRIVATE, WAKE_ONE, nullptr, nullptr, 0); + // NOLINTNEXTLINE(hicpp-signed-bitwise) + futex(GetStateAddr(m), FUTEX_WAKE_PRIVATE, WAKE_ONE, nullptr, nullptr, 0); #endif - } - } - } } void MutexLockForOther(struct fmutex *const m, THREAD_ID thread) { - // Atomic with relaxed order reason: mutex synchronization - ASSERT(ATOMIC_LOAD(&m->state_and_waiters, memory_order_relaxed) == 0); - // Atomic with relaxed order reason: mutex synchronization - ATOMIC_STORE(&m->state_and_waiters, HELD_MASK, memory_order_relaxed); m->recursive_count = 1; // Atomic with relaxed order reason: mutex synchronization ATOMIC_STORE(&m->exclusive_owner, thread, memory_order_relaxed); @@ -309,10 +283,6 @@ void MutexUnlockForOther(struct fmutex *const m, THREAD_ID thread) if (!IsHeld(m, thread)) { FAIL_WITH_MESSAGE("Unlocking for thread which doesn't own this mutex"); } - // Atomic with relaxed order reason: mutex synchronization - ASSERT(ATOMIC_LOAD(&m->state_and_waiters, memory_order_relaxed) == HELD_MASK); - // Atomic with relaxed order reason: mutex synchronization - ATOMIC_STORE(&m->state_and_waiters, 0, memory_order_relaxed); m->recursive_count = 0; // Atomic with relaxed order reason: mutex synchronization ATOMIC_STORE(&m->exclusive_owner, 0, memory_order_relaxed); @@ -366,9 +336,8 @@ struct timespec ConvertTime(uint64_t ms, uint64_t ns) void Wait(struct CondVar *const cond, struct fmutex *const m) { - if (current_tid == 0) { - current_tid = GET_CURRENT_THREAD; - } + // should be set in MutexLock + ASSERT(current_tid != 0); if (!IsHeld(m, current_tid)) { FAIL_WITH_MESSAGE("CondVar Wait failed; provided mutex is not held by current thread"); } @@ -385,7 +354,6 @@ void Wait(struct CondVar *const cond, struct fmutex *const m) // Atomic with relaxed order reason: mutex synchronization ATOMIC_FETCH_ADD(&cond->waiters, 1, memory_order_relaxed); - IncrementWaiters(m); auto old_count = m->recursive_count; m->recursive_count = 1; // Atomic with relaxed order reason: mutex synchronization @@ -393,11 +361,10 @@ void Wait(struct CondVar *const cond, struct fmutex *const m) MutexUnlock(m); #ifdef MC_ON - FutexWait(&cond->cond, cur_cond); + SignalWait(&cond->cond, cur_cond); #else - // NOLINTNEXTLINE(hicpp-signed-bitwise), NOLINTNEXTLINE(C_RULE_ID_FUNCTION_NESTING_LEVEL) + // NOLINTNEXTLINE(hicpp-signed-bitwise) if (futex(GetCondAddr(cond), FUTEX_WAIT_PRIVATE, cur_cond, nullptr, nullptr, 0) != 0) { - // NOLINTNEXTLINE(C_RULE_ID_FUNCTION_NESTING_LEVEL) if ((errno != EAGAIN) && (errno != EINTR)) { LOG(FATAL, COMMON) << "Futex wait failed!"; } @@ -405,16 +372,14 @@ void Wait(struct CondVar *const cond, struct fmutex *const m) #endif MutexLock(m, false); m->recursive_count = old_count; - DecrementWaiters(m); // Atomic with relaxed order reason: mutex synchronization ATOMIC_FETCH_SUB(&cond->waiters, 1, memory_order_relaxed); } bool TimedWait(struct CondVar *const cond, struct fmutex *const m, uint64_t ms, uint64_t ns, bool is_absolute) { - if (current_tid == 0) { - current_tid = GET_CURRENT_THREAD; - } + // should be set in MutexLock + ASSERT(current_tid != 0); if (!IsHeld(m, current_tid)) { FAIL_WITH_MESSAGE("CondVar Wait failed; provided mutex is not held by current thread"); } @@ -432,7 +397,6 @@ bool TimedWait(struct CondVar *const cond, struct fmutex *const m, uint64_t ms, bool timeout = false; // Atomic with relaxed order reason: mutex synchronization ATOMIC_FETCH_ADD(&cond->waiters, 1, memory_order_relaxed); - IncrementWaiters(m); auto old_count = m->recursive_count; m->recursive_count = 1; // Atomic with relaxed order reason: mutex synchronization @@ -440,7 +404,7 @@ bool TimedWait(struct CondVar *const cond, struct fmutex *const m, uint64_t ms, MutexUnlock(m); #ifdef MC_ON - FutexWait(&cond->cond, cur_cond); + SignalWait(&cond->cond, cur_cond); #else int wait_flag; int match_flag; @@ -467,7 +431,6 @@ bool TimedWait(struct CondVar *const cond, struct fmutex *const m, uint64_t ms, #endif MutexLock(m, false); m->recursive_count = old_count; - DecrementWaiters(m); // Atomic with relaxed order reason: mutex synchronization ATOMIC_FETCH_SUB(&cond->waiters, 1, memory_order_relaxed); return timeout; @@ -484,6 +447,7 @@ void SignalCount(struct CondVar *const cond, int32_t to_wake) if (current_tid == 0) { current_tid = GET_CURRENT_THREAD; } + // Atomic with relaxed order reason: mutex synchronization auto mutex = ATOMIC_LOAD(&cond->mutex_ptr, memory_order_relaxed); // If this condvar has waiters, mutex_ptr should be set @@ -492,7 +456,7 @@ void SignalCount(struct CondVar *const cond, int32_t to_wake) ATOMIC_FETCH_ADD(&cond->cond, 1, memory_order_relaxed); #ifdef MC_ON - FutexWake(); + SignalWake(); #else if (IsHeld(mutex, current_tid)) { // This thread is owner of current mutex, do requeue to mutex waitqueue diff --git a/platforms/unix/libpandabase/futex/fmutex.h b/platforms/unix/libpandabase/futex/fmutex.h index 36d08c098451e2e90518dd2c9ab57997839ebafd..c4fbc33b5d792b197d2a97b756e35f86247814b5 100644 --- a/platforms/unix/libpandabase/futex/fmutex.h +++ b/platforms/unix/libpandabase/futex/fmutex.h @@ -84,26 +84,16 @@ inline int futex(volatile int *uaddr, int op, int val, const struct timespec *ti static constexpr int WAKE_ONE = 1; static constexpr int WAKE_ALL = INT_MAX; -static constexpr int32_t HELD_MASK = 1; -static constexpr int32_t WAITER_SHIFT = 1; -// NOLINTNEXTLINE(hicpp-signed-bitwise, cppcoreguidelines-narrowing-conversions, bugprone-narrowing-conversions) -static constexpr int32_t WAITER_INCREMENT = 1 << WAITER_SHIFT; struct fmutex { - // Lowest bit: 0 - unlocked, 1 - locked. - // Other bits: Number of waiters. - // Unified lock state and waiters count to avoid requirement of double seq_cst memory order on mutex unlock - // as it's done in RWLock::WriteUnlock - ATOMIC_INT state_and_waiters; + // state_and_waiters field was removed, as it did not demonstrate any benefit + // Represents lock state: 0 - unlocked, otherwise - locked ATOMIC(THREAD_ID) exclusive_owner; int recursive_count; bool recursive_mutex; }; int *GetStateAddr(struct fmutex *m); -void IncrementWaiters(struct fmutex *m); -void DecrementWaiters(struct fmutex *m); -int32_t GetWaiters(struct fmutex *m); bool IsHeld(struct fmutex *m, THREAD_ID thread); __attribute__((visibility("default"))) bool MutexDoNotCheckOnTerminationLoop(); __attribute__((visibility("default"))) void MutexIgnoreChecksOnTerminationLoop(); diff --git a/platforms/unix/libpandabase/futex/mutex.h b/platforms/unix/libpandabase/futex/mutex.h index f3514891257a448af0b4ac2ea1f13c01c563ff0c..ae9c4af2b34a3ce92d12397bbc6120081533a2d4 100644 --- a/platforms/unix/libpandabase/futex/mutex.h +++ b/platforms/unix/libpandabase/futex/mutex.h @@ -76,23 +76,6 @@ private: return futex::GetStateAddr(&mutex_); } - void IncrementWaiters() - { - futex::IncrementWaiters(&mutex_); - } - - void DecrementWaiters() - { - futex::DecrementWaiters(&mutex_); - } - - int32_t GetWaiters() - { - // Atomic with relaxed order reason: mutex synchronization - // NOLINTNEXTLINE(hicpp-signed-bitwise) - return futex::GetWaiters(&mutex_); - } - bool IsHeld(thread::ThreadId thread) { return futex::IsHeld(&mutex_, thread);