[profiler] Acquire the exclusive buffer lock after the suspend lock.

This prevents possible STW deadlocks.
monojenkins · Aug 30, 2016 · 4f62b5a · 4f62b5a
1 parent 25edbd4
commit 4f62b5a
Show file tree

Hide file tree

Showing 7 changed files with 43 additions and 6 deletions.
diff --git a/libgc/include/gc.h b/libgc/include/gc.h
@@ -93,6 +93,7 @@ GC_API GC_PTR (*GC_oom_fn) GC_PROTO((size_t bytes_requested));
 			/* pointer to a previously allocated heap 	*/
 			/* object.					*/
 
+// Keep somewhat in sync with mono/metadata/profiler.h:enum MonoGCEvent
 typedef enum {
 	GC_EVENT_START,
 	GC_EVENT_MARK_START,

diff --git a/mono/metadata/boehm-gc.c b/mono/metadata/boehm-gc.c
@@ -428,7 +428,6 @@ on_gc_notification (GC_EventType event)
 	switch (e) {
 	case MONO_GC_EVENT_PRE_STOP_WORLD:
 		MONO_GC_WORLD_STOP_BEGIN ();
-		mono_thread_info_suspend_lock ();
 		break;
 
 	case MONO_GC_EVENT_POST_STOP_WORLD:
@@ -441,7 +440,6 @@ on_gc_notification (GC_EventType event)
 
 	case MONO_GC_EVENT_POST_START_WORLD:
 		MONO_GC_WORLD_RESTART_END (1);
-		mono_thread_info_suspend_unlock ();
 		break;
 
 	case MONO_GC_EVENT_START:
@@ -481,7 +479,21 @@ on_gc_notification (GC_EventType event)
 	}
 
 	mono_profiler_gc_event (e, 0);
+
+	switch (e) {
+	case MONO_GC_EVENT_PRE_STOP_WORLD:
+		mono_thread_info_suspend_lock ();
+		mono_profiler_gc_event (MONO_GC_EVENT_PRE_STOP_WORLD_LOCKED, 0);
+		break;
+	case MONO_GC_EVENT_POST_START_WORLD:
+		mono_thread_info_suspend_unlock ();
+		mono_profiler_gc_event (MONO_GC_EVENT_POST_START_WORLD_UNLOCKED, 0);
+		break;
+	default:
+		break;
+	}
 }
+
 
 static void
 on_gc_heap_resize (size_t new_size)

diff --git a/mono/metadata/profiler.h b/mono/metadata/profiler.h
@@ -39,17 +39,34 @@ typedef enum {
 	MONO_PROFILE_FAILED
 } MonoProfileResult;
 
+// Keep somewhat in sync with libgc/include/gc.h:enum GC_EventType
 typedef enum {
 	MONO_GC_EVENT_START,
 	MONO_GC_EVENT_MARK_START,
 	MONO_GC_EVENT_MARK_END,
 	MONO_GC_EVENT_RECLAIM_START,
 	MONO_GC_EVENT_RECLAIM_END,
 	MONO_GC_EVENT_END,
+	/*
+	 * This is the actual arrival order of the following events:
+	 *
+	 * MONO_GC_EVENT_PRE_STOP_WORLD
+	 * MONO_GC_EVENT_PRE_STOP_WORLD_LOCKED
+	 * MONO_GC_EVENT_POST_STOP_WORLD
+	 * MONO_GC_EVENT_PRE_START_WORLD
+	 * MONO_GC_EVENT_POST_START_WORLD_UNLOCKED
+	 * MONO_GC_EVENT_POST_START_WORLD
+	 *
+	 * The LOCKED and UNLOCKED events guarantee that, by the time they arrive,
+	 * the GC and suspend locks will both have been acquired and released,
+	 * respectively.
+	 */
 	MONO_GC_EVENT_PRE_STOP_WORLD,
 	MONO_GC_EVENT_POST_STOP_WORLD,
 	MONO_GC_EVENT_PRE_START_WORLD,
-	MONO_GC_EVENT_POST_START_WORLD
+	MONO_GC_EVENT_POST_START_WORLD,
+	MONO_GC_EVENT_PRE_STOP_WORLD_LOCKED,
+	MONO_GC_EVENT_POST_START_WORLD_UNLOCKED
 } MonoGCEvent;
 
 /* coverage info */

diff --git a/mono/metadata/sgen-stw.c b/mono/metadata/sgen-stw.c
@@ -209,6 +209,8 @@ sgen_client_stop_world (int generation)
 
 	acquire_gc_locks ();
 
+	mono_profiler_gc_event (MONO_GC_EVENT_PRE_STOP_WORLD_LOCKED, generation);
+
 	/* We start to scan after locks are taking, this ensures we won't be interrupted. */
 	sgen_process_togglerefs ();
 
@@ -283,6 +285,8 @@ sgen_client_restart_world (int generation, gint64 *stw_time)
 	 */
 	release_gc_locks ();
 
+	mono_profiler_gc_event (MONO_GC_EVENT_POST_START_WORLD_UNLOCKED, generation);
+
 	*stw_time = usec;
 }
 

diff --git a/mono/profiler/decode.c b/mono/profiler/decode.c
@@ -1862,9 +1862,11 @@ gc_event_name (int ev)
 	case MONO_GC_EVENT_RECLAIM_END: return "reclaim end";
 	case MONO_GC_EVENT_END: return "end";
 	case MONO_GC_EVENT_PRE_STOP_WORLD: return "pre stop";
+	case MONO_GC_EVENT_PRE_STOP_WORLD_LOCKED: return "pre stop lock";
 	case MONO_GC_EVENT_POST_STOP_WORLD: return "post stop";
 	case MONO_GC_EVENT_PRE_START_WORLD: return "pre start";
 	case MONO_GC_EVENT_POST_START_WORLD: return "post start";
+	case MONO_GC_EVENT_POST_START_WORLD_UNLOCKED: return "post start unlock";
 	default:
 		return "unknown";
 	}

diff --git a/mono/profiler/proflog.c b/mono/profiler/proflog.c
@@ -1339,7 +1339,7 @@ gc_event (MonoProfiler *profiler, MonoGCEvent ev, int generation)
 		if (generation == mono_gc_max_generation ())
 			gc_count++;
 		break;
-	case MONO_GC_EVENT_PRE_STOP_WORLD:
+	case MONO_GC_EVENT_PRE_STOP_WORLD_LOCKED:
 		/*
 		 * Ensure that no thread can be in the middle of writing to
 		 * a buffer when the world stops...
@@ -1364,7 +1364,7 @@ gc_event (MonoProfiler *profiler, MonoGCEvent ev, int generation)
 	case MONO_GC_EVENT_PRE_START_WORLD:
 		heap_walk (profiler);
 		break;
-	case MONO_GC_EVENT_POST_START_WORLD:
+	case MONO_GC_EVENT_POST_START_WORLD_UNLOCKED:
 		/*
 		 * Similarly, we must now make sure that any object moves
 		 * written to the GC thread's buffer are flushed. Otherwise,

diff --git a/mono/profiler/proflog.h b/mono/profiler/proflog.h
@@ -5,7 +5,7 @@
 #define LOG_HEADER_ID 0x4D505A01
 #define LOG_VERSION_MAJOR 0
 #define LOG_VERSION_MINOR 4
-#define LOG_DATA_VERSION 12
+#define LOG_DATA_VERSION 13
 /*
  * Changes in data versions:
  * version 2: added offsets in heap walk
@@ -27,6 +27,7 @@
                added TYPE_GC_HANDLE_{CREATED,DESTROYED}_BT
                TYPE_JIT events are no longer guaranteed to have code start/size info (can be zero)
  * version 12: added MONO_COUNTER_PROFILER
+ * version 13: added MONO_GC_EVENT_{PRE_STOP_WORLD_LOCKED,POST_START_WORLD_UNLOCKED}
  */
 
 enum {