Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add hybrid/cooperate suspend runtime support on Windows. #14101

Merged

Conversation

lateralusX
Copy link
Member

@lateralusX lateralusX commented Apr 17, 2019

Adding initial support for hybrid and cooperate suspend on Windows 32/64-bit platforms.

Fixes/adjustments needed in several areas:

Implementation/emulation of gcc's __builtin_unwind_init on MSVC needed by copy_stack_data function.

All wait methods have been extended with a cooperative alternative + handling interrupt under hybrid/cooperative suspend.

Methods that handles IO abort interruption on Windows (file, socket) have been updated to handle interrupt under hybrid/cooperative suspend.

Moved Windows implementations into w32handle.c instead of #ifdef on call site.

Reduced number of GC safe transition in a couple of Win32 file IO methods.

Added restore logic to Win32 last error when exiting GC safe mode. This is needed since we have code that depends on GetLastError that could be clobbered if exiting GC safe mode enter a wait. Also added last error restore logic to uninstall interrupt handler since it is commonly used right after exiting GC safe mode.

mono/utils/mono-os-wait-win32.c Show resolved Hide resolved
mono/metadata/threads.c Show resolved Hide resolved
mono/metadata/w32file-win32.c Show resolved Hide resolved
mono/metadata/threads.c Show resolved Hide resolved
mono/metadata/w32file-win32.c Show resolved Hide resolved
mono/metadata/w32file-win32.c Outdated Show resolved Hide resolved
@jaykrell
Copy link
Contributor

Nice overall, just nit picking.

@lateralusX
Copy link
Member Author

lateralusX commented Apr 18, 2019

First run all Windows configs have pass using default preemtive suspend. Next round will be hybrid suspend + review feedback.

@lateralusX lateralusX force-pushed the lateralusX/add-coop-support-windows-msvc branch 3 times, most recently from 8c855fc to 98a012b Compare April 18, 2019 08:46
mono/utils/mono-threads.h Outdated Show resolved Hide resolved
mono/utils/mono-threads.h Outdated Show resolved Hide resolved
@jaykrell
Copy link
Contributor

jaykrell commented Apr 18, 2019

Now just to add four or eight more CI lanes? :)

@jaykrell
Copy link
Contributor

Again very good, I just nit-picky.
Putting explicit __cdecl on the x86 asm is maybe the only significant point.
The compiling mono with default __fastcall or __stdcall might be catastrophic in other places anyway.

@lateralusX
Copy link
Member Author

lateralusX commented Apr 18, 2019

First run of hybrid suspend on CI for all Windows configuration is done!

Windows x64, all CI test pass on hybrid suspend.
Windows x64 C++, all CI test pass on hybrid suspend.

Windows x64 FullAOT, only one failing test case, error in finalizer-exit:

finalizer-exit.exe
=============== finalizer-exit.exe.stdout ===============
Set ID: foo
finalizer thread ID: foo

=============== EOF ===============
=============== finalizer-exit.exe.stderr ===============

Windows i386, 3 failures, all looks related to the same issue:

MonoTests.runtime.thread-suspend-suspended.exe
MonoTests.test-sgen-regular-ms-simple.sgen-new-threads-collect.exe
MonoTests.test-sgen-regular-ms-simple.finalizer-wait.exe

Cannot transition thread 000001A8 from SELF_SUSPENDED with REQUEST_PULSE

@lateralusX lateralusX force-pushed the lateralusX/add-coop-support-windows-msvc branch 2 times, most recently from 10045aa to 9a89cf9 Compare April 18, 2019 13:55
@lateralusX
Copy link
Member Author

Run with cooperate suspend on CI, all test pass.

@lateralusX
Copy link
Member Author

lateralusX commented Apr 24, 2019

Analyzed failure in thread-suspend-suspended.exe on 32-bit Windows and the identified problem, Cannot transition thread 000001A8 from SELF_SUSPENDED with REQUEST_PULSE, could happened in the following scenarion (emulated in debugger).

  • Have main thread run its start_gc.Set () call but freeze before entering GC Safe of its next wait, finished_gc.WaitOne ()
  • Run GC thread and let it do first phase, this will wait for main thread to suspend.
  • Switch to main thread and resume it, this will make it self suspend on enter GC safe. NOTE, entering GC safe will call into OS WaitForSingleObject to wait for resume.
  • GC thread will now look at the threads and determine if its in a critical region or not. On wow64 this means doing some extra check, mono_threads_platform_in_critical_region, that could fail, especially in hybrid suspend since we have not suspended the thread in the case (it's self suspended or on its way towards self suspend, but still running code).

If it fails due to that check stw will move over to pulse resume the thread but since it is already self suspend, it will hit the assert. The fix is to not do the wow64 critical check, mono_threads_platform_in_critical_region, if thread is not preemptive suspended. This is needed since the call to GetThreadContext is not guaranteed to get a valid context on a running thread.

@lateralusX
Copy link
Member Author

Run with hybrid suspend on CI, all Windows test lanes pass.

Under hybrid suspend a thread can be cooperative suspended but still
checked for a platform critical region under wow64. Since thread is
not preemptive suspended we can't reliably call GetThreadContext and
doing so could also (even correctly) return a context flagged as critical
triggering an assert in stw for cooperative suspended threads.
@lateralusX lateralusX force-pushed the lateralusX/add-coop-support-windows-msvc branch from a1452f9 to 0d39006 Compare April 24, 2019 12:16
@lateralusX lateralusX changed the title WIP: Add hybrid/cooperate suspend runtime support on Windows. Add hybrid/cooperate suspend runtime support on Windows. Apr 24, 2019
@lateralusX
Copy link
Member Author

@monojenkins rebuild failed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants