Merge branch 'wip/tsan/timer' into wip/tsan/all

7 files changed, 65 insertions, 34 deletions

diff --git a/rts/.tsan-suppressions b/rts/.tsan-suppressions
index 03c48fb4b3..734faff5a6 100644
--- a/rts/.tsan-suppressions
+++ b/rts/.tsan-suppressions
@@ -8,3 +8,7 @@ race:capability_is_busy
 # and GHC.Event.Control.closeControl).
 race:ioManagerWakeup
 race:base_GHCziEventziControl_zdwcloseControl_info
+
+# This is a potentially problematic race which I have yet to work out
+# (#17289)
+race:handle_tick
diff --git a/rts/Capability.c b/rts/Capability.c
index eda4f8e691..77ca5e8c0d 100644
--- a/rts/Capability.c
+++ b/rts/Capability.c
@@ -416,36 +416,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
 }
 
diff --git a/rts/Proftimer.c b/rts/Proftimer.c
index 68a73a5446..24f82ead6d 100644
--- a/rts/Proftimer.c
+++ b/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;
diff --git a/rts/RtsStartup.c b/rts/RtsStartup.c
index a3dddb03f5..6dd8c555f1 100644
--- a/rts/RtsStartup.c
+++ b/rts/RtsStartup.c
@@ -285,6 +285,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()).
      */
@@ -366,7 +373,6 @@ hs_init_ghc(int *argc, char **argv[], RtsConfig rts_config)
     initHeapProfiling();
 
     /* start the virtual timer 'subsystem'. */
-    initTimer();
     startTimer();
 
 #if defined(RTS_USER_SIGNALS)
diff --git a/rts/Schedule.c b/rts/Schedule.c
index 67177f2d98..28849f32e8 100644
--- a/rts/Schedule.c
+++ b/rts/Schedule.c
@@ -2251,6 +2251,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)
@@ -2333,6 +2339,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
diff --git a/rts/Timer.c b/rts/Timer.c
index 990eba6b05..97d87ad989 100644
--- a/rts/Timer.c
+++ b/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
diff --git a/rts/posix/itimer/Pthread.c b/rts/posix/itimer/Pthread.c
index dd36137b72..82379b9172 100644
--- a/rts/posix/itimer/Pthread.c
+++ b/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();