Don't call RunAsCPUThread in config callbacks #12117
Conversation
JosJuice
force-pushed
the
config-callback-cpu
branch
3 times, most recently
from
de1bb23
to
f6ce897
Compare
|
@iwubcode FreelookConfig.cpp has a config changed callback that uses RunAsCPUThread in master, but as far as I can tell, this class initialized on the host thread and then accessed on the GPU thread and the hotkey scheduler thread, so it doesn't really have anything to do with the CPU thread. Do you know why this code is using RunAsCPUThread? And also, why FreeLookConfig.cpp is using the callback system at all instead of fetching values from the config system when needed? |
JosJuice
force-pushed
the
config-callback-cpu
branch
from
8f7928c
to
4a94f73
Compare
|
@JosJuice - that code followed the same design principles as VideoConfig because the two are very similar in nature (though I don't recall the comment about As for why it does that at all, there was a time when config access was deemed bad practice in the realtime path. I know you made changes to that but I still out of habit will avoid accessing config when possible. To be fair, this code was written way before that change was made. |
|
I think I'll just allow threads other than the CPU thread to register callbacks. (See the last commit.) @AdmiralCurtiss Do you have any opinion on this PR in general? I know you were working on addressing some of the other thread safety issues of config callbacks. |
| @@ -37,7 +39,7 @@ void Config::Refresh() | |||
| { | |||
| if (!s_has_registered_callback) | |||
| { | |||
| ::Config::AddConfigChangedCallback([] { Core::RunAsCPUThread([] { s_config.Refresh(); }); }); | |||
| CPUThreadConfigCallback::AddConfigChangedCallback([] { s_config.Refresh(); }); | |||
There was a problem hiding this comment.
Just to be safe, this likely needs the same complexity involved as in the VideoConfig as that is what the FreeLookConfig was patterned after. FreeLook is this weird blend of inputs (the controller) and graphics (the "camera") and the config is used by both systems.
There was a problem hiding this comment.
The reason why it was added for VideoConfig was to stop the GPU thread from reading the config before VerifyValidity ran. There's nothing like VerifyValidity here, so we don't have the same problem.
I suppose that if we are to standards lawyer this, this is technically unsafe because there could be tearing, as there's no synchronization between the CPU thread writing and the GPU thread reading. But since every value involved is 32-bit at most, we're safe on every CPU we have first-class support for. If we are to do something about it, I think it would be better addressed by something simpler like a mutex, and this is far from the only config callback in the code base with this problem anyway.
In theory, our config system supports calling Set from any thread. But because we have config callbacks that call RunAsCPUThread, it's a lot more restricted in practice. Calling Set from any thread other than the host thread or the CPU thread is formally thread unsafe, and calling Set on the host thread while the CPU thread is showing a panic alert causes a deadlock. This is especially a problem because 04072f0 made the "Ignore for this session" button in panic alerts call Set. Because so many of our config callbacks want their code to run on the CPU thread, I thought it would make sense to have a centralized way to move execution to the CPU thread for config callbacks. To solve the deadlock problem, this new way is non-blocking. This means that threads other than the CPU thread might continue executing before the CPU thread is informed of the new config, but I don't think there's any problem with that. Intends to fix https://bugs.dolphin-emu.org/issues/13108.
This fixes a problem that started happening in CoreTimingTest after the previous commit. CPUThreadConfigCallback registers a Config callback only once per run of the process, but CoreTimingTest calls Config::Shutdown after each test, and Config::Shutdown was clearing all callbacks, preventing the callback from running after that.
This fixes a bunch of DEBUG_ASSERTs in the unit tests.
This way you can't mix up regular config callback IDs and CPU thread config callback IDs. (It would be rather bad if you did!)
Turns out that we have two subsystems that want to register CPU thread callbacks from a different thread than the CPU thread: FreeLookConfig and VideoConfig. Both seem to happen from the host thread before the CPU thread starts, so there's no thread safety issue. But ideally, if we want to allow registering callbacks from threads other than the CPU thread, we should make registering callbacks actually thread-safe. This is an unsolved problem for the regular Config system, so I would like to leave it outside the scope of this PR.
JosJuice
force-pushed
the
config-callback-cpu
branch
from
4a94f73
to
b62c258
Compare
|
I'm a bit conflicted, this is adding complexity and threading is usually not improved by adding complexity. However, we definitely have a problem with the callbacks on master, so if this resolves some of them, I guess that's okay. |
|
Okay, after thinking about this a bit more, I think this is probably an improvement, though I have one question: If emulation is paused, should the updates apply immediately or should they still be delayed until the first CoreTiming::Advance()? This might be confusing if you're debugging a game and your config changes get delayed until an unknown point in the future. e: Wait, the checks in CPUManager::Run() take care of that, don't they? Yeah okay, I think this is good then. |
Yeah, that was my intent with those checks. |
|
Launched a few games and messed with settings, seems to work as expected. |
In theory, our config system supports calling
Setfrom any thread. But because we have config callbacks that callRunAsCPUThread, it's a lot more restricted in practice. CallingSetfrom any thread other than the host thread or the CPU thread is formally thread unsafe, and callingSeton the host thread while the CPU thread is showing a panic alert causes a deadlock. This is especially a problem because 04072f0 made the "Ignore for this session" button in panic alerts callSet.Because so many of our config callbacks want their code to run on the CPU thread, I thought it would make sense to have a centralized way to move execution to the CPU thread for config callbacks. To solve the deadlock problem, this new way is non-blocking. This means that threads other than the CPU thread might continue executing before the CPU thread is informed of the new config, but I don't think there's any problem with that.
Intends to fix https://bugs.dolphin-emu.org/issues/13108.