diff --git a/include/qutexAcquisitionHistoryTracker.h b/include/qutexAcquisitionHistoryTracker.h index e8b2270..80246cc 100644 --- a/include/qutexAcquisitionHistoryTracker.h +++ b/include/qutexAcquisitionHistoryTracker.h @@ -5,6 +5,7 @@ #include #include #include +#include "spinLock.h" namespace smo { @@ -75,15 +76,20 @@ public: heldLocks ) { - auto it = acquisitionHistory.find(continuation); + acquisitionHistoryLock.acquire(); + auto it = acquisitionHistory.find(continuation); // If a continuation already exists, don't add it again - if (it != acquisitionHistory.end()) { + if (it != acquisitionHistory.end()) + { + acquisitionHistoryLock.release(); return; } acquisitionHistory.emplace(continuation, std::make_pair( std::ref(wantedLock), std::move(heldLocks))); + + acquisitionHistoryLock.release(); } /** @@ -97,20 +103,27 @@ public: std::shared_ptr &continuation ) { + acquisitionHistoryLock.acquire(); + auto it = acquisitionHistory.find(continuation); - if (it != acquisitionHistory.end()) { + if (it != acquisitionHistory.end()) + { acquisitionHistory.erase(it); + + acquisitionHistoryLock.release(); return true; } + + acquisitionHistoryLock.release(); return false; } bool heuristicallyTraceContinuationHistoryForGridlockOn( Qutex &firstFailedQutex, - std::shared_ptr& currentContinuation) - const; + std::shared_ptr& + currentContinuation); bool completelyTraceContinuationHistoryForGridlockOn( - Qutex &firstFailedQutex) const; + Qutex &firstFailedQutex); // Disable copy constructor and assignment operator QutexAcquisitionHistoryTracker( @@ -123,6 +136,15 @@ private: ~QutexAcquisitionHistoryTracker() = default; private: + /** EXPLANATION: + * We use a SpinLock here instead of a Qutex because this acquisition + * history tracker is invoked within the LockerAndInvoker. + * Since LockerAndInvoker is too tightly coupled with Qutex workings, using + * a Qutex here would create a circular dependency or deadlock situation. + * Therefore, it's best to use a SpinLock on the history class to avoid + * these coupling issues. + */ + SpinLock acquisitionHistoryLock; AcquisitionHistoryMap acquisitionHistory; }; diff --git a/smocore/qutexAcquisitionHistoryTracker.cpp b/smocore/qutexAcquisitionHistoryTracker.cpp index 7e2bc05..a00b29e 100644 --- a/smocore/qutexAcquisitionHistoryTracker.cpp +++ b/smocore/qutexAcquisitionHistoryTracker.cpp @@ -66,7 +66,6 @@ bool QutexAcquisitionHistoryTracker ::heuristicallyTraceContinuationHistoryForGridlockOn( Qutex &firstFailedQutex, std::shared_ptr& currentContinuation) -const { /** HEURISTIC APPROACH: * Due to the computational complexity of full circularity detection, @@ -82,6 +81,28 @@ const * explanation. */ + /** NOTICE: + * Generally we should have all global data structures owned by a single + * ComponentThread; and qutexes really should only be used to serialize + * async sequences being enqueued on the same ComponentThread. But this + * doesn't prevent multiple CPUs from trying to add/remove entries to/from + * the acquisition history at the same time. Why? The acquisition history + * isn't per-CPU, it's global. + * + * The problem with using a SpinLock here is that if the STL uses mutexes + * internally to lock containers, we could end up in a situation where + * spinning waiters will be busy-spinning while the owner is sleeping? + * + * But this should not happen since the nature of the order of operations is + * that the spinlock ensures that only one CPU at a time can be + * adding/removing entries; and thus everytime an method is called on the + * unordered_map, the caller will always succeed at acquiring the underlying + * STL mutex. + * + * So it should be safe to use a SpinLock here. + */ + acquisitionHistoryLock.acquire(); + // Iterate through all entries in the acquisition history for (const auto& entry : acquisitionHistory) { const auto& continuation = entry.first; @@ -104,18 +125,20 @@ const /* Found firstFailedQutex in another continuation's held locks * This indicates a potential gridlock */ - if (&heldLock.get() == &firstFailedQutex) { + if (&heldLock.get() == &firstFailedQutex) + { + acquisitionHistoryLock.release(); return true; } } } + acquisitionHistoryLock.release(); return false; } bool QutexAcquisitionHistoryTracker ::completelyTraceContinuationHistoryForGridlockOn(Qutex &firstFailedQutex) -const { /** ALGORITHMICALLY COMPLETE VERSION: * This function is intended to implement the algorithmically complete @@ -128,6 +151,9 @@ const * explanation. */ + // acquisitionHistoryLock.acquire(); + // TODO: Implement full circularity detection algorithm + // acquisitionHistoryLock.release(); return false; }