diff options
Diffstat (limited to 'rts/sm')
| -rw-r--r-- | rts/sm/GC.c | 6 | ||||
| -rw-r--r-- | rts/sm/NonMoving.c | 2 | ||||
| -rw-r--r-- | rts/sm/Sanity.c | 120 | ||||
| -rw-r--r-- | rts/sm/Scav.c | 2 |
4 files changed, 119 insertions, 11 deletions
diff --git a/rts/sm/GC.c b/rts/sm/GC.c index 8a8acb1b53..a5aa7e1f4e 100644 --- a/rts/sm/GC.c +++ b/rts/sm/GC.c @@ -1173,7 +1173,7 @@ any_work (void) } #endif - __atomic_fetch_add(&gct->no_work, 1, __ATOMIC_RELAXED); + gct->no_work++; #if defined(THREADED_RTS) yieldThread(); #endif @@ -1251,7 +1251,7 @@ gcWorkerThread (Capability *cap) // measurements more accurate on Linux, perhaps because it syncs // the CPU time across the multiple cores. Without this, CPU time // is heavily skewed towards GC rather than MUT. - SEQ_CST_STORE(&gct->wakeup, GC_THREAD_STANDING_BY); + gct->wakeup = GC_THREAD_STANDING_BY; debugTrace(DEBUG_gc, "GC thread %d standing by...", gct->thread_index); ACQUIRE_SPIN_LOCK(&gct->gc_spin); @@ -1283,7 +1283,7 @@ gcWorkerThread (Capability *cap) stat_endGCWorker (cap, gct); // This must come *after* stat_endGCWorker since it serves to // synchronize us with the GC leader, which will later aggregate the - // GC statistics. + // GC statistics (#17964,#18717) SEQ_CST_STORE(&gct->wakeup, GC_THREAD_WAITING_TO_CONTINUE); ACQUIRE_SPIN_LOCK(&gct->mut_spin); debugTrace(DEBUG_gc, "GC thread %d on my way...", gct->thread_index); diff --git a/rts/sm/NonMoving.c b/rts/sm/NonMoving.c index 3eafd6be98..5cb754b539 100644 --- a/rts/sm/NonMoving.c +++ b/rts/sm/NonMoving.c @@ -1216,7 +1216,7 @@ static void nonmovingMark_(MarkQueue *mark_queue, StgWeak **dead_weaks, StgTSO * #if defined(THREADED_RTS) finish: - boundTaskExiting(task); + exitMyTask(); // We are done... mark_thread = 0; diff --git a/rts/sm/Sanity.c b/rts/sm/Sanity.c index c09e28c0aa..d3c18adbd5 100644 --- a/rts/sm/Sanity.c +++ b/rts/sm/Sanity.c @@ -233,6 +233,111 @@ checkClosureProfSanity(const StgClosure *p) } #endif +/* Note [Racing weak pointer evacuation] + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + * While debugging a GC crash (#18919) I noticed a spurious crash due to the + * end-of-GC sanity check stumbling across a weak pointer with unevacuated key. + * This can happen when two GC threads race to evacuate a weak pointer. + * Specifically, we start out with a heap with a weak pointer reachable + * from both a generation's weak pointer list and some other root-reachable + * closure (e.g. a Just constructor): + * + * O W + * ┌──────────┐ ┌──────────┐ + * Root ────→ │ Just │ ╭───→ │ Weak# │ ←─────── weak_ptr_list + * Set ├──────────┤ │ ├──────────┤ + * │ │ ────╯ │ value │ ─→ ... + * └──────────┘ │ key │ ───╮ K + * │ ... │ │ ┌──────────┐ + * └──────────┘ ╰──→ │ ... │ + * ├──────────┤ + * + * The situation proceeds as follows: + * + * 1. Thread A initiates a GC, wakes up the GC worker threads, and starts + * evacuating roots. + * 2. Thread A evacuates a weak pointer object O to location O'. + * 3. Thread A fills the block where O' lives and pushes it to its + * work-stealing queue. + * 4. Thread B steals the O' block and starts scavenging it. + * 5. Thread A enters markWeakPtrList. + * 6. Thread A starts evacuating W, resulting in Wb'. + * 7. Thread B scavenges O', evacuating W', resulting in Wa'. + * 8. Thread A and B are now racing to evacuate W. Only one will win the race + * (due to the CAS in copy_tag). Let the winning copy be called W'. + * 9. W will be replaced by a forwarding pointer to the winning copy, W'. + * 10. Whichever thread loses the race will retry evacuation, see + * that W has already been evacuated, and proceed as usual. + * 10. W' will get added to weak_ptr_list by markWeakPtrList. + * 11. Eventually W' will be scavenged. + * 12. traverseWeakPtrList will see that W' has been scavenged and evacuate the + * its key. + * 13. However, the copy that lost the race is not on `weak_ptr_list` + * and will therefore never get its `key` field scavenged (since + * `traverseWeakPtrList` will never see it). + * + * Now the heap looks like: + * + * O' W (from-space) + * ┌──────────┐ ┌──────────┐ + * Root ────→ │ Just │ │ Fwd-ptr │ ───────────╮ + * Set ├──────────┤ ├──────────┤ │ + * │ │ ────╮ │ value │ ─→ ... │ + * └──────────┘ │ │ key │ ────────────────────────╮ + * │ │ ... │ │ │ + * │ └──────────┘ │ │ + * │ │ │ + * │ Wa' │ │ + * │ ┌──────────┐ ╭────╯ │ + * ╰───→ │ Weak# │ ←─────┤ │ + * ├──────────┤ ╰─ weak_ptr_list │ + * │ value │ ─→ ... │ + * │ key │ ───╮ K' │ + * │ ... │ │ ┌──────────┐ │ + * └──────────┘ ╰──→ │ ... │ │ + * ├──────────┤ │ + * Wb' │ + * ┌──────────┐ │ + * │ Weak# │ │ + * ├──────────┤ │ + * │ value │ ─→ ... │ + * │ key │ ───╮ K (from-space) │ + * │ ... │ │ ┌──────────┐ │ + * └──────────┘ ╰──→ │ 0xaaaaa │ ←──╯ + * ├──────────┤ + * + * + * Without sanity checking this is fine; we have introduced a spurious copy of + * W, Wb' into the heap but it is unreachable and therefore won't cause any + * trouble. However, with sanity checking we may encounter this spurious copy + * when walking the heap. Moreover, this copy was never added to weak_ptr_list, + * meaning that its key field (along with the other fields mark as + * non-pointers) will not get scavenged and will therefore point into + * from-space. + * + * To avoid this checkClosure skips over the key field when it sees a weak + * pointer. Note that all fields of Wb' *other* than the key field should be + * valid, so we don't skip the closure entirely. + * + * We then do additional checking of all closures on the weak_ptr_lists, where + * we *do* check `key`. + */ + +// Check validity of objects on weak_ptr_list. +// See Note [Racing weak pointer evacuation]. +static void +checkGenWeakPtrList( uint32_t g ) +{ + for (StgWeak *w = generations[g].weak_ptr_list; w != NULL; w = w->link) { + ASSERT(LOOKS_LIKE_CLOSURE_PTR(w)); + ASSERT(w->header.info == &stg_WEAK_info); + ASSERT(LOOKS_LIKE_CLOSURE_PTR(w->key)); + ASSERT(LOOKS_LIKE_CLOSURE_PTR(w->value)); + ASSERT(LOOKS_LIKE_CLOSURE_PTR(w->finalizer)); + ASSERT(LOOKS_LIKE_CLOSURE_PTR(w->cfinalizers)); + } +} + // Returns closure size in words StgOffset checkClosure( const StgClosure* p ) @@ -352,12 +457,9 @@ checkClosure( const StgClosure* p ) * representative of the actual layout. */ { StgWeak *w = (StgWeak *)p; - ASSERT(LOOKS_LIKE_CLOSURE_PTR(w->key)); - ASSERT(LOOKS_LIKE_CLOSURE_PTR(w->value)); - ASSERT(LOOKS_LIKE_CLOSURE_PTR(w->finalizer)); - if (w->link) { - ASSERT(LOOKS_LIKE_CLOSURE_PTR(w->link)); - } + // N.B. Checking most of the fields here is not safe. + // See Note [Racing weak pointer evacuation] for why. + ASSERT(LOOKS_LIKE_CLOSURE_PTR(w->cfinalizers)); return sizeW_fromITBL(info); } @@ -852,6 +954,12 @@ static void checkGeneration (generation *gen, checkHeapChain(ws->scavd_list); } + // Check weak pointer lists + // See Note [Racing weak pointer evacuation]. + for (uint32_t g = 0; g < RtsFlags.GcFlags.generations; g++) { + checkGenWeakPtrList(g); + } + checkLargeObjects(gen->large_objects); checkCompactObjects(gen->compact_objects); } diff --git a/rts/sm/Scav.c b/rts/sm/Scav.c index 9fe2c6006e..c1f3faf998 100644 --- a/rts/sm/Scav.c +++ b/rts/sm/Scav.c @@ -437,7 +437,7 @@ scavenge_block (bdescr *bd) saved_eager_promotion = gct->eager_promotion; gct->failed_to_evac = false; - ws = &gct->gens[bd->gen->no]; + ws = &gct->gens[bd->gen_no]; p = bd->u.scan; |