[mono/mono] cf8c8312: [runtime]Split thread unregistration in two steps, one with no locks, one with locks taken. Fixes #15916.

   Branch: refs/heads/master
     Home: https://github.com/mono/mono
  Compare: https://github.com/mono/mono/compare/1832b006bad3...cf8c83129deb

   Commit: cf8c83129debd5a5f34ec9c1a821a31c6de7866c
   Author: Rodrigo Kumpera <[email protected]> (kumpera)
     Date: 2013-11-05 14:59:27 GMT
      URL: https://github.com/mono/mono/commit/cf8c83129debd5a5f34ec9c1a821a31c6de7866c

[runtime]Split thread unregistration in two steps, one with no locks, one with locks taken. Fixes #15916.

In order allow for arbitrary code in the profiler callback we must invoke it without
holding any runtime leaf lock - this happens quite frequently with sdb as it tries to
suspend the world on those callbacks.

So thread shutdown is now split in two phases, first one that does the no-locks part
where the thread remains suspendable afterwards and then onewith locks taken where we
do the final cleanup and final shutdown.

Changed paths:
  M mono/metadata/sgen-gc.c
  M mono/utils/mono-threads.c
  M mono/utils/mono-threads.h

Modified: mono/metadata/sgen-gc.c
===================================================================
@@ -4139,7 +4139,7 @@ struct _EphemeronLinkNode {
 }
 
 static void
-sgen_thread_unregister (SgenThreadInfo *p)
+sgen_thread_detach (SgenThreadInfo *p)
 {
 	/* If a delegate is passed to native code and invoked on a thread we dont
 	 * know about, the jit will register it with mono_jit_thread_attach, but
@@ -4149,7 +4149,11 @@ struct _EphemeronLinkNode {
 	 */
 	if (mono_domain_get ())
 		mono_thread_detach (mono_thread_current ());
+}
 
+static void
+sgen_thread_unregister (SgenThreadInfo *p)
+{
 	binary_protocol_thread_unregister ((gpointer)mono_thread_info_get_tid (p));
 	SGEN_LOG (3, "unregister thread %p (%p)", p, (gpointer)mono_thread_info_get_tid (p));
 
@@ -4840,6 +4844,7 @@ void mono_gc_wbarrier_value_copy_bitmap (gpointer _dest, gpointer _src, int size
 	gc_debug_file = stderr;
 
 	cb.thread_register = sgen_thread_register;
+	cb.thread_detach = sgen_thread_detach;
 	cb.thread_unregister = sgen_thread_unregister;
 	cb.thread_attach = sgen_thread_attach;
 	cb.mono_method_is_critical = (gpointer)is_critical_method;

Modified: mono/utils/mono-threads.c
===================================================================
@@ -177,7 +177,23 @@
 	mono_native_tls_set_value (small_id_key, GUINT_TO_POINTER (info->small_id + 1));
 
 	info->thread_state = STATE_SHUTTING_DOWN;
+
+	/*
+	First perform the callback that requires no locks.
+	This callback has the potential of taking other locks, so we do it before.
+	After it completes, the thread remains functional.
+	*/
+	if (threads_callbacks.thread_detach)
+		threads_callbacks.thread_detach (info);
+
 	mono_thread_info_suspend_lock ();
+
+	/*
+	Now perform the callback that must be done under locks.
+	This will render the thread useless and non-suspendable, so it must
+	be done while holding the suspend lock to give no other thread chance
+	to suspend it.
+	*/
 	if (threads_callbacks.thread_unregister)
 		threads_callbacks.thread_unregister (info);
 	mono_threads_unregister_current_thread (info);

Modified: mono/utils/mono-threads.h
===================================================================
@@ -137,10 +137,19 @@ enum {
 typedef struct {
 	void* (*thread_register)(THREAD_INFO_TYPE *info, void *baseaddr);
 	/*
-	This callback is called after @info is removed from the thread list.
+	This callback is called with @info still on the thread list.
+	This call is made while holding the suspend lock, so don't do callbacks.
 	SMR remains functional as its small_id has not been reclaimed.
 	*/
 	void (*thread_unregister)(THREAD_INFO_TYPE *info);
+	/*
+	This callback is called right before thread_unregister. This is called
+	without any locks held so it's the place for complicated cleanup.
+
+	The thread must remain operational between this call and thread_unregister.
+	It must be possible to successfully suspend it after thread_unregister completes.
+	*/
+	void (*thread_detach)(THREAD_INFO_TYPE *info);
 	void (*thread_attach)(THREAD_INFO_TYPE *info);
 	gboolean (*mono_method_is_critical) (void *method);
 #ifndef HOST_WIN32


_______________________________________________
Mono-patches maillist  -  [email protected]
http://lists.ximian.com/mailman/listinfo/mono-patches