From d3fc362ec2ce1cf095950dddf24061915f836c22 Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Fri, 28 Dec 2001 18:16:43 +0000
Subject: [PATCH] Ensure that all direct uses of spinlock-protected data
structures use 'volatile' pointers to access those structures, so that
optimizing compilers will not decide to move the structure accesses outside
of the spinlock-acquire-to-spinlock-release sequence. There are no known
bugs in these uses at present, but based on bad experience with lwlock.c, it
seems prudent to ensure that we protect these other uses too. Per pghackers
discussion around 12-Dec. (Note: it should not be necessary to worry about
structures protected by LWLocks, since the LWLock acquire and release
operations are not inline macros.)
---
src/backend/access/transam/xlog.c | 78 ++++++++++++++++++++-----------
src/backend/storage/ipc/shmem.c | 14 +++---
src/backend/storage/lmgr/proc.c | 25 ++++++----
3 files changed, 75 insertions(+), 42 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index b1ea722674e..46e3470ae38 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -7,7 +7,7 @@
* Portions Copyright (c) 1996-2001, PostgreSQL Global Development Group
* Portions Copyright (c) 1994, Regents of the University of California
*
- * $Header: /cvsroot/pgsql/src/backend/access/transam/xlog.c,v 1.84 2001/12/23 07:25:39 tgl Exp $
+ * $Header: /cvsroot/pgsql/src/backend/access/transam/xlog.c,v 1.85 2001/12/28 18:16:41 tgl Exp $
*
*-------------------------------------------------------------------------
*/
@@ -617,10 +617,15 @@ begin:;
START_CRIT_SECTION();
/* update LogwrtResult before doing cache fill check */
- SpinLockAcquire_NoHoldoff(&XLogCtl->info_lck);
- LogwrtRqst = XLogCtl->LogwrtRqst;
- LogwrtResult = XLogCtl->LogwrtResult;
- SpinLockRelease_NoHoldoff(&XLogCtl->info_lck);
+ {
+ /* use volatile pointer to prevent code rearrangement */
+ volatile XLogCtlData *xlogctl = XLogCtl;
+
+ SpinLockAcquire_NoHoldoff(&xlogctl->info_lck);
+ LogwrtRqst = xlogctl->LogwrtRqst;
+ LogwrtResult = xlogctl->LogwrtResult;
+ SpinLockRelease_NoHoldoff(&xlogctl->info_lck);
+ }
/*
* If cache is half filled then try to acquire write lock and do
@@ -838,16 +843,20 @@ begin:;
if (updrqst)
{
- SpinLockAcquire_NoHoldoff(&XLogCtl->info_lck);
+ /* use volatile pointer to prevent code rearrangement */
+ volatile XLogCtlData *xlogctl = XLogCtl;
+
+ SpinLockAcquire_NoHoldoff(&xlogctl->info_lck);
/* advance global request to include new block(s) */
- if (XLByteLT(XLogCtl->LogwrtRqst.Write, WriteRqst))
- XLogCtl->LogwrtRqst.Write = WriteRqst;
+ if (XLByteLT(xlogctl->LogwrtRqst.Write, WriteRqst))
+ xlogctl->LogwrtRqst.Write = WriteRqst;
/* update local result copy while I have the chance */
- LogwrtResult = XLogCtl->LogwrtResult;
- SpinLockRelease_NoHoldoff(&XLogCtl->info_lck);
+ LogwrtResult = xlogctl->LogwrtResult;
+ SpinLockRelease_NoHoldoff(&xlogctl->info_lck);
}
END_CRIT_SECTION();
+
return (RecPtr);
}
@@ -892,11 +901,16 @@ AdvanceXLInsertBuffer(void)
FinishedPageRqstPtr = XLogCtl->xlblocks[Insert->curridx];
/* Before waiting, get info_lck and update LogwrtResult */
- SpinLockAcquire_NoHoldoff(&XLogCtl->info_lck);
- if (XLByteLT(XLogCtl->LogwrtRqst.Write, FinishedPageRqstPtr))
- XLogCtl->LogwrtRqst.Write = FinishedPageRqstPtr;
- LogwrtResult = XLogCtl->LogwrtResult;
- SpinLockRelease_NoHoldoff(&XLogCtl->info_lck);
+ {
+ /* use volatile pointer to prevent code rearrangement */
+ volatile XLogCtlData *xlogctl = XLogCtl;
+
+ SpinLockAcquire_NoHoldoff(&xlogctl->info_lck);
+ if (XLByteLT(xlogctl->LogwrtRqst.Write, FinishedPageRqstPtr))
+ xlogctl->LogwrtRqst.Write = FinishedPageRqstPtr;
+ LogwrtResult = xlogctl->LogwrtResult;
+ SpinLockRelease_NoHoldoff(&xlogctl->info_lck);
+ }
update_needed = false; /* Did the shared-request update */
@@ -1149,13 +1163,18 @@ XLogWrite(XLogwrtRqst WriteRqst)
* 'result' values. This is not absolutely essential, but it saves
* some code in a couple of places.
*/
- SpinLockAcquire_NoHoldoff(&XLogCtl->info_lck);
- XLogCtl->LogwrtResult = LogwrtResult;
- if (XLByteLT(XLogCtl->LogwrtRqst.Write, LogwrtResult.Write))
- XLogCtl->LogwrtRqst.Write = LogwrtResult.Write;
- if (XLByteLT(XLogCtl->LogwrtRqst.Flush, LogwrtResult.Flush))
- XLogCtl->LogwrtRqst.Flush = LogwrtResult.Flush;
- SpinLockRelease_NoHoldoff(&XLogCtl->info_lck);
+ {
+ /* use volatile pointer to prevent code rearrangement */
+ volatile XLogCtlData *xlogctl = XLogCtl;
+
+ SpinLockAcquire_NoHoldoff(&xlogctl->info_lck);
+ xlogctl->LogwrtResult = LogwrtResult;
+ if (XLByteLT(xlogctl->LogwrtRqst.Write, LogwrtResult.Write))
+ xlogctl->LogwrtRqst.Write = LogwrtResult.Write;
+ if (XLByteLT(xlogctl->LogwrtRqst.Flush, LogwrtResult.Flush))
+ xlogctl->LogwrtRqst.Flush = LogwrtResult.Flush;
+ SpinLockRelease_NoHoldoff(&xlogctl->info_lck);
+ }
Write->LogwrtResult = LogwrtResult;
}
@@ -1206,11 +1225,16 @@ XLogFlush(XLogRecPtr record)
WriteRqstPtr = record;
/* read LogwrtResult and update local state */
- SpinLockAcquire_NoHoldoff(&XLogCtl->info_lck);
- if (XLByteLT(WriteRqstPtr, XLogCtl->LogwrtRqst.Write))
- WriteRqstPtr = XLogCtl->LogwrtRqst.Write;
- LogwrtResult = XLogCtl->LogwrtResult;
- SpinLockRelease_NoHoldoff(&XLogCtl->info_lck);
+ {
+ /* use volatile pointer to prevent code rearrangement */
+ volatile XLogCtlData *xlogctl = XLogCtl;
+
+ SpinLockAcquire_NoHoldoff(&xlogctl->info_lck);
+ if (XLByteLT(WriteRqstPtr, xlogctl->LogwrtRqst.Write))
+ WriteRqstPtr = xlogctl->LogwrtRqst.Write;
+ LogwrtResult = xlogctl->LogwrtResult;
+ SpinLockRelease_NoHoldoff(&xlogctl->info_lck);
+ }
/* done already? */
if (!XLByteLE(record, LogwrtResult.Flush))
diff --git a/src/backend/storage/ipc/shmem.c b/src/backend/storage/ipc/shmem.c
index 3304ec67c59..2b0eee5e4f2 100644
--- a/src/backend/storage/ipc/shmem.c
+++ b/src/backend/storage/ipc/shmem.c
@@ -8,7 +8,7 @@
*
*
* IDENTIFICATION
- * $Header: /cvsroot/pgsql/src/backend/storage/ipc/shmem.c,v 1.62 2001/10/25 05:49:42 momjian Exp $
+ * $Header: /cvsroot/pgsql/src/backend/storage/ipc/shmem.c,v 1.63 2001/12/28 18:16:43 tgl Exp $
*
*-------------------------------------------------------------------------
*/
@@ -132,21 +132,23 @@ ShmemAlloc(Size size)
{
uint32 newFree;
void *newSpace;
+ /* use volatile pointer to prevent code rearrangement */
+ volatile PGShmemHeader *shmemseghdr = ShmemSegHdr;
/*
* ensure all space is adequately aligned.
*/
size = MAXALIGN(size);
- Assert(ShmemSegHdr != NULL);
+ Assert(shmemseghdr != NULL);
SpinLockAcquire(ShmemLock);
- newFree = ShmemSegHdr->freeoffset + size;
- if (newFree <= ShmemSegHdr->totalsize)
+ newFree = shmemseghdr->freeoffset + size;
+ if (newFree <= shmemseghdr->totalsize)
{
- newSpace = (void *) MAKE_PTR(ShmemSegHdr->freeoffset);
- ShmemSegHdr->freeoffset = newFree;
+ newSpace = (void *) MAKE_PTR(shmemseghdr->freeoffset);
+ shmemseghdr->freeoffset = newFree;
}
else
newSpace = NULL;
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 5fa8f133e57..b1be68a881f 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -8,7 +8,7 @@
*
*
* IDENTIFICATION
- * $Header: /cvsroot/pgsql/src/backend/storage/lmgr/proc.c,v 1.116 2001/11/08 20:37:52 momjian Exp $
+ * $Header: /cvsroot/pgsql/src/backend/storage/lmgr/proc.c,v 1.117 2001/12/28 18:16:43 tgl Exp $
*
*-------------------------------------------------------------------------
*/
@@ -203,12 +203,14 @@ void
InitProcess(void)
{
SHMEM_OFFSET myOffset;
+ /* use volatile pointer to prevent code rearrangement */
+ volatile PROC_HDR *procglobal = ProcGlobal;
/*
* ProcGlobal should be set by a previous call to InitProcGlobal (if
* we are a backend, we inherit this by fork() from the postmaster).
*/
- if (ProcGlobal == NULL)
+ if (procglobal == NULL)
elog(STOP, "InitProcess: Proc Header uninitialized");
if (MyProc != NULL)
@@ -219,12 +221,12 @@ InitProcess(void)
*/
SpinLockAcquire(ProcStructLock);
- myOffset = ProcGlobal->freeProcs;
+ myOffset = procglobal->freeProcs;
if (myOffset != INVALID_OFFSET)
{
MyProc = (PROC *) MAKE_PTR(myOffset);
- ProcGlobal->freeProcs = MyProc->links.next;
+ procglobal->freeProcs = MyProc->links.next;
SpinLockRelease(ProcStructLock);
}
else
@@ -437,6 +439,9 @@ ProcReleaseLocks(bool isCommit)
static void
ProcKill(void)
{
+ /* use volatile pointer to prevent code rearrangement */
+ volatile PROC_HDR *procglobal = ProcGlobal;
+
Assert(MyProc != NULL);
/* Release any LW locks I am holding */
@@ -463,8 +468,8 @@ ProcKill(void)
ProcFreeSem(MyProc->sem.semId, MyProc->sem.semNum);
/* Add PROC struct to freelist so space can be recycled in future */
- MyProc->links.next = ProcGlobal->freeProcs;
- ProcGlobal->freeProcs = MAKE_OFFSET(MyProc);
+ MyProc->links.next = procglobal->freeProcs;
+ procglobal->freeProcs = MAKE_OFFSET(MyProc);
/* PROC struct isn't mine anymore */
MyProc = NULL;
@@ -1044,10 +1049,12 @@ disable_sigalrm_interrupt(void)
static void
ProcGetNewSemIdAndNum(IpcSemaphoreId *semId, int *semNum)
{
- int i;
- int semMapEntries = ProcGlobal->semMapEntries;
- SEM_MAP_ENTRY *procSemMap = ProcGlobal->procSemMap;
+ /* use volatile pointer to prevent code rearrangement */
+ volatile PROC_HDR *procglobal = ProcGlobal;
+ int semMapEntries = procglobal->semMapEntries;
+ volatile SEM_MAP_ENTRY *procSemMap = procglobal->procSemMap;
int32 fullmask = (1 << PROC_NSEMS_PER_SET) - 1;
+ int i;
SpinLockAcquire(ProcStructLock);
--
GitLab