From 06e52c567d55d873c73e21097cef70a142cef413 Mon Sep 17 00:00:00 2001 From: Gabriel Baraldi Date: Thu, 22 Feb 2024 14:24:54 -0300 Subject: [PATCH] Fix synchronization issue on the GC scheduler (#53355) This aims to slightly simplify the synchronization by making `n_threads_marking` the sole memory location of relevance for it, it also removes the fast path, because being protected by the lock is quite important so that the observed gc state arrays are valid. Fixes: #53350 Fixes: #52757 Maybe fixes: #53026 Co-authored-by: Jameson Nash (cherry picked from commit a96726b84d9dfcaec304c5643956a2e9d2176079) --- src/gc.c | 63 ++++++++++++++------------------------- src/julia_threads.h | 11 +++---- src/llvm-codegen-shared.h | 2 +- src/safepoint.c | 4 +-- src/scheduler.c | 10 +++---- src/signal-handling.c | 2 +- src/threading.c | 2 +- 7 files changed, 37 insertions(+), 57 deletions(-) diff --git a/src/gc.c b/src/gc.c index a920958307c48..c2d194b8a28bb 100644 --- a/src/gc.c +++ b/src/gc.c @@ -3,6 +3,7 @@ #include "gc.h" #include "gc-page-profiler.h" #include "julia.h" +#include "julia_atomics.h" #include "julia_gcext.h" #include "julia_assert.h" #ifdef __GLIBC__ @@ -540,7 +541,7 @@ void jl_gc_run_all_finalizers(jl_task_t *ct) void jl_gc_add_finalizer_(jl_ptls_t ptls, void *v, void *f) JL_NOTSAFEPOINT { - assert(jl_atomic_load_relaxed(&ptls->gc_state) == 0); + assert(jl_atomic_load_relaxed(&ptls->gc_state) == JL_GC_STATE_UNSAFE); arraylist_t *a = &ptls->finalizers; // This acquire load and the release store at the end are used to // synchronize with `finalize_object` on another thread. Apart from the GC, @@ -1676,7 +1677,7 @@ void gc_sweep_wake_all(jl_ptls_t ptls, jl_gc_padded_page_stack_t *new_gc_allocd_ void gc_sweep_wait_for_all(void) { jl_atomic_store(&gc_allocd_scratch, NULL); - while (jl_atomic_load_relaxed(&gc_n_threads_sweeping) != 0) { + while (jl_atomic_load_acquire(&gc_n_threads_sweeping) != 0) { jl_cpu_pause(); } } @@ -2966,9 +2967,7 @@ void gc_mark_and_steal(jl_ptls_t ptls) jl_gc_markqueue_t *mq = &ptls->mark_queue; jl_gc_markqueue_t *mq_master = NULL; int master_tid = jl_atomic_load(&gc_master_tid); - if (master_tid == -1) { - return; - } + assert(master_tid != -1); mq_master = &gc_all_tls_states[master_tid]->mark_queue; void *new_obj; jl_gc_chunk_t c; @@ -3060,54 +3059,37 @@ size_t gc_count_work_in_queue(jl_ptls_t ptls) JL_NOTSAFEPOINT * Correctness argument for the mark-loop termination protocol. * * Safety properties: - * - No work items shall be in any thread's queues when `gc_mark_loop_barrier` observes + * - No work items shall be in any thread's queues when `gc_should_mark` observes * that `gc_n_threads_marking` is zero. * * - No work item shall be stolen from the master thread (i.e. mutator thread which started * GC and which helped the `jl_n_markthreads` - 1 threads to mark) after - * `gc_mark_loop_barrier` observes that `gc_n_threads_marking` is zero. This property is + * `gc_should_mark` observes that `gc_n_threads_marking` is zero. This property is * necessary because we call `gc_mark_loop_serial` after marking the finalizer list in * `_jl_gc_collect`, and want to ensure that we have the serial mark-loop semantics there, * and that no work is stolen from us at that point. * * Proof: - * - Suppose the master thread observes that `gc_n_threads_marking` is zero in - * `gc_mark_loop_barrier` and there is a work item left in one thread's queue at that point. - * Since threads try to steal from all threads' queues, this implies that all threads must - * have tried to steal from the queue which still has a work item left, but failed to do so, - * which violates the semantics of Chase-Lev's work-stealing queue. - * - * - Let E1 be the event "master thread writes -1 to gc_master_tid" and E2 be the event - * "master thread observes that `gc_n_threads_marking` is zero". Since we're using - * sequentially consistent atomics, E1 => E2. Now suppose one thread which is spinning in - * `gc_should_mark` tries to enter the mark-loop after E2. In order to do so, it must - * increment `gc_n_threads_marking` to 1 in an event E3, and then read `gc_master_tid` in an - * event E4. Since we're using sequentially consistent atomics, E3 => E4. Since we observed - * `gc_n_threads_marking` as zero in E2, then E2 => E3, and we conclude E1 => E4, so that - * the thread which is spinning in `gc_should_mark` must observe that `gc_master_tid` is -1 - * and therefore won't enter the mark-loop. + * - If a thread observes that `gc_n_threads_marking` is zero inside `gc_should_mark`, that + * means that no thread has work on their queue, this is guaranteed because a thread may only exit + * `gc_mark_and_steal` when its own queue is empty, this information is synchronized by the + * seq-cst fetch_add to a thread that is in `gc_should_mark`. `gc_queue_observer_lock` + * guarantees that once `gc_n_threads_marking` reaches zero, no thread will increment it again, + * because incrementing is only legal from inside the lock. Therefore, no thread will reenter + * the mark-loop after `gc_n_threads_marking` reaches zero. */ int gc_should_mark(void) { int should_mark = 0; - int n_threads_marking = jl_atomic_load(&gc_n_threads_marking); - // fast path - if (n_threads_marking == 0) { - return 0; - } uv_mutex_lock(&gc_queue_observer_lock); while (1) { - int tid = jl_atomic_load(&gc_master_tid); - // fast path - if (tid == -1) { - break; - } - n_threads_marking = jl_atomic_load(&gc_n_threads_marking); - // fast path + int n_threads_marking = jl_atomic_load(&gc_n_threads_marking); if (n_threads_marking == 0) { break; } + int tid = jl_atomic_load_relaxed(&gc_master_tid); + assert(tid != -1); size_t work = gc_count_work_in_queue(gc_all_tls_states[tid]); for (tid = gc_first_tid; tid < gc_first_tid + jl_n_markthreads; tid++) { jl_ptls_t ptls2 = gc_all_tls_states[tid]; @@ -3118,7 +3100,8 @@ int gc_should_mark(void) } // if there is a lot of work left, enter the mark loop if (work >= 16 * n_threads_marking) { - jl_atomic_fetch_add(&gc_n_threads_marking, 1); + jl_atomic_fetch_add(&gc_n_threads_marking, 1); // A possibility would be to allow a thread that found lots + // of work to increment this should_mark = 1; break; } @@ -3130,9 +3113,7 @@ int gc_should_mark(void) void gc_wake_all_for_marking(jl_ptls_t ptls) { - jl_atomic_store(&gc_master_tid, ptls->tid); uv_mutex_lock(&gc_threads_lock); - jl_atomic_fetch_add(&gc_n_threads_marking, 1); uv_cond_broadcast(&gc_threads_cond); uv_mutex_unlock(&gc_threads_lock); } @@ -3140,6 +3121,8 @@ void gc_wake_all_for_marking(jl_ptls_t ptls) void gc_mark_loop_parallel(jl_ptls_t ptls, int master) { if (master) { + jl_atomic_store(&gc_master_tid, ptls->tid); + jl_atomic_fetch_add(&gc_n_threads_marking, 1); gc_wake_all_for_marking(ptls); gc_mark_and_steal(ptls); jl_atomic_fetch_add(&gc_n_threads_marking, -1); @@ -3166,10 +3149,8 @@ void gc_mark_loop(jl_ptls_t ptls) void gc_mark_loop_barrier(void) { - jl_atomic_store(&gc_master_tid, -1); - while (jl_atomic_load(&gc_n_threads_marking) != 0) { - jl_cpu_pause(); - } + assert(jl_atomic_load_relaxed(&gc_n_threads_marking) == 0); + jl_atomic_store_relaxed(&gc_master_tid, -1); } void gc_mark_clean_reclaim_sets(void) diff --git a/src/julia_threads.h b/src/julia_threads.h index 1c64a722850f2..3f8f5391919b4 100644 --- a/src/julia_threads.h +++ b/src/julia_threads.h @@ -192,6 +192,9 @@ typedef struct _jl_tls_states_t { _Atomic(volatile size_t *) safepoint; // may be changed to the suspend page by any thread _Atomic(int8_t) sleep_check_state; // read/write from foreign threads // Whether it is safe to execute GC at the same time. +#define JL_GC_STATE_UNSAFE 0 + // gc_state = 0 means the thread is running Julia code and is not + // safe to run concurrently to the GC #define JL_GC_STATE_WAITING 1 // gc_state = 1 means the thread is doing GC or is waiting for the GC to // finish. @@ -330,9 +333,7 @@ STATIC_INLINE int8_t jl_gc_state_set(jl_ptls_t ptls, int8_t state, int8_t old_state) { jl_atomic_store_release(&ptls->gc_state, state); - if (state == JL_GC_STATE_SAFE && old_state == 0) - jl_gc_safepoint_(ptls); - if (state == 0 && old_state == JL_GC_STATE_SAFE) + if (state == JL_GC_STATE_UNSAFE || old_state == JL_GC_STATE_UNSAFE) jl_gc_safepoint_(ptls); return old_state; } @@ -347,8 +348,8 @@ void jl_gc_unsafe_leave(jl_ptls_t ptls, int8_t state) JL_NOTSAFEPOINT JL_NOTSAFE int8_t jl_gc_safe_enter(jl_ptls_t ptls) JL_NOTSAFEPOINT JL_NOTSAFEPOINT_ENTER; void jl_gc_safe_leave(jl_ptls_t ptls, int8_t state) JL_NOTSAFEPOINT_LEAVE; // this might not be a safepoint, but we have to assume it could be (statically) #else -#define jl_gc_unsafe_enter(ptls) jl_gc_state_save_and_set(ptls, 0) -#define jl_gc_unsafe_leave(ptls, state) ((void)jl_gc_state_set(ptls, (state), 0)) +#define jl_gc_unsafe_enter(ptls) jl_gc_state_save_and_set(ptls, JL_GC_STATE_UNSAFE) +#define jl_gc_unsafe_leave(ptls, state) ((void)jl_gc_state_set(ptls, (state), JL_GC_STATE_UNSAFE)) #define jl_gc_safe_enter(ptls) jl_gc_state_save_and_set(ptls, JL_GC_STATE_SAFE) #define jl_gc_safe_leave(ptls, state) ((void)jl_gc_state_set(ptls, (state), JL_GC_STATE_SAFE)) #endif diff --git a/src/llvm-codegen-shared.h b/src/llvm-codegen-shared.h index a4f77bc1b3b38..2c60907ca086c 100644 --- a/src/llvm-codegen-shared.h +++ b/src/llvm-codegen-shared.h @@ -325,7 +325,7 @@ static inline llvm::Value *emit_gc_unsafe_enter(llvm::IRBuilder<> &builder, llvm static inline llvm::Value *emit_gc_unsafe_leave(llvm::IRBuilder<> &builder, llvm::Type *T_size, llvm::Value *ptls, llvm::Value *state, bool final) { using namespace llvm; - Value *old_state = builder.getInt8(0); + Value *old_state = builder.getInt8(JL_GC_STATE_UNSAFE); return emit_gc_state_set(builder, T_size, ptls, state, old_state, final); } diff --git a/src/safepoint.c b/src/safepoint.c index a05d37e492813..22cda0a89444d 100644 --- a/src/safepoint.c +++ b/src/safepoint.c @@ -234,7 +234,7 @@ void jl_safepoint_wait_gc(void) JL_NOTSAFEPOINT jl_task_t *ct = jl_current_task; (void)ct; JL_TIMING_SUSPEND_TASK(GC_SAFEPOINT, ct); // The thread should have set this is already - assert(jl_atomic_load_relaxed(&ct->ptls->gc_state) != 0); + assert(jl_atomic_load_relaxed(&ct->ptls->gc_state) != JL_GC_STATE_UNSAFE); // Use normal volatile load in the loop for speed until GC finishes. // Then use an acquire load to make sure the GC result is visible on this thread. while (jl_atomic_load_relaxed(&jl_gc_running) || jl_atomic_load_acquire(&jl_gc_running)) { @@ -309,7 +309,7 @@ int jl_safepoint_suspend_thread(int tid, int waitstate) } while (jl_atomic_load_acquire(&ptls2->suspend_count) != 0) { int8_t state2 = jl_atomic_load_acquire(&ptls2->gc_state); - if (waitstate <= 2 && state2 != 0) + if (waitstate <= 2 && state2 != JL_GC_STATE_UNSAFE) break; if (waitstate == 3 && state2 == JL_GC_STATE_WAITING) break; diff --git a/src/scheduler.c b/src/scheduler.c index 2af88db89ca14..dbc913353ce4c 100644 --- a/src/scheduler.c +++ b/src/scheduler.c @@ -131,7 +131,7 @@ void jl_parallel_gc_threadfun(void *arg) jl_ptls_t ptls = jl_init_threadtls(targ->tid); // wait for all threads - jl_gc_state_set(ptls, JL_GC_STATE_WAITING, 0); + jl_gc_state_set(ptls, JL_GC_STATE_WAITING, JL_GC_STATE_UNSAFE); uv_barrier_wait(targ->barrier); // free the thread argument here @@ -143,9 +143,7 @@ void jl_parallel_gc_threadfun(void *arg) uv_cond_wait(&gc_threads_cond, &gc_threads_lock); } uv_mutex_unlock(&gc_threads_lock); - if (may_mark()) { - gc_mark_loop_parallel(ptls, 0); - } + gc_mark_loop_parallel(ptls, 0); if (may_sweep(ptls)) { // not an else! gc_sweep_pool_parallel(ptls); jl_atomic_fetch_add(&ptls->gc_sweeps_requested, -1); @@ -162,7 +160,7 @@ void jl_concurrent_gc_threadfun(void *arg) jl_ptls_t ptls = jl_init_threadtls(targ->tid); // wait for all threads - jl_gc_state_set(ptls, JL_GC_STATE_WAITING, 0); + jl_gc_state_set(ptls, JL_GC_STATE_WAITING, JL_GC_STATE_UNSAFE); uv_barrier_wait(targ->barrier); // free the thread argument here @@ -190,7 +188,7 @@ void jl_threadfun(void *arg) JL_GC_PROMISE_ROOTED(ct); // wait for all threads - jl_gc_state_set(ptls, JL_GC_STATE_SAFE, 0); + jl_gc_state_set(ptls, JL_GC_STATE_SAFE, JL_GC_STATE_UNSAFE); uv_barrier_wait(targ->barrier); // free the thread argument here diff --git a/src/signal-handling.c b/src/signal-handling.c index abe63ba6d2d7f..2ddbf2ad1cc8e 100644 --- a/src/signal-handling.c +++ b/src/signal-handling.c @@ -440,7 +440,7 @@ void jl_task_frame_noreturn(jl_task_t *ct) JL_NOTSAFEPOINT ct->ptls->in_finalizer = 0; ct->ptls->defer_signal = 0; // forcibly exit GC (if we were in it) or safe into unsafe, without the mandatory safepoint - jl_atomic_store_release(&ct->ptls->gc_state, 0); + jl_atomic_store_release(&ct->ptls->gc_state, JL_GC_STATE_UNSAFE); // allow continuing to use a Task that should have already died--unsafe necromancy! jl_atomic_store_relaxed(&ct->_state, JL_TASK_STATE_RUNNABLE); } diff --git a/src/threading.c b/src/threading.c index 648f7d935bac2..dcfc6ac7e93bb 100644 --- a/src/threading.c +++ b/src/threading.c @@ -364,7 +364,7 @@ jl_ptls_t jl_init_threadtls(int16_t tid) } } #endif - jl_atomic_store_relaxed(&ptls->gc_state, 0); // GC unsafe + jl_atomic_store_relaxed(&ptls->gc_state, JL_GC_STATE_UNSAFE); // GC unsafe // Conditionally initialize the safepoint address. See comment in // `safepoint.c` if (tid == 0) {