GCAdapter: improve thread safety #3805

mathieui · 2016-04-30T11:31:04Z

make sure Reset() can’t be run concurrently with AddGCAdapter(), which can cause crashes (issue #9462).

As the crash happens only on windows, I can’t test this.

This change is

phire · 2016-04-30T12:18:58Z

Well it fixes the crash.

But the multithreading code still looks a bit messy.

mathieui · 2016-04-30T14:03:33Z

Well, the multithreading code is a bit spotty but I don’t see any easy way around it as we can’t block the CPU thread and need another thread to fetch data from the controller; doing away with the scanning thread is kind of difficult if we don’t want users to have a manual interaction (we probably could make it run only when there is no adapter detected, although it may cause issues).

We could put everything which isn’t on the CPU thread behind a mutex, but it may be a brutal thing to do.

phire · 2016-04-30T14:43:08Z

What's worrying me is I looked up libusb, and it's supposed to be thread safe.

Yet somehow we are getting errors. Also s_handle and s_detected are set/accessed on multiple threads and should be atomic.

Sonicadvance1 · 2016-04-30T16:54:44Z

Maybe you should glean some ideas of how to make it stable by looking at my Android implementation? :P

JMC47 · 2016-04-30T20:36:03Z

Fixes my crash finally. I'm happy.

Source/Core/InputCommon/GCAdapter.cpp

@@ -37,6 +37,7 @@ static std::atomic<int> s_controller_payload_size = {0};
 static std::thread s_adapter_thread;
 static Common::Flag s_adapter_thread_running;

+static std::recursive_mutex s_init_mutex;


JMC47 · 2016-05-04T02:19:52Z

Are we going to review/merge this, or pursue a better solution?

mimimi085181 · 2016-05-11T17:40:36Z

@mathieui: Is this still WIP?

mathieui · 2016-05-11T23:55:23Z

@mimimi085181 This should be fine now, one last code review to make sure I haven’t missed a spot, and maybe another test from @JMC47 before merging.

JMC47 · 2016-05-12T12:30:17Z

It's crashed in master about 6 more times since I last reviewed it. I really need to just generally use this build in master to know for sure the crash is completely gone. I think it is though.

JMC47 · 2016-05-14T01:37:24Z

Is this ready for review?

JMC47 · 2016-05-14T01:44:19Z

This actually fixes both of my GC Adapter crashes. The one running two dolphin's and closing the first one that has the GC adapter used to crash the second Dolphin. It no longer does that in this pull request.

RisingFog · 2016-05-14T01:45:56Z

Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions.

Source/Core/InputCommon/GCAdapter.cpp, line 476 [r3] (raw file):

  DEBUG_LOG(SERIALINTERFACE, "Rumble state reset");
}

Unnecessary extra line.

Comments from Reviewable

mathieui · 2016-05-14T01:50:30Z

Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions.

Source/Core/InputCommon/GCAdapter.cpp, line 476 [r3] (raw file):

Previously, RisingFog (Chris Burgener) wrote…

Unnecessary extra line.

Done.

Comments from Reviewable

RisingFog · 2016-05-14T01:50:40Z

Reviewed 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

JMC47 · 2016-05-14T01:50:41Z

LGTM

RisingFog · 2016-05-16T15:10:13Z

Anything holding this up from merge?

degasus · 2016-05-18T14:58:17Z

Review status: all files reviewed at latest revision, 2 unresolved discussions.

Source/Core/InputCommon/GCAdapter.cpp, line 454 [r4] (raw file):

void ResetRumble()
{
  if (!s_init_mutex.try_lock())

May you add a comment why this is required? eg in which path is it already locked, and why it's not needed to call this function here.

Comments from Reviewable

degasus · 2016-05-18T15:03:41Z

Reviewed 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

Comments from Reviewable

lioncash · 2016-05-18T15:07:47Z

Source/Core/InputCommon/GCAdapter.cpp, line 456 [r4] (raw file):

  if (!s_init_mutex.try_lock())
      return;
  ResetRumbleNoLock();

This name is kind of misleading. It has "NoLock" in the name, yet is being used between a mutex that is trying to be locked, effectively stating the opposite of what the function name intends.

Comments from Reviewable

lioncash · 2016-05-18T18:39:47Z

Review status: all files reviewed at latest revision, 5 unresolved discussions.

Source/Core/InputCommon/GCAdapter.cpp, line 338 [r4] (raw file):

void Reset()
{
  if (!s_init_mutex.try_lock())

Now that I look at this again, this can just be:

std::unique_lock<std::mutex> lock(s_init_mutex, std::defer_lock);
if (!lock.try_lock())
    return;

which would let you get rid of the explicit locking and unlocking elsewhere.

Source/Core/InputCommon/GCAdapter.cpp, line 454 [r4] (raw file):

void ResetRumble()
{
  if (!s_init_mutex.try_lock())

This can also be:

std::unique_lock<std::mutex> lock(s_init_mutex, std::defer_lock);
if (!lock.try_lock())
    return;

Comments from Reviewable

make sure Reset() can’t be run concurrently with AddGCAdapter() or ResetRumble() (which is called on other threads) which can cause crashes (issue dolphin-emu#9462)

mathieui · 2016-05-18T18:47:06Z

Review status: 0 of 1 files reviewed at latest revision, 5 unresolved discussions.

Source/Core/InputCommon/GCAdapter.cpp, line 338 [r4] (raw file):

Previously, lioncash (Mat M.) wrote…

Now that I look at this again, this can just be:
std::unique_lock<std::mutex> lock(s_init_mutex, std::defer_lock);
if (!lock.try_lock())
    return;
which would let you get rid of the explicit locking and unlocking elsewhere.

Thanks for the tip, done.

Source/Core/InputCommon/GCAdapter.cpp, line 454 [r4] (raw file):

Previously, lioncash (Mat M.) wrote…

This can also be:

std::unique_lock<std::mutex> lock(s_init_mutex, std::defer_lock);
if (!lock.try_lock())
    return;

Done.

Source/Core/InputCommon/GCAdapter.cpp, line 454 [r4] (raw file):

Previously, degasus (Markus Wick) wrote…

May you add a comment why this is required? eg in which path is it already locked, and why it's not needed to call this function here.

Done in the comment for ResetRumbleLockNeeded()

Source/Core/InputCommon/GCAdapter.cpp, line 456 [r4] (raw file):

Previously, lioncash (Mat M.) wrote…

This name is kind of misleading. It has "NoLock" in the name, yet is being used between a mutex that is trying to be locked, effectively stating the opposite of what the function name intends.

Done.

Comments from Reviewable

RisingFog · 2016-05-18T18:55:53Z

Reviewed 1 of 1 files at r6.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

Reset() and Setup() are not used outside of this namespace

degasus · 2016-05-19T20:03:50Z

Previously, RisingFog (Chris Burgener) wrote…

Anything holding this up from merge?

Reviewed 2 of 2 files at r7, 2 of 2 files at r8.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

mathieui force-pushed the gcadapter-unplug-crash branch from a566e86 to cea56d2 Compare April 30, 2016 12:08

lioncash reviewed Apr 30, 2016
View reviewed changes

Parlane added the WIP / do not merge Work in progress (do not merge) label May 3, 2016

mathieui force-pushed the gcadapter-unplug-crash branch from 67d9a6c to 9915b1d Compare May 11, 2016 23:51

mathieui changed the title ~~[WIP] GCAdapter: improve thread safety~~ GCAdapter: improve thread safety May 12, 2016

Parlane removed the WIP / do not merge Work in progress (do not merge) label May 12, 2016

mathieui force-pushed the gcadapter-unplug-crash branch from 9915b1d to a62e457 Compare May 14, 2016 01:49

mathieui force-pushed the gcadapter-unplug-crash branch from a62e457 to f7dc777 Compare May 18, 2016 18:40

GCAdapter: improve thread safety

54b4eff

make sure Reset() can’t be run concurrently with AddGCAdapter() or ResetRumble() (which is called on other threads) which can cause crashes (issue dolphin-emu#9462)

mathieui force-pushed the gcadapter-unplug-crash branch from f7dc777 to 54b4eff Compare May 18, 2016 18:45

GCAdapter: protect some more functions

1bbbe92

Reset() and Setup() are not used outside of this namespace

mathieui force-pushed the gcadapter-unplug-crash branch from 849b84d to 1bbbe92 Compare May 19, 2016 20:01

degasus added this to the Dolphin Release 5.0 milestone May 19, 2016

degasus merged commit bb6604d into dolphin-emu:master May 19, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GCAdapter: improve thread safety #3805

GCAdapter: improve thread safety #3805

mathieui commented Apr 30, 2016 •

edited by delroth

Loading

phire commented Apr 30, 2016

mathieui commented Apr 30, 2016

phire commented Apr 30, 2016

Sonicadvance1 commented Apr 30, 2016

JMC47 commented Apr 30, 2016

This comment was marked as off-topic.

This comment was marked as off-topic.

JMC47 commented May 4, 2016

mimimi085181 commented May 11, 2016

mathieui commented May 11, 2016

JMC47 commented May 12, 2016

JMC47 commented May 14, 2016

JMC47 commented May 14, 2016

RisingFog commented May 14, 2016

mathieui commented May 14, 2016

RisingFog commented May 14, 2016

JMC47 commented May 14, 2016

RisingFog commented May 16, 2016

degasus commented May 18, 2016

degasus commented May 18, 2016

lioncash commented May 18, 2016

lioncash commented May 18, 2016

mathieui commented May 18, 2016

RisingFog commented May 18, 2016

degasus commented May 19, 2016

GCAdapter: improve thread safety #3805

GCAdapter: improve thread safety #3805

Conversation

mathieui commented Apr 30, 2016 • edited by delroth Loading

phire commented Apr 30, 2016

mathieui commented Apr 30, 2016

phire commented Apr 30, 2016

Sonicadvance1 commented Apr 30, 2016

JMC47 commented Apr 30, 2016

This comment was marked as off-topic.

This comment was marked as off-topic.

JMC47 commented May 4, 2016

mimimi085181 commented May 11, 2016

mathieui commented May 11, 2016

JMC47 commented May 12, 2016

JMC47 commented May 14, 2016

JMC47 commented May 14, 2016

RisingFog commented May 14, 2016

mathieui commented May 14, 2016

RisingFog commented May 14, 2016

JMC47 commented May 14, 2016

RisingFog commented May 16, 2016

degasus commented May 18, 2016

degasus commented May 18, 2016

lioncash commented May 18, 2016

lioncash commented May 18, 2016

mathieui commented May 18, 2016

RisingFog commented May 18, 2016

degasus commented May 19, 2016

mathieui commented Apr 30, 2016 •

edited by delroth

Loading