diff --git a/src/gc.c b/src/gc.c index f3a57fffe09a8..a7ff7c6c06201 100644 --- a/src/gc.c +++ b/src/gc.c @@ -3100,7 +3100,7 @@ static void sweep_finalizer_list(arraylist_t *list) } // collector entry point and control -static _Atomic(uint32_t) jl_gc_disable_counter = 1; +_Atomic(uint32_t) jl_gc_disable_counter = 1; JL_DLLEXPORT int jl_gc_enable(int on) { @@ -3497,7 +3497,7 @@ JL_DLLEXPORT void jl_gc_collect(jl_gc_collection_t collection) jl_task_t *ct = jl_current_task; jl_ptls_t ptls = ct->ptls; - if (jl_atomic_load_relaxed(&jl_gc_disable_counter)) { + if (jl_atomic_load_acquire(&jl_gc_disable_counter)) { size_t localbytes = jl_atomic_load_relaxed(&ptls->gc_num.allocd) + gc_num.interval; jl_atomic_store_relaxed(&ptls->gc_num.allocd, -(int64_t)gc_num.interval); static_assert(sizeof(_Atomic(uint64_t)) == sizeof(gc_num.deferred_alloc), ""); @@ -3508,11 +3508,10 @@ JL_DLLEXPORT void jl_gc_collect(jl_gc_collection_t collection) int8_t old_state = jl_atomic_load_relaxed(&ptls->gc_state); jl_atomic_store_release(&ptls->gc_state, JL_GC_STATE_WAITING); - // `jl_safepoint_start_gc()` makes sure only one thread can - // run the GC. + // `jl_safepoint_start_gc()` makes sure only one thread can run the GC. uint64_t t0 = jl_hrtime(); if (!jl_safepoint_start_gc()) { - // Multithread only. See assertion in `safepoint.c` + // either another thread is running GC, or the GC got disabled just now. jl_gc_state_set(ptls, old_state, JL_GC_STATE_WAITING); return; } @@ -3549,7 +3548,7 @@ JL_DLLEXPORT void jl_gc_collect(jl_gc_collection_t collection) gc_invoke_callbacks(jl_gc_cb_pre_gc_t, gc_cblist_pre_gc, (collection)); - if (!jl_atomic_load_relaxed(&jl_gc_disable_counter)) { + if (!jl_atomic_load_acquire(&jl_gc_disable_counter)) { JL_LOCK_NOGC(&finalizers_lock); // all the other threads are stopped, so this does not make sense, right? otherwise, failing that, this seems like plausibly a deadlock #ifndef __clang_gcanalyzer__ if (_jl_gc_collect(ptls, collection)) { diff --git a/src/julia_internal.h b/src/julia_internal.h index c518a348cb5fd..e649819f4a3e0 100644 --- a/src/julia_internal.h +++ b/src/julia_internal.h @@ -884,6 +884,7 @@ STATIC_INLINE int jl_addr_is_safepoint(uintptr_t addr) return addr >= safepoint_addr && addr < safepoint_addr + jl_page_size * 3; } extern _Atomic(uint32_t) jl_gc_running; +extern _Atomic(uint32_t) jl_gc_disable_counter; // All the functions are safe to be called from within a signal handler // provided that the thread will not be interrupted by another asynchronous // signal. diff --git a/src/safepoint.c b/src/safepoint.c index d64df084b0349..c6f9a42059d1a 100644 --- a/src/safepoint.c +++ b/src/safepoint.c @@ -124,6 +124,14 @@ int jl_safepoint_start_gc(void) jl_safepoint_wait_gc(); return 0; } + // Foreign thread adoption disables the GC and waits for it to finish, however, that may + // introduce a race between it and this thread checking if the GC is enabled and only + // then setting jl_gc_running. To avoid that, check again now that we won that race. + if (jl_atomic_load_acquire(&jl_gc_disable_counter)) { + jl_atomic_store_release(&jl_gc_running, 0); + uv_mutex_unlock(&safepoint_lock); + return 0; + } jl_safepoint_enable(1); jl_safepoint_enable(2); uv_mutex_unlock(&safepoint_lock); diff --git a/src/threading.c b/src/threading.c index 83d2e942e960f..691fa931f1a3f 100644 --- a/src/threading.c +++ b/src/threading.c @@ -406,18 +406,28 @@ jl_ptls_t jl_init_threadtls(int16_t tid) return ptls; } -JL_DLLEXPORT jl_gcframe_t **jl_adopt_thread(void) JL_NOTSAFEPOINT_LEAVE -{ +JL_DLLEXPORT jl_gcframe_t **jl_adopt_thread(void) +{ + // `jl_init_threadtls` puts us in a GC unsafe region, so ensure GC isn't running. + // we can't use a normal safepoint because we don't have signal handlers yet. + // we also can't use jl_safepoint_wait_gc because that assumes we're in a task. + jl_atomic_fetch_add(&jl_gc_disable_counter, 1); + while (jl_atomic_load_acquire(&jl_gc_running)) { + jl_cpu_pause(); + } + // this check is coupled with the one in `jl_safepoint_wait_gc`, where we observe if a + // foreign thread has asked to disable the GC, guaranteeing the order of events. + // initialize this thread (assign tid, create heap, set up root task) jl_ptls_t ptls = jl_init_threadtls(-1); void *stack_lo, *stack_hi; jl_init_stack_limits(0, &stack_lo, &stack_hi); - (void)jl_gc_unsafe_enter(ptls); // warning: this changes `jl_current_task`, so be careful not to call that from this function - jl_task_t *ct = jl_init_root_task(ptls, stack_lo, stack_hi); + jl_task_t *ct = jl_init_root_task(ptls, stack_lo, stack_hi); // assumes the GC is disabled JL_GC_PROMISE_ROOTED(ct); uv_random(NULL, NULL, &ct->rngState, sizeof(ct->rngState), 0, NULL); + jl_atomic_fetch_add(&jl_gc_disable_counter, -1); return &ct->gcstack; }