Hi hackers,
Here are some more experimental patches to reduce system calls.
0001 skips sending signals when the recipient definitely isn't
waiting, using a new flag-and-memory-barrier dance. This seems to
skip around 12% of the kill() calls for "make check", and probably
helps with some replication configurations that do a lot of
signalling. Patch by Andres (with a small memory barrier adjustment
by me).
0002 gets rid of the latch self-pipe on Linux systems.
0003 does the same on *BSD/macOS systems.
The idea for 0002 and 0003 is to use a new dedicated signal just for
latch wakeups, and keep it blocked (Linux) or ignored (BSD), except
while waiting. There may be other ways to achieve this without
bringing in a new signal, but it seemed important to leave SIGUSR1
unblocked for procsignals, and hard to figure out how to multiplex
with existing SIGUSR2 users, so for the first attempt at prototyping
this I arbitrarily chose SIGURG.
From a45e9edaa315e62a0f823dbd55b42950fc8d5df3 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.mu...@gmail.com>
Date: Sat, 8 Aug 2020 15:08:09 +1200
Subject: [PATCH 1/3] Optimize latches to send fewer signals.
Author: Andres Freund <and...@anarazel.de>
---
src/backend/storage/ipc/latch.c | 26 ++++++++++++++++++++++++++
src/include/storage/latch.h | 1 +
2 files changed, 27 insertions(+)
diff --git a/src/backend/storage/ipc/latch.c b/src/backend/storage/ipc/latch.c
index 4153cc8557..37528f29b5 100644
--- a/src/backend/storage/ipc/latch.c
+++ b/src/backend/storage/ipc/latch.c
@@ -274,6 +274,7 @@ void
InitLatch(Latch *latch)
{
latch->is_set = false;
+ latch->maybe_sleeping = false;
latch->owner_pid = MyProcPid;
latch->is_shared = false;
@@ -321,6 +322,7 @@ InitSharedLatch(Latch *latch)
#endif
latch->is_set = false;
+ latch->maybe_sleeping = false;
latch->owner_pid = 0;
latch->is_shared = true;
}
@@ -523,6 +525,10 @@ SetLatch(Latch *latch)
latch->is_set = true;
+ pg_memory_barrier();
+ if (!latch->maybe_sleeping)
+ return;
+
#ifndef WIN32
/*
@@ -589,6 +595,7 @@ ResetLatch(Latch *latch)
{
/* Only the owner should reset the latch */
Assert(latch->owner_pid == MyProcPid);
+ Assert(latch->maybe_sleeping == false);
latch->is_set = false;
@@ -1273,6 +1280,14 @@ WaitEventSetWait(WaitEventSet *set, long timeout,
* ordering, so that we cannot miss seeing is_set if a notification
* has already been queued.
*/
+ if (set->latch && !set->latch->is_set)
+ {
+ /* about to sleep on a latch */
+ set->latch->maybe_sleeping = true;
+ pg_memory_barrier();
+ /* and recheck */
+ }
+
if (set->latch && set->latch->is_set)
{
occurred_events->fd = PGINVALID_SOCKET;
@@ -1283,6 +1298,9 @@ WaitEventSetWait(WaitEventSet *set, long timeout,
occurred_events++;
returned_events++;
+ /* could have been set above */
+ set->latch->maybe_sleeping = false;
+
break;
}
@@ -1294,6 +1312,12 @@ WaitEventSetWait(WaitEventSet *set, long timeout,
rc = WaitEventSetWaitBlock(set, cur_timeout,
occurred_events, nevents);
+ if (set->latch)
+ {
+ Assert(set->latch->maybe_sleeping);
+ set->latch->maybe_sleeping = false;
+ }
+
if (rc == -1)
break; /* timeout occurred */
else
@@ -1383,6 +1407,8 @@ WaitEventSetWaitBlock(WaitEventSet *set, int cur_timeout,
if (cur_event->events == WL_LATCH_SET &&
cur_epoll_event->events & (EPOLLIN | EPOLLERR | EPOLLHUP))
{
+ Assert(set->latch);
+
/* There's data in the self-pipe, clear it. */
drainSelfPipe();
diff --git a/src/include/storage/latch.h b/src/include/storage/latch.h
index 7c742021fb..d0da7e5d31 100644
--- a/src/include/storage/latch.h
+++ b/src/include/storage/latch.h
@@ -110,6 +110,7 @@
typedef struct Latch
{
sig_atomic_t is_set;
+ sig_atomic_t maybe_sleeping;
bool is_shared;
int owner_pid;
#ifdef WIN32
--
2.20.1
From 0cac26c1499fffa2a6f4e71f59e7a4e9a31be833 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.mu...@gmail.com>
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 <poll.h>
#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
From 8f3d33d5e854bd963ec72ff06b4f3b9302ff2b4b Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.mu...@gmail.com>
Date: Sat, 8 Aug 2020 07:52:47 +1200
Subject: [PATCH 3/3] Remove self-pipe in WAIT_USE_KQUEUE builds.
Instead of using a signal handler for latch wakeup, use EVFILT_SIGNAL.
This closes the race condition traditionally solved by the self-pipe
trick.
---
src/backend/storage/ipc/latch.c | 41 +++++++++++++++++++++++++--------
1 file changed, 32 insertions(+), 9 deletions(-)
diff --git a/src/backend/storage/ipc/latch.c b/src/backend/storage/ipc/latch.c
index 0cb5875228..ae3d904791 100644
--- a/src/backend/storage/ipc/latch.c
+++ b/src/backend/storage/ipc/latch.c
@@ -24,6 +24,9 @@
* Since epoll_pwait() atomically unblocks and begins waiting, there is no
* race, so there is no need for the self-pipe trick.
*
+ * The kqueue() implementation uses yet another technique: it uses SIGURG,
+ * but installs no signal handler, and waits for signals using EVFILT_SIGNAL.
+ *
* The Windows implementation uses Windows events that are inherited by all
* postmaster child processes. There's no need for the self-pipe trick there.
*
@@ -85,7 +88,7 @@
#error "no wait set implementation available"
#endif
-#if defined(WAIT_USE_POLL) || defined(WAIT_USE_KQUEUE)
+#if defined(WAIT_USE_POLL)
#define NEED_SELF_PIPE_TRICK
/* Use a signal that is overloaded with the procsignal mechanism. */
#define LATCH_SIGNAL SIGUSR1
@@ -267,6 +270,9 @@ InitializeLatchSupport(void)
#ifdef WAIT_USE_EPOLL
pqsignal(SIGURG, sigurg_handler);
#endif
+#ifdef WAIT_USE_KQUEUE
+ pqsignal(SIGURG, SIG_IGN);
+#endif
}
void
@@ -888,7 +894,9 @@ AddWaitEventToSet(WaitEventSet *set, uint32 events, pgsocket fd, Latch *latch,
event->fd = selfpipe_readfd;
#else
event->fd = PGINVALID_SOCKET;
+#ifdef WAIT_USE_EPOLL
return event->pos;
+#endif
#endif
}
else if (events == WL_POSTMASTER_DEATH)
@@ -1096,6 +1104,18 @@ WaitEventAdjustKqueueAddPostmaster(struct kevent *k_ev, WaitEvent *event)
AccessWaitEvent(k_ev) = event;
}
+static inline void
+WaitEventAdjustKqueueAddLatch(struct kevent *k_ev, WaitEvent *event)
+{
+ /* For now latch can only be added, not removed. */
+ k_ev->ident = SIGURG;
+ k_ev->filter = EVFILT_SIGNAL;
+ k_ev->flags = EV_ADD;
+ k_ev->fflags = 0;
+ k_ev->data = 0;
+ AccessWaitEvent(k_ev) = event;
+}
+
/*
* old_events is the previous event mask, used to compute what has changed.
*/
@@ -1127,6 +1147,14 @@ WaitEventAdjustKqueue(WaitEventSet *set, WaitEvent *event, int old_events)
*/
WaitEventAdjustKqueueAddPostmaster(&k_ev[count++], event);
}
+ else if (event->events == WL_LATCH_SET)
+ {
+ /*
+ * Unlike all the other implementations, we detect latch wakeup using
+ * signal events, rather than installing a signal handler.
+ */
+ WaitEventAdjustKqueueAddLatch(&k_ev[count++], event);
+ }
else
{
/*
@@ -1134,11 +1162,9 @@ WaitEventAdjustKqueue(WaitEventSet *set, WaitEvent *event, int old_events)
* old event mask to the new event mask, since kevent treats readable
* and writable as separate events.
*/
- if (old_events == WL_LATCH_SET ||
- (old_events & WL_SOCKET_READABLE))
+ if (old_events & WL_SOCKET_READABLE)
old_filt_read = true;
- if (event->events == WL_LATCH_SET ||
- (event->events & WL_SOCKET_READABLE))
+ if (event->events & WL_SOCKET_READABLE)
new_filt_read = true;
if (old_events & WL_SOCKET_WRITEABLE)
old_filt_write = true;
@@ -1591,11 +1617,8 @@ WaitEventSetWaitBlock(WaitEventSet *set, int cur_timeout,
occurred_events->events = 0;
if (cur_event->events == WL_LATCH_SET &&
- cur_kqueue_event->filter == EVFILT_READ)
+ cur_kqueue_event->filter == EVFILT_SIGNAL)
{
- /* There's data in the self-pipe, clear it. */
- drainSelfPipe();
-
if (set->latch->is_set)
{
occurred_events->fd = PGINVALID_SOCKET;
--
2.20.1
