Fix Frame Advance (Reverts PR 2396, Fixes issue 8718) #3712
Conversation
RisingFog
force-pushed
the
revert_fix_racing_cpu_core
branch
from
992116c
to
6127552
Compare
|
So, uh, yeah, what about the part where that fixes a race condition? I honestly couldn't care less about TASers abilities to frame advance if it means reintroducing a data race |
|
That PR fixed one race condition and introduced a much more obvious race condition. If we're wanting to release stable any time soon, we need to get the current issues out of the way. As far as I know, reverting this PR reverts to behaviour from 4.0.2, and can be addressed once 5.0 is released. |
|
I disagree with fixing a race condition by reverting to another one that's less significant. Consider actually fixing the underlying issue. |
|
Re-opening this PR while I look into a better solution. |
|
I'm still working on getting a Linux setup in place to do some further debugging on the issue. After getting some more information on the original PR, it appears that it was to fix issues when debugging the JIT. I'm not sure if this issue would happen on normal operation. |
|
After finally setting up my linux environment, I did some testing with tsan enabled. I didn't find any races relating to the original PR, but I did find a few race conditions which should be looked at in separate PRs. |
RisingFog
changed the title
|
@dolphin-emu-bot rebuild |
|
This is clearly not the correct method of fixing this, closing this PR. |
|
After some debate on IRC, re-opening yet again. |
|
The problem appears to be a race condition in CPU::EnableStepping + Core::GetState. Core::GetState returns CORE_PAUSED when CPU::IsStepping returns true but PowerPC::Pause happens first in CPU::EnableStepping(false) before the FIFO is paused. This enables a race condition where Movie::DoFrameStep tries to set SetState(CORE_RUN) on the GUI Thread before the SetState(CORE_PAUSED) in Movie::FrameUpdate has finished on the GPU Thread and the FIFO then gets stuck in the Fifo::EmulatorState(false) state. |
|
Of course, the solution to race conditions is always more locks. |
|
Another problem: In Single Thread mode, Movie::FrameUpdate's Core::SetState(CORE_PAUSE) will deadlock itself since PowerPC::Pause will wait on s_state_change to be set by FinishStateMove which is not going to happen in a combined CPU-GPU thread. Currently it happens to work through a desynchronization bug — the FinishStateMove from the previous Movie::FrameUpdate will linger in s_state_change causing the WaitFor to succeed when it should not, but the first Frame Advance will always deadlock. The desynchronization bug causes the profiling bug from 2396 to resurface. |
|
http://hastebin.com/imixevowup.xml This was the tsan trace I was able to get yesterday with the current master with frame advance. |
This reverts PR #2396, which caused frame advance to unpause randomly instead of properly stepping through frames.
This fixes issue 8718: https://bugs.dolphin-emu.org/issues/8718