From c2c650fd897f7c7a9fdbe140dc5131d0fd26f332 Mon Sep 17 00:00:00 2001 From: Ivan Maidanski Date: Thu, 29 May 2014 23:32:45 +0400 Subject: [PATCH] Fix PARALLEL_MARK for Windows 7+ (Apply commit 57cc049 from 'master' branch.) * doc/README.macros (DONT_USE_SIGNALANDWAIT): Remove. * win32_threads.c (DONT_USE_SIGNALANDWAIT, GC_mark_mutex_waitcnt, SignalObjectAndWait_type, signalObjectAndWait_func): Likewise. * win32_threads.c (GC_marker_cv, GC_mark_thread, start_mark_threads, GC_mark_mutex_state, GC_acquire_mark_lock, GC_release_mark_lock, GC_wait_marker, GC_notify_all_marker, GC_thr_init): Do not check DONT_USE_SIGNALANDWAIT macro (assume it is always on as multi-core marker based on NT SignalObjectAndWait is broken in Windows 7+ leading to a deadlock sometimes because the function is no longer atomic). Conflicts: doc/README.macros --- doc/README.macros | 7 -- win32_threads.c | 231 +++++++++++----------------------------------- 2 files changed, 55 insertions(+), 183 deletions(-) diff --git a/doc/README.macros b/doc/README.macros index a822f0f4..d2fef9a4 100644 --- a/doc/README.macros +++ b/doc/README.macros @@ -393,13 +393,6 @@ USE_COMPILER_TLS Causes thread local allocation to use PARALLEL_MARK Allows the marker to run in multiple threads. Recommended for multiprocessors. -DONT_USE_SIGNALANDWAIT (Win32 only) Use an alternate implementation for - marker threads (if PARALLEL_MARK defined) synchronization routines based - on InterlockedExchange() (instead of AO_fetch_and_add()) and on multiple - event objects (one per each marker instead of that based on Win32 - SignalObjectAndWait() using a single event object). This is the default - for WinCE. - GC_WINMAIN_REDIRECT (Win32 only) Redirect (rename) an application WinMain to GC_WinMain; implement the "real" WinMain which starts a new thread to call GC_WinMain after initializing the GC. Useful for WinCE. diff --git a/win32_threads.c b/win32_threads.c index 4d57bc5e..200395c2 100644 --- a/win32_threads.c +++ b/win32_threads.c @@ -68,20 +68,11 @@ # include # endif -#else - -# ifdef MSWINCE - /* Force DONT_USE_SIGNALANDWAIT implementation of PARALLEL_MARK */ - /* for WinCE (since Win32 SignalObjectAndWait() is missing). */ -# ifndef DONT_USE_SIGNALANDWAIT -# define DONT_USE_SIGNALANDWAIT -# endif -# else -# include /* For _beginthreadex, _endthreadex */ -# include /* for errno, EAGAIN */ -# endif +#elif !defined(MSWINCE) +# include /* For _beginthreadex, _endthreadex */ +# include /* for errno, EAGAIN */ -#endif +#endif /* !GC_PTHREADS && !MSWINCE */ /* DllMain-based thread registration is currently incompatible */ /* with thread-local allocation, pthreads and WinCE. */ @@ -1675,7 +1666,7 @@ GC_INNER void GC_get_next_stack(char *start, char *limit, # define GC_PTHREADS_PARAMARK # endif -# if !defined(GC_PTHREADS_PARAMARK) && defined(DONT_USE_SIGNALANDWAIT) +# if !defined(GC_PTHREADS_PARAMARK) STATIC HANDLE GC_marker_cv[MAX_MARKERS - 1] = {0}; /* Events with manual reset (one for each */ /* mark helper). */ @@ -1705,7 +1696,7 @@ GC_INNER void GC_get_next_stack(char *start, char *limit, # ifdef IA64 marker_bsp[(word)id] = GC_save_regs_in_stack(); # endif -# if !defined(GC_PTHREADS_PARAMARK) && defined(DONT_USE_SIGNALANDWAIT) +# if !defined(GC_PTHREADS_PARAMARK) GC_marker_Id[(word)id] = GetCurrentThreadId(); # endif @@ -1898,17 +1889,15 @@ GC_INNER void GC_get_next_stack(char *start, char *limit, unsigned thread_id; # endif -# ifdef DONT_USE_SIGNALANDWAIT - /* Initialize GC_marker_cv[] fully before starting the */ - /* first helper thread. */ - for (i = 0; i < GC_markers_m1; ++i) { - if ((GC_marker_cv[i] = CreateEvent(NULL /* attrs */, + /* Initialize GC_marker_cv[] fully before starting the */ + /* first helper thread. */ + for (i = 0; i < GC_markers_m1; ++i) { + if ((GC_marker_cv[i] = CreateEvent(NULL /* attrs */, TRUE /* isManualReset */, FALSE /* initialState */, NULL /* name (A/W) */)) == (HANDLE)0) - ABORT("CreateEvent failed"); - } -# endif + ABORT("CreateEvent failed"); + } for (i = 0; i < GC_markers_m1; ++i) { marker_last_stack_min[i] = ADDR_LIMIT; @@ -1942,14 +1931,10 @@ GC_INNER void GC_get_next_stack(char *start, char *limit, } /* Adjust GC_markers_m1 (and free unused resources) if failed. */ -# ifdef DONT_USE_SIGNALANDWAIT - while (GC_markers_m1 > i) { - GC_markers_m1--; - CloseHandle(GC_marker_cv[GC_markers_m1]); - } -# else - GC_markers_m1 = i; -# endif + while (GC_markers_m1 > i) { + GC_markers_m1--; + CloseHandle(GC_marker_cv[GC_markers_m1]); + } GC_COND_LOG_PRINTF("Started %d mark helper threads\n", GC_markers_m1); if (i == 0) { CloseHandle(mark_cv); @@ -1969,16 +1954,11 @@ GC_INNER void GC_get_next_stack(char *start, char *limit, } while (0) # endif /* GC_ASSERTIONS */ -# ifdef DONT_USE_SIGNALANDWAIT - STATIC /* volatile */ LONG GC_mark_mutex_state = 0; + STATIC /* volatile */ LONG GC_mark_mutex_state = 0; /* Mutex state: 0 - unlocked, */ /* 1 - locked and no other waiters, */ /* -1 - locked and waiters may exist. */ /* Accessed by InterlockedExchange(). */ -# else - STATIC volatile AO_t GC_mark_mutex_waitcnt = 0; - /* Number of waiters + 1; 0 - unlocked. */ -# endif /* #define LOCK_STATS */ # ifdef LOCK_STATS @@ -1989,21 +1969,13 @@ GC_INNER void GC_get_next_stack(char *start, char *limit, GC_INNER void GC_acquire_mark_lock(void) { GC_ASSERT(GC_mark_lock_holder != GetCurrentThreadId()); -# ifdef DONT_USE_SIGNALANDWAIT - if (InterlockedExchange(&GC_mark_mutex_state, 1 /* locked */) != 0) -# else - if (AO_fetch_and_add1_acquire(&GC_mark_mutex_waitcnt) != 0) -# endif - { + if (InterlockedExchange(&GC_mark_mutex_state, 1 /* locked */) != 0) { # ifdef LOCK_STATS (void)AO_fetch_and_add1(&GC_block_count); # endif -# ifdef DONT_USE_SIGNALANDWAIT - /* Repeatedly reset the state and wait until acquire the lock. */ - while (InterlockedExchange(&GC_mark_mutex_state, - -1 /* locked_and_has_waiters */) != 0) -# endif - { + /* Repeatedly reset the state and wait until acquire the lock. */ + while (InterlockedExchange(&GC_mark_mutex_state, + -1 /* locked_and_has_waiters */) != 0) { if (WaitForSingleObject(mark_mutex_event, INFINITE) == WAIT_FAILED) ABORT("WaitForSingleObject failed"); } @@ -2021,17 +1993,11 @@ GC_INNER void GC_get_next_stack(char *start, char *limit, GC_INNER void GC_release_mark_lock(void) { UNSET_MARK_LOCK_HOLDER; -# ifdef DONT_USE_SIGNALANDWAIT - if (InterlockedExchange(&GC_mark_mutex_state, 0 /* unlocked */) < 0) -# else - GC_ASSERT(AO_load(&GC_mark_mutex_waitcnt) != 0); - if (AO_fetch_and_sub1_release(&GC_mark_mutex_waitcnt) > 1) -# endif - { - /* wake a waiter */ - if (SetEvent(mark_mutex_event) == FALSE) - ABORT("SetEvent failed"); - } + if (InterlockedExchange(&GC_mark_mutex_state, 0 /* unlocked */) < 0) { + /* wake a waiter */ + if (SetEvent(mark_mutex_event) == FALSE) + ABORT("SetEvent failed"); + } } /* In GC_wait_for_reclaim/GC_notify_all_builder() we emulate POSIX */ @@ -2066,115 +2032,41 @@ GC_INNER void GC_get_next_stack(char *start, char *limit, ABORT("SetEvent failed"); } -# ifdef DONT_USE_SIGNALANDWAIT - - /* mark_cv is used (for waiting) by a non-helper thread. */ - - GC_INNER void GC_wait_marker(void) - { - HANDLE event = mark_cv; - DWORD thread_id = GetCurrentThreadId(); - int i = GC_markers_m1; - - while (i-- > 0) { - if (GC_marker_Id[i] == thread_id) { - event = GC_marker_cv[i]; - break; - } - } - - if (ResetEvent(event) == FALSE) - ABORT("ResetEvent failed"); - GC_release_mark_lock(); - if (WaitForSingleObject(event, INFINITE) == WAIT_FAILED) - ABORT("WaitForSingleObject failed"); - GC_acquire_mark_lock(); - } + /* mark_cv is used (for waiting) by a non-helper thread. */ - GC_INNER void GC_notify_all_marker(void) - { - DWORD thread_id = GetCurrentThreadId(); - int i = GC_markers_m1; + GC_INNER void GC_wait_marker(void) + { + HANDLE event = mark_cv; + DWORD thread_id = GetCurrentThreadId(); + int i = GC_markers_m1; - while (i-- > 0) { - /* Notify every marker ignoring self (for efficiency). */ - if (SetEvent(GC_marker_Id[i] != thread_id ? GC_marker_cv[i] : - mark_cv) == FALSE) - ABORT("SetEvent failed"); + while (i-- > 0) { + if (GC_marker_Id[i] == thread_id) { + event = GC_marker_cv[i]; + break; } } -# else /* DONT_USE_SIGNALANDWAIT */ - - /* For GC_wait_marker/GC_notify_all_marker() the above technique */ - /* does not work because they are used with different checked */ - /* conditions in different places (and, in addition, notifying is */ - /* done after leaving critical section) and this could result in */ - /* a signal losing between checking for a particular condition */ - /* and calling WaitForSingleObject. So, we use PulseEvent() and */ - /* NT SignalObjectAndWait() (which atomically sets mutex event to */ - /* signaled state and starts waiting on condvar). A special */ - /* case here is GC_mark_mutex_waitcnt == 1 (i.e. nobody waits for */ - /* mark lock at this moment) - we don't change it (otherwise we */ - /* may lose a signal sent between decrementing mark_mutex_waitcnt */ - /* and calling WaitForSingleObject). */ - -# ifdef MSWINCE - /* SignalObjectAndWait() is missing in WinCE (for now), so you */ - /* should supply its emulation (externally) to use this code. */ - WINBASEAPI DWORD WINAPI SignalObjectAndWait(HANDLE, HANDLE, DWORD, - BOOL); -# define signalObjectAndWait_func SignalObjectAndWait -# else - typedef DWORD (WINAPI * SignalObjectAndWait_type)(HANDLE, HANDLE, - DWORD, BOOL); - static SignalObjectAndWait_type signalObjectAndWait_func = 0; -# endif - - GC_INNER void GC_wait_marker(void) - { - /* Here we assume that GC_wait_marker() is always called */ - /* from a while(check_cond) loop. */ - AO_t waitcnt; - GC_ASSERT(mark_cv != 0); - - /* We inline GC_release_mark_lock() to have atomic */ - /* unlock-and-wait action here. */ - UNSET_MARK_LOCK_HOLDER; - if ((waitcnt = AO_load(&GC_mark_mutex_waitcnt)) > 1) { - (void)AO_fetch_and_sub1_release(&GC_mark_mutex_waitcnt); - } else { - GC_ASSERT(AO_load(&GC_mark_mutex_waitcnt) != 0); - } - - /* The state of mark_cv is non-signaled here. */ - if (signalObjectAndWait_func(mark_mutex_event /* hObjectToSignal */, - mark_cv /* hObjectToWaitOn */, - INFINITE /* timeout */, - FALSE /* isAlertable */) == WAIT_FAILED) - ABORT("SignalObjectAndWait failed"); - /* The state of mark_cv is non-signaled here again. */ - - if (waitcnt > 1) { - GC_acquire_mark_lock(); - } else { - GC_ASSERT(GC_mark_mutex_waitcnt != 0); - /* Acquire mark lock */ - if (WaitForSingleObject(mark_mutex_event, INFINITE) == WAIT_FAILED) - ABORT("WaitForSingleObject failed"); - GC_ASSERT(GC_mark_lock_holder == NO_THREAD); - SET_MARK_LOCK_HOLDER; - } - } + if (ResetEvent(event) == FALSE) + ABORT("ResetEvent failed"); + GC_release_mark_lock(); + if (WaitForSingleObject(event, INFINITE) == WAIT_FAILED) + ABORT("WaitForSingleObject failed"); + GC_acquire_mark_lock(); + } - GC_INNER void GC_notify_all_marker(void) - { - GC_ASSERT(mark_cv != 0); - if (PulseEvent(mark_cv) == FALSE) - ABORT("PulseEvent failed"); + GC_INNER void GC_notify_all_marker(void) + { + DWORD thread_id = GetCurrentThreadId(); + int i = GC_markers_m1; + + while (i-- > 0) { + /* Notify every marker ignoring self (for efficiency). */ + if (SetEvent(GC_marker_Id[i] != thread_id ? GC_marker_cv[i] : + mark_cv) == FALSE) + ABORT("SetEvent failed"); } - -# endif /* !DONT_USE_SIGNALANDWAIT */ + } # endif /* ! GC_PTHREADS_PARAMARK */ @@ -2496,20 +2388,7 @@ GC_INNER void GC_thr_init(void) /* Check whether parallel mode could be enabled. */ { -# if !defined(GC_PTHREADS_PARAMARK) && !defined(MSWINCE) \ - && !defined(DONT_USE_SIGNALANDWAIT) - HMODULE hK32; - /* SignalObjectAndWait() API call works only under NT. */ -# endif - if (GC_win32_dll_threads || available_markers_m1 <= 0 -# if !defined(GC_PTHREADS_PARAMARK) && !defined(MSWINCE) \ - && !defined(DONT_USE_SIGNALANDWAIT) - || GC_wnt == FALSE - || (hK32 = GetModuleHandle(TEXT("kernel32.dll"))) == (HMODULE)0 - || (signalObjectAndWait_func = (SignalObjectAndWait_type) - GetProcAddress(hK32, "SignalObjectAndWait")) == 0 -# endif - ) { + if (GC_win32_dll_threads || available_markers_m1 <= 0) { /* Disable parallel marking. */ GC_parallel = FALSE; GC_COND_LOG_PRINTF( -- 2.40.0