From 7f1e5b368e5bb2f7b500076ca0dccc29267f0075 Mon Sep 17 00:00:00 2001 From: elsid Date: Wed, 24 Nov 2021 19:10:02 +0100 Subject: [PATCH] Fix deadlock in physics system * Reorder unlock and notify_all calls to avoid notifying when not all worker threads are waiting. * Make sure main thread does not attempt to exclusively lock mSimulationMutex while not all workers are done with previous frame. * Replace mNewFrame flag by counter to avoid modification from multiple threads. --- apps/openmw/mwphysics/mtphysics.cpp | 38 +++++++++++++++++++++++------ apps/openmw/mwphysics/mtphysics.hpp | 7 +++++- 2 files changed, 36 insertions(+), 9 deletions(-) diff --git a/apps/openmw/mwphysics/mtphysics.cpp b/apps/openmw/mwphysics/mtphysics.cpp index 29d1e0b7c..5a7f5f044 100644 --- a/apps/openmw/mwphysics/mtphysics.cpp +++ b/apps/openmw/mwphysics/mtphysics.cpp @@ -138,7 +138,7 @@ namespace MWPhysics , mRemainingSteps(0) , mLOSCacheExpiry(Settings::Manager::getInt("lineofsight keep inactive cache", "Physics")) , mDeferAabbUpdate(Settings::Manager::getBool("defer aabb update", "Physics")) - , mNewFrame(false) + , mFrameCounter(0) , mAdvanceSimulation(false) , mQuit(false) , mNextJob(0) @@ -176,12 +176,13 @@ namespace MWPhysics PhysicsTaskScheduler::~PhysicsTaskScheduler() { + waitForWorkers(); std::unique_lock lock(mSimulationMutex); mQuit = true; mNumJobs = 0; mRemainingSteps = 0; - lock.unlock(); mHasJob.notify_all(); + lock.unlock(); for (auto& thread : mThreads) thread.join(); } @@ -234,9 +235,10 @@ namespace MWPhysics const std::vector& PhysicsTaskScheduler::moveActors(float & timeAccum, std::vector&& actorsData, osg::Timer_t frameStart, unsigned int frameNumber, osg::Stats& stats) { + waitForWorkers(); + // This function run in the main thread. // While the mSimulationMutex is held, background physics threads can't run. - std::unique_lock lock(mSimulationMutex); double timeStart = mTimer->tick(); @@ -282,7 +284,7 @@ namespace MWPhysics mPhysicsDt = newDelta; mActorsFrameData = std::move(actorsData); mAdvanceSimulation = (mRemainingSteps != 0); - mNewFrame = true; + ++mFrameCounter; mNumJobs = mActorsFrameData.size(); mNextLOS.store(0, std::memory_order_relaxed); mNextJob.store(0, std::memory_order_release); @@ -302,8 +304,8 @@ namespace MWPhysics } mAsyncStartTime = mTimer->tick(); - lock.unlock(); mHasJob.notify_all(); + lock.unlock(); if (mAdvanceSimulation) mBudget.update(mTimer->delta_s(timeStart, mTimer->tick()), 1, mBudgetCursor); return mMovedActors; @@ -311,6 +313,7 @@ namespace MWPhysics const std::vector& PhysicsTaskScheduler::resetSimulation(const ActorMap& actors) { + waitForWorkers(); std::unique_lock lock(mSimulationMutex); mBudget.reset(mDefaultPhysicsDt); mAsyncBudget.reset(0.0f); @@ -477,11 +480,13 @@ namespace MWPhysics void PhysicsTaskScheduler::worker() { + std::size_t lastFrame = 0; std::shared_lock lock(mSimulationMutex); while (!mQuit) { - if (!mNewFrame) - mHasJob.wait(lock, [&]() { return mQuit || mNewFrame; }); + if (mRemainingSteps == 0 && lastFrame == mFrameCounter) + mHasJob.wait(lock, [&] { return mQuit || lastFrame != mFrameCounter; }); + lastFrame = mFrameCounter; mPreStepBarrier->wait([this] { afterPreStep(); }); @@ -618,7 +623,6 @@ namespace MWPhysics void PhysicsTaskScheduler::afterPostSim() { - mNewFrame = false; if (mLOSCacheExpiry >= 0) { std::unique_lock lock(mLOSCacheMutex); @@ -628,5 +632,23 @@ namespace MWPhysics mLOSCache.end()); } mTimeEnd = mTimer->tick(); + std::unique_lock lock(mWorkersDoneMutex); + ++mWorkersFrameCounter; + mWorkersDone.notify_all(); + } + + // Attempt to acquire unique lock on mSimulationMutex while not all worker + // threads are holding shared lock but will have to may lead to a deadlock because + // C++ standard does not guarantee priority for exclusive and shared locks + // for std::shared_mutex. For example microsoft STL implementation points out + // for the absence of such priority: + // https://docs.microsoft.com/en-us/windows/win32/sync/slim-reader-writer--srw--locks + void PhysicsTaskScheduler::waitForWorkers() + { + if (mNumThreads == 0) + return; + std::unique_lock lock(mWorkersDoneMutex); + if (mFrameCounter != mWorkersFrameCounter) + mWorkersDone.wait(lock); } } diff --git a/apps/openmw/mwphysics/mtphysics.hpp b/apps/openmw/mwphysics/mtphysics.hpp index 3fd4d5a69..0c4402579 100644 --- a/apps/openmw/mwphysics/mtphysics.hpp +++ b/apps/openmw/mwphysics/mtphysics.hpp @@ -69,6 +69,7 @@ namespace MWPhysics void afterPreStep(); void afterPostStep(); void afterPostSim(); + void waitForWorkers(); std::unique_ptr mWorldFrameData; std::vector mActorsFrameData; @@ -91,7 +92,7 @@ namespace MWPhysics int mRemainingSteps; int mLOSCacheExpiry; bool mDeferAabbUpdate; - bool mNewFrame; + std::size_t mFrameCounter; bool mAdvanceSimulation; bool mThreadSafeBullet; bool mQuit; @@ -99,6 +100,10 @@ namespace MWPhysics std::atomic mNextLOS; std::vector mThreads; + std::size_t mWorkersFrameCounter = 0; + std::condition_variable mWorkersDone; + std::mutex mWorkersDoneMutex; + mutable std::shared_mutex mSimulationMutex; mutable std::shared_mutex mCollisionWorldMutex; mutable std::shared_mutex mLOSCacheMutex;