From 17a7730c282018d02e3d82990f4fd84e0d2f6ce0 Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Mon, 8 Jun 2020 17:10:26 -0700 Subject: [PATCH v1 1/4] WORKAROUND: Avoid spinlock use in signal handler by using 32bit generations. As 64bit atomic reads aren't aren't guaranteed to be atomic on all platforms we can end up with spinlocks being used inside signal handlers. Which obviously is dangerous, as that can easily create self deadlocks. 32bit atomics don't have that problem. But 32bit generations might not be enough in all situations (?). So this possibly isn't the right fix. The problem can easily be reproduced by running make check in a --disable-spinlocks --disable-atomics build. Author: Reviewed-By: Discussion: https://postgr.es/m/ Backpatch: --- src/include/storage/procsignal.h | 4 +-- src/backend/storage/ipc/procsignal.c | 48 ++++++++++++++-------------- 2 files changed, 26 insertions(+), 26 deletions(-) diff --git a/src/include/storage/procsignal.h b/src/include/storage/procsignal.h index a0c0bc3ce55..c5a7e362dd9 100644 --- a/src/include/storage/procsignal.h +++ b/src/include/storage/procsignal.h @@ -65,8 +65,8 @@ extern void ProcSignalInit(int pss_idx); extern int SendProcSignal(pid_t pid, ProcSignalReason reason, BackendId backendId); -extern uint64 EmitProcSignalBarrier(ProcSignalBarrierType type); -extern void WaitForProcSignalBarrier(uint64 generation); +extern uint32 EmitProcSignalBarrier(ProcSignalBarrierType type); +extern void WaitForProcSignalBarrier(uint32 generation); extern void ProcessProcSignalBarrier(void); extern void procsignal_sigusr1_handler(SIGNAL_ARGS); diff --git a/src/backend/storage/ipc/procsignal.c b/src/backend/storage/ipc/procsignal.c index c809196d06a..eba424e15a7 100644 --- a/src/backend/storage/ipc/procsignal.c +++ b/src/backend/storage/ipc/procsignal.c @@ -60,7 +60,7 @@ typedef struct { pid_t pss_pid; sig_atomic_t pss_signalFlags[NUM_PROCSIGNALS]; - pg_atomic_uint64 pss_barrierGeneration; + pg_atomic_uint32 pss_barrierGeneration; pg_atomic_uint32 pss_barrierCheckMask; } ProcSignalSlot; @@ -72,7 +72,7 @@ typedef struct */ typedef struct { - pg_atomic_uint64 psh_barrierGeneration; + pg_atomic_uint32 psh_barrierGeneration; ProcSignalSlot psh_slot[FLEXIBLE_ARRAY_MEMBER]; } ProcSignalHeader; @@ -126,7 +126,7 @@ ProcSignalShmemInit(void) { int i; - pg_atomic_init_u64(&ProcSignal->psh_barrierGeneration, 0); + pg_atomic_init_u32(&ProcSignal->psh_barrierGeneration, 0); for (i = 0; i < NumProcSignalSlots; ++i) { @@ -134,7 +134,7 @@ ProcSignalShmemInit(void) slot->pss_pid = 0; MemSet(slot->pss_signalFlags, 0, sizeof(slot->pss_signalFlags)); - pg_atomic_init_u64(&slot->pss_barrierGeneration, PG_UINT64_MAX); + pg_atomic_init_u32(&slot->pss_barrierGeneration, PG_UINT32_MAX); pg_atomic_init_u32(&slot->pss_barrierCheckMask, 0); } } @@ -151,7 +151,7 @@ void ProcSignalInit(int pss_idx) { volatile ProcSignalSlot *slot; - uint64 barrier_generation; + uint32 barrier_generation; Assert(pss_idx >= 1 && pss_idx <= NumProcSignalSlots); @@ -178,8 +178,8 @@ ProcSignalInit(int pss_idx) */ pg_atomic_write_u32(&slot->pss_barrierCheckMask, 0); barrier_generation = - pg_atomic_read_u64(&ProcSignal->psh_barrierGeneration); - pg_atomic_write_u64(&slot->pss_barrierGeneration, barrier_generation); + pg_atomic_read_u32(&ProcSignal->psh_barrierGeneration); + pg_atomic_write_u32(&slot->pss_barrierGeneration, barrier_generation); pg_memory_barrier(); /* Mark slot with my PID */ @@ -230,7 +230,7 @@ CleanupProcSignalState(int status, Datum arg) * Make this slot look like it's absorbed all possible barriers, so that * no barrier waits block on it. */ - pg_atomic_write_u64(&slot->pss_barrierGeneration, PG_UINT64_MAX); + pg_atomic_write_u32(&slot->pss_barrierGeneration, PG_UINT32_MAX); slot->pss_pid = 0; } @@ -317,11 +317,11 @@ SendProcSignal(pid_t pid, ProcSignalReason reason, BackendId backendId) * Callers are entitled to assume that this function will not throw ERROR * or FATAL. */ -uint64 +uint32 EmitProcSignalBarrier(ProcSignalBarrierType type) { - uint64 flagbit = UINT64CONST(1) << (uint64) type; - uint64 generation; + uint32 flagbit = 1 << (uint32) type; + uint32 generation; /* * Set all the flags. @@ -329,7 +329,7 @@ EmitProcSignalBarrier(ProcSignalBarrierType type) * Note that pg_atomic_fetch_or_u32 has full barrier semantics, so this is * totally ordered with respect to anything the caller did before, and * anything that we do afterwards. (This is also true of the later call to - * pg_atomic_add_fetch_u64.) + * pg_atomic_add_fetch_u32.) */ for (int i = 0; i < NumProcSignalSlots; i++) { @@ -342,7 +342,7 @@ EmitProcSignalBarrier(ProcSignalBarrierType type) * Increment the generation counter. */ generation = - pg_atomic_add_fetch_u64(&ProcSignal->psh_barrierGeneration, 1); + pg_atomic_add_fetch_u32(&ProcSignal->psh_barrierGeneration, 1); /* * Signal all the processes, so that they update their advertised barrier @@ -379,16 +379,16 @@ EmitProcSignalBarrier(ProcSignalBarrierType type) * 1 second. */ void -WaitForProcSignalBarrier(uint64 generation) +WaitForProcSignalBarrier(uint32 generation) { long timeout = 125L; for (int i = NumProcSignalSlots - 1; i >= 0; i--) { volatile ProcSignalSlot *slot = &ProcSignal->psh_slot[i]; - uint64 oldval; + uint32 oldval; - oldval = pg_atomic_read_u64(&slot->pss_barrierGeneration); + oldval = pg_atomic_read_u32(&slot->pss_barrierGeneration); while (oldval < generation) { int events; @@ -401,7 +401,7 @@ WaitForProcSignalBarrier(uint64 generation) timeout, WAIT_EVENT_PROC_SIGNAL_BARRIER); ResetLatch(MyLatch); - oldval = pg_atomic_read_u64(&slot->pss_barrierGeneration); + oldval = pg_atomic_read_u32(&slot->pss_barrierGeneration); if (events & WL_TIMEOUT) timeout = Min(timeout * 2, 1000L); } @@ -428,7 +428,7 @@ WaitForProcSignalBarrier(uint64 generation) void ProcessProcSignalBarrier(void) { - uint64 generation; + uint32 generation; uint32 flags; /* Exit quickly if there's no work to do. */ @@ -443,7 +443,7 @@ ProcessProcSignalBarrier(void) * happens before we atomically extract the flags, and that any subsequent * state changes happen afterward. */ - generation = pg_atomic_read_u64(&ProcSignal->psh_barrierGeneration); + generation = pg_atomic_read_u32(&ProcSignal->psh_barrierGeneration); flags = pg_atomic_exchange_u32(&MyProcSignalSlot->pss_barrierCheckMask, 0); /* @@ -466,7 +466,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_u32(&MyProcSignalSlot->pss_barrierGeneration, generation); } static void @@ -515,11 +515,11 @@ CheckProcSignalBarrier(void) if (slot != NULL) { - uint64 mygen; - uint64 curgen; + uint32 mygen; + uint32 curgen; - mygen = pg_atomic_read_u64(&slot->pss_barrierGeneration); - curgen = pg_atomic_read_u64(&ProcSignal->psh_barrierGeneration); + mygen = pg_atomic_read_u32(&slot->pss_barrierGeneration); + curgen = pg_atomic_read_u32(&ProcSignal->psh_barrierGeneration); return (mygen != curgen); } -- 2.25.0.114.g5b0ca878e0