From 1badceb455dae28a93a51da6c5cda0b510f93701 Mon Sep 17 00:00:00 2001 From: Filoppi Date: Wed, 17 Nov 2021 22:55:02 +0200 Subject: [PATCH] ControllerInterface: fix UpdateReferences() deadlock Removed useless locks to DeviceContainer::m_devices_mutex, as they were all already protected by m_devices_population_mutex. We have no interest in blocking other threads that were potentially reading devices at the same time so this seems fine. This simplifies the code, and I've adjusted a few comments which mentioned possible deadlock that should now be totally gone. The deadlock could have happen if a thread directly called EmulatedController::UpdateReferences(), while another another thread also reached EmulatedController::UpdateReferences() within a call to ControllerInterface::UpdateDevices(), as the mentioned function locked both the DeviceContainer::m_devices_mutex and s_get_state_mutex at the same time. The deadlock was frequent on game emulation startup on Android, due to the UpdateReferences() call in InputConfig::LoadConfig() and the UI thread triggering calls to ControllerInterface::UpdateDevices(). It could also have happened on Desktop if a user pressed "Refresh Devices" manually in the UI while the input config was loading. Also brought some UpdateReferences() comments and thread safety fixes from https://github.com/dolphin-emu/dolphin/pull/9489 --- Source/Core/DolphinQt/Settings.cpp | 3 +- .../ControllerEmu/ControllerEmu.cpp | 3 ++ .../ControllerInterface.cpp | 51 +++++++------------ 3 files changed, 21 insertions(+), 36 deletions(-) diff --git a/Source/Core/DolphinQt/Settings.cpp b/Source/Core/DolphinQt/Settings.cpp index 0232929e52..7e91064a25 100644 --- a/Source/Core/DolphinQt/Settings.cpp +++ b/Source/Core/DolphinQt/Settings.cpp @@ -80,8 +80,7 @@ Settings::Settings() // they'd continue living until the queued event has run, but some devices can't be recreated // until they are destroyed. // This is safe from any thread. Devices will be refreshed and re-acquired and in - // DevicesChanged(). Waiting on QueueOnObject() to have finished running was not feasible as - // it would cause deadlocks without heavy workarounds. + // DevicesChanged(). Calling it without queueing shouldn't cause any deadlocks but is slow. emit ReleaseDevices(); QueueOnObject(this, [this] { emit DevicesChanged(); }); diff --git a/Source/Core/InputCommon/ControllerEmu/ControllerEmu.cpp b/Source/Core/InputCommon/ControllerEmu/ControllerEmu.cpp index c804e303f2..9cd7ced6f9 100644 --- a/Source/Core/InputCommon/ControllerEmu/ControllerEmu.cpp +++ b/Source/Core/InputCommon/ControllerEmu/ControllerEmu.cpp @@ -18,6 +18,8 @@ namespace ControllerEmu { +// This should theoretically be per EmulatedController instance, +// though no EmulatedController usually run in parallel, so it makes little difference static std::recursive_mutex s_get_state_mutex; std::string EmulatedController::GetDisplayName() const @@ -77,6 +79,7 @@ void EmulatedController::UpdateSingleControlReference(const ControllerInterface& { ciface::ExpressionParser::ControlEnvironment env(devi, GetDefaultDevice(), m_expression_vars); + const auto lock = GetStateLock(); ref->UpdateReference(env); env.CleanUnusedVariables(); diff --git a/Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp b/Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp index 6a9cfc25fd..aaaa3971e6 100644 --- a/Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp +++ b/Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp @@ -54,8 +54,6 @@ void ControllerInterface::Initialize(const WindowSystemInfo& wsi) m_populating_devices_counter = 1; - m_devices_mutex.lock(); - #ifdef CIFACE_USE_WIN32 ciface::Win32::Init(wsi.render_window); #endif @@ -87,8 +85,9 @@ void ControllerInterface::Initialize(const WindowSystemInfo& wsi) RefreshDevices(); + // Devices writes are already protected by m_devices_population_mutex but this won't hurt + m_devices_mutex.lock(); const bool devices_empty = m_devices.empty(); - m_devices_mutex.unlock(); if (m_populating_devices_counter.fetch_sub(1) == 1 && !devices_empty) @@ -121,17 +120,17 @@ void ControllerInterface::RefreshDevices(RefreshReason reason) std::lock_guard lk_pre_population(m_pre_population_mutex); // This is needed to stop its threads before locking our mutexes, to avoid deadlocks // (in case it tried to add a device after we had locked m_devices_population_mutex). - // There doesn't seem to be an easy to way to repopulate OSX devices without restarting - // its hotplug thread. This will not release its devices, that's still done below. + // There doesn't seem to be an easy to way to repopulate OSX devices without restarting its + // hotplug thread. This should not remove its devices, and if it did, calls should be ignored. ciface::OSX::DeInit(); } #endif - // This lock has two main functions: - // -Avoid a deadlock between m_devices_mutex and ControllerEmu::s_state_mutex when - // InvokeDevicesChangedCallbacks() is called concurrently by two different threads. - // -Avoid devices being destroyed while others of the same type are being created. - // This wasn't thread safe in multiple device sources. + // We lock m_devices_population_mutex here to make everything simpler. + // Multiple devices classes have their own "hotplug" thread, and can add/remove devices at any + // time, while actual writes to "m_devices" are safe, the order in which they happen is not. That + // means a thread could be adding devices while we are removing them, or removing them as we are + // populating them (causing missing or duplicate devices). std::lock_guard lk_population(m_devices_population_mutex); #if defined(CIFACE_USE_WIN32) && !defined(CIFACE_USE_XLIB) && !defined(CIFACE_USE_OSX) @@ -141,12 +140,9 @@ void ControllerInterface::RefreshDevices(RefreshReason reason) { m_populating_devices_counter.fetch_add(1); - { - std::lock_guard lk(m_devices_mutex); - // No need to do anything else in this case. - // Only (Win32) DInput needs the window handle to be updated. - ciface::Win32::ChangeWindow(m_wsi.render_window); - } + // No need to do anything else in this case. + // Only (Win32) DInput needs the window handle to be updated. + ciface::Win32::ChangeWindow(m_wsi.render_window); if (m_populating_devices_counter.fetch_sub(1) == 1) InvokeDevicesChangedCallbacks(); @@ -156,13 +152,6 @@ void ControllerInterface::RefreshDevices(RefreshReason reason) m_populating_devices_counter.fetch_add(1); - // We lock m_devices_mutex here to make everything simpler. - // Multiple devices classes have their own "hotplug" thread, and can add/remove devices at any - // time, while actual writes to "m_devices" are safe, the order in which they happen is not. That - // means a thread could be adding devices while we are removing them, or removing them as we are - // populating them (causing missing or duplicate devices). - m_devices_mutex.lock(); - // Make sure shared_ptr objects are released before repopulating. ClearDevices(); @@ -206,8 +195,6 @@ void ControllerInterface::RefreshDevices(RefreshReason reason) WiimoteReal::PopulateDevices(); - m_devices_mutex.unlock(); - if (m_populating_devices_counter.fetch_sub(1) == 1) InvokeDevicesChangedCallbacks(); } @@ -221,11 +208,7 @@ void ControllerInterface::PlatformPopulateDevices(std::function callback m_populating_devices_counter.fetch_add(1); - { - std::lock_guard lk(m_devices_mutex); - - callback(); - } + callback(); if (m_populating_devices_counter.fetch_sub(1) == 1) InvokeDevicesChangedCallbacks(); @@ -270,8 +253,8 @@ void ControllerInterface::Shutdown() // Make sure no devices had been added within Shutdown() in the time // between checking they checked atomic m_is_init bool and we changed it. - // We couldn't have locked m_devices_mutex nor m_devices_population_mutex for the whole Shutdown() - // as they could cause deadlocks. Note that this is still not 100% safe as some backends are + // We couldn't have locked m_devices_population_mutex for the whole Shutdown() + // as it could cause deadlocks. Note that this is still not 100% safe as some backends are // shut down in other places, possibly adding devices after we have shut down, but the chances of // that happening are basically zero. ClearDevices(); @@ -294,8 +277,8 @@ void ControllerInterface::ClearDevices() o->SetState(0); } - // Devices will still be alive after this: there are shared ptrs around the code holding them, - // but InvokeDevicesChangedCallbacks() will clean all of them. + // Devices could still be alive after this as there might be shared ptrs around holding them. + // The InvokeDevicesChangedCallbacks() underneath should always clean all of them (it needs to). m_devices.clear(); }