[Git][ghc/ghc][wip/tsan/timer] 6 commits: rts: Accept benign races in Proftimer
Ben Gamari
gitlab at gitlab.haskell.org
Tue Oct 20 18:45:58 UTC 2020
Ben Gamari pushed to branch wip/tsan/timer at Glasgow Haskell Compiler / GHC
Commits:
1ce9c5c7 by Ben Gamari at 2020-10-20T14:45:50-04:00
rts: Accept benign races in Proftimer
- - - - -
03b5dc44 by Ben Gamari at 2020-10-20T14:45:50-04:00
rts: Pause timer while changing capability count
This avoids #17289.
- - - - -
f8257342 by Ben Gamari at 2020-10-20T14:45:51-04:00
Fix #17289
- - - - -
60aeb584 by Ben Gamari at 2020-10-20T14:45:51-04:00
suppress #17289 (ticker) race
- - - - -
d7885a85 by Ben Gamari at 2020-10-20T14:45:51-04:00
rts: Fix timer initialization
Previously `initScheduler` would attempt to pause the ticker and in so
doing acquire the ticker mutex. However, initTicker, which is
responsible for initializing said mutex, hadn't been called
yet.
- - - - -
c5da3791 by Ben Gamari at 2020-10-20T14:45:51-04:00
rts: Annotate benign race in ticker shutdown logic
We are guaranteed to see the change in the `stopped` flag at some point
so the race is harmless.
- - - - -
7 changed files:
- rts/.tsan-suppressions
- rts/Capability.c
- rts/Proftimer.c
- rts/RtsStartup.c
- rts/Schedule.c
- rts/Timer.c
- rts/posix/itimer/Pthread.c
Changes:
=====================================
rts/.tsan-suppressions
=====================================
@@ -7,3 +7,7 @@ race:capability_is_busy
# This is a benign race during IO manager shutdown (between ioManagerWakeup
# and GHC.Event.Control.closeControl).
race:ioManagerWakeup
+
+# This is a potentially problematic race which I have yet to work out
+# (#17289)
+race:handle_tick
=====================================
rts/Capability.c
=====================================
@@ -410,36 +410,44 @@ void
moreCapabilities (uint32_t from USED_IF_THREADS, uint32_t to USED_IF_THREADS)
{
#if defined(THREADED_RTS)
- uint32_t i;
- Capability **old_capabilities = capabilities;
+ Capability **new_capabilities = stgMallocBytes(to * sizeof(Capability*), "moreCapabilities");
- capabilities = stgMallocBytes(to * sizeof(Capability*), "moreCapabilities");
+ // We must disable the timer while we do this since the tick handler may
+ // call contextSwitchAllCapabilities, which may see the capabilities array
+ // as we free it. The alternative would be to protect the capabilities
+ // array with a lock but this seems more expensive than necessary.
+ // See #17289.
+ stopTimer();
if (to == 1) {
// THREADED_RTS must work on builds that don't have a mutable
// BaseReg (eg. unregisterised), so in this case
// capabilities[0] must coincide with &MainCapability.
- capabilities[0] = &MainCapability;
+ new_capabilities[0] = &MainCapability;
initCapability(&MainCapability, 0);
}
else
{
- for (i = 0; i < to; i++) {
+ for (uint32_t i = 0; i < to; i++) {
if (i < from) {
- capabilities[i] = old_capabilities[i];
+ new_capabilities[i] = capabilities[i];
} else {
- capabilities[i] = stgMallocBytes(sizeof(Capability),
- "moreCapabilities");
- initCapability(capabilities[i], i);
+ new_capabilities[i] = stgMallocBytes(sizeof(Capability),
+ "moreCapabilities");
+ initCapability(new_capabilities[i], i);
}
}
}
debugTrace(DEBUG_sched, "allocated %d more capabilities", to - from);
+ Capability **old_capabilities = ACQUIRE_LOAD(&capabilities);
+ RELEASE_STORE(&capabilities, new_capabilities);
if (old_capabilities != NULL) {
stgFree(old_capabilities);
}
+
+ startTimer();
#endif
}
=====================================
rts/Proftimer.c
=====================================
@@ -30,7 +30,7 @@ void
stopProfTimer( void )
{
#if defined(PROFILING)
- do_prof_ticks = false;
+ RELAXED_STORE(&do_prof_ticks, false);
#endif
}
@@ -38,14 +38,14 @@ void
startProfTimer( void )
{
#if defined(PROFILING)
- do_prof_ticks = true;
+ RELAXED_STORE(&do_prof_ticks, true);
#endif
}
void
stopHeapProfTimer( void )
{
- do_heap_prof_ticks = false;
+ RELAXED_STORE(&do_heap_prof_ticks, false);
}
void
@@ -74,7 +74,7 @@ handleProfTick(void)
{
#if defined(PROFILING)
total_ticks++;
- if (do_prof_ticks) {
+ if (RELAXED_LOAD(&do_prof_ticks)) {
uint32_t n;
for (n=0; n < n_capabilities; n++) {
capabilities[n]->r.rCCCS->time_ticks++;
@@ -83,7 +83,7 @@ handleProfTick(void)
}
#endif
- if (do_heap_prof_ticks) {
+ if (RELAXED_LOAD(&do_heap_prof_ticks)) {
ticks_to_heap_profile--;
if (ticks_to_heap_profile <= 0) {
ticks_to_heap_profile = RtsFlags.ProfFlags.heapProfileIntervalTicks;
=====================================
rts/RtsStartup.c
=====================================
@@ -284,6 +284,13 @@ hs_init_ghc(int *argc, char **argv[], RtsConfig rts_config)
/* Initialise libdw session pool */
libdwPoolInit();
+ /* Start the "ticker" and profiling timer but don't start until the
+ * scheduler is up. However, the ticker itself needs to be initialized
+ * before the scheduler to ensure that the ticker mutex is initialized as
+ * moreCapabilities will attempt to acquire it.
+ */
+ initTimer();
+
/* initialise scheduler data structures (needs to be done before
* initStorage()).
*/
@@ -359,7 +366,6 @@ hs_init_ghc(int *argc, char **argv[], RtsConfig rts_config)
initHeapProfiling();
/* start the virtual timer 'subsystem'. */
- initTimer();
startTimer();
#if defined(RTS_USER_SIGNALS)
=====================================
rts/Schedule.c
=====================================
@@ -2253,6 +2253,12 @@ setNumCapabilities (uint32_t new_n_capabilities USED_IF_THREADS)
cap = rts_lock();
task = cap->running_task;
+
+ // N.B. We must stop the interval timer while we are changing the
+ // capabilities array lest handle_tick may try to context switch
+ // an old capability. See #17289.
+ stopTimer();
+
stopAllCapabilities(&cap, task);
if (new_n_capabilities < enabled_capabilities)
@@ -2335,6 +2341,8 @@ setNumCapabilities (uint32_t new_n_capabilities USED_IF_THREADS)
// Notify IO manager that the number of capabilities has changed.
rts_evalIO(&cap, ioManagerCapabilitiesChanged_closure, NULL);
+ startTimer();
+
rts_unlock(cap);
#endif // THREADED_RTS
=====================================
rts/Timer.c
=====================================
@@ -25,6 +25,15 @@
#include "Capability.h"
#include "RtsSignals.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;
@@ -92,7 +101,9 @@ void
handle_tick(int unused STG_UNUSED)
{
handleProfTick();
- if (RtsFlags.ConcFlags.ctxtSwitchTicks > 0) {
+ if (RtsFlags.ConcFlags.ctxtSwitchTicks > 0
+ && SEQ_CST_LOAD(&timer_disabled) == 0)
+ {
ticks_to_ctxt_switch--;
if (ticks_to_ctxt_switch <= 0) {
ticks_to_ctxt_switch = RtsFlags.ConcFlags.ctxtSwitchTicks;
@@ -148,15 +159,6 @@ handle_tick(int unused STG_UNUSED)
}
}
-// 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;
-
void
initTimer(void)
{
@@ -164,7 +166,7 @@ initTimer(void)
if (RtsFlags.MiscFlags.tickInterval != 0) {
initTicker(RtsFlags.MiscFlags.tickInterval, handle_tick);
}
- timer_disabled = 1;
+ SEQ_CST_STORE(&timer_disabled, 1);
}
void
=====================================
rts/posix/itimer/Pthread.c
=====================================
@@ -85,11 +85,11 @@ static Time itimer_interval = DEFAULT_TICK_INTERVAL;
// Should we be firing ticks?
// Writers to this must hold the mutex below.
-static volatile bool stopped = false;
+static bool stopped = false;
// should the ticker thread exit?
// This can be set without holding the mutex.
-static volatile bool exited = true;
+static bool exited = true;
// Signaled when we want to (re)start the timer
static Condition start_cond;
@@ -120,7 +120,9 @@ static void *itimer_thread_func(void *_handle_tick)
}
#endif
- while (!exited) {
+ // Relaxed is sufficient: If we don't see that exited was set in one iteration we will
+ // see it next time.
+ while (!RELAXED_LOAD(&exited)) {
if (USE_TIMERFD_FOR_ITIMER) {
ssize_t r = read(timerfd, &nticks, sizeof(nticks));
if ((r == 0) && (errno == 0)) {
@@ -142,7 +144,8 @@ static void *itimer_thread_func(void *_handle_tick)
}
// first try a cheap test
- if (stopped) {
+ TSAN_ANNOTATE_BENIGN_RACE(&stopped, "itimer_thread_func");
+ if (RELAXED_LOAD(&stopped)) {
OS_ACQUIRE_LOCK(&mutex);
// should we really stop?
if (stopped) {
@@ -186,7 +189,7 @@ void
startTicker(void)
{
OS_ACQUIRE_LOCK(&mutex);
- stopped = 0;
+ RELAXED_STORE(&stopped, false);
signalCondition(&start_cond);
OS_RELEASE_LOCK(&mutex);
}
@@ -196,7 +199,7 @@ void
stopTicker(void)
{
OS_ACQUIRE_LOCK(&mutex);
- stopped = 1;
+ RELAXED_STORE(&stopped, true):
OS_RELEASE_LOCK(&mutex);
}
@@ -204,8 +207,8 @@ stopTicker(void)
void
exitTicker (bool wait)
{
- ASSERT(!exited);
- exited = true;
+ ASSERT(!SEQ_CST_LOAD(&exited));
+ SEQ_CST_STORE(&exited, true);
// ensure that ticker wakes up if stopped
startTicker();
View it on GitLab: https://gitlab.haskell.org/ghc/ghc/-/compare/7341c75fc3b3abde26062d600533ee6ea6d576fe...c5da3791f9a4ba6bdf91c231c7bbaed0ee1a6b2e
--
View it on GitLab: https://gitlab.haskell.org/ghc/ghc/-/compare/7341c75fc3b3abde26062d600533ee6ea6d576fe...c5da3791f9a4ba6bdf91c231c7bbaed0ee1a6b2e
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/20201020/56391611/attachment-0001.html>
More information about the ghc-commits
mailing list