encounter/bdwgc
0
0
Fork 0
mirror of https://github.com/encounter/bdwgc.git synced 2026-03-30 10:57:55 -07:00
Code Issues Packages Projects Releases Wiki Activity

Fix concurrent bitmap update in GC_dirty

Browse Source 
Issue #235 (bdwgc).

* darwin_stop_world.c [!GC_NO_THREADS_DISCOVERY]
(GC_suspend_thread_list): Call GC_acquire_dirty_lock and
GC_release_dirty_lock around thread_suspend().
* darwin_stop_world.c (GC_stop_world): Likewise.
* include/private/gc_priv.h (set_pht_entry_from_index_safe): Remove
unused macro.
* include/private/gc_priv.h [THREADS] (GC_acquire_dirty_lock,
GC_release_dirty_lock): Define new macro; move comment from
win32_threads.c.
* include/private/gc_priv.h [THREADS && !GC_DISABLE_INCREMENTAL]
(GC_fault_handler_lock): Declare global variable (even if not
MPROTECT_VDB).
* os_dep.c [!GC_DISABLE_INCREMENTAL] (async_set_pht_entry_from_index):
Define even if not MPROTECT_VDB; reformat comments.
* os_dep.c [!GC_DISABLE_INCREMENTAL && AO_HAVE_test_and_set_acquire]
(GC_fault_handler_lock): Likewise.
* os_dep.c [THREADS && AO_HAVE_test_and_set_acquire]
(async_set_pht_entry_from_index): Use GC_acquire_dirty_lock and
GC_release_dirty_lock; remove wrong comment.
* os_dep.c [THREADS && !AO_HAVE_test_and_set_acquire]
(currently_updating, async_set_pht_entry_from_index): Remove incorrect
implementation.
* os_dep.c [!GC_DISABLE_INCREMENTAL] (GC_dirty_inner): Call
async_set_pht_entry_from_index instead of set_pht_entry_from_index;
remove FIXME.
* pthread_stop_world.c [GC_ENABLE_SUSPEND_THREAD] (GC_suspend_thread):
Refine comment for AO_store_release call; call GC_acquire_dirty_lock
and GC_release_dirty_lock around a block of RAISE_SIGNAL() and
sem_wait().
* pthread_stop_world.c [GC_OPENBSD_UTHREADS] (GC_suspend_all): Call
GC_acquire_dirty_lock and GC_release_dirty_lock around
pthread_suspend_np().
* pthread_stop_world.c [!GC_OPENBSD_UTHREADS] (GC_suspend_all): Add
comment.
* pthread_stop_world.c [!GC_OPENBSD_UTHREADS && !NACL] (GC_stop_world):
If GC_manual_vdb then call GC_acquire_dirty_lock and
GC_release_dirty_lock around a block of GC_suspend_all() and
suspend_restart_barrier(); add comment.
* win32_threads.c (GC_suspend): Use GC_acquire_dirty_lock and
GC_release_dirty_lock (regardless of MPROTECT_VDB).
This commit is contained in:
Ivan Maidanski
2018-09-24 09:54:03 +03:00
parent 7305e56195
commit 0c0e4cd0b9
5 changed files with 64 additions and 77 deletions
Download Patch File Download Diff File Expand all files Collapse all files
darwin_stop_world.c
+4 -1
View File
@@ -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)
include/private/gc_priv.h
+11 -5
View File
@@ -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
os_dep.c
+26 -59
View File
@@ -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 */
pthread_stop_world.c
+21
View File
@@ -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
win32_threads.c
+2 -12
View File
@@ -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));
}