Rodrigo Mesquita pushed to branch wip/romes/27729 at Glasgow Haskell Compiler / GHC Commits: 9dfc886c by Rodrigo Mesquita at 2026-08-25T10:36:32+01:00 rts: Fix race condition in MSG_UPD_TSO_FLAGS execution The code for processing the MSG_UPD_TSO_FLAGS message was not taking into consideration that the TSO's owner might have moved in between that capability receiving the message (since it was its previous owner) and starting to process its inbox (a point at which it was no longer the owner) Added Note [TSO owner may change in between Msg being sent and received] to explain this race and the pattern used to fix this, where we just forward the message to the new owner. Fixes #27729 - - - - - 4 changed files: - rts/CloneStack.c - rts/Messages.c - rts/Threads.c - rts/Threads.h Changes: ===================================== rts/CloneStack.c ===================================== @@ -88,6 +88,7 @@ void sendCloneStackMessage(StgTSO *tso, HsStablePtr mvar) { void handleCloneStackMessage(Capability *cap, MessageCloneStack *msg){ // We must check that the current owner of the thread we want to clone the stack for // is still this capability. + // See Note [TSO owner may change in between Msg being sent and received] Capability *owner = RELAXED_LOAD(&msg->tso->cap); if (owner != cap) { // The target TSO may have migrated after the message was queued on the old ===================================== rts/Messages.c ===================================== @@ -66,6 +66,62 @@ void sendMessage(Capability *from_cap, Capability *to_cap, Message *msg) Handle a message ------------------------------------------------------------------------- */ +/* +Note [TSO owner may change in between Msg being sent and received] +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +When a message is sent from Capability (C1) to a target TSO (T2) (e.g. +MessageUpdTSOFlag, MessageCloneStack, ...), it is queued on the TSO's owner +Capability (C3) inbox (inboxes are owned by Capabilities, not TSOs). + +At a later point, the Capability (C3) will process its inbox. Upon receiving +the message meant for a specific TSO (T2), it must first always check that the +TSO's owner is *still* itself (C3). + +The target TSO (T2) may have migrated after the message was queued on its old +capability (C3). In that case we must forward the request to the new owner +(say, C4); otherwise the Capability C3 could be modifying a TSO it no longer +owns, racing with its actual owner mutating it, since it is no longer the owner. + +The message meant for a TSO should only be executed when the receiving +Capability is still the owner of that TSO. Otherwise, it must be forwarded to +the new owner. + +The general pattern is one where there's a top-level function which assumes it +can be called by capabilities other than the TSO's owner. The function checks +whether the current capability is the TSO owner. If yes, execute the action. If +not, then it sends a message to the current TSO's owner. On receiving the +message, the new capability will just call that top-level function, which will +ensure the message is forwarded again if the TSO owner changed. +It will look something like: + + runMyMsg(Capability *from, StgTSO *target, ...) { + +#if defined(THREADED_RTS) + Capability *owner = RELAXED_LOAD(&target->cap) + if (owner != from) { + MessageMyMsg* msg = ... + sendMessage(cap, owner, msg) + return + } +#endif + + actuallyDoTheWork(...) + } + + executeMessage(...) { + + if (i == &stg_MY_MSG_info) { + + MessageMyMsg* msg = (MessageMyMsg*) m + runMyMsg(cap, m->tso, ...) + + } + } + +See example `updThreadFlag` and `executeMessage`'s `stg_MSG_UPD_TSO_FLAG_info`, +or `tryWakeUpThread` and `stg_MSG_TRY_WAKEUP_info` for two live examples. +*/ + #if defined(THREADED_RTS) void @@ -142,13 +198,9 @@ loop: } else if(i == &stg_MSG_UPD_TSO_FLAG_info){ MessageUpdTSOFlag *u = (MessageUpdTSOFlag*) m; - if (u->set) { - u->tso->flags |= u->flag; - } - else { - u->tso->flags &= ~u->flag; - } - return; + + StgTSO *tso = RELAXED_LOAD(&u->tso); + updThreadFlag(cap, tso, u->flag, u->set); } else { ===================================== rts/Threads.c ===================================== @@ -379,23 +379,25 @@ migrateThread (Capability *from, StgTSO *tso, Capability *to) sets or unsets a flag in a given TSO ------------------------------------------------------------------------- */ -static void -updThreadFlag(Capability *from, StgTSO *tso, StgWord32 flag, StgBool set); - void setThreadFlag(Capability *from, StgTSO *tso, StgWord32 flag) { - updThreadFlag(from, tso, flag, 1); + updThreadFlag(from, tso, flag, true); } void unsetThreadFlag(Capability *from, StgTSO *tso, StgWord32 flag) { - updThreadFlag(from, tso, flag, 0); + updThreadFlag(from, tso, flag, false); } -static void -updThreadFlag(Capability *from, StgTSO *tso, StgWord32 flag, StgBool set /* true=set, false=unset */) +void +updThreadFlag(Capability *from USED_IF_THREADS, StgTSO *tso, StgWord32 flag, StgBool set /* true=set, false=unset */) { #if defined(THREADED_RTS) + // If we're the current owner of the thread we want to modify, do it. + // Otherwise, we must forward the message to the actual owner. + // When executing the upd message, we check again that we're still the TSO + // owner (which may have changed since the message was queued on this cap.) + // See Note [TSO owner may change in between Msg being sent and received] Capability *tso_owner = RELAXED_LOAD(&tso->cap); if (from != tso_owner) { MessageUpdTSOFlag *msg; @@ -407,8 +409,6 @@ updThreadFlag(Capability *from, StgTSO *tso, StgWord32 flag, StgBool set /* true sendMessage(from, tso_owner, (Message*)msg); return; } -#else - (void)from; // unused in non-threaded case #endif if (set) { ===================================== rts/Threads.h ===================================== @@ -21,6 +21,7 @@ void migrateThread (Capability *from, StgTSO *tso, Capability *to); void setThreadFlag (Capability *from, StgTSO *tso, StgWord32 flag); void unsetThreadFlag (Capability *from, StgTSO *tso, StgWord32 flag); +void updThreadFlag (Capability *from, StgTSO *tso, StgWord32 flag, StgBool set); // Wakes up a thread on a Capability (probably a different Capability // from the one held by the current Task). View it on GitLab: https://gitlab.haskell.org/ghc/ghc/-/commit/9dfc886ce2ae090792614ceb5b88e9a9... -- View it on GitLab: https://gitlab.haskell.org/ghc/ghc/-/commit/9dfc886ce2ae090792614ceb5b88e9a9... You're receiving this email because of your account on gitlab.haskell.org. Manage all notifications: https://gitlab.haskell.org/-/profile/notifications | Help: https://gitlab.haskell.org/help