When adopting a thread, spin until GC isn't running. #49934

maleadt · 2023-05-23T09:47:32Z

Fixes #49928, alternative to #49930.

maleadt · 2023-05-23T11:10:34Z

I'm not sure how to fix the analyzer error; jl_adopt_thread is currently annotated JL_NOTSAFEPOINT_LEAVE (what does the LEAVE mean?), but many of the functions it uses call into the GC. I guess the analyzer wasn't visiting this function before?

julia/src/threading.c:424:21: error: Calling potential safepoint as SimpleFunctionCall from function annotated JL_NOTSAFEPOINT [julia.GCChecker]
    jl_task_t *ct = jl_init_root_task(ptls, stack_lo, stack_hi);
                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
julia/src/julia_internal.h:863:12: note: Tried to call method defined here
jl_task_t *jl_init_root_task(jl_ptls_t ptls, void *stack_lo, void *stack_hi);
           ^~~~~~~~~~~~~~~~~

We can't annotate jl_init_root_task JL_NOTSAFEPOINT as it calls jl_gc_alloc.

vchuravy · 2023-05-23T11:41:00Z

This only fixes part of the issue. You still need to fix the issue where we get past this check and then GC starts and we hit a safepoint without being able to execute that safepoint on this thread yet.

src/threading.c

src/safepoint.c

src/threading.c

vchuravy · 2023-05-23T16:32:47Z

Besides the discussion around memory model this looks.good to me.

I will leave final review to @vtjnash and @JeffBezanson

gbaraldi · 2023-05-23T16:38:33Z

What happens if a thread calls GC.enable() and tries to enter GC in the meanwhile?

maleadt · 2023-05-23T17:01:30Z

What happens if a thread calls GC.enable() and tries to enter GC in the meanwhile?

GC.enable() only does anything if you've previously disabled GC, so that should be fine.

gbaraldi · 2023-05-23T17:09:49Z

Oh, I see it now we have a per thread check.

maleadt · 2023-05-24T09:58:44Z

Couple of CI hangs, with one printing the following backtrace:

KILLING BY THREAD TEST WATCHDOG
======================================================================================
Information request received. A stacktrace will print followed by a 1.0 second profile
======================================================================================
cmd: /cache/build/default-armageddon-1/julialang/julia-master/julia-c2ef05e211/bin/julia 9614 running 2 of 5
signal (10): User defined signal 1
pthread_cond_wait at /lib/aarch64-linux-gnu/libpthread.so.0 (unknown line)
uv_cond_wait at /workspace/srcdir/libuv/src/unix/thread.c:883
jl_safepoint_wait_gc at /cache/build/default-armageddon-7/julialang/julia-master/src/safepoint.c:172
jl_safepoint_start_gc at /cache/build/default-armageddon-7/julialang/julia-master/src/safepoint.c:124
ijl_gc_collect at /cache/build/default-armageddon-7/julialang/julia-master/src/gc.c:3513
gc at ./gcutils.jl:129 [inlined]
macro expansion at /cache/build/default-armageddon-1/julialang/julia-master/julia-c2ef05e211/share/julia/test/threads_exec.jl:209 [inlined]
#251#threadsfor_fun#21 at ./threadingconstructs.jl:184
#251#threadsfor_fun at ./threadingconstructs.jl:151 [inlined]
#1 at ./threadingconstructs.jl:129
unknown function (ip: 0xffffa1e52f4b)

So that seems related.

Otherwise GC might run after the foreign thread exits the spin loop, but before it is initialized and ready to handle GC safepoints.

…ent a data race.

Co-authored-by: Valentin Churavy <vchuravy@users.noreply.github.com>

Stores already use seq_cst.

vtjnash · 2023-05-26T15:16:09Z

src/safepoint.c

+    // 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);


This does not seem logical to be a release, since there are no other stores in this function to act upon

It looks like you intended the much more expensive seq-cst semantics so that it establishes an atomic ordering with the prior load?

I'm not sure. The load during jl_adopt_thread is an atomic acquire, which I intended to match with this release store. I'm not sure why it would need to be matched up with stores in this very function?

Co-authored-by: Valentin Churavy <vchuravy@users.noreply.github.com> (cherry-picked from commit 4ef9fb1)

maleadt requested review from vchuravy, vtjnash and JeffBezanson May 23, 2023 09:47

maleadt force-pushed the tb/adopt_gc branch 2 times, most recently from c7cb0b0 to 1bd2f6a Compare May 23, 2023 12:36