Remove atomic usage and fix mutex locking in GCAdapter code #10540
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
You can safely read or write non-atomic integers on multiple threads, as long as every thread reading or writing it holds the same mutex while doing so (here, s_mutex). Removing the atomic accesses makes the code faster, but the actual performance difference is probably negligible.
s_controller_write_payload_size needs to remain an atomic because Read() loads and stores without holding a mutex, Output() stores while holding s_write_mutex, and ResetRumble() stores while holding s_read_mutex! I'm pretty sure this code is wrong, specifically ResetRumble().
I am not confident there are no race conditions between s_write_mutex, s_controller_write_payload_size, and s_controller_write_payload. But this code should be safer than before.
|
I would say let's change |
|
bump
|
AdmiralCurtiss
approved these changes
Apr 12, 2022
|
If you have further changes it's probably better to make another PR for them. |
Hmm... Not sure then. I guess it's fine either way. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Makes
s_controller_payload_sizenon-atomic. You can safely read or write non-atomic integers on multiple threads, as long as every thread reading or writing it holds the same mutex while doing so (here,s_mutex).Removing the atomic accesses makes the code faster, but the actual performance difference is probably negligible.
Makes
s_controller_payload_sizenon-atomic for the same reason. Howevers_controller_write_payload_sizemust remain atomic to avoid UB (though I doubt it needs to be seq_cst), since it's being read without a lock held. This threading system may have race conditions, I'm not sure.Along the way I also noticed that
ResetRumble()(apparently called on Core.cpp#void SetState(State state)) holds the wrong mutex, so I fixed that as well.I did not test my changes on-device to verify this does not introduce deadlocksthis cannot introduce deadlocks sinceResetRumble()is never called on Android! If it does, the old code was wrong as well, and a different change will have to be made (either different usage of mutexes, or a lock-free handshake), requiring that I study the control flow more carefully (which is difficult, and I would benefit from help on).Background
The atomic usage was introduced in 07caff3 and a62a9b8. The
ResetRumble()bug dates back to the file's creation at e62503c. I did not look into why only the Android implementation has two mutexes.Android fails to ResetRumble entirely!
I decided to test
ResetRumble()on Android:This caused the rumble to remain running.
If I read the code properly, this is supposed to call
MENU_ACTION_PAUSE_EMULATION->NativeLibrary.PauseEmulation() = Java_org_dolphinemu_dolphinemu_NativeLibrary_PauseEmulation->namespace Core::SetState(Core::State::Paused)->ResetRumble()->GCAdapter::ResetRumble(). Unfortunately, CMakeLists.txt doesn't set__LIBUSB__on Android, so Core.cpp#ResetRumble()doesn't callGCAdapter::ResetRumble()which would map to GCAdapter_Android.cpp#GCAdapter::ResetRumble(). Consequently, Android never callsResetRumble(), so the current code is dead and the wrong mutex would never be reached!Should I remove Core.cpp
#if defined(__LIBUSB__)in yet another commit in this PR, or open a separate PR? At that point, I'd probably want to compile an Android build for testing. Also I didn't review all other uses of__LIBUSB__for correctness, though the build seems to work on all OSes right now.