From c00c3f14824d50955ba1096fae550657f5f057e5 Mon Sep 17 00:00:00 2001 From: Rodrigo Kumpera Date: Mon, 22 Apr 2013 17:17:52 -0400 Subject: [PATCH] Introduce a new kind of mutex, the suspension safe one. * mono-mutex.c: Add mono_mutex_init_suspend_safe so special targets like OSX that have broken implementations of suspend for pthread mutex will hang less. A suspension safe mutex means one that can handle this scenario: mutex M thread 1: 1)lock M 2)suspend thread 2 3)unlock M 4)lock M thread 2: 5)lock M Say (1) happens before (5) and (5) happens before (2). This means that thread 2 was suspended by the kernel because it's waiting on mutext M. Thread 1 then proceed to suspend thread 2 and unlock/lock the mutex. If the kernel implements mutexes with FIFO wait lists, this means that thread 1 will be blocked waiting for thread 2 acquire the lock. Since thread 2 is suspended, we have a deadlock. A suspend safe mutex is an unfair lock but will schedule any runable thread that is waiting for a the lock. This problem was witnessed on OSX in mono/tests/thread-exit.cs. --- mono/utils/mono-mutex.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++ mono/utils/mono-mutex.h | 2 ++ mono/utils/mono-threads.c | 30 ++++++++++++------------- mono/utils/mono-threads.h | 6 ++--- 4 files changed, 75 insertions(+), 19 deletions(-) diff --git a/mono/utils/mono-mutex.c b/mono/utils/mono-mutex.c index f30d61fccf4..6d5796e220a 100644 --- a/mono/utils/mono-mutex.c +++ b/mono/utils/mono-mutex.c @@ -20,6 +20,10 @@ #include "mono-mutex.h" +#if defined(__APPLE__) +#define _DARWIN_C_SOURCE +#include +#endif #ifndef HAVE_PTHREAD_MUTEX_TIMEDLOCK /* Android does not implement pthread_mutex_timedlock(), but does provide an @@ -83,3 +87,55 @@ mono_once (mono_once_t *once, void (*once_init) (void)) return 0; } + +/* +Returns a recursive mutex that is safe under suspension. + +A suspension safe mutex means one that can handle this scenario: + +mutex M + +thread 1: +1)lock M +2)suspend thread 2 +3)unlock M +4)lock M + +thread 2: +5)lock M + +Say (1) happens before (5) and (5) happens before (2). +This means that thread 2 was suspended by the kernel because +it's waiting on mutext M. + +Thread 1 then proceed to suspend thread 2 and unlock/lock the +mutex. + +If the kernel implements mutexes with FIFO wait lists, this means +that thread 1 will be blocked waiting for thread 2 acquire the lock. +Since thread 2 is suspended, we have a deadlock. + +A suspend safe mutex is an unfair lock but will schedule any runable +thread that is waiting for a the lock. + +This problem was witnessed on OSX in mono/tests/thread-exit.cs. + +*/ +int +mono_mutex_init_suspend_safe (mono_mutex_t *mutex) +{ +#if defined(__APPLE__) + int res; + pthread_mutexattr_t attr; + + pthread_mutexattr_init (&attr); + pthread_mutexattr_settype (&attr, PTHREAD_MUTEX_RECURSIVE); + pthread_mutexattr_setpolicy_np (&attr, _PTHREAD_MUTEX_POLICY_FIRSTFIT); + res = pthread_mutex_init (mutex, &attr); + pthread_mutexattr_destroy (&attr); + + return res; +#else + return mono_mutex_init (mutex); +#endif +} diff --git a/mono/utils/mono-mutex.h b/mono/utils/mono-mutex.h index c2a2af9b1ec..39bd0a2b372 100644 --- a/mono/utils/mono-mutex.h +++ b/mono/utils/mono-mutex.h @@ -94,6 +94,8 @@ typedef HANDLE mono_cond_t; #endif +int mono_mutex_init_suspend_safe (mono_mutex_t *mutex); + G_END_DECLS #endif /* __MONO_MUTEX_H__ */ diff --git a/mono/utils/mono-threads.c b/mono/utils/mono-threads.c index b2e00b67892..29da4ba1522 100644 --- a/mono/utils/mono-threads.c +++ b/mono/utils/mono-threads.c @@ -33,7 +33,7 @@ The GC has to acquire this lock before starting a STW to make sure a runtime suspend won't make it wronly see a thread in a safepoint when it is in fact not. */ -static CRITICAL_SECTION global_suspend_lock; +static mono_mutex_t global_suspend_lock; static int thread_info_size; @@ -103,7 +103,7 @@ free_thread_info (gpointer mem) { MonoThreadInfo *info = mem; - DeleteCriticalSection (&info->suspend_lock); + mono_mutex_destroy (&info->suspend_lock); MONO_SEM_DESTROY (&info->resume_semaphore); MONO_SEM_DESTROY (&info->finish_resume_semaphore); mono_threads_platform_free (info); @@ -127,7 +127,7 @@ register_thread (MonoThreadInfo *info, gpointer baseptr) mono_thread_info_set_tid (info, mono_native_thread_id_get ()); info->small_id = small_id; - InitializeCriticalSection (&info->suspend_lock); + mono_mutex_init_suspend_safe (&info->suspend_lock); MONO_SEM_INIT (&info->resume_semaphore, 0); MONO_SEM_INIT (&info->finish_resume_semaphore, 0); @@ -274,7 +274,7 @@ mono_threads_init (MonoThreadInfoCallbacks *callbacks, size_t info_size) res = mono_native_tls_alloc (&small_id_key, NULL); g_assert (res); - InitializeCriticalSection (&global_suspend_lock); + mono_mutex_init_suspend_safe (&global_suspend_lock); mono_lls_init (&thread_list, NULL); mono_thread_smr_init (); @@ -314,7 +314,7 @@ mono_thread_info_suspend_sync (MonoNativeThreadId tid, gboolean interrupt_kernel if (!info) return NULL; - EnterCriticalSection (&info->suspend_lock); + mono_mutex_lock (&info->suspend_lock); /*thread is on the process of detaching*/ if (mono_thread_info_run_state (info) > STATE_RUNNING) { @@ -327,12 +327,12 @@ mono_thread_info_suspend_sync (MonoNativeThreadId tid, gboolean interrupt_kernel if (info->suspend_count) { ++info->suspend_count; mono_hazard_pointer_clear (hp, 1); - LeaveCriticalSection (&info->suspend_lock); + mono_mutex_unlock (&info->suspend_lock); return info; } if (!mono_threads_core_suspend (info)) { - LeaveCriticalSection (&info->suspend_lock); + mono_mutex_unlock (&info->suspend_lock); mono_hazard_pointer_clear (hp, 1); return NULL; } @@ -342,7 +342,7 @@ mono_thread_info_suspend_sync (MonoNativeThreadId tid, gboolean interrupt_kernel ++info->suspend_count; info->thread_state |= STATE_SUSPENDED; - LeaveCriticalSection (&info->suspend_lock); + mono_mutex_unlock (&info->suspend_lock); mono_hazard_pointer_clear (hp, 1); return info; @@ -356,7 +356,7 @@ mono_thread_info_self_suspend (void) if (!info) return; - EnterCriticalSection (&info->suspend_lock); + mono_mutex_lock (&info->suspend_lock); THREADS_DEBUG ("self suspend IN COUNT %d\n", info->suspend_count); @@ -368,7 +368,7 @@ mono_thread_info_self_suspend (void) ret = mono_threads_get_runtime_callbacks ()->thread_state_init_from_sigctx (&info->suspend_state, NULL); g_assert (ret); - LeaveCriticalSection (&info->suspend_lock); + mono_mutex_unlock (&info->suspend_lock); while (MONO_SEM_WAIT (&info->resume_semaphore) != 0) { /*if (EINTR != errno) ABORT("sem_wait failed"); */ @@ -404,12 +404,12 @@ mono_thread_info_resume (MonoNativeThreadId tid) if (!info) return FALSE; - EnterCriticalSection (&info->suspend_lock); + mono_mutex_lock (&info->suspend_lock); THREADS_DEBUG ("resume %x IN COUNT %d\n",tid, info->suspend_count); if (info->suspend_count <= 0) { - LeaveCriticalSection (&info->suspend_lock); + mono_mutex_unlock (&info->suspend_lock); mono_hazard_pointer_clear (hp, 1); return FALSE; } @@ -423,7 +423,7 @@ mono_thread_info_resume (MonoNativeThreadId tid) if (--info->suspend_count == 0) result = mono_thread_info_resume_internal (info); - LeaveCriticalSection (&info->suspend_lock); + mono_mutex_unlock (&info->suspend_lock); mono_hazard_pointer_clear (hp, 1); return result; @@ -526,13 +526,13 @@ STW to make sure no unsafe pending suspend is in progress. void mono_thread_info_suspend_lock (void) { - EnterCriticalSection (&global_suspend_lock); + mono_mutex_lock (&global_suspend_lock); } void mono_thread_info_suspend_unlock (void) { - LeaveCriticalSection (&global_suspend_lock); + mono_mutex_unlock (&global_suspend_lock); } void diff --git a/mono/utils/mono-threads.h b/mono/utils/mono-threads.h index 43dd376fe8b..92d6d16d7ef 100644 --- a/mono/utils/mono-threads.h +++ b/mono/utils/mono-threads.h @@ -13,9 +13,7 @@ #include #include #include - -/* FIXME used for CRITICAL_SECTION replace with mono-mutex */ -#include +#include #include @@ -99,7 +97,7 @@ typedef struct { int thread_state; /* suspend machinery, fields protected by the suspend_lock */ - CRITICAL_SECTION suspend_lock; + mono_mutex_t suspend_lock; int suspend_count; MonoSemType finish_resume_semaphore; -- 2.11.4.GIT