diff --git a/darwin_stop_world.c b/darwin_stop_world.c index 547bf3f40..ab0be9d8f 100644 --- a/darwin_stop_world.c +++ b/darwin_stop_world.c @@ -517,9 +517,11 @@ STATIC GC_bool GC_suspend_thread_list(thread_act_array_t act_list, int count, # endif /* Unconditionally suspend the thread. It will do no */ /* harm if it is already suspended by the client logic. */ + GC_acquire_dirty_lock(); do { kern_result = thread_suspend(thread); } while (kern_result == KERN_ABORTED); + GC_release_dirty_lock(); if (kern_result != KERN_SUCCESS) { /* The thread may have quit since the thread_threads() call we */ /* mark already suspended so it's not dealt with anymore later. */ @@ -618,10 +620,11 @@ GC_INNER void GC_stop_world(void) for (p = GC_threads[i]; p != NULL; p = p->next) { if ((p->flags & FINISHED) == 0 && !p->thread_blocked && p->stop_info.mach_thread != my_thread) { - + GC_acquire_dirty_lock(); do { kern_result = thread_suspend(p->stop_info.mach_thread); } while (kern_result == KERN_ABORTED); + GC_release_dirty_lock(); if (kern_result != KERN_SUCCESS) ABORT("thread_suspend failed"); if (GC_on_thread_event) diff --git a/include/private/gc_priv.h b/include/private/gc_priv.h index 1b6a992a0..53ec41c2e 100644 --- a/include/private/gc_priv.h +++ b/include/private/gc_priv.h @@ -961,10 +961,6 @@ typedef word page_hash_table[PHT_SIZE]; (((bl)[divWORDSZ(index)] >> modWORDSZ(index)) & 1) # define set_pht_entry_from_index(bl, index) \ (bl)[divWORDSZ(index)] |= (word)1 << modWORDSZ(index) -/* And a dumb but thread-safe version of set_pht_entry_from_index. */ -/* This sets (many) extra bits. */ -# define set_pht_entry_from_index_safe(bl, index) \ - (bl)[divWORDSZ(index)] = ONES /* And, one more version for GC_add_to_black_list_normal/stack. */ /* The latter ones are invoked (indirectly) by GC_do_local_mark. */ @@ -2324,7 +2320,17 @@ GC_EXTERN signed_word GC_bytes_found; /* protected by GC_write_cs. */ # endif -# ifdef MPROTECT_VDB +# if defined(GC_DISABLE_INCREMENTAL) +# define GC_acquire_dirty_lock() (void)0 +# define GC_release_dirty_lock() (void)0 +# else + /* Acquire the spin lock we use to update dirty bits. */ + /* Threads should not get stopped holding it. But we may */ + /* acquire and release it during GC_remove_protection call. */ +# define GC_acquire_dirty_lock() \ + do { /* empty */ \ + } while (AO_test_and_set_acquire(&GC_fault_handler_lock) == AO_TS_SET) +# define GC_release_dirty_lock() AO_CLEAR(&GC_fault_handler_lock) GC_EXTERN volatile AO_TS_t GC_fault_handler_lock; /* defined in os_dep.c */ # endif diff --git a/os_dep.c b/os_dep.c index 03cd7ecbd..c2ae775f5 100644 --- a/os_dep.c +++ b/os_dep.c @@ -2958,6 +2958,29 @@ GC_API GC_push_other_roots_proc GC_CALL GC_get_push_other_roots(void) } #endif /* DEFAULT_VDB */ +#ifndef GC_DISABLE_INCREMENTAL +# ifndef THREADS +# define async_set_pht_entry_from_index(db, index) \ + set_pht_entry_from_index(db, index) +# elif defined(AO_HAVE_test_and_set_acquire) + /* We need to lock around the bitmap update (in the write fault */ + /* handler or GC_dirty) in order to avoid the risk of losing a bit. */ + /* We do this with a test-and-set spin lock if possible. */ + GC_INNER volatile AO_TS_t GC_fault_handler_lock = AO_TS_INITIALIZER; + + GC_ATTR_NO_SANITIZE_THREAD + static void async_set_pht_entry_from_index(volatile page_hash_table db, + size_t index) + { + GC_acquire_dirty_lock(); + set_pht_entry_from_index(db, index); + GC_release_dirty_lock(); + } +# else +# error No test_and_set operation: Introduces a race. +# endif /* THREADS && !AO_HAVE_test_and_set_acquire */ +#endif /* !GC_DISABLE_INCREMENTAL */ + #ifdef MPROTECT_VDB /* * This implementation maintains dirty bits itself by catching write @@ -3066,60 +3089,7 @@ GC_API GC_push_other_roots_proc GC_CALL GC_get_push_other_roots(void) # endif /* !MSWIN32 */ #endif /* !DARWIN */ -#if defined(THREADS) -/* We need to lock around the bitmap update in the write fault handler */ -/* in order to avoid the risk of losing a bit. We do this with a */ -/* test-and-set spin lock if we know how to do that. Otherwise we */ -/* check whether we are already in the handler and use the dumb but */ -/* safe fallback algorithm of setting all bits in the word. */ -/* Contention should be very rare, so we do the minimum to handle it */ -/* correctly. */ -#ifdef AO_HAVE_test_and_set_acquire - GC_INNER volatile AO_TS_t GC_fault_handler_lock = AO_TS_INITIALIZER; - - GC_ATTR_NO_SANITIZE_THREAD - static void async_set_pht_entry_from_index(volatile page_hash_table db, - size_t index) - { - while (AO_test_and_set_acquire(&GC_fault_handler_lock) == AO_TS_SET) { - /* empty */ - } - /* Could also revert to set_pht_entry_from_index_safe if initial */ - /* GC_test_and_set fails. */ - set_pht_entry_from_index(db, index); - AO_CLEAR(&GC_fault_handler_lock); - } -#else /* !AO_HAVE_test_and_set_acquire */ -# error No test_and_set operation: Introduces a race. - /* THIS WOULD BE INCORRECT! */ - /* The dirty bit vector may be temporarily wrong, */ - /* just before we notice the conflict and correct it. We may end up */ - /* looking at it while it's wrong. But this requires contention */ - /* exactly when a GC is triggered, which seems far less likely to */ - /* fail than the old code, which had no reported failures. Thus we */ - /* leave it this way while we think of something better, or support */ - /* GC_test_and_set on the remaining platforms. */ - static int * volatile currently_updating = 0; - static void async_set_pht_entry_from_index(volatile page_hash_table db, - size_t index) - { - int update_dummy; - currently_updating = &update_dummy; - set_pht_entry_from_index(db, index); - /* If we get contention in the 10 or so instruction window here, */ - /* and we get stopped by a GC between the two updates, we lose! */ - if (currently_updating != &update_dummy) { - set_pht_entry_from_index_safe(db, index); - /* We claim that if two threads concurrently try to update the */ - /* dirty bit vector, the first one to execute UPDATE_START */ - /* will see it changed when UPDATE_END is executed. (Note that */ - /* &update_dummy must differ in two distinct threads.) It */ - /* will then execute set_pht_entry_from_index_safe, thus */ - /* returning us to a safe state, though not soon enough. */ - } - } -#endif /* !AO_HAVE_test_and_set_acquire */ - +#ifdef THREADS /* This function is used only by the fault handler. Potential data */ /* race between this function and GC_install_header, GC_remove_header */ /* should not be harmful because the added or removed header should */ @@ -3135,10 +3105,7 @@ GC_API GC_push_other_roots_proc GC_CALL GC_get_push_other_roots(void) return HDR_INNER(addr) != NULL; # endif } - -#else /* !THREADS */ -# define async_set_pht_entry_from_index(db, index) \ - set_pht_entry_from_index(db, index) +#else # define is_header_found_async(addr) (HDR(addr) != NULL) #endif /* !THREADS */ @@ -3720,7 +3687,7 @@ GC_INNER GC_bool GC_dirty_init(void) /* page unprotection. */ GC_ASSERT(GC_manual_vdb); # endif - set_pht_entry_from_index(GC_dirty_pages, index); /* FIXME: concurrent */ + async_set_pht_entry_from_index(GC_dirty_pages, index); } /* Retrieve system dirty bits for the heap to a local buffer (unless */ diff --git a/pthread_stop_world.c b/pthread_stop_world.c index c20ec17bc..92605974b 100644 --- a/pthread_stop_world.c +++ b/pthread_stop_world.c @@ -545,6 +545,9 @@ STATIC void GC_restart_handler(int sig) } /* Set the flag making the change visible to the signal handler. */ + /* This also removes the protection for t object, preventing */ + /* write faults in GC_store_stack_ptr (thus double-locking should */ + /* not occur in async_set_pht_entry_from_index). */ AO_store_release(&t->suspended_ext, TRUE); if (THREAD_EQUAL((pthread_t)thread, pthread_self())) { @@ -575,6 +578,7 @@ STATIC void GC_restart_handler(int sig) # endif /* TODO: Support GC_retry_signals (not needed for TSan) */ + GC_acquire_dirty_lock(); switch (RAISE_SIGNAL(t, GC_sig_suspend)) { /* ESRCH cannot happen as terminated threads are handled above. */ case 0: @@ -590,6 +594,7 @@ STATIC void GC_restart_handler(int sig) if (errno != EINTR) ABORT("sem_wait for handler failed (suspend_self)"); } + GC_release_dirty_lock(); RESTORE_CANCEL(cancel_state); UNLOCK(); } @@ -763,8 +768,11 @@ STATIC int GC_suspend_all(void) # ifdef GC_OPENBSD_UTHREADS { stack_t stack; + + GC_acquire_dirty_lock(); if (pthread_suspend_np(p -> id) != 0) ABORT("pthread_suspend_np failed"); + GC_release_dirty_lock(); if (pthread_stackseg_np(p->id, &stack)) ABORT("pthread_stackseg_np failed"); p -> stop_info.stack_ptr = (ptr_t)stack.ss_sp - stack.ss_size; @@ -773,6 +781,11 @@ STATIC int GC_suspend_all(void) (void *)p->id); } # else + /* The synchronization between GC_dirty (based on */ + /* test-and-set) and the signal-based thread suspension */ + /* is performed in GC_stop_world because */ + /* GC_release_dirty_lock cannot be called before */ + /* acknowledging the thread is really suspended. */ result = RAISE_SIGNAL(p, GC_sig_suspend); switch(result) { case ESRCH: @@ -876,11 +889,19 @@ GC_INNER void GC_stop_world(void) # else AO_store(&GC_stop_count, (AO_t)((word)GC_stop_count + 2)); /* Only concurrent reads are possible. */ + if (GC_manual_vdb) { + GC_acquire_dirty_lock(); + /* The write fault handler cannot be called if GC_manual_vdb */ + /* (thus double-locking should not occur in */ + /* async_set_pht_entry_from_index based on test-and-set). */ + } AO_store_release(&GC_world_is_stopped, TRUE); n_live_threads = GC_suspend_all(); if (GC_retry_signals) n_live_threads = resend_lost_signals(n_live_threads, GC_suspend_all); suspend_restart_barrier(n_live_threads); + if (GC_manual_vdb) + GC_release_dirty_lock(); /* cannot be done in GC_suspend_all */ # endif # ifdef PARALLEL_MARK diff --git a/win32_threads.c b/win32_threads.c index 2701b11c9..225d8e0dc 100644 --- a/win32_threads.c +++ b/win32_threads.c @@ -1186,15 +1186,7 @@ STATIC void GC_suspend(GC_thread t) return; } # endif -# if defined(MPROTECT_VDB) - /* Acquire the spin lock we use to update dirty bits. */ - /* Threads shouldn't get stopped holding it. But we may */ - /* acquire and release it in the UNPROTECT_THREAD call. */ - while (AO_test_and_set_acquire(&GC_fault_handler_lock) == AO_TS_SET) { - /* empty */ - } -# endif - + GC_acquire_dirty_lock(); # ifdef MSWINCE /* SuspendThread() will fail if thread is running kernel code. */ while (SuspendThread(THREAD_HANDLE(t)) == (DWORD)-1) @@ -1204,9 +1196,7 @@ STATIC void GC_suspend(GC_thread t) ABORT("SuspendThread failed"); # endif /* !MSWINCE */ t -> suspended = (unsigned char)TRUE; -# if defined(MPROTECT_VDB) - AO_CLEAR(&GC_fault_handler_lock); -# endif + GC_release_dirty_lock(); if (GC_on_thread_event) GC_on_thread_event(GC_EVENT_THREAD_SUSPENDED, THREAD_HANDLE(t)); }