From 72f8f994b26fe51bf4828e17096b7ec0abe1f0d3 Mon Sep 17 00:00:00 2001 From: Thiago Macieira Date: Thu, 22 Mar 2018 23:06:56 +0800 Subject: [PATCH] QSemaphore: attempt to fix again the 64-bit Linux semaphore MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This time, the issue was that we could race a wait and a wake. A multi- token waiter would not notice that the number of tokens changed because it only performed a fetch-and-OR, then waited on the high part which did not change. That means the futex_wait() would succeed, when we wanted it to have failed. So we have to bring back a portion of what commit 081c001deb75fa38385d3ff8cbbdb4f788529133 removed: we need to keep both the token count and the waiter count in the high word. Task-number: QTBUG-67214 Change-Id: I04a43ee94975482f9e32fffd151e467a9e0030b3 Reviewed-by: MÃ¥rten Nordheim Reviewed-by: Thiago Macieira --- src/corelib/thread/qsemaphore.cpp | 33 +++++++++++++++++++------------ 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/src/corelib/thread/qsemaphore.cpp b/src/corelib/thread/qsemaphore.cpp index c2e4d7d9d14..c440d6db39f 100644 --- a/src/corelib/thread/qsemaphore.cpp +++ b/src/corelib/thread/qsemaphore.cpp @@ -120,12 +120,12 @@ using namespace QtFutex; If the system has the ability to wake up a precise number of threads, has Linux's FUTEX_WAKE_OP functionality, and is 64-bit, instead of using a - single bit indicating a contended semaphore, we'll store the total number - of waiters in the high word. Additionally, all multi-token waiters will be - waiting on that high word. So when releasing n tokens on those systems, we - tell the kernel to wake up n single-token threads and all of the - multi-token ones. Which threads get woken up is unspecified, but it's - likely single-token threads will get woken up first. + single bit indicating a contended semaphore, we'll store the number of + tokens *plus* total number of waiters in the high word. Additionally, all + multi-token waiters will be waiting on that high word. So when releasing n + tokens on those systems, we tell the kernel to wake up n single-token + threads and all of the multi-token ones. Which threads get woken up is + unspecified, but it's likely single-token threads will get woken up first. */ #if defined(FUTEX_OP) && QT_POINTER_SIZE > 4 @@ -231,10 +231,14 @@ template bool futexSemaphoreTryAcquire(QBasicAtomicInteger= n) { // try to acquire - quintptr newValue = curValue - n; + quintptr newValue = curValue - nn; if (u.testAndSetOrdered(curValue, newValue, curValue)) return true; // succeeded! } @@ -242,7 +246,6 @@ template bool futexSemaphoreTryAcquire(QBasicAtomicInteger bool futexSemaphoreTryAcquire(QBasicAtomicInteger> 32) == 0x7fffffff) + return false; // overflow! curValue += oneWaiter; - // Also adjust valueToSubtract to subtract oneWaiter when we succeed in - // acquiring. - valueToSubtract += oneWaiter; + // Also adjust nn to subtract oneWaiter when we succeed in acquiring. + nn += oneWaiter; } - if (futexSemaphoreTryAcquire_loop(u, curValue, valueToSubtract, timeout)) + if (futexSemaphoreTryAcquire_loop(u, curValue, nn, timeout)) return true; if (futexHasWaiterCount) { @@ -345,7 +349,10 @@ void QSemaphore::release(int n) Q_ASSERT_X(n >= 0, "QSemaphore::release", "parameter 'n' must be non-negative"); if (futexAvailable()) { - quintptr prevValue = u.fetchAndAddRelease(n); + quintptr nn = unsigned(n); + if (futexHasWaiterCount) + nn |= quint64(nn) << 32; // token count replicated in high word + quintptr prevValue = u.fetchAndAddRelease(nn); if (futexNeedsWake(prevValue)) { #ifdef FUTEX_OP if (!futexHasWaiterCount) {