ShuriZma
/
dolphin
mirror of https://github.com/dolphin-emu/dolphin.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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
This commit is contained in:
Filoppi 2021-11-17 22:55:02 +02:00
parent 0b81640dd1
commit 1badceb455
3 changed files with 21 additions and 36 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
Source/Core/DolphinQt/Settings.cpp
View File

@ -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(); });

3
Source/Core/InputCommon/ControllerEmu/ControllerEmu.cpp
View File

@ -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();

43
Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp
View File

@ -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);
}
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<Device> 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<void()> callback
m_populating_devices_counter.fetch_add(1);
{
std::lock_guard lk(m_devices_mutex);
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();
}