From: Ivan Maidanski Date: Tue, 22 Sep 2015 05:42:40 +0000 (+0300) Subject: Fix race (and potential deadlock) at marker threads initialization X-Git-Tag: gc7_4_4~70 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=378d7e1a58390da53cf1a643fe7ff67a925ac943;p=gc Fix race (and potential deadlock) at marker threads initialization * include/private/gc_priv.h (GC_wait_for_markers_init): New prototype. * mark.c (GC_wait_for_markers_init): New function (if PARALLEL_MARK). * pthread_support.c (GC_mark_thread): Notify start_mark_threads() about completion of marker data initialization. * win32_threads.c (GC_mark_thread): Likewise. * pthread_support.c (start_mark_threads): Add assertion about GC_fl_builder_count; call GC_wait_for_markers_init. * win32_threads.c (start_mark_threads): Likewise. * win32_threads.c (GC_mark_thread): Reformat code. --- diff --git a/include/private/gc_priv.h b/include/private/gc_priv.h index d267c7e5..5ef73e81 100644 --- a/include/private/gc_priv.h +++ b/include/private/gc_priv.h @@ -2372,6 +2372,7 @@ GC_INNER ptr_t GC_store_debug_info(ptr_t p, word sz, const char *str, /* GC_notify_all_builder() is called when GC_fl_builder_count */ /* reaches 0. */ + GC_INNER void GC_wait_for_markers_init(void); GC_INNER void GC_acquire_mark_lock(void); GC_INNER void GC_release_mark_lock(void); GC_INNER void GC_notify_all_builder(void); diff --git a/mark.c b/mark.c index 6e594804..869265fc 100644 --- a/mark.c +++ b/mark.c @@ -888,6 +888,24 @@ GC_INNER word GC_mark_no = 0; /* we don't overflow half of it in a single call to */ /* GC_mark_from. */ +/* Wait all markers to finish initialization (i.e. store */ +/* marker_[b]sp, marker_mach_threads, GC_marker_Id). */ +GC_INNER void GC_wait_for_markers_init(void) +{ + word count; + + if (GC_markers_m1 == 0) + return; + + /* Reuse marker lock and builders count to synchronize */ + /* marker threads startup. */ + GC_acquire_mark_lock(); + GC_fl_builder_count += (word)GC_markers_m1; + count = GC_fl_builder_count; + GC_release_mark_lock(); + if (count != 0) + GC_wait_for_reclaim(); +} /* Steal mark stack entries starting at mse low into mark stack local */ /* until we either steal mse high, or we have max entries. */ diff --git a/pthread_support.c b/pthread_support.c index 883a5690..8ec39bf3 100644 --- a/pthread_support.c +++ b/pthread_support.c @@ -370,6 +370,12 @@ STATIC void * GC_mark_thread(void * id) marker_mach_threads[(word)id] = mach_thread_self(); # endif + /* Inform start_mark_threads() about completion of marker data init. */ + GC_acquire_mark_lock(); + if (0 == --GC_fl_builder_count) + GC_notify_all_builder(); + GC_release_mark_lock(); + for (;; ++my_mark_no) { /* GC_mark_no is passed only to allow GC_help_marker to terminate */ /* promptly. This is important if it were called from the signal */ @@ -406,6 +412,7 @@ start_mark_threads(void) pthread_attr_t attr; GC_ASSERT(I_DONT_HOLD_LOCK()); + GC_ASSERT(GC_fl_builder_count == 0); # ifdef CAN_HANDLE_FORK if (available_markers_m1 <= 0 || GC_parallel) return; /* Skip if parallel markers disabled or already started. */ @@ -444,6 +451,7 @@ start_mark_threads(void) } GC_markers_m1 = i; (void)pthread_attr_destroy(&attr); + GC_wait_for_markers_init(); GC_COND_LOG_PRINTF("Started %d mark helper threads\n", GC_markers_m1); } diff --git a/win32_threads.c b/win32_threads.c index 2073b6a7..bb15259c 100644 --- a/win32_threads.c +++ b/win32_threads.c @@ -1681,12 +1681,10 @@ GC_INNER void GC_get_next_stack(char *start, char *limit, /* GC_mark_thread() is the same as in pthread_support.c */ # ifdef GC_PTHREADS_PARAMARK STATIC void * GC_mark_thread(void * id) +# elif defined(MSWINCE) + STATIC DWORD WINAPI GC_mark_thread(LPVOID id) # else -# ifdef MSWINCE - STATIC DWORD WINAPI GC_mark_thread(LPVOID id) -# else - STATIC unsigned __stdcall GC_mark_thread(void * id) -# endif + STATIC unsigned __stdcall GC_mark_thread(void * id) # endif { word my_mark_no = 0; @@ -1700,6 +1698,12 @@ GC_INNER void GC_get_next_stack(char *start, char *limit, GC_marker_Id[(word)id] = GetCurrentThreadId(); # endif + /* Inform start_mark_threads() about completion of marker data init. */ + GC_acquire_mark_lock(); + if (0 == --GC_fl_builder_count) + GC_notify_all_builder(); + GC_release_mark_lock(); + for (;; ++my_mark_no) { if (my_mark_no - GC_mark_no > (word)2) { /* resynchronize if we get far off, e.g. because GC_mark_no */ @@ -1749,6 +1753,7 @@ GC_INNER void GC_get_next_stack(char *start, char *limit, pthread_t new_thread; GC_ASSERT(I_DONT_HOLD_LOCK()); + GC_ASSERT(GC_fl_builder_count == 0); # ifdef CAN_HANDLE_FORK if (available_markers_m1 <= 0 || GC_parallel) return; /* Skip if parallel markers disabled or already started. */ @@ -1769,6 +1774,7 @@ GC_INNER void GC_get_next_stack(char *start, char *limit, } GC_markers_m1 = i; (void)pthread_attr_destroy(&attr); + GC_wait_for_markers_init(); GC_COND_LOG_PRINTF("Started %d mark helper threads\n", GC_markers_m1); } @@ -1890,6 +1896,7 @@ GC_INNER void GC_get_next_stack(char *start, char *limit, unsigned thread_id; # endif + GC_ASSERT(GC_fl_builder_count == 0); /* Initialize GC_marker_cv[] fully before starting the */ /* first helper thread. */ for (i = 0; i < GC_markers_m1; ++i) { @@ -1936,6 +1943,7 @@ GC_INNER void GC_get_next_stack(char *start, char *limit, GC_markers_m1--; CloseHandle(GC_marker_cv[GC_markers_m1]); } + GC_wait_for_markers_init(); GC_COND_LOG_PRINTF("Started %d mark helper threads\n", GC_markers_m1); if (i == 0) { CloseHandle(mark_cv);