Merge remote-tracking branch 'origin/wip/tsan/timer' into wip/angerman/aarch64-ncg

7 files changed, 58 insertions, 26 deletions

diff --git a/rts/.tsan-suppressions b/rts/.tsan-suppressions
index b990c6cfc1..7375d6e48b 100644
--- a/rts/.tsan-suppressions
+++ b/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
diff --git a/rts/Capability.c b/rts/Capability.c
index 0910bc200a..bd222ca855 100644
--- a/rts/Capability.c
+++ b/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
 }
 
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 f73d0bd742..7004391e8b 100644
--- a/rts/RtsStartup.c
+++ b/rts/RtsStartup.c
@@ -231,6 +231,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()).
      */
@@ -303,7 +310,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 a0a471a4ac..25676c1b12 100644
--- a/rts/Schedule.c
+++ b/rts/Schedule.c
@@ -2246,6 +2246,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)
@@ -2328,6 +2334,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..1ee96e4b6e 100644
--- a/rts/posix/itimer/Pthread.c
+++ b/rts/posix/itimer/Pthread.c
@@ -120,6 +120,9 @@ static void *itimer_thread_func(void *_handle_tick)
     }
 #endif
 
+    // Benign race: If we don't see that exited was set in one iteration we will
+    // see it next time.
+    TSAN_ANNOTATE_BENIGN_RACE(&exited, "itimer_thread_func");
     while (!exited) {
         if (USE_TIMERFD_FOR_ITIMER) {
             ssize_t r = read(timerfd, &nticks, sizeof(nticks));
@@ -142,6 +145,7 @@ static void *itimer_thread_func(void *_handle_tick)
         }
 
         // first try a cheap test
+        TSAN_ANNOTATE_BENIGN_RACE(&stopped, "itimer_thread_func");
         if (stopped) {
             OS_ACQUIRE_LOCK(&mutex);
             // should we really stop?