From d06995710bc7e347d39866c1793ae282498d65e0 Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Tue, 3 Feb 2015 23:25:00 +0100 Subject: [PATCH] Remove the option to service interrupts during PGSemaphoreLock(). The remaining caller (lwlocks) doesn't need that facility, and we plan to remove ImmedidateInterruptOK entirely. That means that interrupts can't be serviced race-free and portably anyway, so there's little reason for keeping the feature. Reviewed-By: Heikki Linnakangas --- src/backend/port/posix_sema.c | 12 ++------- src/backend/port/sysv_sema.c | 43 +++---------------------------- src/backend/port/win32_sema.c | 6 +---- src/backend/storage/lmgr/lwlock.c | 12 +++------ src/include/storage/pg_sema.h | 2 +- 5 files changed, 12 insertions(+), 63 deletions(-) diff --git a/src/backend/port/posix_sema.c b/src/backend/port/posix_sema.c index 633bf5afeb..ad9c80a8b5 100644 --- a/src/backend/port/posix_sema.c +++ b/src/backend/port/posix_sema.c @@ -236,22 +236,14 @@ PGSemaphoreReset(PGSemaphore sema) * Lock a semaphore (decrement count), blocking if count would be < 0 */ void -PGSemaphoreLock(PGSemaphore sema, bool interruptOK) +PGSemaphoreLock(PGSemaphore sema) { int errStatus; - /* - * See notes in sysv_sema.c's implementation of PGSemaphoreLock. Just as - * that code does for semop(), we handle both the case where sem_wait() - * returns errno == EINTR after a signal, and the case where it just keeps - * waiting. - */ + /* See notes in sysv_sema.c's implementation of PGSemaphoreLock. */ do { - ImmediateInterruptOK = interruptOK; - CHECK_FOR_INTERRUPTS(); errStatus = sem_wait(PG_SEM_REF(sema)); - ImmediateInterruptOK = false; } while (errStatus < 0 && errno == EINTR); if (errStatus < 0) diff --git a/src/backend/port/sysv_sema.c b/src/backend/port/sysv_sema.c index 5a0193f06b..ba46c640fc 100644 --- a/src/backend/port/sysv_sema.c +++ b/src/backend/port/sysv_sema.c @@ -364,7 +364,7 @@ PGSemaphoreReset(PGSemaphore sema) * Lock a semaphore (decrement count), blocking if count would be < 0 */ void -PGSemaphoreLock(PGSemaphore sema, bool interruptOK) +PGSemaphoreLock(PGSemaphore sema) { int errStatus; struct sembuf sops; @@ -378,48 +378,13 @@ PGSemaphoreLock(PGSemaphore sema, bool interruptOK) * from the operation prematurely because we were sent a signal. So we * try and lock the semaphore again. * - * Each time around the loop, we check for a cancel/die interrupt. On - * some platforms, if such an interrupt comes in while we are waiting, it - * will cause the semop() call to exit with errno == EINTR, allowing us to - * service the interrupt (if not in a critical section already) during the - * next loop iteration. - * - * Once we acquire the lock, we do NOT check for an interrupt before - * returning. The caller needs to be able to record ownership of the lock - * before any interrupt can be accepted. - * - * There is a window of a few instructions between CHECK_FOR_INTERRUPTS - * and entering the semop() call. If a cancel/die interrupt occurs in - * that window, we would fail to notice it until after we acquire the lock - * (or get another interrupt to escape the semop()). We can avoid this - * problem by temporarily setting ImmediateInterruptOK to true before we - * do CHECK_FOR_INTERRUPTS; then, a die() interrupt in this interval will - * execute directly. However, there is a huge pitfall: there is another - * window of a few instructions after the semop() before we are able to - * reset ImmediateInterruptOK. If an interrupt occurs then, we'll lose - * control, which means that the lock has been acquired but our caller did - * not get a chance to record the fact. Therefore, we only set - * ImmediateInterruptOK if the caller tells us it's OK to do so, ie, the - * caller does not need to record acquiring the lock. (This is currently - * true for lockmanager locks, since the process that granted us the lock - * did all the necessary state updates. It's not true for SysV semaphores - * used to implement LW locks or emulate spinlocks --- but the wait time - * for such locks should not be very long, anyway.) - * - * On some platforms, signals marked SA_RESTART (which is most, for us) - * will not interrupt the semop(); it will just keep waiting. Therefore - * it's necessary for cancel/die interrupts to be serviced directly by the - * signal handler. On these platforms the behavior is really the same - * whether the signal arrives just before the semop() begins, or while it - * is waiting. The loop on EINTR is thus important only for platforms - * without SA_RESTART. + * We used to check interrupts here, but that required servicing + * interrupts directly from signal handlers. Which is hard to do safely + * and portably. */ do { - ImmediateInterruptOK = interruptOK; - CHECK_FOR_INTERRUPTS(); errStatus = semop(sema->semId, &sops, 1); - ImmediateInterruptOK = false; } while (errStatus < 0 && errno == EINTR); if (errStatus < 0) diff --git a/src/backend/port/win32_sema.c b/src/backend/port/win32_sema.c index f848ff82b0..011e2fd4a6 100644 --- a/src/backend/port/win32_sema.c +++ b/src/backend/port/win32_sema.c @@ -116,13 +116,11 @@ PGSemaphoreReset(PGSemaphore sema) * Serve the interrupt if interruptOK is true. */ void -PGSemaphoreLock(PGSemaphore sema, bool interruptOK) +PGSemaphoreLock(PGSemaphore sema) { HANDLE wh[2]; bool done = false; - ImmediateInterruptOK = interruptOK; - /* * Note: pgwin32_signal_event should be first to ensure that it will be * reported when multiple events are set. We want to guarantee that @@ -173,8 +171,6 @@ PGSemaphoreLock(PGSemaphore sema, bool interruptOK) break; } } - - ImmediateInterruptOK = false; } /* diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c index 7cb002357a..7ca8dc0007 100644 --- a/src/backend/storage/lmgr/lwlock.c +++ b/src/backend/storage/lmgr/lwlock.c @@ -863,8 +863,7 @@ LWLockDequeueSelf(LWLock *lock) */ for (;;) { - /* "false" means cannot accept cancel/die interrupt here. */ - PGSemaphoreLock(&MyProc->sem, false); + PGSemaphoreLock(&MyProc->sem); if (!MyProc->lwWaiting) break; extraWaits++; @@ -1034,8 +1033,7 @@ LWLockAcquireCommon(LWLock *lock, LWLockMode mode, uint64 *valptr, uint64 val) for (;;) { - /* "false" means cannot accept cancel/die interrupt here. */ - PGSemaphoreLock(&proc->sem, false); + PGSemaphoreLock(&proc->sem); if (!proc->lwWaiting) break; extraWaits++; @@ -1195,8 +1193,7 @@ LWLockAcquireOrWait(LWLock *lock, LWLockMode mode) for (;;) { - /* "false" means cannot accept cancel/die interrupt here. */ - PGSemaphoreLock(&proc->sem, false); + PGSemaphoreLock(&proc->sem); if (!proc->lwWaiting) break; extraWaits++; @@ -1397,8 +1394,7 @@ LWLockWaitForVar(LWLock *lock, uint64 *valptr, uint64 oldval, uint64 *newval) for (;;) { - /* "false" means cannot accept cancel/die interrupt here. */ - PGSemaphoreLock(&proc->sem, false); + PGSemaphoreLock(&proc->sem); if (!proc->lwWaiting) break; extraWaits++; diff --git a/src/include/storage/pg_sema.h b/src/include/storage/pg_sema.h index 5eaf2bf582..9f8be759fb 100644 --- a/src/include/storage/pg_sema.h +++ b/src/include/storage/pg_sema.h @@ -72,7 +72,7 @@ extern void PGSemaphoreCreate(PGSemaphore sema); extern void PGSemaphoreReset(PGSemaphore sema); /* Lock a semaphore (decrement count), blocking if count would be < 0 */ -extern void PGSemaphoreLock(PGSemaphore sema, bool interruptOK); +extern void PGSemaphoreLock(PGSemaphore sema); /* Unlock a semaphore (increment count) */ extern void PGSemaphoreUnlock(PGSemaphore sema);