From 38a348c991049fb0134e27774292c2d8929fda22 Mon Sep 17 00:00:00 2001 From: Ludovic Henry Date: Wed, 9 Nov 2016 15:59:36 -0500 Subject: [PATCH] [threads] Always use the `suspended` MonoOSEvent for self-suspend (#3915) * [stw] Refactor to treat error/success cases first * [threads] Always use the `suspended` MonoOSEvent for self-suspend The previous self-suspend mechanism was inherently racy: - T1 self suspend: - T1 calls mono_thread_info_begin_self_suspend, but doesn't call mono_thread_info_end_self_suspend yet - T2 suspends T1 <-- the suspend_count is > 0, trigerring the assertion at mono-threads.c:184 --- mcs/class/corlib/System.Threading/Thread.cs | 1 + mcs/class/corlib/System/Environment.cs | 2 +- mono/metadata/appdomain.c | 2 +- mono/metadata/object-internals.h | 1 + mono/metadata/sgen-stw.c | 106 +++++++++++++++------------- mono/metadata/threads.c | 39 +++++----- mono/tests/Makefile.am | 3 +- mono/tests/thread-suspend-selfsuspended.cs | 42 +++++++++++ mono/utils/mono-threads-state-machine.c | 44 ------------ mono/utils/mono-threads.c | 47 ------------ mono/utils/mono-threads.h | 10 --- 11 files changed, 122 insertions(+), 175 deletions(-) create mode 100644 mono/tests/thread-suspend-selfsuspended.cs diff --git a/mcs/class/corlib/System.Threading/Thread.cs b/mcs/class/corlib/System.Threading/Thread.cs index 24ada26034c..4bce5168e7d 100644 --- a/mcs/class/corlib/System.Threading/Thread.cs +++ b/mcs/class/corlib/System.Threading/Thread.cs @@ -94,6 +94,7 @@ namespace System.Threading { private int priority = (int) ThreadPriority.Normal; private IntPtr owned_mutex; private IntPtr suspended_event; + private int self_suspended; /* * These fields are used to avoid having to increment corlib versions * when a new field is added to the unmanaged MonoThread structure. diff --git a/mcs/class/corlib/System/Environment.cs b/mcs/class/corlib/System/Environment.cs index 91e76352ddc..c8b87782310 100644 --- a/mcs/class/corlib/System/Environment.cs +++ b/mcs/class/corlib/System/Environment.cs @@ -57,7 +57,7 @@ namespace System { * of icalls, do not require an increment. */ #pragma warning disable 169 - private const int mono_corlib_version = 162; + private const int mono_corlib_version = 163; #pragma warning restore 169 [ComVisible (true)] diff --git a/mono/metadata/appdomain.c b/mono/metadata/appdomain.c index db45a76a95e..e18c3febebd 100644 --- a/mono/metadata/appdomain.c +++ b/mono/metadata/appdomain.c @@ -84,7 +84,7 @@ * Changes which are already detected at runtime, like the addition * of icalls, do not require an increment. */ -#define MONO_CORLIB_VERSION 162 +#define MONO_CORLIB_VERSION 163 typedef struct { diff --git a/mono/metadata/object-internals.h b/mono/metadata/object-internals.h index 1a60d2d85f3..46269e2a377 100644 --- a/mono/metadata/object-internals.h +++ b/mono/metadata/object-internals.h @@ -384,6 +384,7 @@ struct _MonoInternalThread { gint32 priority; GPtrArray *owned_mutexes; MonoOSEvent *suspended; + gint32 self_suspended; // TRUE | FALSE /* * These fields are used to avoid having to increment corlib versions * when a new field is added to this structure. diff --git a/mono/metadata/sgen-stw.c b/mono/metadata/sgen-stw.c index 9b9de7180ed..22ff814df69 100644 --- a/mono/metadata/sgen-stw.c +++ b/mono/metadata/sgen-stw.c @@ -248,30 +248,35 @@ sgen_is_thread_in_current_stw (SgenThreadInfo *info, int *reason) static void sgen_unified_suspend_stop_world (void) { - int restart_counter; int sleep_duration = -1; mono_threads_begin_global_suspend (); THREADS_STW_DEBUG ("[GC-STW-BEGIN][%p] *** BEGIN SUSPEND *** \n", mono_thread_info_get_tid (mono_thread_info_current ())); FOREACH_THREAD (info) { - int reason; info->client_info.skip = FALSE; info->client_info.suspend_done = FALSE; - if (sgen_is_thread_in_current_stw (info, &reason)) { - info->client_info.skip = !mono_thread_info_begin_suspend (info); - THREADS_STW_DEBUG ("[GC-STW-BEGIN-SUSPEND] SUSPEND thread %p skip %s\n", mono_thread_info_get_tid (info), info->client_info.skip ? "true" : "false"); - } else { + + int reason; + if (!sgen_is_thread_in_current_stw (info, &reason)) { THREADS_STW_DEBUG ("[GC-STW-BEGIN-SUSPEND] IGNORE thread %p skip %s reason %d\n", mono_thread_info_get_tid (info), info->client_info.skip ? "true" : "false", reason); + continue; } + + info->client_info.skip = !mono_thread_info_begin_suspend (info); + + THREADS_STW_DEBUG ("[GC-STW-BEGIN-SUSPEND] SUSPEND thread %p skip %s\n", mono_thread_info_get_tid (info), info->client_info.skip ? "true" : "false"); } FOREACH_THREAD_END mono_thread_info_current ()->client_info.suspend_done = TRUE; mono_threads_wait_pending_operations (); for (;;) { - restart_counter = 0; + gint restart_counter = 0; + FOREACH_THREAD (info) { + gint suspend_count; + int reason = 0; if (info->client_info.suspend_done || !sgen_is_thread_in_current_stw (info, &reason)) { THREADS_STW_DEBUG ("[GC-STW-RESTART] IGNORE RESUME thread %p not been processed done %d current %d reason %d\n", mono_thread_info_get_tid (info), info->client_info.suspend_done, !sgen_is_thread_in_current_stw (info, NULL), reason); @@ -284,27 +289,28 @@ sgen_unified_suspend_stop_world (void) - We haven't accepted the previous suspend as good. - We haven't gave up on it for this STW (it's either bad or asked not to) */ - if (mono_thread_info_in_critical_location (info)) { - gboolean res; - gint suspend_count = mono_thread_info_suspend_count (info); - if (!(suspend_count == 1)) - g_error ("[%p] suspend_count = %d, but should be 1", mono_thread_info_get_tid (info), suspend_count); - res = mono_thread_info_begin_resume (info); - if (res) - ++restart_counter; - else - info->client_info.skip = TRUE; - THREADS_STW_DEBUG ("[GC-STW-RESTART] RESTART thread %p skip %s\n", mono_thread_info_get_tid (info), info->client_info.skip ? "true" : "false"); - } else { - THREADS_STW_DEBUG ("[GC-STW-RESTART] DONE thread %p deemed fully suspended\n", mono_thread_info_get_tid (info)); - g_assert (!info->client_info.in_critical_region); + if (!mono_thread_info_in_critical_location (info)) { info->client_info.suspend_done = TRUE; + + THREADS_STW_DEBUG ("[GC-STW-RESTART] DONE thread %p deemed fully suspended\n", mono_thread_info_get_tid (info)); + continue; } + + suspend_count = mono_thread_info_suspend_count (info); + if (!(suspend_count == 1)) + g_error ("[%p] suspend_count = %d, but should be 1", mono_thread_info_get_tid (info), suspend_count); + + info->client_info.skip = !mono_thread_info_begin_resume (info); + if (!info->client_info.skip) + restart_counter += 1; + + THREADS_STW_DEBUG ("[GC-STW-RESTART] RESTART thread %p skip %s\n", mono_thread_info_get_tid (info), info->client_info.skip ? "true" : "false"); } FOREACH_THREAD_END + mono_threads_wait_pending_operations (); + if (restart_counter == 0) break; - mono_threads_wait_pending_operations (); if (sleep_duration < 0) { mono_thread_info_yield (); @@ -321,47 +327,51 @@ sgen_unified_suspend_stop_world (void) continue; } - if (mono_thread_info_is_running (info)) { - gboolean res = mono_thread_info_begin_suspend (info); - if (!res) - info->client_info.skip = TRUE; - THREADS_STW_DEBUG ("[GC-STW-RESTART] SUSPEND thread %p skip %s\n", mono_thread_info_get_tid (info), info->client_info.skip ? "true" : "false"); + if (!mono_thread_info_is_running (info)) { + THREADS_STW_DEBUG ("[GC-STW-RESTART] IGNORE SUSPEND thread %p not running\n", mono_thread_info_get_tid (info)); + continue; } + + info->client_info.skip = !mono_thread_info_begin_suspend (info); + + THREADS_STW_DEBUG ("[GC-STW-RESTART] SUSPEND thread %p skip %s\n", mono_thread_info_get_tid (info), info->client_info.skip ? "true" : "false"); } FOREACH_THREAD_END mono_threads_wait_pending_operations (); } FOREACH_THREAD (info) { + gpointer stopped_ip; + int reason = 0; - if (sgen_is_thread_in_current_stw (info, &reason)) { - gpointer stopped_ip; + if (!sgen_is_thread_in_current_stw (info, &reason)) { + g_assert (!info->client_info.suspend_done || info == mono_thread_info_current ()); - g_assert (info->client_info.suspend_done); + THREADS_STW_DEBUG ("[GC-STW-SUSPEND-END] thread %p is NOT suspended, reason %d\n", mono_thread_info_get_tid (info), reason); + continue; + } - info->client_info.ctx = mono_thread_info_get_suspend_state (info)->ctx; + g_assert (info->client_info.suspend_done); - /* Once we remove the old suspend code, we should move sgen to directly access the state in MonoThread */ - info->client_info.stack_start = (gpointer) ((char*)MONO_CONTEXT_GET_SP (&info->client_info.ctx) - REDZONE_SIZE); + info->client_info.ctx = mono_thread_info_get_suspend_state (info)->ctx; - /* altstack signal handler, sgen can't handle them, mono-threads should have handled this. */ - if (!info->client_info.stack_start - || info->client_info.stack_start < info->client_info.stack_start_limit - || info->client_info.stack_start >= info->client_info.stack_end) { - g_error ("BAD STACK: stack_start = %p, stack_start_limit = %p, stack_end = %p", - info->client_info.stack_start, info->client_info.stack_start_limit, info->client_info.stack_end); - } + /* Once we remove the old suspend code, we should move sgen to directly access the state in MonoThread */ + info->client_info.stack_start = (gpointer) ((char*)MONO_CONTEXT_GET_SP (&info->client_info.ctx) - REDZONE_SIZE); - stopped_ip = (gpointer) (MONO_CONTEXT_GET_IP (&info->client_info.ctx)); + /* altstack signal handler, sgen can't handle them, mono-threads should have handled this. */ + if (!info->client_info.stack_start + || info->client_info.stack_start < info->client_info.stack_start_limit + || info->client_info.stack_start >= info->client_info.stack_end) { + g_error ("BAD STACK: stack_start = %p, stack_start_limit = %p, stack_end = %p", + info->client_info.stack_start, info->client_info.stack_start_limit, info->client_info.stack_end); + } - THREADS_STW_DEBUG ("[GC-STW-SUSPEND-END] thread %p is suspended, stopped_ip = %p, stack = %p -> %p\n", - mono_thread_info_get_tid (info), stopped_ip, info->client_info.stack_start, info->client_info.stack_start ? info->client_info.stack_end : NULL); + stopped_ip = (gpointer) (MONO_CONTEXT_GET_IP (&info->client_info.ctx)); - binary_protocol_thread_suspend ((gpointer) mono_thread_info_get_tid (info), stopped_ip); - } else { - THREADS_STW_DEBUG ("[GC-STW-SUSPEND-END] thread %p is NOT suspended, reason %d\n", mono_thread_info_get_tid (info), reason); - g_assert (!info->client_info.suspend_done || info == mono_thread_info_current ()); - } + binary_protocol_thread_suspend ((gpointer) mono_thread_info_get_tid (info), stopped_ip); + + THREADS_STW_DEBUG ("[GC-STW-SUSPEND-END] thread %p is suspended, stopped_ip = %p, stack = %p -> %p\n", + mono_thread_info_get_tid (info), stopped_ip, info->client_info.stack_start, info->client_info.stack_start ? info->client_info.stack_end : NULL); } FOREACH_THREAD_END } diff --git a/mono/metadata/threads.c b/mono/metadata/threads.c index 36bbf09aa11..83ac3dd994b 100644 --- a/mono/metadata/threads.c +++ b/mono/metadata/threads.c @@ -2334,8 +2334,7 @@ mono_thread_suspend (MonoInternalThread *thread) } thread->state |= ThreadState_SuspendRequested; - if (mono_threads_is_coop_enabled ()) - mono_os_event_reset (thread->suspended); + mono_os_event_reset (thread->suspended); if (thread == mono_thread_internal_current ()) { /* calls UNLOCK_THREAD (thread) */ @@ -2363,9 +2362,8 @@ mono_thread_resume (MonoInternalThread *thread) { if ((thread->state & ThreadState_SuspendRequested) != 0) { // MOSTLY_ASYNC_SAFE_PRINTF ("RESUME (1) thread %p\n", thread_get_tid (thread)); - if (mono_threads_is_coop_enabled ()) - mono_os_event_set (thread->suspended); thread->state &= ~ThreadState_SuspendRequested; + mono_os_event_set (thread->suspended); return TRUE; } @@ -2380,9 +2378,9 @@ mono_thread_resume (MonoInternalThread *thread) // MOSTLY_ASYNC_SAFE_PRINTF ("RESUME (3) thread %p\n", thread_get_tid (thread)); - if (mono_threads_is_coop_enabled ()) { - mono_os_event_set (thread->suspended); - } else { + mono_os_event_set (thread->suspended); + + if (!thread->self_suspended) { UNLOCK_THREAD (thread); /* Awake the thread */ @@ -3277,8 +3275,7 @@ void mono_thread_suspend_all_other_threads (void) thread->state &= ~ThreadState_AbortRequested; thread->state |= ThreadState_SuspendRequested; - if (mono_threads_is_coop_enabled ()) - mono_os_event_reset (thread->suspended); + mono_os_event_reset (thread->suspended); /* Signal the thread to suspend + calls UNLOCK_THREAD (thread) */ async_suspend_internal (thread, TRUE); @@ -4802,6 +4799,8 @@ async_suspend_internal (MonoInternalThread *thread, gboolean interrupt) // MOSTLY_ASYNC_SAFE_PRINTF ("ASYNC SUSPEND thread %p\n", thread_get_tid (thread)); + thread->self_suspended = FALSE; + data.thread = thread; data.interrupt = interrupt; data.interrupt_token = NULL; @@ -4818,32 +4817,26 @@ static void self_suspend_internal (void) { MonoInternalThread *thread; + MonoOSEvent *event; + MonoOSEventWaitRet res; thread = mono_thread_internal_current (); // MOSTLY_ASYNC_SAFE_PRINTF ("SELF SUSPEND thread %p\n", thread_get_tid (thread)); - if (!mono_threads_is_coop_enabled ()) - mono_thread_info_begin_self_suspend (); + thread->self_suspended = TRUE; thread->state &= ~ThreadState_SuspendRequested; thread->state |= ThreadState_Suspended; UNLOCK_THREAD (thread); - if (!mono_threads_is_coop_enabled ()) - mono_thread_info_end_self_suspend (); - else { - MonoOSEvent *event; - MonoOSEventWaitRet res; - - event = thread->suspended; + event = thread->suspended; - MONO_ENTER_GC_SAFE; - res = mono_os_event_wait_one (event, MONO_INFINITE_WAIT); - g_assert (res == MONO_OS_EVENT_WAIT_RET_SUCCESS_0 || res == MONO_OS_EVENT_WAIT_RET_ALERTED); - MONO_EXIT_GC_SAFE; - } + MONO_ENTER_GC_SAFE; + res = mono_os_event_wait_one (event, MONO_INFINITE_WAIT); + g_assert (res == MONO_OS_EVENT_WAIT_RET_SUCCESS_0 || res == MONO_OS_EVENT_WAIT_RET_ALERTED); + MONO_EXIT_GC_SAFE; } /* diff --git a/mono/tests/Makefile.am b/mono/tests/Makefile.am index 92276ab63b2..b3114b1524d 100644 --- a/mono/tests/Makefile.am +++ b/mono/tests/Makefile.am @@ -473,7 +473,8 @@ BASE_TEST_CS_SRC_UNIVERSAL= \ abort-cctor.cs \ thread-native-exit.cs \ reference-loader.cs \ - thread-suspend-suspended.cs + thread-suspend-suspended.cs \ + thread-suspend-selfsuspended.cs if INSTALL_MOBILE_STATIC BASE_TEST_CS_SRC= \ diff --git a/mono/tests/thread-suspend-selfsuspended.cs b/mono/tests/thread-suspend-selfsuspended.cs new file mode 100644 index 00000000000..c9681dffed2 --- /dev/null +++ b/mono/tests/thread-suspend-selfsuspended.cs @@ -0,0 +1,42 @@ + +using System; +using System.Threading; + +class Driver +{ + public static void Main () + { + bool finished = false; + + Thread t1 = Thread.CurrentThread; + + Thread t2 = new Thread (() => { + while (!finished) { + GC.Collect (); + + try { + t1.Resume (); + } catch (ThreadStateException) { + } + + Thread.Yield (); + } + }); + + t2.Start (); + + Thread.Sleep (10); + + for (int i = 0; i < 50 * 40 * 20; ++i) { + Thread.CurrentThread.Suspend (); + if ((i + 1) % (50) == 0) + Console.Write ("."); + if ((i + 1) % (50 * 40) == 0) + Console.WriteLine (); + } + + finished = true; + + t2.Join (); + } +} diff --git a/mono/utils/mono-threads-state-machine.c b/mono/utils/mono-threads-state-machine.c index 76963632297..27acdda1f71 100644 --- a/mono/utils/mono-threads-state-machine.c +++ b/mono/utils/mono-threads-state-machine.c @@ -157,50 +157,6 @@ STATE_BLOCKING_AND_SUSPENDED: This is a bug in coop x suspend that resulted the } /* -This transition initiates the suspension of the current thread. -*/ -void -mono_threads_transition_request_self_suspension (MonoThreadInfo *info) -{ - int raw_state, cur_state, suspend_count; - g_assert (info == mono_thread_info_current ()); - -retry_state_change: - UNWRAP_THREAD_STATE (raw_state, cur_state, suspend_count, info); - - switch (cur_state) { - case STATE_RUNNING: //Post a self suspend request - if (!(suspend_count == 0)) - mono_fatal_with_history ("suspend_count = %d, but should be == 0", suspend_count); - if (InterlockedCompareExchange (&info->thread_state, build_thread_state (STATE_SELF_SUSPEND_REQUESTED, 1), raw_state) != raw_state) - goto retry_state_change; - trace_state_change ("SELF_SUSPEND_REQUEST", info, raw_state, STATE_SELF_SUSPEND_REQUESTED, 1); - break; - - case STATE_ASYNC_SUSPEND_REQUESTED: //Bump the suspend count but don't change the request type as async takes preference - if (!(suspend_count > 0 && suspend_count < THREAD_SUSPEND_COUNT_MAX)) - mono_fatal_with_history ("suspend_count = %d, but should be > 0 and < THREAD_SUSPEND_COUNT_MAX", suspend_count); - if (InterlockedCompareExchange (&info->thread_state, build_thread_state (cur_state, suspend_count + 1), raw_state) != raw_state) - goto retry_state_change; - trace_state_change ("SUSPEND_REQUEST", info, raw_state, cur_state, 1); - break; -/* -Other states: -STATE_ASYNC_SUSPENDED: Code should not be running while suspended. -STATE_SELF_SUSPENDED: Code should not be running while suspended. -STATE_SELF_SUSPEND_REQUESTED: Self suspends should not nest as begin/end should be paired. [1] -STATE_BLOCKING: -STATE_BLOCKING_AND_SUSPENDED: Self suspension cannot be started when the thread is in blocking state as it must finish first - -[1] This won't trap this sequence of requests: self suspend, async suspend and self suspend. -If this turns to be an issue we can introduce a new suspend request state for when both have been requested. -*/ - default: - mono_fatal_with_history ("Cannot transition thread %p from %s with SUSPEND_REQUEST", mono_thread_info_get_tid (info), state_name (cur_state)); - } -} - -/* This transition initiates the suspension of another thread. Returns one of the following values: diff --git a/mono/utils/mono-threads.c b/mono/utils/mono-threads.c index 5d8696a2ca6..0b3f71b85d3 100644 --- a/mono/utils/mono-threads.c +++ b/mono/utils/mono-threads.c @@ -721,53 +721,6 @@ mono_threads_get_runtime_callbacks (void) return &runtime_callbacks; } -/* -Signal that the current thread wants to be suspended. -This function can be called without holding the suspend lock held. -To finish suspending, call mono_suspend_check. -*/ -void -mono_thread_info_begin_self_suspend (void) -{ - g_assert (!mono_threads_is_coop_enabled ()); - - MonoThreadInfo *info = mono_thread_info_current_unchecked (); - if (!info) - return; - - THREADS_SUSPEND_DEBUG ("BEGIN SELF SUSPEND OF %p\n", info); - mono_threads_transition_request_self_suspension (info); -} - -void -mono_thread_info_end_self_suspend (void) -{ - MonoThreadInfo *info; - - g_assert (!mono_threads_is_coop_enabled ()); - - info = mono_thread_info_current (); - if (!info) - return; - THREADS_SUSPEND_DEBUG ("FINISH SELF SUSPEND OF %p\n", info); - - mono_threads_get_runtime_callbacks ()->thread_state_init (&info->thread_saved_state [SELF_SUSPEND_STATE_INDEX]); - - /* commit the saved state and notify others if needed */ - switch (mono_threads_transition_state_poll (info)) { - case SelfSuspendResumed: - return; - case SelfSuspendWait: - mono_thread_info_wait_for_resume (info); - break; - case SelfSuspendNotifyAndWait: - mono_threads_notify_initiator_of_suspend (info); - mono_thread_info_wait_for_resume (info); - mono_threads_notify_initiator_of_resume (info); - break; - } -} - static gboolean mono_thread_info_core_resume (MonoThreadInfo *info) { diff --git a/mono/utils/mono-threads.h b/mono/utils/mono-threads.h index a9776c32dbf..23f587877f6 100644 --- a/mono/utils/mono-threads.h +++ b/mono/utils/mono-threads.h @@ -330,15 +330,6 @@ mono_thread_info_resume (MonoNativeThreadId tid); void mono_thread_info_safe_suspend_and_run (MonoNativeThreadId id, gboolean interrupt_kernel, MonoSuspendThreadCallback callback, gpointer user_data); -//XXX new API, fix the world -void -mono_thread_info_begin_self_suspend (void); - -void -mono_thread_info_end_self_suspend (void); - -//END of new API - void mono_thread_info_setup_async_call (THREAD_INFO_TYPE *info, void (*target_func)(void*), void *user_data); @@ -565,7 +556,6 @@ typedef enum { void mono_threads_transition_attach (THREAD_INFO_TYPE* info); gboolean mono_threads_transition_detach (THREAD_INFO_TYPE *info); -void mono_threads_transition_request_self_suspension (THREAD_INFO_TYPE *info); MonoRequestAsyncSuspendResult mono_threads_transition_request_async_suspension (THREAD_INFO_TYPE *info); MonoSelfSupendResult mono_threads_transition_state_poll (THREAD_INFO_TYPE *info); MonoResumeResult mono_threads_transition_request_resume (THREAD_INFO_TYPE* info); -- 2.11.4.GIT