From 0cac26c1499fffa2a6f4e71f59e7a4e9a31be833 Mon Sep 17 00:00:00 2001 From: Thomas Munro Date: Sat, 8 Aug 2020 16:32:22 +1200 Subject: [PATCH 2/3] Remove self-pipe in WAIT_USE_EPOLL builds. Use SIGURG instead of SIGUSR1 for latch wake-ups, and unblock it only while in epoll_pwait() to avoid the race traditionally solved by the self-pipe trick. --- src/backend/libpq/pqsignal.c | 13 +++- src/backend/postmaster/postmaster.c | 4 +- src/backend/storage/ipc/latch.c | 97 +++++++++++++++++++++++------ src/backend/tcop/postgres.c | 2 +- src/include/libpq/pqsignal.h | 3 +- src/include/storage/latch.h | 2 + 6 files changed, 96 insertions(+), 25 deletions(-) diff --git a/src/backend/libpq/pqsignal.c b/src/backend/libpq/pqsignal.c index 9289493118..13ec3cd533 100644 --- a/src/backend/libpq/pqsignal.c +++ b/src/backend/libpq/pqsignal.c @@ -20,6 +20,7 @@ /* Global variables */ sigset_t UnBlockSig, + UnBlockAllSig, BlockSig, StartupBlockSig; @@ -35,13 +36,21 @@ sigset_t UnBlockSig, * collection; it's essentially BlockSig minus SIGTERM, SIGQUIT, SIGALRM. * * UnBlockSig is the set of signals to block when we don't want to block - * signals (is this ever nonzero??) + * signals. */ void -pqinitmask(void) +pqinitmask(bool block_sigurg) { + sigemptyset(&UnBlockAllSig); sigemptyset(&UnBlockSig); + /* + * In WAIT_USE_EPOLL builds, we want to keep SIGURG blocked except while + * waiting. + */ + if (block_sigurg) + sigaddset(&UnBlockSig, SIGURG); + /* First set all signals, then clear some. */ sigfillset(&BlockSig); sigfillset(&StartupBlockSig); diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index 5b5fc97c72..b2bc5e9898 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -633,7 +633,7 @@ PostmasterMain(int argc, char *argv[]) * postmaster/syslogger.c, postmaster/bgworker.c and * postmaster/checkpointer.c. */ - pqinitmask(); + pqinitmask(LatchBlockedSIGURG()); PG_SETMASK(&BlockSig); pqsignal_pm(SIGHUP, SIGHUP_handler); /* reread config file and have @@ -4966,7 +4966,7 @@ SubPostmasterMain(int argc, char *argv[]) AutovacuumWorkerIAm(); /* In EXEC_BACKEND case we will not have inherited these settings */ - pqinitmask(); + pqinitmask(LatchBlockedSIGURG()); PG_SETMASK(&BlockSig); /* Read in remaining GUC variables */ diff --git a/src/backend/storage/ipc/latch.c b/src/backend/storage/ipc/latch.c index 37528f29b5..0cb5875228 100644 --- a/src/backend/storage/ipc/latch.c +++ b/src/backend/storage/ipc/latch.c @@ -3,7 +3,7 @@ * latch.c * Routines for inter-process latches * - * The Unix implementation uses the so-called self-pipe trick to overcome the + * The poll() implementation uses the so-called self-pipe trick to overcome the * race condition involved with poll() (or epoll_wait() on linux) and setting * a global flag in the signal handler. When a latch is set and the current * process is waiting for it, the signal handler wakes up the poll() in @@ -19,6 +19,11 @@ * process, SIGUSR1 is sent and the signal handler in the waiting process * writes the byte to the pipe on behalf of the signaling process. * + * The epoll() implementation overcomes the race with a different technique: + * it uses SIGURG to wake processes, but only unblocks it while waiting. + * Since epoll_pwait() atomically unblocks and begins waiting, there is no + * race, so there is no need for the self-pipe trick. + * * The Windows implementation uses Windows events that are inherited by all * postmaster child processes. There's no need for the self-pipe trick there. * @@ -46,6 +51,7 @@ #include #endif +#include "libpq/pqsignal.h" #include "miscadmin.h" #include "pgstat.h" #include "port/atomics.h" @@ -79,6 +85,15 @@ #error "no wait set implementation available" #endif +#if defined(WAIT_USE_POLL) || defined(WAIT_USE_KQUEUE) +#define NEED_SELF_PIPE_TRICK +/* Use a signal that is overloaded with the procsignal mechanism. */ +#define LATCH_SIGNAL SIGUSR1 +#else +/* Use a dedicated signal for latches if we have race-free waiting. */ +#define LATCH_SIGNAL SIGURG +#endif + /* typedef in latch.h */ struct WaitEventSet { @@ -140,6 +155,7 @@ static WaitEventSet *LatchWaitSet; /* Are we currently in WaitLatch? The signal handler would like to know. */ static volatile sig_atomic_t waiting = false; +#ifdef NEED_SELF_PIPE_TRICK /* Read and write ends of the self-pipe */ static int selfpipe_readfd = -1; static int selfpipe_writefd = -1; @@ -150,10 +166,12 @@ static int selfpipe_owner_pid = 0; /* Private function prototypes */ static void sendSelfPipeByte(void); static void drainSelfPipe(void); +#endif #endif /* WIN32 */ #if defined(WAIT_USE_EPOLL) static void WaitEventAdjustEpoll(WaitEventSet *set, WaitEvent *event, int action); +static void sigurg_handler(SIGNAL_ARGS); #elif defined(WAIT_USE_KQUEUE) static void WaitEventAdjustKqueue(WaitEventSet *set, WaitEvent *event, int old_events); #elif defined(WAIT_USE_POLL) @@ -174,7 +192,7 @@ static inline int WaitEventSetWaitBlock(WaitEventSet *set, int cur_timeout, void InitializeLatchSupport(void) { -#ifndef WIN32 +#ifdef NEED_SELF_PIPE_TRICK int pipefd[2]; if (IsUnderPostmaster) @@ -244,8 +262,10 @@ InitializeLatchSupport(void) /* Tell fd.c about these two long-lived FDs */ ReserveExternalFD(); ReserveExternalFD(); -#else - /* currently, nothing to do here for Windows */ +#endif + +#ifdef WAIT_USE_EPOLL + pqsignal(SIGURG, sigurg_handler); #endif } @@ -278,10 +298,10 @@ InitLatch(Latch *latch) latch->owner_pid = MyProcPid; latch->is_shared = false; -#ifndef WIN32 +#ifdef NEED_SELF_PIPE_TRICK /* Assert InitializeLatchSupport has been called in this process */ Assert(selfpipe_readfd >= 0 && selfpipe_owner_pid == MyProcPid); -#else +#elif defined(WIN32) latch->event = CreateEvent(NULL, TRUE, FALSE, NULL); if (latch->event == NULL) elog(ERROR, "CreateEvent failed: error code %lu", GetLastError()); @@ -337,8 +357,8 @@ InitSharedLatch(Latch *latch) * there is any risk of that, caller must provide an interlock to prevent it. * * In any process that calls OwnLatch(), make sure that - * latch_sigusr1_handler() is called from the SIGUSR1 signal handler, - * as shared latches use SIGUSR1 for inter-process communication. + * latch_sigusr1_handler() is called from the SIGUSR1 signal handler, as shared + * latches use SIGUSR1 for inter-process communication in some builds. */ void OwnLatch(Latch *latch) @@ -346,7 +366,7 @@ OwnLatch(Latch *latch) /* Sanity checks */ Assert(latch->is_shared); -#ifndef WIN32 +#ifdef NEED_SELF_PIPE_TRICK /* Assert InitializeLatchSupport has been called in this process */ Assert(selfpipe_readfd >= 0 && selfpipe_owner_pid == MyProcPid); #endif @@ -558,11 +578,17 @@ SetLatch(Latch *latch) return; else if (owner_pid == MyProcPid) { +#ifdef NEED_SELF_PIPE_TRICK if (waiting) sendSelfPipeByte(); +#else + if (waiting) + kill(MyProcPid, LATCH_SIGNAL); +#endif } else - kill(owner_pid, SIGUSR1); + kill(owner_pid, LATCH_SIGNAL); + #else /* @@ -858,8 +884,11 @@ AddWaitEventToSet(WaitEventSet *set, uint32 events, pgsocket fd, Latch *latch, { set->latch = latch; set->latch_pos = event->pos; -#ifndef WIN32 +#ifdef NEED_SELF_PIPE_TRICK event->fd = selfpipe_readfd; +#else + event->fd = PGINVALID_SOCKET; + return event->pos; #endif } else if (events == WL_POSTMASTER_DEATH) @@ -932,6 +961,9 @@ ModifyWaitEvent(WaitEventSet *set, int pos, uint32 events, Latch *latch) if (events == WL_LATCH_SET) { set->latch = latch; +#ifndef NEED_SELF_PIPE_TRICK + return; +#endif } #if defined(WAIT_USE_EPOLL) @@ -1346,7 +1378,7 @@ WaitEventSetWait(WaitEventSet *set, long timeout, #if defined(WAIT_USE_EPOLL) /* - * Wait using linux's epoll_wait(2). + * Wait using linux's epoll_pwait(2). * * This is the preferable wait method, as several readiness notifications are * delivered, without having to iterate through all of set->events. The return @@ -1363,8 +1395,8 @@ WaitEventSetWaitBlock(WaitEventSet *set, int cur_timeout, struct epoll_event *cur_epoll_event; /* Sleep */ - rc = epoll_wait(set->epoll_fd, set->epoll_ret_events, - nevents, cur_timeout); + rc = epoll_pwait(set->epoll_fd, set->epoll_ret_events, + nevents, cur_timeout, &UnBlockAllSig); /* Check return code */ if (rc < 0) @@ -1409,8 +1441,10 @@ WaitEventSetWaitBlock(WaitEventSet *set, int cur_timeout, { Assert(set->latch); +#ifdef NEED_SELF_PIPE_TRICK /* There's data in the self-pipe, clear it. */ drainSelfPipe(); +#endif if (set->latch->is_set) { @@ -1926,7 +1960,8 @@ WaitEventSetWaitBlock(WaitEventSet *set, int cur_timeout, #endif /* - * SetLatch uses SIGUSR1 to wake up the process waiting on the latch. + * SetLatch uses SIGUSR1 to wake up the process waiting on the latch in some + * builds. * * Wake up WaitLatch, if we're waiting. (We might not be, since SIGUSR1 is * overloaded for multiple purposes; or we might not have reached WaitLatch @@ -1939,13 +1974,15 @@ WaitEventSetWaitBlock(WaitEventSet *set, int cur_timeout, void latch_sigusr1_handler(void) { +#ifdef NEED_SELF_PIPE_TRICK if (waiting) sendSelfPipeByte(); +#endif } #endif /* !WIN32 */ /* Send one byte to the self-pipe, to wake up WaitLatch */ -#ifndef WIN32 +#ifdef NEED_SELF_PIPE_TRICK static void sendSelfPipeByte(void) { @@ -1975,7 +2012,7 @@ retry: return; } } -#endif /* !WIN32 */ +#endif /* !NEED_SELF_PIPE_TRICK */ /* * Read all available data from the self-pipe @@ -1984,7 +2021,7 @@ retry: * return, it must reset that flag first (though ideally, this will never * happen). */ -#ifndef WIN32 +#ifdef NEED_SELF_PIPE_TRICK static void drainSelfPipe(void) { @@ -2023,4 +2060,26 @@ drainSelfPipe(void) /* else buffer wasn't big enough, so read again */ } } -#endif /* !WIN32 */ +#endif /* !NEED_SELF_PIPE_TRICK */ + +#ifdef WAIT_USE_EPOLL +/* + * SIGURG handler. + * + * No action; this signal is intended only to wake up epoll_pwait(). + */ +static void +sigurg_handler(SIGNAL_ARGS) +{ +} +#endif + +bool +LatchBlockedSIGURG(void) +{ +#ifdef WAIT_USE_EPOLL + return true; +#else + return false; +#endif +} diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index c9424f167c..af5692d0ef 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -3871,7 +3871,7 @@ PostgresMain(int argc, char *argv[], * platforms */ } - pqinitmask(); + pqinitmask(LatchBlockedSIGURG()); if (IsUnderPostmaster) { diff --git a/src/include/libpq/pqsignal.h b/src/include/libpq/pqsignal.h index 695c4dc3f8..021f7fd6be 100644 --- a/src/include/libpq/pqsignal.h +++ b/src/include/libpq/pqsignal.h @@ -31,10 +31,11 @@ extern int pqsigsetmask(int mask); #endif /* WIN32 */ extern sigset_t UnBlockSig, + UnBlockAllSig, BlockSig, StartupBlockSig; -extern void pqinitmask(void); +extern void pqinitmask(bool should_block_sigurg); /* pqsigfunc is declared in src/include/port.h */ extern pqsigfunc pqsignal_pm(int signo, pqsigfunc func); diff --git a/src/include/storage/latch.h b/src/include/storage/latch.h index d0da7e5d31..01b98c326b 100644 --- a/src/include/storage/latch.h +++ b/src/include/storage/latch.h @@ -179,6 +179,8 @@ extern int WaitLatchOrSocket(Latch *latch, int wakeEvents, pgsocket sock, long timeout, uint32 wait_event_info); extern void InitializeLatchWaitSet(void); +extern bool LatchBlockedSIGURG(void); + /* * Unix implementation uses SIGUSR1 for inter-process signaling. * Win32 doesn't need this. -- 2.20.1