[coop] Attach external native threads in GC Safe mode #31765
Closed
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
monojenkins
force-pushed
the
sync-pr-18708-from-mono
branch
3 times, most recently
from
ede39a3
to
d5368e6
Compare
monojenkins
force-pushed
the
sync-pr-18708-from-mono
branch
8 times, most recently
from
be7edef
to
2a4ad7c
Compare
monojenkins
force-pushed
the
sync-pr-18708-from-mono
branch
from
2a4ad7c
to
030b92e
Compare
monojenkins
requested review from
BrzVlad,
CoffeeFlux,
lambdageek,
lateralusX,
marek-safar,
SamMonoRT,
thaystg and
vargaz
as code owners
lambdageek
added
the
NO-MERGE
(Broke out the second part of mono/mono#18656 into a separate PR for easier reviewing and added a test case) Mark `mono_thread_attach` external only. Runtime should use `mono_thread_internal_attach`. The difference is that threads that are started on behalf of Mono will start in GC Unsafe mode, whereas external threads that attach to the runtime will be in GC Safe mode. This is consistent with how native-to-managed thunks behave, too. They will attach the thread and switch to GC Unsafe mode while running managed code, but will switch to GC Safe mode when they return. If the runtime needs to suspend one of these native threads while it's not running any runtime or managed code, it will use preemptive suspension under the hybrid (and full-async) suspend policy. There is the orthogonal question of whether the attached thread will be foreground or background. This PR does not change the current behavior: if an external native thread calls a native-to-managed thunk it will be attached as a background thread. If it explicitly calls `mono_thread_attach`, it will be foreground. Additionally, change `mono_thread_detach` to switch to GC Unsafe mode while calling `mono_thread_detach_internal`, to undo the effects of `mono_thread_attach`. Switch external thread to GC Safe only if it wasn't already. If an embedder calls `mono_thread_attach` / `mono_thread_detach` themselves, the `MonoThreadInfo*` persists. And since the thread is external to the runtime, it is in GC Safe mode while it isn't calling into the runtime. So after the detach, if we try to re-attach it, performing a transition to GC Safe again will assert, since GC Safe mode doesn't nest. Additionally, if we need to create the managed `MonoInternalThread` and `MonoThread` objects, we should do that in GC Unsafe mode, so add transitions to ensure that's the case.
monojenkins
force-pushed
the
sync-pr-18708-from-mono
branch
from
030b92e
to
ecd024c
Compare
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
!! This PR is a copy of mono/mono#18708, please do not edit or review it in this repo !!
!! Merge the PR only after the original PR is merged !!
(Broke out the second part of mono/mono#18656 into a separate PR for easier reviewing and added a test case)
Mark
mono_thread_attachexternal only. Runtime should usemono_thread_internal_attach.The difference is that threads that are started on behalf of Mono will start in GC Unsafe mode, whereas external threads that attach to the runtime will be in GC Safe mode. This is consistent with how native-to-managed thunks behave, too. They will attach the thread and switch to GC Unsafe mode while running managed code, but will switch to GC Safe mode when they return. If the runtime needs to suspend one of these native threads while it's not running any runtime or managed code, it will use preemptive suspension under the hybrid (and full-async) suspend policy.
There is the orthogonal question of whether the attached thread will be foreground or background. This PR does not change the current behavior: if an external native thread calls a native-to-managed thunk it will be attached as a background thread. If it explicitly calls
mono_thread_attach, it will be foreground.Additionally, change
mono_thread_detachto switch to GC Unsafe mode while callingmono_thread_detach_internal, to undo the effects ofmono_thread_attach.Switch external thread to GC Safe only if it wasn't already. If an embedder calls
mono_thread_attach/mono_thread_detachthemselves, theMonoThreadInfo*persists. And since the thread is external to the runtime, it is in GC Safe mode while it isn't calling into the runtime. So after the detach, if we try to re-attach it, performing a transition to GC Safe again will assert, since GC Safe mode doesn't nest.Additionally, if we need to create the managed
MonoInternalThreadandMonoThreadobjects, we should do that in GC Unsafe mode, so add transitions to ensure that's the case.