From e92313bf7e2346f44deec4dd4ea70289a621c4ca Mon Sep 17 00:00:00 2001 From: Olivier Goffart Date: Tue, 28 Aug 2012 15:47:35 +0200 Subject: Add comments to document the internals of QMutex Change-Id: Ieb5632017e5e8e09a11dc6b929efa19b4f350086 Reviewed-by: Thiago Macieira --- src/corelib/thread/qmutex.cpp | 49 +++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 47 insertions(+), 2 deletions(-) (limited to 'src/corelib/thread/qmutex.cpp') diff --git a/src/corelib/thread/qmutex.cpp b/src/corelib/thread/qmutex.cpp index 2bf3176787..745455dc3f 100644 --- a/src/corelib/thread/qmutex.cpp +++ b/src/corelib/thread/qmutex.cpp @@ -2,6 +2,7 @@ ** ** Copyright (C) 2012 Nokia Corporation and/or its subsidiary(-ies). ** Copyright (C) 2012 Intel Corporation +** Copyright (C) 2012 Olivier Goffart ** Contact: http://www.qt-project.org/ ** ** This file is part of the QtCore module of the Qt Toolkit. @@ -371,6 +372,27 @@ bool QBasicMutex::isRecursive() */ #ifndef QT_LINUX_FUTEX //linux implementation is in qmutex_linux.cpp + +/* + For a rough introduction on how this works, refer to + http://woboq.com/blog/internals-of-qmutex-in-qt5.html + which explains a slightly simplified version of it. + The differences are that here we try to work with timeout (requires the + possiblyUnlocked flag) and that we only wake one thread when unlocking + (requires maintaining the waiters count) + We also support recursive mutexes which always have a valid d_ptr. + + The waiters flag represents the number of threads that are waiting or about + to wait on the mutex. There are two tricks to keep in mind: + We don't want to increment waiters after we checked no threads are waiting + (waiters == 0). That's why we atomically set the BigNumber flag on waiters when + we check waiters. Similarily, if waiters is decremented right after we checked, + the mutex would be unlocked (d->wakeUp() has (or will) be called), but there is + no thread waiting. This is only happening if there was a timeout in tryLock at the + same time as the mutex is unlocked. So when there was a timeout, we set the + possiblyUnlocked flag. +*/ + /*! \internal helper for lock() */ @@ -394,6 +416,8 @@ bool QBasicMutex::lockInternal(int timeout) QT_MUTEX_LOCK_NOEXCEPT if (copy == dummyLocked()) { if (timeout == 0) return false; + // The mutex is locked but does not have a QMutexPrivate yet. + // we need to allocate a QMutexPrivate QMutexPrivate *newD = QMutexPrivate::allocate(); if (!d_ptr.testAndSetOrdered(dummyLocked(), newD)) { //Either the mutex is already unlocked, or another thread already set it. @@ -408,15 +432,25 @@ bool QBasicMutex::lockInternal(int timeout) QT_MUTEX_LOCK_NOEXCEPT if (timeout == 0 && !d->possiblyUnlocked.load()) return false; + // At this point we have a pointer to a QMutexPrivate. But the other thread + // may unlock the mutex at any moment and release the QMutexPrivate to the pool. + // We will try to reference it to avoid unlock to release it to the pool to make + // sure it won't be released. But if the refcount is already 0 it has been released. if (!d->ref()) continue; //that QMutexData was already released + // We now hold a reference to the QMutexPrivate. It won't be released and re-used. + // But it is still possible that it was already re-used by another QMutex right before + // we did the ref(). So check if we still hold a pointer to the right mutex. if (d != d_ptr.loadAcquire()) { //Either the mutex is already unlocked, or relocked with another mutex d->deref(); continue; } + // In this part, we will try to increment the waiters count. + // We just need to take care of the case in which the old_waiters + // is set to the BigNumber magic value set in unlockInternal() int old_waiters; do { old_waiters = d->waiters.load(); @@ -440,7 +474,7 @@ bool QBasicMutex::lockInternal(int timeout) QT_MUTEX_LOCK_NOEXCEPT } while (!d->waiters.testAndSetRelaxed(old_waiters, old_waiters + 1)); if (d != d_ptr.loadAcquire()) { - // Mutex was unlocked. + // The mutex was unlocked before we incremented waiters. if (old_waiters != QMutexPrivate::BigNumber) { //we did not break the previous loop Q_ASSERT(d->waiters.load() >= 1); @@ -451,6 +485,7 @@ bool QBasicMutex::lockInternal(int timeout) QT_MUTEX_LOCK_NOEXCEPT } if (d->wait(timeout)) { + // reset the possiblyUnlocked flag if needed (and deref its corresponding reference) if (d->possiblyUnlocked.load() && d->possiblyUnlocked.testAndSetRelaxed(true, false)) d->deref(); d->derefWaiters(1); @@ -463,8 +498,12 @@ bool QBasicMutex::lockInternal(int timeout) QT_MUTEX_LOCK_NOEXCEPT d->derefWaiters(1); //There may be a race in which the mutex is unlocked right after we timed out, // and before we deref the waiters, so maybe the mutex is actually unlocked. - if (!d->possiblyUnlocked.testAndSetRelaxed(false, true)) + // Set the possiblyUnlocked flag to indicate this possibility. + if (!d->possiblyUnlocked.testAndSetRelaxed(false, true)) { + // We keep a reference when possiblyUnlocked is true. + // but if possiblyUnlocked was already true, we don't need to keep the reference. d->deref(); + } return false; } } @@ -484,9 +523,15 @@ void QBasicMutex::unlockInternal() Q_DECL_NOTHROW QMutexPrivate *d = reinterpret_cast(copy); + // If no one is waiting for the lock anymore, we shoud reset d to 0x0. + // Using fetchAndAdd, we atomically check that waiters was equal to 0, and add a flag + // to the waiters variable (BigNumber). That way, we avoid the race in which waiters is + // incremented right after we checked, because we won't increment waiters if is + // equal to -BigNumber if (d->waiters.fetchAndAddRelease(-QMutexPrivate::BigNumber) == 0) { //there is no one waiting on this mutex anymore, set the mutex as unlocked (d = 0) if (d_ptr.testAndSetRelease(d, 0)) { + // reset the possiblyUnlocked flag if needed (and deref its corresponding reference) if (d->possiblyUnlocked.load() && d->possiblyUnlocked.testAndSetRelaxed(true, false)) d->deref(); } -- cgit v1.2.3