From 73d72693fc7a55ae327212f3932d2a45eb47d13b Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Mon, 15 Jun 2020 18:23:10 -0700 Subject: [PATCH v1 2/4] Avoid potential spinlock use inside a signal handler for 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 Discussion: https://postgr.es/m/20200609193723.eu5ilsjxwdpyxhgz@alap3.anarazel.de Backpatch: 13-, where the code was introduced. --- src/include/storage/procsignal.h | 1 + src/backend/storage/ipc/procsignal.c | 87 ++++++++++++++++------------ 2 files changed, 52 insertions(+), 36 deletions(-) diff --git a/src/include/storage/procsignal.h b/src/include/storage/procsignal.h index a0c0bc3ce55..5cb39697f38 100644 --- 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, diff --git a/src/backend/storage/ipc/procsignal.c b/src/backend/storage/ipc/procsignal.c index c809196d06a..4fa385b0ece 100644 --- 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(); -- 2.25.0.114.g5b0ca878e0