Duncan Coutts pushed to branch wip/dcoutts/capability-yield at Glasgow Haskell Compiler / GHC Commits: be623efb by Duncan Coutts at 2026-08-20T00:20:01+01:00 Eliminate a use of releaseCapability_ with always_wakeup This one was purely artificial, just due to the unnecessarily strong pre-condition. We can just weaken the precondition. The capability inbox is non-empty so releaseCapability_ will certainly wake up a task for the capability anyway. We are trying to eliminate the always_wakeup parameter entirely since it is a bit of a design wart. - - - - - 5a0f5849 by Duncan Coutts at 2026-08-20T00:21:23+01:00 Eliminate another use of releaseCapability_ with always_wakeup Previusly in schedulePushWork, it checked if there are sparks for the capability and called releaseAndWakeupCapability if there were and releaseCapability if there were none. This is unnecessary: releaseCapability already ensures that a task will be worken if there are sparks available for the capability. This also lets us remove the now unused releaseAndWakeupCapability, eliminating another use of always_wakeup==true. - - - - - 29ed4da7 by Duncan Coutts at 2026-08-24T12:21:45+01:00 Make prodCapability reliable, fix race condition Also eliminate the last use of releaseCapability_ using the always_wakeup param. Add a Note that describes the problem and solution. Now that prodCapability also does an interruptCapability (if the capability is active) then we don't need to use interruptCapability as well at call sites of prodCapability. - - - - - 9a4e2330 by Duncan Coutts at 2026-08-24T12:24:23+01:00 Eliminate the now-unused always_wakeup param from releaseCapability_ releaseCapability_ had an extra bool param: always_wakeup. This was rather a design wart. We have now eliminated all uses of it so we can remove the param entirely. This will also reduce churn at call site when we add a new (rarely used) parameter in the subsequent commit. - - - - - 65241766 by Duncan Coutts at 2026-08-24T12:41:33+01:00 Add releaseCapability_ worker with a wakeup_worker modifier Split releaseCapability_ into a worker and wrapper. The worker gains the extra wakeup_worker parameter. Document within releaseCapability__ the basic approach of looking for a series of conditions in priority order and acting on them. Then add a modifier, wakeup_worker and explain it in similar terms. What it does is skip two of the conditions in the priority list, with the effect that we prioritise waking up a worker task over a returning task or bound task. This feature is not yet used in this commit, but it will be used as part of a scheme to allow in-RTS I/O managers in the threaded RTS. This scheme will make use of being able to start a background worker thread, and that will use this feature to start it promptly. Also correct the yieldCapability docs to cover all the conditions, and in priority order for consistency. - - - - - 3428302b by Duncan Coutts at 2026-08-24T12:45:07+01:00 Move enqueueWorker next to where it is used. It's not general purpose at all. It's very specifically crafted to work with it's only caller: yieldCapability. It does very suprising things like releaseCapability_, release locks and terminate threads. This logic would be much clearer if done within yieldCapability. - - - - - efe433ca by Duncan Coutts at 2026-08-24T12:47:14+01:00 Move code out of enqueueWorker and into releaseCapability_ Instead of directly releasing locks and terminating tasks, have it return whether the enqueue was successful or not. In the latter case, releaseCapability_ itself will release locks and terminate the task. This makes the logic of releaseCapability_ a lot clearer. Fiddling with tasks is what releaseCapability_ does, so it's better not to try and encapsulate this within a helper function. - - - - - e0e3f390 by Duncan Coutts at 2026-08-24T12:47:19+01:00 Clarify the logic and control flow in yieldCapability yieldCapability is unfortunately a bit complicated. This change restructures things slightly but should keep the behaviour the same. Previously after calling releaseCapability_ we had a bunch of alternatives, where in each branch we would use RELEASE_LOCK(cap->lock) and do various things before/after the lock is released. This was a bit hard to follow, or to extend (which we need to do). So now we have unconditional acquire and release of the cap->lock, so it's clear where that happens, with releaseCapability_ in between. Then in between these steps we have the various other pre/post actions. Some before releaseCapability_, some after while holing the lock, and some after having released the lock. We explain this structure in a longer comment, and refer back to the structure from the code. - - - - - 6 changed files: - rts/Capability.c - rts/Capability.h - rts/Messages.c - rts/RtsAPI.c - rts/Schedule.c - rts/sm/GC.c Changes: ===================================== rts/Capability.c ===================================== @@ -560,7 +560,7 @@ giveCapabilityToTask (Capability *cap USED_IF_DEBUG, Task *task) #endif /* ---------------------------------------------------------------------------- - * releaseCapability_ + * releaseCapability and releaseCapability_ * * This serves two purposes: * @@ -570,32 +570,37 @@ giveCapabilityToTask (Capability *cap USED_IF_DEBUG, Task *task) * * 2. There is no current task (cap->task == NULL), and thus the Capability * is idle, and we want to wake up an idle Task to animate the Capability. - * In this case set always_wakeup. See also prodCapability. + * See also prodCapability. * - * Setting the always_wakeup parameter (almost) ensures that the capability is - * not left idle: even if there is no known work to do, the capability will be - * given to a worker task. There are two exceptions to this: - * 1. if there is a pending sync then the capability is left idle, but in - * anticipation of whichever task initiated the sync picking it up shortly. - * 2. if the scheduler is shutting down and there are no threads on the run - * queue and there are no spare workers then the capability is left idle. - * It is not entirely clear if this corner case is intentional. - * - * The caller must hold cap->lock and will still hold it after the call returns. + * Difference: + * - releaseCapability the caller /must not/ hold cap->lock. + * - releaseCapability_ the caller /must/ hold cap->lock. * * N.B. May need to take all_tasks_mutex, if it needs to start a new task. * * ------------------------------------------------------------------------- */ #if defined(THREADED_RTS) -void -releaseCapability_ (Capability* cap, - bool always_wakeup) +static void releaseCapability__ (Capability* cap, bool wakeup_worker); + +void releaseCapability (Capability* cap) +{ + ACQUIRE_LOCK(&cap->lock); + releaseCapability__(cap, false /*wakeup_worker*/); + RELEASE_LOCK(&cap->lock); +} + +void releaseCapability_ (Capability* cap) +{ + releaseCapability__(cap, false /*wakeup_worker*/); +} + +static void releaseCapability__ (Capability* cap, + bool wakeup_worker) { { Task *task = cap->running_task; - ASSERT(task || always_wakeup); // To cover purpose 2 above, we allow the cap->running_task to be // NULL, to handle cases where a thread (that is not itself a Task) // needs to wake up an idle task for the capability. @@ -609,6 +614,33 @@ releaseCapability_ (Capability* cap, // Remove the current Task owning the Capability (if any, see purpose 2). RELAXED_STORE(&cap->running_task, NULL); + // We now look for a task to give the capability to, or otherwise we leave + // the capability free. + // + // We take one of these guarded actions, in priority order: + // + // 1. If there's a pending synchronisation of all capabilities (e.g. GC), + // then give the capability to the task performing the sync. + // 2. If there's a task returning (e.g. from safe FFI) on this capability, + // then give the capability to the first such task. + // 3. If the next runnable thread on this capability is a bound thread, + // then give the capability to the corresponding bound task. + // 4. If there are no spare worker tasks for this capability, + // then start one and give the capability to the new task. + // 5. If there is some work to do on this capability (e.g. runnable thread), + // then give the capability to a worker task. + // 6. Otherwise leave the capability free/idle. + // + // There is one modifier to this priority list: + // + // * Setting wakeup_worker skips cases 2 & 3. This prioritises waking a + // worker over returning tasks or bound tasks. + + + // Guarded action 1: + // If there's a pending synchronisation of all capabilities (e.g. GC), + // then give the capability to the task performing the sync. + // // If there is a pending sync, the task that requested the sync will // subsequently use acquireAllCapabilities to place itself on the (front of // the) returning_task list (of all capabilities). We will then be in one @@ -656,17 +688,22 @@ releaseCapability_ (Capability* cap, return; } - // Check to see whether a worker thread can be given - // the go-ahead to return the result of an external call.. - if (cap->n_returning_tasks != 0) { + // Skip guarded actions 2 & 3 if wakeup_worker. See the list of actions and + // modifiers above. + + // Guarded action 2: + // If there's a task returning (e.g. from safe FFI) on this capability, + // then give the capability to the first such task. + if (!wakeup_worker && cap->n_returning_tasks != 0) { giveCapabilityToTask(cap,cap->returning_tasks_hd); // The Task pops itself from the queue (see waitForCapability()) return; } - // If the next thread on the run queue is a bound thread, - // give this Capability to the appropriate Task. - if (!emptyRunQueue(cap) && peekRunQueue(cap)->bound) { + // Guarded action 3: + // If the next runnable thread on this capability is a bound thread, + // then give the capability to the bound thread's corresponding task. + if (!wakeup_worker && !emptyRunQueue(cap) && peekRunQueue(cap)->bound) { // Make sure we're not about to try to wake ourselves up // ASSERT(task != cap->run_queue_hd->bound); // assertion is false: in schedule() we force a yield after @@ -677,11 +714,13 @@ releaseCapability_ (Capability* cap, return; } + // Guarded action 4: + // If there are no spare worker tasks for this capability, + // then start one and give the capability to the new task. if (!cap->spare_workers) { - // Create a worker thread if we don't have one. If the system - // is interrupted, we only create a worker task if there - // are threads that need to be completed. If the system is - // shutting down, we never create a new worker. + // If the system is interrupted, we only create a worker task if there + // are threads that need to be completed. If the system is shutting + // down, we never create a new worker. if (getSchedState() < SCHED_SHUTTING_DOWN || !emptyRunQueue(cap)) { debugTrace(DEBUG_sched, "starting new worker on capability %d", cap->no); @@ -690,10 +729,14 @@ releaseCapability_ (Capability* cap, } } - // If we have an unbound thread on the run queue, or if there's - // anything else to do, give the Capability to a worker thread. - if (always_wakeup || - !emptyRunQueue(cap) || !emptyInbox(cap) || + // Guarded action 5: + // If there is some work to do on this capability (e.g. runnable thread), + // then give the capability to a worker task. + // + // We also check the cap->interrupt flag to avoid a race condition. + // See Note [prodCapability reliability]. + // + if (!emptyRunQueue(cap) || !emptyInbox(cap) || cap->interrupt || (!cap->disabled && !emptySparkPoolCap(cap)) || globalWorkToDo()) { if (cap->spare_workers) { giveCapabilityToTask(cap, cap->spare_workers); @@ -702,59 +745,14 @@ releaseCapability_ (Capability* cap, } } + // Guarded action 6: + // Otherwise leave the capability free/idle. #if defined(PROFILING) cap->r.rCCCS = CCS_IDLE; #endif RELAXED_STORE(&last_free_capability[cap->node], cap); debugTrace(DEBUG_sched, "freeing capability %d", cap->no); } - -void -releaseCapability (Capability* cap) -{ - ACQUIRE_LOCK(&cap->lock); - releaseCapability_(cap, false); - RELEASE_LOCK(&cap->lock); -} - -void -releaseAndWakeupCapability (Capability* cap) -{ - ACQUIRE_LOCK(&cap->lock); - releaseCapability_(cap, true); - RELEASE_LOCK(&cap->lock); -} - -static void -enqueueWorker (Capability* cap) -{ - Task *task; - - task = cap->running_task; - - // If the Task is stopped, we shouldn't be yielding, we should - // be just exiting. - ASSERT(!task->stopped); - ASSERT(task->worker); - - if (cap->n_spare_workers < MAX_SPARE_WORKERS) - { - task->next = cap->spare_workers; - cap->spare_workers = task; - cap->n_spare_workers++; - } - else - { - debugTrace(DEBUG_sched, "%d spare workers already, exiting", - cap->n_spare_workers); - releaseCapability_(cap,false); - // hold the lock until after workerTaskStop; c.f. scheduleWorker() - workerTaskStop(task); - RELEASE_LOCK(&cap->lock); - shutdownThread(); - } -} - #endif /* @@ -1051,12 +1049,6 @@ static void waitForCapability_ (Task *task, * when either we know that the Capability should be given to another Task, or * there is nothing to do right now. One of the following is true: * - * - The current Task is a worker, and there's a bound thread at the head of - * the run queue (or vice versa) - * - * - The run queue is empty. We'll be woken up again when there's work to - * do. - * * - Another Task is trying to do parallel GC (pending_sync == SYNC_GC_PAR). * We should become a GC worker for a while. * @@ -1064,12 +1056,21 @@ static void waitForCapability_ (Task *task, * SYNC_GC_PAR), either to do a sequential GC, forkProcess, or * setNumCapabilities. We should give up the Capability temporarily. * + * - There is a Task returning from a safe FFI call. + * + * - The current Task is a worker, and there's a bound thread at the head of + * the run queue (or vice versa) + * + * - There is no work to do (empty run queue, inbox etc). We'll be woken up + * again when there's work to do. + * * When yieldCapability returns *pCap will have been updated to the new * capability held by the caller. * * ------------------------------------------------------------------------- */ #if defined(THREADED_RTS) +static bool tryEnqueueWorker (Capability* cap); /* See Note [GC livelock] in Schedule.c for why we have gcAllowed and return the bool */ @@ -1124,28 +1125,61 @@ yieldCapability // We must now release the capability and wait to be woken up again. task->wakeup = false; + // What happens next is a bit complicated. It has the following outline: + // + // 1. take the cap->lock + // 2. "various stuff part A", pre-releaseCapability_ holding cap->lock + // 3. release the capability + // 4. "various stuff part B", post-releaseCapability_ holding cap->lock + // 5. release the cap->lock + // 6. "various stuff part C", post release cap->lock + // + // Much of the "various stuff" is also conditional, which complicates + // matters further. To try and maintain clarity we use the following + // variables in the conditions for the in-between steps. + // + bool terminate_worker = false; + bool task_is_worker = isWorker(task); + bool task_is_bound = isBoundTask(task); + + // Step 1: take the cap->lock ACQUIRE_LOCK(&cap->lock); - // If this is a worker thread, put it on the spare_workers queue - if (isWorker(task)) { - enqueueWorker(cap); + // Step 2: "various stuff part A", pre-releaseCapability_ holding cap->lock + if (task_is_worker) { + // If this is a worker thread, try to put it on the spare_workers + // queue or if it is surplus then we will terminate it. + terminate_worker = !tryEnqueueWorker(cap); } - releaseCapability_(cap, false); + // Step 3: release the capability + releaseCapability_(cap); - if (isWorker(task) || isBoundTask(task)) { - RELEASE_LOCK(&cap->lock); - cap = waitForWorkerCapability(task); - } else { + // Step 4: "various stuff part B", post-releaseCapability_ holding cap->lock + if (terminate_worker) { + // hold the lock until after workerTaskStop; c.f. scheduleWorker() + workerTaskStop(task); + } else if (!task_is_worker && !task_is_bound) { // Not a worker Task, or a bound Task. The only way we can be woken up // again is to put ourselves on the returning_tasks queue, so that's - // what we do. We still hold cap->lock at this point - // The Task waiting for this Capability does not have it - // yet, so we can be sure to be woken up later. (see #10545) + // what we do. We still hold cap->lock at this point. The Task waiting + // for this Capability does not have it yet, so we can be sure to be + // woken up later. (see #10545) appendToReturningTaskQueue(cap,task); - RELEASE_LOCK(&cap->lock); + } + + // Step 5: release the cap->lock + RELEASE_LOCK(&cap->lock); + + // Step 6. "various stuff part C", post release cap->lock + if (terminate_worker) { + shutdownThread(); + } else if (task_is_worker || task_is_bound) { + cap = waitForWorkerCapability(task); + } else { cap = waitForReturnCapability(task); } + // End of step 6. debugTrace(DEBUG_sched, "resuming capability %d", cap->no); ASSERT(cap->running_task == task); @@ -1161,6 +1195,33 @@ yieldCapability return false; } +// Returns true if it could enqueue, and false if the worker is surplus to +// requirements and should be terminated. +static bool tryEnqueueWorker (Capability* cap) +{ + Task *task = cap->running_task; + + // If the Task is stopped, we shouldn't be yielding, we should + // be just exiting. + ASSERT(!task->stopped); + ASSERT(task->worker); + ASSERT_LOCK_HELD(&cap->lock); + + if (cap->n_spare_workers < MAX_SPARE_WORKERS) + { + task->next = cap->spare_workers; + cap->spare_workers = task; + cap->n_spare_workers++; + return true; + } + else + { + debugTrace(DEBUG_sched, "%d spare workers already, exiting", + cap->n_spare_workers); + return false; + } +} + #endif /* THREADED_RTS */ @@ -1391,8 +1452,40 @@ void releaseAllCapabilities(uint32_t n, Capability *keep_cap, Task *task) /* ---------------------------------------------------------------------------- * prodCapability * - * If a Capability is currently idle, wake up a Task on it. Used to - * get every Capability into the GC. + * If a Capability is currently idle, wake up a Task on it. If it is not idle, + * interrupt it. + * + * Used to get every Capability into the GC. Also used for ctl-C handling + * to get the capability to run the scheduler. + * + * Note [prodCapability reliability] + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + * + * There's a potential race condition with prodCapability: a task running a + * capability may be just about to yield when it is prodded and then let the + * capability go idle, thus missing the prod. To avoid this we must check if + * the capability has been prodded in a reliable fashion when the task is + * yielding the capability. This is much like the issue of the race between + * sending a capability a message and the capability going idle (where it's + * vital that we don't let a capability go idle if there's a pending message). + * The solution we use is much the same as the solution for messages: rely on + * a shared variable set and tested while holding the cap->lock. + * + * The scheme is as follows: + * 1. Set the cap->interrupt flag in prodCapability while holding the cap->lock + * 2. Test the cap->interrupt flag in releaseCapability_ while holding the + * cap->lock. If the flag is set then make sure to pass the capability to + * a worker task (which often would be the task that was just releasing it). + * 3. Make sure to reset the cap->interrupt flag at the start of the scheduler + * loop. (Historically it was only reset when running a Haskell thread.) + * We must do this before the scheduler yields again or we could loop + * indefinitely. + * + * This scheme ensures that we run the scheduler loop once more, which will + * react to the prod or reset the flag and yield again. In particular for + * getting tasks into GC they will do that in yieldCapability, and for ctl-c + * the scheduler will asks the I/O manager to poll for events which will pick + * up pending signals. * ------------------------------------------------------------------------- */ #if defined(THREADED_RTS) @@ -1401,9 +1494,22 @@ void prodCapability (Capability *cap) { ACQUIRE_LOCK(&cap->lock); - if (!cap->running_task) { - releaseCapability_(cap,true); + if (cap->running_task) { + /* Set the cap->interrupt so that the capability will not go idle + * before attending to the reason for the interrupt. + * See Note [prodCapability reliability]. + */ + interruptCapability(cap); + } else { + /* Set the cap->interrupt first so that releaseCapability_ will see + * that there is something to do on this capability and ensure the + * cap is given to a task. See Note [prodCapability reliability]. + */ + interruptCapability(cap); + releaseCapability_(cap); } + /* Notice that we use interruptCapability either way, but for different + * reasons */ RELEASE_LOCK(&cap->lock); } @@ -1523,7 +1629,7 @@ shutdownCapability (Capability *cap USED_IF_THREADS, if (!emptyRunQueue(cap) || cap->spare_workers) { debugTrace(DEBUG_sched, "runnable threads or workers still alive, yielding"); - releaseCapability_(cap,false); // this will wake up a worker + releaseCapability_(cap); // this will wake up a worker RELEASE_LOCK(&cap->lock); yieldThread(); continue; ===================================== rts/Capability.h ===================================== @@ -262,16 +262,13 @@ void moreCapabilities (uint32_t from, uint32_t to); // ASSUMES: cap->running_task is the current Task. // #if defined(THREADED_RTS) -void releaseCapability (Capability* cap); -void releaseAndWakeupCapability (Capability* cap); -void releaseCapability_ (Capability* cap, bool always_wakeup); +void releaseCapability (Capability* cap); +void releaseCapability_ (Capability* cap); // assumes cap->lock is held #else // releaseCapability() is empty in non-threaded RTS INLINE_HEADER void releaseCapability (Capability* cap STG_UNUSED) {}; -INLINE_HEADER void releaseAndWakeupCapability (Capability* cap STG_UNUSED) {}; -INLINE_HEADER void releaseCapability_ (Capability* cap STG_UNUSED, - bool always_wakeup STG_UNUSED) {}; +INLINE_HEADER void releaseCapability_ (Capability* cap STG_UNUSED) {}; #endif // declared in rts/include/rts/Threads.h: ===================================== rts/Messages.c ===================================== @@ -49,11 +49,7 @@ void sendMessage(Capability *from_cap, Capability *to_cap, Message *msg) recordClosureMutated(from_cap,(StgClosure*)msg); if (to_cap->running_task == NULL) { - /* Precond for releaseCapability_ is: running_task || always_wakeup. - * We have running_task == NULL, hence we must use always_wakeup. This - * is ok since the inbox is now non-empty, so we wake a task anyway. - */ - releaseCapability_(to_cap, true /*always_wakeup*/); + releaseCapability_(to_cap); } else { interruptCapability(to_cap); } ===================================== rts/RtsAPI.c ===================================== @@ -691,7 +691,7 @@ rts_unlock (Capability *cap) // random point in the future, which causes problems for // freeTaskManager(). ACQUIRE_LOCK(&cap->lock); - releaseCapability_(cap,false); + releaseCapability_(cap); // Finally, we can release the Task to the free list. exitMyTask(); ===================================== rts/Schedule.c ===================================== @@ -283,6 +283,10 @@ schedule (Capability *initialCapability, Task *task) barf("sched_state: %" FMT_Word, sched_state); } + // Reset the interrupt flag upon starting the scheduler loop. + // See Note [prodCapability reliability]. + RELAXED_STORE(&cap->interrupt, false); + scheduleFindWork(&cap); /* work pushing, currently relevant only for THREADED_RTS: @@ -430,9 +434,6 @@ run_thread: SetLastError(t->saved_winerror); #endif - // reset the interrupt flag before running Haskell code - RELAXED_STORE(&cap->interrupt, false); - cap->in_haskell = true; RELAXED_STORE(&cap->idle, false); @@ -837,13 +838,9 @@ schedulePushWork(Capability *cap USED_IF_THREADS, // release the capabilities for (i = 0; i < n_free_caps; i++) { task->cap = free_caps[i]; - if (sparkPoolSizeCap(cap) > 0) { - // If we have sparks to steal, wake up a worker on the - // capability, even if it has no threads to run. - releaseAndWakeupCapability(free_caps[i]); - } else { - releaseCapability(free_caps[i]); - } + // If there are sparks available, this will wake up a Task to run + // the Capability, even if it has no threads to run. + releaseCapability(free_caps[i]); } } task->cap = cap; // reset to point to our Capability. @@ -1905,7 +1902,7 @@ forkProcess(HsStablePtr *entry #endif for (i=0; i < n_capabilities; i++) { - releaseCapability_(getCapability(i),false); + releaseCapability_(getCapability(i)); RELEASE_LOCK(&getCapability(i)->lock); } @@ -2320,7 +2317,7 @@ suspendThread (StgRegTable *reg, bool interruptible) suspendTask(cap,task); cap->in_haskell = false; - releaseCapability_(cap,false); + releaseCapability_(cap); RELEASE_LOCK(&cap->lock); @@ -2513,7 +2510,7 @@ void scheduleWorker (Capability *cap, Task *task) // Capability has been shut down. // ACQUIRE_LOCK(&cap->lock); - releaseCapability_(cap,false); + releaseCapability_(cap); workerTaskStop(task); RELEASE_LOCK(&cap->lock); } ===================================== rts/sm/GC.c ===================================== @@ -1523,7 +1523,6 @@ waitForGcThreads (Capability *cap, bool idle_cap[]) if (i == me || idle_cap[i]) { continue; } if (SEQ_CST_LOAD(&gc_threads[i]->wakeup) != GC_THREAD_STANDING_BY) { prodCapability(getCapability(i)); - interruptCapability(getCapability(i)); } } // this 1ms timeout is not well justified. It's the shortest timeout we View it on GitLab: https://gitlab.haskell.org/ghc/ghc/-/compare/9cfe2ee2cb65cc5d63ce6dbc9789297... -- View it on GitLab: https://gitlab.haskell.org/ghc/ghc/-/compare/9cfe2ee2cb65cc5d63ce6dbc9789297... 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