Mitigate data races in event manager startup/shutdownwip/tsan/event-mgr

3 files changed, 33 insertions, 21 deletions

diff --git a/libraries/base/GHC/Event/Control.hs b/libraries/base/GHC/Event/Control.hs
index a9f23e07d2..9054da4f22 100644
--- a/libraries/base/GHC/Event/Control.hs
+++ b/libraries/base/GHC/Event/Control.hs
@@ -123,6 +123,10 @@ newControl shouldRegister = allocaArray 2 $ \fds -> do
 -- the RTS, then *BEFORE* the wakeup file is closed, we must call
 -- c_setIOManagerWakeupFd (-1), so that the RTS does not try to use the wakeup
 -- file after it has been closed.
+--
+-- Note, however, that even if we do the above, this function is still racy
+-- since we do not synchronize between here and ioManagerWakeup.
+-- ioManagerWakeup ignores failures that arise from this case.
 closeControl :: Control -> IO ()
 closeControl w = do
   _ <- atomicSwapIORef (controlIsDead w) True
diff --git a/rts/Capability.c b/rts/Capability.c
index 181075b963..9a0c92174d 100644
--- a/rts/Capability.c
+++ b/rts/Capability.c
@@ -1256,7 +1256,7 @@ void
 setIOManagerControlFd(uint32_t cap_no USED_IF_THREADS, int fd USED_IF_THREADS) {
 #if defined(THREADED_RTS)
     if (cap_no < n_capabilities) {
-        capabilities[cap_no]->io_manager_control_wr_fd = fd;
+        RELAXED_STORE(&capabilities[cap_no]->io_manager_control_wr_fd, fd);
     } else {
         errorBelch("warning: setIOManagerControlFd called with illegal capability number.");
     }
diff --git a/rts/posix/Signals.c b/rts/posix/Signals.c
index 2e534042f3..5ad688bc2f 100644
--- a/rts/posix/Signals.c
+++ b/rts/posix/Signals.c
@@ -128,7 +128,7 @@ more_handlers(int sig)
 }
 
 // Here's the pipe into which we will send our signals
-static volatile int io_manager_wakeup_fd = -1;
+static int io_manager_wakeup_fd = -1;
 static int timer_manager_control_wr_fd = -1;
 
 #define IO_MANAGER_WAKEUP 0xff
@@ -136,7 +136,7 @@ static int timer_manager_control_wr_fd = -1;
 #define IO_MANAGER_SYNC   0xfd
 
 void setTimerManagerControlFd(int fd) {
-    timer_manager_control_wr_fd = fd;
+    RELAXED_STORE(&timer_manager_control_wr_fd, fd);
 }
 
 void
@@ -144,7 +144,7 @@ setIOManagerWakeupFd (int fd)
 {
     // only called when THREADED_RTS, but unconditionally
     // compiled here because GHC.Event.Control depends on it.
-    io_manager_wakeup_fd = fd;
+    SEQ_CST_STORE(&io_manager_wakeup_fd, fd);
 }
 
 /* -----------------------------------------------------------------------------
@@ -154,14 +154,15 @@ void
 ioManagerWakeup (void)
 {
     int r;
+    const int wakeup_fd = SEQ_CST_LOAD(&io_manager_wakeup_fd);
     // Wake up the IO Manager thread by sending a byte down its pipe
-    if (io_manager_wakeup_fd >= 0) {
+    if (wakeup_fd >= 0) {
 #if defined(HAVE_EVENTFD)
         StgWord64 n = (StgWord64)IO_MANAGER_WAKEUP;
-        r = write(io_manager_wakeup_fd, (char *) &n, 8);
+        r = write(wakeup_fd, (char *) &n, 8);
 #else
         StgWord8 byte = (StgWord8)IO_MANAGER_WAKEUP;
-        r = write(io_manager_wakeup_fd, &byte, 1);
+        r = write(wakeup_fd, &byte, 1);
 #endif
         /* N.B. If the TimerManager is shutting down as we run this
          * then there is a possibility that our first read of
@@ -174,7 +175,7 @@ ioManagerWakeup (void)
          * Since this is not an error condition, we do not print the error
          * message in this case.
          */
-        if (r == -1 && io_manager_wakeup_fd >= 0) {
+        if (r == -1 && SEQ_CST_LOAD(&io_manager_wakeup_fd) >= 0) {
             sysErrorBelch("ioManagerWakeup: write");
         }
     }
@@ -186,21 +187,27 @@ ioManagerDie (void)
 {
     StgWord8 byte = (StgWord8)IO_MANAGER_DIE;
     uint32_t i;
-    int fd;
     int r;
 
-    if (0 <= timer_manager_control_wr_fd) {
-        r = write(timer_manager_control_wr_fd, &byte, 1);
-        if (r == -1) { sysErrorBelch("ioManagerDie: write"); }
-        timer_manager_control_wr_fd = -1;
-    }
-
-    for (i=0; i < n_capabilities; i++) {
-        fd = capabilities[i]->io_manager_control_wr_fd;
+    {
+        // Shut down timer manager
+        const int fd = RELAXED_LOAD(&timer_manager_control_wr_fd);
         if (0 <= fd) {
             r = write(fd, &byte, 1);
             if (r == -1) { sysErrorBelch("ioManagerDie: write"); }
-            capabilities[i]->io_manager_control_wr_fd = -1;
+            RELAXED_STORE(&timer_manager_control_wr_fd, -1);
+        }
+    }
+
+    {
+        // Shut down IO managers
+        for (i=0; i < n_capabilities; i++) {
+            const int fd = RELAXED_LOAD(&capabilities[i]->io_manager_control_wr_fd);
+            if (0 <= fd) {
+                r = write(fd, &byte, 1);
+                if (r == -1) { sysErrorBelch("ioManagerDie: write"); }
+                RELAXED_STORE(&capabilities[i]->io_manager_control_wr_fd, -1);
+            }
         }
     }
 }
@@ -216,7 +223,7 @@ ioManagerStart (void)
 {
     // Make sure the IO manager thread is running
     Capability *cap;
-    if (timer_manager_control_wr_fd < 0 || io_manager_wakeup_fd < 0) {
+    if (SEQ_CST_LOAD(&timer_manager_control_wr_fd) < 0 || SEQ_CST_LOAD(&io_manager_wakeup_fd) < 0) {
         cap = rts_lock();
         ioManagerStartCap(&cap);
         rts_unlock(cap);
@@ -258,9 +265,10 @@ generic_handler(int sig USED_IF_THREADS,
         memcpy(buf+1, info, sizeof(siginfo_t));
     }
 
-    if (0 <= timer_manager_control_wr_fd)
+    int timer_control_fd = RELAXED_LOAD(&timer_manager_control_wr_fd);
+    if (0 <= timer_control_fd)
     {
-        r = write(timer_manager_control_wr_fd, buf, sizeof(siginfo_t)+1);
+        r = write(timer_control_fd, buf, sizeof(siginfo_t)+1);
         if (r == -1 && errno == EAGAIN) {
             errorBelch("lost signal due to full pipe: %d\n", sig);
         }