[Git][ghc/ghc][wip/dcoutts/issue-27105-stopTicker] 8 commits: posix ticker: split out ppoll/select helper functions

Duncan Coutts pushed to branch wip/dcoutts/issue-27105-stopTicker at Glasgow Haskell Compiler / GHC Commits: 7bcd9b93 by Duncan Coutts at 2026-06-13T21:56:25+01:00 posix ticker: split out ppoll/select helper functions Move the #ifdefs out of the main code body by introducing local helper functions and types, which themselves have two implementations (with a common API) based on ppoll or select. This helps improve clarity/readability. - - - - - de07e21c by Duncan Coutts at 2026-06-13T21:56:25+01:00 posix ticker: improve the implementation The existing implementation supported pausing and exiting, with the implementation of pausing reling on a mutex and condition variable. It needed to check the pause and stop shared variables on every iteration. It relies on ppoll or select, to wait on the timeout and also wait on an interrupt fd. The interrupt fd was only used for prompt exit/shutdown, and not for pausing or other notification. The pause only needed a lock and a memory operation, but the pause was not prompt. The resume used a lock, and signaling a cond var. The new implementation uses a somewhat more regular design: every notification is done by setting a shared variable and interrupting/notifying the ticker via the fd. The ticker thread does not need to check any shared variables on normal timer expiry, only when it recevies notification. This may be a micro-optimisation, but the tick occurs 100 times a second by default so any improvements in the hot path are amplified. When the ticker thread does receive notification it can check the various shared variables and update its local state. The blocking relies on using ppoll/select but without a timeout. This avoids the condition var and also allows further notifications when paused (also used for unpausing). This design can be extended with further notification types if needed by using and checking further shared vars (or making existing shared vars an enum or counter). This may be used in future for additional notifications to the ticker thread. This will likely be used to proxy wakeUpRts from a single handler context for example. And this approach, avoiding mutexes, is compatible with use from signal handlers. So overall, it's: * slightly simpler / more regular; * easier to extend with additional notifications; * probably slightly more efficient (but a micro-optimisation); * and supports calling notification from signal handlers - - - - - 3252c63e by Duncan Coutts at 2026-06-13T21:56:25+01:00 posix ticker: further minor local renaming for code clarity Improve the clarity with better choice of names for several local vars and function. - - - - - c91e49b5 by Duncan Coutts at 2026-06-13T21:56:25+01:00 win32 ticker: split out local helper functions - - - - - 6a01ae12 by Duncan Coutts at 2026-06-13T21:56:25+01:00 win32 ticker: provide guarantee about concurrency and idempotency Use a lock to ensure pause/unpause can be used concurrently. Use a paused variable, protected by the lock, to ensure that pause and unpause are both idempotent. This is what the portable API expects. - - - - - 6b243d4d by Duncan Coutts at 2026-06-13T21:56:25+01:00 win32 ticker: make the initial tick be after one wait interval There is no need to tick immediately. This is consistent with the posix implementation. - - - - - 3d9758af by Duncan Coutts at 2026-06-13T21:56:25+01:00 ticker: remove now-unnecessary layer of enable/disable There was an atomic variable used to block *part* of the actions of the tick handler. This still did not make stopTimer synchronous, even for the part of the the handle_tick actions it covered. It also added a more expensive (sequentuially consistent) atomic operation in the hot path for the handle_tick action, whereas our new design requires no atomic ops at all. Now that we have eliminate the need for synchronous stop/startTicker, we don't need this not-quite-working-anyway atomic protocol. The new pause/unpauseTicker is explicitly asynchronous and idempotent. - - - - - c52752fb by Duncan Coutts at 2026-06-13T21:56:25+01:00 ticker: add TODOs about issue #27250: too much being done from handle_tick The handle_tick should not perform I/O, block, perform long-running operations or call arbitrary user code. Unfortunately, everything to do with the eventlog (at the moment) falls into all those categories. - - - - - 3 changed files: - rts/Timer.c - rts/posix/Ticker.c - rts/win32/Ticker.c Changes: ===================================== rts/Timer.c ===================================== @@ -28,15 +28,6 @@ #include "RtsSignals.h" #include "rts/EventLogWriter.h" -// This global counter is used to allow multiple threads to stop the -// timer temporarily with a stopTimer()/startTimer() pair. If -// timer_enabled == 0 timer is enabled -// timer_disabled == N, N > 0 timer is disabled by N threads -// When timer_enabled makes a transition to 0, we enable the timer, -// and when it makes a transition to non-0 we disable it. - -static StgWord timer_disabled; - /* ticks left before next pre-emptive context switch */ static int ticks_to_ctxt_switch = 0; @@ -107,9 +98,9 @@ static void handle_tick(int unused STG_UNUSED) { - handleProfTick(); - if (RtsFlags.ConcFlags.ctxtSwitchTicks > 0 - && SEQ_CST_LOAD_ALWAYS(&timer_disabled) == 0) + handleProfTick(); // Bad or worse: see issue #27250. + + if (RtsFlags.ConcFlags.ctxtSwitchTicks > 0) { ticks_to_ctxt_switch--; if (ticks_to_ctxt_switch <= 0) { @@ -123,7 +114,7 @@ handle_tick(int unused STG_UNUSED) ticks_to_eventlog_flush--; if (ticks_to_eventlog_flush <= 0) { ticks_to_eventlog_flush = RtsFlags.TraceFlags.eventlogFlushTicks; - flushEventLog(NULL); + flushEventLog(NULL); // Bad or worse: see issue #27250. } } #endif @@ -183,7 +174,6 @@ void initTimer(void) if (RtsFlags.MiscFlags.tickInterval != 0) { initTicker(RtsFlags.MiscFlags.tickInterval, handle_tick); } - SEQ_CST_STORE_ALWAYS(&timer_disabled, 1); #endif } @@ -200,10 +190,8 @@ void startTimer(void) { /* no-op */ } void pauseTimer(void) { #if defined(HAVE_PREEMPTION) - if (SEQ_CST_SUB_ALWAYS(&timer_disabled, 1) == 0) { - if (RtsFlags.MiscFlags.tickInterval != 0) { - pauseTicker(); - } + if (RtsFlags.MiscFlags.tickInterval != 0) { + pauseTicker(); } #endif } @@ -211,10 +199,8 @@ void pauseTimer(void) void unpauseTimer(void) { #if defined(HAVE_PREEMPTION) - if (SEQ_CST_ADD_ALWAYS(&timer_disabled, 1) == 1) { - if (RtsFlags.MiscFlags.tickInterval != 0) { - unpauseTicker(); - } + if (RtsFlags.MiscFlags.tickInterval != 0) { + unpauseTicker(); } #endif } ===================================== rts/posix/Ticker.c ===================================== @@ -103,120 +103,112 @@ #include #include -static Time itimer_interval = DEFAULT_TICK_INTERVAL; -// Should we be firing ticks? -// Writers to this must hold the mutex below. -static bool stopped = false; +// Forward declarations of local types and helper functions to hide the +// difference between ppoll() and select() +#if defined(HAVE_DECL_PPOLL) && HAVE_DECL_PPOLL == 1 +typedef struct timespec timeout; // for ppoll() +typedef struct { struct pollfd pollfds[1]; } fdset; +#else +typedef struct timeval timeout; // for select() +typedef struct { int fd; fd_set selectfds; } fdset; // need to stash fd +#endif +static void poll_init_timeout(timeout *tv, Time t); +static void poll_init_fdset(fdset *fds, int fd); // single fd only +// poll_*_timeout returns >0 if fd ready, ==0 if timeout, <0 if error +static int poll_no_timeout(fdset *fdset); +static int poll_with_timeout(fdset *fdset, timeout *t); + -// should the ticker thread exit? -// This can be set without holding the mutex. -static bool exited = true; +static Time ticker_interval = DEFAULT_TICK_INTERVAL; -// Signaled when we want to (re)start the timer -static Condition start_cond; -static Mutex mutex; -static OSThreadId thread; +// Atomic variable used by client threads to communicate that they want the +// ticker thread to pause. This communication is one-way, with no +// acknowledgement. +static bool pause_request; -// fds for interrupting the ticker -static int interruptfd_r = -1, interruptfd_w = -1; +// Atomic variable used by other threads to communicate that they want the +// ticker thread to exit. +static bool exit_request; -static void *itimer_thread_func(void *_handle_tick) +// Used to wait for the ticker thread to terminate after asking it to exit. +static OSThreadId ticker_thread_id; + +// Fds used with sendFdWakeup to notify the ticker thread that any of the +// *_request variables above have been set. +static int notifyfd_r = -1, notifyfd_w = -1; + +static void *ticker_thread_func(void *_handle_tick) { TickProc handle_tick = _handle_tick; -#if defined(HAVE_DECL_PPOLL) && HAVE_DECL_PPOLL == 1 - struct pollfd pollfds[1]; + // Thread-local view of our state. We compare these with the corresponding + // atomic shared variables used to request state changes. + bool paused = true; // updated from atomic shared var pause_request + bool exit = false; // updated from atomic shared var exit_request + // Note that we start paused. - pollfds[0].fd = interruptfd_r; - pollfds[0].events = POLLIN; + timeout timeout; + fdset fdset; + poll_init_timeout(&timeout, ticker_interval); + poll_init_fdset(&fdset, notifyfd_r); - struct timespec ts = { .tv_sec = TimeToSeconds(itimer_interval) - , .tv_nsec = TimeToNS(itimer_interval) % 1000000000 - }; -#else - fd_set selectfds; - FD_ZERO(&selectfds); - FD_SET(interruptfd_r, &selectfds); - - struct timeval tv = { .tv_sec = TimeToSeconds(itimer_interval) - /* convert remainder time in nanoseconds - to microseconds, rounding up: */ - , .tv_usec = ((TimeToNS(itimer_interval) % 1000000000) - + 999) / 1000 - }; -#endif - - // Relaxed is sufficient: If we don't see that exited was set in one iteration we will - // see it next time. - while (!RELAXED_LOAD_ALWAYS(&exited)) { + while (!exit) { -#if defined(HAVE_DECL_PPOLL) && HAVE_DECL_PPOLL == 1 - int nfds = 1; - int nready = ppoll(pollfds, nfds, &ts, NULL); -#else - struct timeval tv_tmp = tv; // copy since select may change this value. - int nfds = interruptfd_r+1; - int nready = select(nfds, &selectfds, NULL, NULL, &tv_tmp); -#endif - // In either case (ppoll or select), the result nready is the number - // of fds that are ready. - if (RTS_LIKELY(nready == 0)) { - // Timer expired, not interrupted, continue. - } else if (nready > 0) { - // We only monitor one fd (the interruptfd_r), so we know - // it is that fd that is ready without any further checks. - collectFdWakeup(interruptfd_r); - // No further action needed, continue on to handling the final tick - // and then stop. - - // Note that we rely on sendFdWakeup and select/poll to provide the - // happens-before relation. So if 'exited' was set before calling - // sendFdWakeup, then we should be able to reliably read it after. - // And thus reading 'exited' in the while loop guard is ok. + int notify; + if (paused) { + notify = poll_no_timeout(&fdset); } else { - // While the RTS attempts to mask signals, some foreign libraries - // that rely on signal delivery may unmask them. Consequently we - // may see EINTR. See #24610. - if (errno != EINTR) { - sysErrorBelch("Ticker: poll failed: %s", strerror(errno)); - } + notify = poll_with_timeout(&fdset, &timeout); } - // first try a cheap test - if (RELAXED_LOAD_ALWAYS(&stopped)) { - OS_ACQUIRE_LOCK(&mutex); - // should we really stop? - if (stopped) { - waitCondition(&start_cond, &mutex); - } - OS_RELEASE_LOCK(&mutex); - } else { + if (RTS_LIKELY(notify == 0)) { + // The time expired, no state change notification. handle_tick(0); + + } else if (notify > 0) { + // State change notification, check the request variables. + + // We rely on sendFdWakeup and select/poll to provide the + // happens-before relation. So if the request variables are set + // before calling sendFdWakeup, then we should be able to reliably + // read them here afterwards. + collectFdWakeup(notifyfd_r); + + paused = ACQUIRE_LOAD_ALWAYS(&pause_request); + exit = RELAXED_LOAD_ALWAYS(&exit_request); + } else if (errno != EINTR) { + // While the RTS attempts to mask signals, some foreign libraries + // that rely on signal delivery may unmask them. Consequently we + // may see EINTR. See #24610. + sysErrorBelch("Ticker: poll failed: %s", strerror(errno)); } } return NULL; } +/* Initialise the ticker on startup or re-initialise the ticker after a fork(). + * In the fork case, the thread will not be present, but fds are inherited. + * + * The ticker is started in the paused state. Use unpauseTicker to continue. + */ void initTicker (Time interval, TickProc handle_tick) { - itimer_interval = interval; - stopped = true; - exited = false; + ticker_interval = interval; + pause_request = true; + exit_request = false; + #if defined(HAVE_SIGNAL_H) sigset_t mask, omask; int sigret; #endif int ret; - initCondition(&start_cond); - initMutex(&mutex); - /* Open the interrupt fd synchronously. * - * We used to do it in itimer_thread_func (i.e. in the timer thread) but it + * We used to do it in ticker_thread_func (i.e. in the timer thread) but it * meant that some user code could run before it and get confused by the * allocation of the timerfd. * @@ -226,11 +218,11 @@ initTicker (Time interval, TickProc handle_tick) * descriptor closed by the first call! (see #20618) */ - if (interruptfd_r != -1) { + if (notifyfd_r != -1) { // don't leak the old file descriptors after a fork (#25280) - closeFdWakeup(interruptfd_r, interruptfd_w); + closeFdWakeup(notifyfd_r, notifyfd_w); } - newFdWakeup(&interruptfd_r, &interruptfd_w); + newFdWakeup(¬ifyfd_r, ¬ifyfd_w); /* * Create the thread with all blockable signals blocked, leaving signal @@ -242,7 +234,7 @@ initTicker (Time interval, TickProc handle_tick) sigfillset(&mask); sigret = pthread_sigmask(SIG_SETMASK, &mask, &omask); #endif - ret = createAttachedOSThread(&thread, "ghc_ticker", itimer_thread_func, (void*)handle_tick); + ret = createAttachedOSThread(&ticker_thread_id, "ghc_ticker", ticker_thread_func, (void*)handle_tick); #if defined(HAVE_SIGNAL_H) if (sigret == 0) pthread_sigmask(SIG_SETMASK, &omask, NULL); @@ -256,10 +248,8 @@ initTicker (Time interval, TickProc handle_tick) /* Asynchronous. Idempotent. */ void unpauseTicker(void) { - OS_ACQUIRE_LOCK(&mutex); - RELAXED_STORE(&stopped, false); - signalCondition(&start_cond); - OS_RELEASE_LOCK(&mutex); + RELEASE_STORE_ALWAYS(&pause_request, false); + sendFdWakeup(notifyfd_w); } /* Asynchronous. Idempotent. @@ -268,9 +258,8 @@ void unpauseTicker(void) */ void pauseTicker(void) { - OS_ACQUIRE_LOCK(&mutex); - RELAXED_STORE(&stopped, true); - OS_RELEASE_LOCK(&mutex); + RELEASE_STORE_ALWAYS(&pause_request, true); + sendFdWakeup(notifyfd_w); } /* Synchronous. Not idempotent. @@ -278,21 +267,90 @@ void pauseTicker(void) */ void exitTicker(void) { - ASSERT(!SEQ_CST_LOAD(&exited)); - SEQ_CST_STORE(&exited, true); - // ensure that ticker wakes up if stopped - unpauseTicker(); - sendFdWakeup(interruptfd_w); - - // wait for ticker to terminate - if (pthread_join(thread, NULL)) { + ASSERT(!RELAXED_LOAD_ALWAYS(&exit_request)); + RELEASE_STORE_ALWAYS(&exit_request, true); + sendFdWakeup(notifyfd_w); + + // wait for ticker thread to terminate + if (pthread_join(ticker_thread_id, NULL)) { sysErrorBelch("Ticker: Failed to join: %s", strerror(errno)); } - closeFdWakeup(interruptfd_r, interruptfd_w); - closeMutex(&mutex); - closeCondition(&start_cond); + closeFdWakeup(notifyfd_r, notifyfd_w); } +/* Implementation of the local helpers, to hide the difference between ppoll() + * and select(). + */ +#if defined(HAVE_DECL_PPOLL) && HAVE_DECL_PPOLL == 1 +static void poll_init_timeout(timeout *tv, Time t) +{ + tv->tv_sec = TimeToSeconds(t); + tv->tv_nsec = TimeToNS(t) % 1000000000; +} + +static void poll_init_fdset(fdset *fds, int fd) +{ + fds->pollfds[0].fd = fd; + fds->pollfds[0].events = POLLIN; +} + +static int poll_no_timeout(fdset *fds) +{ + int nfds = 1; + return ppoll(fds->pollfds, nfds, NULL, NULL); +} + +static int poll_with_timeout(fdset *fds, timeout *ts) +{ + int nfds = 1; + return ppoll(fds->pollfds, nfds, ts, NULL); +} + +#else // select() + +static void poll_init_timeout(timeout *tv, Time t) +{ + tv->tv_sec = TimeToSeconds(t); + /* convert remainder time in nanoseconds to microseconds, rounding up: */ + tv->tv_usec = ((TimeToNS(t) % 1000000000) + 999) / 1000; +} + +static void poll_init_fdset(fdset *fds, int fd) +{ + /* select() modifies the fd_set: it uses the same fd_set for reporting as + * for input. Thus we must rebuild it every time. We can optimise this + * rebuilding somewhat however if we rely on select() not modifying the + * bits that we didn't ask it to look at. So we can zero the fd_set just + * once, and then only reset the single bit for the single fd, before each + * call to selct(). + */ + fds->fd = fd; + FD_ZERO(&fds->selectfds); +} + +static int poll_no_timeout(fdset *fds) +{ + /* select() modifies the fd_set so we must set it every time, but we rely + * on it not touching other bits to avoid having to FD_ZERO it every time + */ + FD_SET(fds->fd, &fds->selectfds); + int nfds = fds->fd+1; + return select(nfds, &fds->selectfds, NULL, NULL, NULL); +} + +static int poll_with_timeout(fdset *fds, timeout *tv) +{ + struct timeval tv_tmp = *tv; // copy since select may change this value. + /* select() modifies the fd_set so we must set it every time, but we rely + * on it not touching other bits to avoid having to FD_ZERO it every time + */ + FD_SET(fds->fd, &fds->selectfds); + int nfds = fds->fd+1; + return select(nfds, &fds->selectfds, NULL, NULL, &tv_tmp); +} +#endif + +/* This is obsolete, but is used in the unix package for now */ int rtsTimerSignal(void) { ===================================== rts/win32/Ticker.c ===================================== @@ -11,7 +11,11 @@ static TickProc tick_proc = NULL; static HANDLE timer_queue = NULL; + +static Mutex lock; // To protect the timer and paused var below static HANDLE timer = NULL; +static bool paused; + static Time tick_interval = 0; static VOID CALLBACK tick_callback( @@ -36,12 +40,19 @@ static VOID CALLBACK tick_callback( // This seems to be the case starting at some point during the // Windows 7 lifetime and any newer versions of windows. +// Forward decls +static void startTicker(void); +static void stopTicker(bool synchronous); + void initTicker (Time interval, TickProc handle_tick) { + ASSERT(timer_queue == NULL); tick_interval = interval; tick_proc = handle_tick; + OS_INIT_LOCK(&lock); + paused = true; // starts paused timer_queue = CreateTimerQueue(); if (timer_queue == NULL) { sysErrorBelch("CreateTimerQueue"); @@ -49,43 +60,81 @@ initTicker (Time interval, TickProc handle_tick) } } +// Asynchronous. Idempotent. void unpauseTicker(void) { - BOOL r; - - r = CreateTimerQueueTimer(&timer, - timer_queue, - tick_callback, - 0, - 0, - TimeToMS(tick_interval), // ms - WT_EXECUTEINTIMERTHREAD); - if (r == 0) { - sysErrorBelch("CreateTimerQueueTimer"); - stg_exit(EXIT_FAILURE); + OS_ACQUIRE_LOCK(&lock); + if (paused) { + startTicker(); } + paused = false; + OS_RELEASE_LOCK(&lock); } +// Asynchronous. Idempotent. void pauseTicker(void) { - if (timer_queue != NULL && timer != NULL) { - DeleteTimerQueueTimer(timer_queue, timer, NULL); - timer = NULL; + OS_ACQUIRE_LOCK(&lock); + if (!paused) { + /* pauseTicker is called from within the handle_tick, so stopping + * the ticker here /must/ be asynchronous or we will deadlock! */ + stopTicker(false /* asynchronous */); } + paused = true; + OS_RELEASE_LOCK(&lock); } void exitTicker(void) { - pauseTicker(); - if (timer_queue != NULL) { - // From the docs for DeleteTimerQueueEx: - // If this parameter is INVALID_HANDLE_VALUE, the function waits - // for all callback functions to complete before returning. - HANDLE completion = INVALID_HANDLE_VALUE; - DeleteTimerQueueEx(timer_queue, completion); - timer_queue = NULL; + ASSERT(timer_queue != NULL); + + OS_ACQUIRE_LOCK(&lock); + if (!paused) { + stopTicker(true /* synchronous */); + } + OS_RELEASE_LOCK(&lock); + + // From the docs for DeleteTimerQueueEx: + // If this parameter is INVALID_HANDLE_VALUE, the function waits + // for all callback functions to complete before returning. + // This is a belt-and-braces approach to ensuring exitTicker is synchronous, + // since stopTicker(true) is already synchronous and there's only one timer. + HANDLE completion = INVALID_HANDLE_VALUE; + DeleteTimerQueueEx(timer_queue, completion); + timer_queue = NULL; +} + +static void startTicker(void) { + ASSERT(timer_queue != NULL && timer == NULL); + DWORD interval = TimeToMS(tick_interval); // ms + BOOL r = CreateTimerQueueTimer(&timer, + timer_queue, + tick_callback, + NULL, // callback param + interval, // inital interval + interval, // recurrant interval + WT_EXECUTEINTIMERTHREAD); + //TODO: using WT_EXECUTEINTIMERTHREAD is fine for context switching, and + // plausibly also ok for profile sampling but is way out for eventlog + // flushing. The eventlog flush does a global synchronisation of all + // capabilities and I/O! And with eventlog providers, it calls arbitrary + // user code. This is not ok! See issue #27250. + if (r == 0) { + sysErrorBelch("CreateTimerQueueTimer"); + stg_exit(EXIT_FAILURE); } + ASSERT(timer != NULL); +} + +static void stopTicker(bool synchronous) { + ASSERT(timer_queue != NULL && timer != NULL); + // From the docs for DeleteTimerQueueTimer: + // If this parameter is INVALID_HANDLE_VALUE, the function waits for any + // running timer callback functions to complete before returning. + HANDLE completion = synchronous ? INVALID_HANDLE_VALUE : NULL; + DeleteTimerQueueTimer(timer_queue, timer, completion); + timer = NULL; } View it on GitLab: https://gitlab.haskell.org/ghc/ghc/-/compare/8f58e22a271d5d20b7c06d3bc903328... -- View it on GitLab: https://gitlab.haskell.org/ghc/ghc/-/compare/8f58e22a271d5d20b7c06d3bc903328... You're receiving this email because of your account on gitlab.haskell.org.