[Git][ghc/ghc][wip/tsan/fix-thunk-update-9.6] Fix thunk update ordering
Ben Gamari (@bgamari)
gitlab at gitlab.haskell.org
Wed Dec 13 17:42:50 UTC 2023
Ben Gamari pushed to branch wip/tsan/fix-thunk-update-9.6 at Glasgow Haskell Compiler / GHC
Commits:
a9489a22 by Ben Gamari at 2023-12-13T12:42:34-05:00
Fix thunk update ordering
Previously we attempted to ensure soundness of concurrent thunk update
by synchronizing on the access of the thunk's info table pointer field.
This was believed to be sufficient since the indirectee (which may
expose a closure allocated by another core) would not be examined
until the info table pointer update is complete.
However, it turns out that this can result in data races in the presence
of multiple threads racing a update a single thunk. For instance,
consider this interleaving under the old scheme:
Thread A Thread B
--------- ---------
t=0 Enter t
1 Push update frame
2 Begin evaluation
4 Pause thread
5 t.indirectee=tso
6 Release t.info=BLACKHOLE
7 ... (e.g. GC)
8 Resume thread
9 Finish evaluation
10 Relaxed t.indirectee=x
11 Load t.info
12 Acquire fence
13 Inspect t.indirectee
14 Release t.info=BLACKHOLE
Here Thread A enters thunk `t` but is soon paused, resulting in `t`
being lazily blackholed at t=6. Then, at t=10 Thread A finishes
evaluation and updates `t.indirectee` with a relaxed store.
Meanwhile, Thread B enters the blackhole. Under the old scheme this
would introduce an acquire-fence but this would only synchronize with
Thread A at t=6. Consequently, the result of the evaluation, `x`, is not
visible to Thread B, introducing a data race.
We fix this by treating the `indirectee` field as we do all other
mutable fields. This means we must always access this field with
acquire-loads and release-stores.
See #23185.
(cherry picked from commit fa63b5902389aa929af5ec04b93b601fd456633f)
- - - - -
18 changed files:
- compiler/GHC/StgToCmm/Bind.hs
- rts/Apply.cmm
- rts/Compact.cmm
- rts/Heap.c
- rts/Interpreter.c
- rts/Messages.c
- rts/PrimOps.cmm
- rts/StableName.c
- rts/StgMiscClosures.cmm
- rts/ThreadPaused.c
- rts/Threads.c
- rts/Updates.cmm
- rts/Updates.h
- rts/include/Cmm.h
- rts/sm/Evac.c
- rts/sm/NonMovingMark.c
- rts/sm/Storage.c
- utils/genapply/Main.hs
Changes:
=====================================
compiler/GHC/StgToCmm/Bind.hs
=====================================
@@ -701,10 +701,19 @@ emitBlackHoleCode node = do
when eager_blackholing $ do
whenUpdRemSetEnabled $ emitUpdRemSetPushThunk node
- emitStore (cmmOffsetW platform node (fixedHdrSizeW profile)) currentTSOExpr
+ emitAtomicStore platform MemOrderRelease
+ (cmmOffsetW platform node (fixedHdrSizeW profile))
+ currentTSOExpr
-- See Note [Heap memory barriers] in SMP.h.
- let w = wordWidth platform
- emitPrimCall [] (MO_AtomicWrite w MemOrderRelease) [node, CmmReg (CmmGlobal EagerBlackholeInfo)]
+ emitAtomicStore platform MemOrderRelease
+ node
+ (CmmReg (CmmGlobal EagerBlackholeInfo))
+
+emitAtomicStore :: Platform -> MemoryOrdering -> CmmExpr -> CmmExpr -> FCode ()
+emitAtomicStore platform mord addr val =
+ emitPrimCall [] (MO_AtomicWrite w mord) [addr, val]
+ where
+ w = typeWidth $ cmmExprType platform val
setupUpdate :: ClosureInfo -> LocalReg -> FCode () -> FCode ()
-- Nota Bene: this function does not change Node (even if it's a CAF),
=====================================
rts/Apply.cmm
=====================================
@@ -108,7 +108,7 @@ again:
IND,
IND_STATIC:
{
- fun = StgInd_indirectee(fun);
+ fun = %acquire StgInd_indirectee(fun);
goto again;
}
case BCO:
@@ -693,7 +693,7 @@ INFO_TABLE(stg_AP_STACK,/*special layout*/0,0,AP_STACK,"AP_STACK","AP_STACK")
}
// Can't add StgInd_indirectee(ap) to UpdRemSet here because the old value is
// not reachable.
- StgInd_indirectee(ap) = CurrentTSO;
+ %release StgInd_indirectee(ap) = CurrentTSO;
SET_INFO_RELEASE(ap, __stg_EAGER_BLACKHOLE_info);
/* ensure there is at least AP_STACK_SPLIM words of headroom available
=====================================
rts/Compact.cmm
=====================================
@@ -100,7 +100,7 @@ eval:
// Follow indirections:
case IND, IND_STATIC: {
- p = StgInd_indirectee(p);
+ p = %acquire StgInd_indirectee(p);
goto eval;
}
=====================================
rts/Heap.c
=====================================
@@ -173,7 +173,7 @@ StgWord collect_pointers(StgClosure *closure, StgClosure *ptrs[]) {
case IND:
case IND_STATIC:
case BLACKHOLE:
- ptrs[nptrs++] = (StgClosure *)(((StgInd *)closure)->indirectee);
+ ptrs[nptrs++] = (StgClosure *) ACQUIRE_LOAD(&((StgInd *)closure)->indirectee);
break;
case MUT_ARR_PTRS_CLEAN:
=====================================
rts/Interpreter.c
=====================================
@@ -420,7 +420,7 @@ eval_obj:
case IND:
case IND_STATIC:
{
- tagged_obj = ((StgInd*)obj)->indirectee;
+ tagged_obj = ACQUIRE_LOAD(&((StgInd*)obj)->indirectee);
goto eval_obj;
}
=====================================
rts/Messages.c
=====================================
@@ -191,9 +191,6 @@ uint32_t messageBlackHole(Capability *cap, MessageBlackHole *msg)
StgClosure *p;
const StgInfoTable *info;
do {
- // If we are being called from stg_BLACKHOLE then TSAN won't know about the
- // previous read barrier that makes the following access safe.
- TSAN_ANNOTATE_BENIGN_RACE(&((StgInd*)bh)->indirectee, "messageBlackHole");
p = UNTAG_CLOSURE(ACQUIRE_LOAD(&((StgInd*)bh)->indirectee));
info = RELAXED_LOAD(&p->header.info);
} while (info == &stg_IND_info);
@@ -291,7 +288,7 @@ uint32_t messageBlackHole(Capability *cap, MessageBlackHole *msg)
// makes it into the update remembered set
updateRemembSetPushClosure(cap, (StgClosure*)bq->queue);
}
- RELAXED_STORE(&msg->link, bq->queue);
+ msg->link = bq->queue;
bq->queue = msg;
// No barrier is necessary here: we are only exposing the
// closure to the GC. See Note [Heap memory barriers] in SMP.h.
=====================================
rts/PrimOps.cmm
=====================================
@@ -1767,7 +1767,7 @@ loop:
qinfo = GET_INFO_ACQUIRE(q);
if (qinfo == stg_IND_info ||
qinfo == stg_MSG_NULL_info) {
- q = StgInd_indirectee(q);
+ q = %acquire StgInd_indirectee(q);
goto loop;
}
@@ -1835,7 +1835,7 @@ loop:
if (qinfo == stg_IND_info ||
qinfo == stg_MSG_NULL_info) {
- q = StgInd_indirectee(q);
+ q = %acquire StgInd_indirectee(q);
goto loop;
}
@@ -1937,7 +1937,7 @@ loop:
if (qinfo == stg_IND_info ||
qinfo == stg_MSG_NULL_info) {
- q = StgInd_indirectee(q);
+ q = %acquire StgInd_indirectee(q);
goto loop;
}
@@ -2026,7 +2026,7 @@ loop:
if (qinfo == stg_IND_info ||
qinfo == stg_MSG_NULL_info) {
- q = StgInd_indirectee(q);
+ q = %acquire StgInd_indirectee(q);
goto loop;
}
@@ -2307,7 +2307,7 @@ loop:
//Possibly IND added by removeFromMVarBlockedQueue
if (StgHeader_info(q) == stg_IND_info ||
StgHeader_info(q) == stg_MSG_NULL_info) {
- q = StgInd_indirectee(q);
+ q = %acquire StgInd_indirectee(q);
goto loop;
}
=====================================
rts/StableName.c
=====================================
@@ -156,11 +156,11 @@ removeIndirections (StgClosure* p)
switch (get_itbl(q)->type) {
case IND:
case IND_STATIC:
- p = ((StgInd *)q)->indirectee;
+ p = ACQUIRE_LOAD(&((StgInd *)q)->indirectee);
continue;
case BLACKHOLE:
- p = ((StgInd *)q)->indirectee;
+ p = ACQUIRE_LOAD(&((StgInd *)q)->indirectee);
if (GET_CLOSURE_TAG(p) != 0) {
continue;
} else {
=====================================
rts/StgMiscClosures.cmm
=====================================
@@ -509,7 +509,9 @@ INFO_TABLE(stg_IND,1,0,IND,"IND","IND")
(P_ node)
{
TICK_ENT_DYN_IND(); /* tick */
- node = UNTAG(StgInd_indirectee(node));
+ ACQUIRE_FENCE;
+ node = %acquire StgInd_indirectee(node);
+ node = UNTAG(node);
TICK_ENT_VIA_NODE();
jump %GET_ENTRY(node) (node);
}
@@ -517,7 +519,10 @@ INFO_TABLE(stg_IND,1,0,IND,"IND","IND")
/* explicit stack */
{
TICK_ENT_DYN_IND(); /* tick */
- R1 = UNTAG(StgInd_indirectee(R1));
+ ACQUIRE_FENCE;
+ P_ p;
+ p = %acquire StgInd_indirectee(R1);
+ R1 = UNTAG(p);
TICK_ENT_VIA_NODE();
jump %GET_ENTRY(R1) [R1];
}
@@ -527,7 +532,10 @@ INFO_TABLE(stg_IND_STATIC,1,0,IND_STATIC,"IND_STATIC","IND_STATIC")
/* explicit stack */
{
TICK_ENT_STATIC_IND(); /* tick */
- R1 = UNTAG(StgInd_indirectee(R1));
+ ACQUIRE_FENCE;
+ P_ p;
+ p = %acquire StgInd_indirectee(R1);
+ R1 = UNTAG(p);
TICK_ENT_VIA_NODE();
jump %GET_ENTRY(R1) [R1];
}
@@ -661,6 +669,7 @@ loop:
// defined in CMM.
goto loop;
}
+ ACQUIRE_FENCE;
jump %ENTRY_CODE(info) (node);
#else
ccall barf("WHITEHOLE object (%p) entered!", R1) never returns;
=====================================
rts/ThreadPaused.c
=====================================
@@ -352,7 +352,7 @@ threadPaused(Capability *cap, StgTSO *tso)
OVERWRITING_CLOSURE_SIZE(bh, closure_sizeW_(bh, INFO_PTR_TO_STRUCT(bh_info)));
// The payload of the BLACKHOLE points to the TSO
- ((StgInd *)bh)->indirectee = (StgClosure *)tso;
+ RELEASE_STORE(&((StgInd *)bh)->indirectee, (StgClosure *)tso);
SET_INFO_RELEASE(bh,&stg_BLACKHOLE_info);
// .. and we need a write barrier, since we just mutated the closure:
=====================================
rts/Threads.c
=====================================
@@ -437,7 +437,7 @@ checkBlockingQueues (Capability *cap, StgTSO *tso)
p = UNTAG_CLOSURE(bq->bh);
const StgInfoTable *pinfo = ACQUIRE_LOAD(&p->header.info);
if (pinfo != &stg_BLACKHOLE_info ||
- ((StgInd *)p)->indirectee != (StgClosure*)bq)
+ (RELAXED_LOAD(&((StgInd *)p)->indirectee) != (StgClosure*)bq))
{
wakeBlockingQueue(cap,bq);
}
@@ -468,7 +468,7 @@ updateThunk (Capability *cap, StgTSO *tso, StgClosure *thunk, StgClosure *val)
return;
}
- v = UNTAG_CLOSURE(((StgInd*)thunk)->indirectee);
+ v = UNTAG_CLOSURE(ACQUIRE_LOAD(&((StgInd*)thunk)->indirectee));
updateWithIndirection(cap, thunk, val);
@@ -808,7 +808,7 @@ loop:
qinfo = ACQUIRE_LOAD(&q->header.info);
if (qinfo == &stg_IND_info ||
qinfo == &stg_MSG_NULL_info) {
- q = (StgMVarTSOQueue*)((StgInd*)q)->indirectee;
+ q = (StgMVarTSOQueue*) ACQUIRE_LOAD(&((StgInd*)q)->indirectee);
goto loop;
}
=====================================
rts/Updates.cmm
=====================================
@@ -59,7 +59,7 @@ INFO_TABLE_RET ( stg_marked_upd_frame, UPDATE_FRAME,
ASSERT(HpAlloc == 0); // Note [HpAlloc]
// we know the closure is a BLACKHOLE
- v = StgInd_indirectee(updatee);
+ v = %acquire StgInd_indirectee(updatee);
if (GETTAG(v) != 0) (likely: False) {
// updated by someone else: discard our value and use the
=====================================
rts/Updates.h
=====================================
@@ -59,8 +59,8 @@
} \
\
OVERWRITING_CLOSURE(p1); \
- %relaxed StgInd_indirectee(p1) = p2; \
- SET_INFO_RELEASE(p1, stg_BLACKHOLE_info); \
+ %release StgInd_indirectee(p1) = p2; \
+ %release SET_INFO(p1, stg_BLACKHOLE_info); \
LDV_RECORD_CREATE(p1); \
and_then;
@@ -76,9 +76,9 @@ INLINE_HEADER void updateWithIndirection (Capability *cap,
/* See Note [Heap memory barriers] in SMP.h */
bdescr *bd = Bdescr((StgPtr)p1);
if (bd->gen_no != 0) {
- IF_NONMOVING_WRITE_BARRIER_ENABLED {
- updateRemembSetPushThunk(cap, (StgThunk*)p1);
- }
+ IF_NONMOVING_WRITE_BARRIER_ENABLED {
+ updateRemembSetPushThunk(cap, (StgThunk*)p1);
+ }
recordMutableCap(p1, cap, bd->gen_no);
TICK_UPD_OLD_IND();
} else {
=====================================
rts/include/Cmm.h
=====================================
@@ -309,7 +309,7 @@
#define ENTER(x) ENTER_(return,x)
#endif
-#define ENTER_R1() ENTER_(RET_R1,R1)
+#define ENTER_R1() P_ _r1; _r1 = R1; ENTER_(RET_R1, _r1)
#define RET_R1(x) jump %ENTRY_CODE(Sp(0)) [R1]
@@ -324,7 +324,7 @@
IND, \
IND_STATIC: \
{ \
- x = StgInd_indirectee(x); \
+ x = %acquire StgInd_indirectee(x); \
goto again; \
} \
case \
=====================================
rts/sm/Evac.c
=====================================
@@ -1543,7 +1543,7 @@ selector_loop:
bale_out:
// We didn't manage to evaluate this thunk; restore the old info
// pointer. But don't forget: we still need to evacuate the thunk itself.
- SET_INFO((StgClosure *)p, (const StgInfoTable *)info_ptr);
+ SET_INFO_RELAXED((StgClosure *)p, (const StgInfoTable *)info_ptr);
// THREADED_RTS: we just unlocked the thunk, so another thread
// might get in and update it. copy() will lock it again and
// check whether it was updated in the meantime.
=====================================
rts/sm/NonMovingMark.c
=====================================
@@ -688,8 +688,9 @@ void updateRemembSetPushThunkEager(Capability *cap,
case IND:
{
StgInd *ind = (StgInd *) thunk;
- if (check_in_nonmoving_heap(ind->indirectee)) {
- push_closure(queue, ind->indirectee, NULL);
+ StgClosure *indirectee = ACQUIRE_LOAD(&ind->indirectee);
+ if (check_in_nonmoving_heap(indirectee)) {
+ push_closure(queue, indirectee, NULL);
}
break;
}
@@ -1587,7 +1588,7 @@ mark_closure (MarkQueue *queue, const StgClosure *p0, StgClosure **origin)
// Synchronizes with the release-store in updateWithIndirection.
// See Note [Heap memory barriers] in SMP.h.
StgInd *ind = (StgInd *) p;
- ACQUIRE_FENCE();
+ ACQUIRE_FENCE_ON(&p->header.info);
StgClosure *indirectee = RELAXED_LOAD(&ind->indirectee);
markQueuePushClosure(queue, indirectee, &ind->indirectee);
if (GET_CLOSURE_TAG(indirectee) == 0 || origin == NULL) {
=====================================
rts/sm/Storage.c
=====================================
@@ -569,8 +569,6 @@ lockCAF (StgRegTable *reg, StgIndStatic *caf)
bh->indirectee = (StgClosure *)cap->r.rCurrentTSO;
SET_HDR(bh, &stg_CAF_BLACKHOLE_info, caf->header.prof.ccs);
- // RELEASE ordering to ensure that above writes are visible before we
- // introduce reference as CAF indirectee.
RELEASE_STORE(&caf->indirectee, (StgClosure *) bh);
SET_INFO_RELEASE((StgClosure*)caf, &stg_IND_STATIC_info);
=====================================
utils/genapply/Main.hs
=====================================
@@ -785,7 +785,11 @@ genApply regstatus args =
text "case IND,",
text " IND_STATIC: {",
nest 4 (vcat [
- text "R1 = StgInd_indirectee(R1);",
+ -- N.B. annoyingly the %acquire syntax must place its result in a local register
+ -- as it is a Cmm prim call node.
+ text "P_ p;",
+ text "p = %acquire StgInd_indirectee(R1);",
+ text "R1 = p;",
-- An indirection node might contain a tagged pointer
text "goto again;"
]),
View it on GitLab: https://gitlab.haskell.org/ghc/ghc/-/commit/a9489a220c7ad78599f96bf03d9b1b73d82cba14
--
View it on GitLab: https://gitlab.haskell.org/ghc/ghc/-/commit/a9489a220c7ad78599f96bf03d9b1b73d82cba14
You're receiving this email because of your account on gitlab.haskell.org.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.haskell.org/pipermail/ghc-commits/attachments/20231213/0c97313b/attachment-0001.html>
More information about the ghc-commits
mailing list