Avoid potential spinlock in a signal handler as part of global barriers.

On platforms without support for 64bit atomic operations where we also
cannot rely on 64bit reads to have single copy atomicity, such atomics
are implemented using a spinlock based fallback. That means it's not
safe to even read such atomics from within a signal handler (since the
signal handler might run when the spinlock already is held).

To avoid this issue defer global barrier processing out of the signal
handler. Instead of checking local / shared barrier generation to
determine whether to set ProcSignalBarrierPending, introduce
PROCSIGNAL_BARRIER and always set ProcSignalBarrierPending when
receiving such a signal. Additionally avoid redundant work in
ProcessProcSignalBarrier if ProcSignalBarrierPending is unnecessarily.

Also do a small amount of other polishing.

Author: Andres Freund
Reviewed-By: Robert Haas
Discussion: https://postgr.es/m/20200609193723.eu5ilsjxwdpyxhgz@alap3.anarazel.de
Backpatch: 13-, where the code was introduced.

diff --git a/src/backend/storage/ipc/procsignal.c b/src/backend/storage/ipc/procsignal.c
index c809196d06a41094d111753cf0005185cea81594..4fa385b0ece46b3f9693bea990e515ba1616e2ef 100644 (file)
--- a/src/backend/storage/ipc/procsignal.c
+++ b/src/backend/storage/ipc/procsignal.c
@@ -320,7 +320,7 @@ SendProcSignal(pid_t pid, ProcSignalReason reason, BackendId backendId)
 uint64
 EmitProcSignalBarrier(ProcSignalBarrierType type)
 {
-   uint64      flagbit = UINT64CONST(1) << (uint64) type;
+   uint32      flagbit = 1 << (uint32) type;
    uint64      generation;
 
    /*
@@ -363,7 +363,11 @@ EmitProcSignalBarrier(ProcSignalBarrierType type)
        pid_t       pid = slot->pss_pid;
 
        if (pid != 0)
+       {
+           /* see SendProcSignal for details */
+           slot->pss_signalFlags[PROCSIG_BARRIER] = true;
            kill(pid, SIGUSR1);
+       }
    }
 
    return generation;
@@ -383,6 +387,8 @@ WaitForProcSignalBarrier(uint64 generation)
 {
    long        timeout = 125L;
 
+   Assert(generation <= pg_atomic_read_u64(&ProcSignal->psh_barrierGeneration));
+
    for (int i = NumProcSignalSlots - 1; i >= 0; i--)
    {
        volatile ProcSignalSlot *slot = &ProcSignal->psh_slot[i];
@@ -417,6 +423,23 @@ WaitForProcSignalBarrier(uint64 generation)
    pg_memory_barrier();
 }
 
+/*
+ * Handle receipt of an interrupt indicating a global barrier event.
+ *
+ * All the actual work is deferred to ProcessProcSignalBarrier(), because we
+ * cannot safely access the barrier generation inside the signal handler as
+ * 64bit atomics might use spinlock based emulation, even for reads. As this
+ * routine only gets called when PROCSIG_BARRIER is sent that won't cause a
+ * lot fo unnecessary work.
+ */
+static void
+HandleProcSignalBarrierInterrupt(void)
+{
+   InterruptPending = true;
+   ProcSignalBarrierPending = true;
+   /* latch will be set by procsignal_sigusr1_handler */
+}
+
 /*
  * Perform global barrier related interrupt checking.
  *
@@ -428,22 +451,38 @@ WaitForProcSignalBarrier(uint64 generation)
 void
 ProcessProcSignalBarrier(void)
 {
-   uint64      generation;
+   uint64      local_gen;
+   uint64      shared_gen;
    uint32      flags;
 
+   Assert(MyProcSignalSlot);
+
    /* Exit quickly if there's no work to do. */
    if (!ProcSignalBarrierPending)
        return;
    ProcSignalBarrierPending = false;
 
    /*
-    * Read the current barrier generation, and then get the flags that are
-    * set for this backend. Note that pg_atomic_exchange_u32 is a full
-    * barrier, so we're guaranteed that the read of the barrier generation
-    * happens before we atomically extract the flags, and that any subsequent
-    * state changes happen afterward.
+    * It's not unlikely to process multiple barriers at once, before the
+    * signals for all the barriers have arrived. To avoid unnecessary work in
+    * response to subsequent signals, exit early if we already have processed
+    * all of them.
+    */
+   local_gen = pg_atomic_read_u64(&MyProcSignalSlot->pss_barrierGeneration);
+   shared_gen = pg_atomic_read_u64(&ProcSignal->psh_barrierGeneration);
+
+   Assert(local_gen <= shared_gen);
+
+   if (local_gen == shared_gen)
+       return;
+
+   /*
+    * Get and clear the flags that are set for this backend. Note that
+    * pg_atomic_exchange_u32 is a full barrier, so we're guaranteed that the
+    * read of the barrier generation above happens before we atomically
+    * extract the flags, and that any subsequent state changes happen
+    * afterward.
     */
-   generation = pg_atomic_read_u64(&ProcSignal->psh_barrierGeneration);
    flags = pg_atomic_exchange_u32(&MyProcSignalSlot->pss_barrierCheckMask, 0);
 
    /*
@@ -466,7 +505,7 @@ ProcessProcSignalBarrier(void)
     * things have changed further, it'll get fixed up when this function is
     * next called.
     */
-   pg_atomic_write_u64(&MyProcSignalSlot->pss_barrierGeneration, generation);
+   pg_atomic_write_u64(&MyProcSignalSlot->pss_barrierGeneration, shared_gen);
 }
 
 static void
@@ -505,27 +544,6 @@ CheckProcSignal(ProcSignalReason reason)
    return false;
 }
 
-/*
- * CheckProcSignalBarrier - check for new barriers we need to absorb
- */
-static bool
-CheckProcSignalBarrier(void)
-{
-   volatile ProcSignalSlot *slot = MyProcSignalSlot;
-
-   if (slot != NULL)
-   {
-       uint64      mygen;
-       uint64      curgen;
-
-       mygen = pg_atomic_read_u64(&slot->pss_barrierGeneration);
-       curgen = pg_atomic_read_u64(&ProcSignal->psh_barrierGeneration);
-       return (mygen != curgen);
-   }
-
-   return false;
-}
-
 /*
  * procsignal_sigusr1_handler - handle SIGUSR1 signal.
  */
@@ -546,6 +564,9 @@ procsignal_sigusr1_handler(SIGNAL_ARGS)
    if (CheckProcSignal(PROCSIG_WALSND_INIT_STOPPING))
        HandleWalSndInitStopping();
 
+   if (CheckProcSignal(PROCSIG_BARRIER))
+       HandleProcSignalBarrierInterrupt();
+
    if (CheckProcSignal(PROCSIG_RECOVERY_CONFLICT_DATABASE))
        RecoveryConflictInterrupt(PROCSIG_RECOVERY_CONFLICT_DATABASE);
 
@@ -564,12 +585,6 @@ procsignal_sigusr1_handler(SIGNAL_ARGS)
    if (CheckProcSignal(PROCSIG_RECOVERY_CONFLICT_BUFFERPIN))
        RecoveryConflictInterrupt(PROCSIG_RECOVERY_CONFLICT_BUFFERPIN);
 
-   if (CheckProcSignalBarrier())
-   {
-       InterruptPending = true;
-       ProcSignalBarrierPending = true;
-   }
-
    SetLatch(MyLatch);
 
    latch_sigusr1_handler();

diff --git a/src/include/storage/procsignal.h b/src/include/storage/procsignal.h
index a0c0bc3ce55348eb07bf485bfcc38d8711d35c4c..5cb39697f38f9f34f1e5e3873a99d7899051c785 100644 (file)
--- a/src/include/storage/procsignal.h
+++ b/src/include/storage/procsignal.h
@@ -33,6 +33,7 @@ typedef enum
    PROCSIG_NOTIFY_INTERRUPT,   /* listen/notify interrupt */
    PROCSIG_PARALLEL_MESSAGE,   /* message from cooperating parallel backend */
    PROCSIG_WALSND_INIT_STOPPING,   /* ask walsenders to prepare for shutdown  */
+   PROCSIG_BARRIER,            /* global barrier interrupt  */
 
    /* Recovery conflict reasons */
    PROCSIG_RECOVERY_CONFLICT_DATABASE,