Hi,
On 2014-09-10 14:53:07 -0400, Robert Haas wrote:
As discussed on the thread Spinlocks and compiler/memory barriers,
now that we've made the spinlock primitives function as compiler
barriers (we think), it should be possible to remove volatile
qualifiers from many places in the source code. The attached patch
does this in lwlock.c. If the changes in commit
0709b7ee72e4bc71ad07b7120acd117265ab51d0 (and follow-on commits) are
correct and complete, applying this shouldn't break anything, while
possibly giving the compiler room to optimize things better than it
does today.
However, demonstrating the necessity of that commit for these changes
seems to be non-trivial. I tried applying this patch and reverting
commits 5b26278822c69dd76ef89fd50ecc7cdba9c3f035,
b4c28d1b92c81941e4fc124884e51a7c110316bf, and
0709b7ee72e4bc71ad07b7120acd117265ab51d0 on a PPC64 POWER8 box with a
whopping 192 hardware threads (thanks, IBM!). I then ran the
regression tests repeatedly, and I ran several long pgbench runs with
as many as 350 concurrent clients. No failures.
There's actually one more commit to revert. What I used was:
git revert 5b26278822c69dd76ef89fd50ecc7cdba9c3f035 \
b4c28d1b92c81941e4fc124884e51a7c110316bf \
68e66923ff629c324e219090860dc9e0e0a6f5d6 \
0709b7ee72e4bc71ad07b7120acd117265ab51d0
So I'm posting this patch in the hope that others can help. The
relevant tests are:
1. If you apply this patch to master and run tests of whatever kind
strikes your fancy, does anything break under high concurrency? If it
does, then the above commits weren't enough to make this safe on your
platform.
2. If you apply this patch to master, revert the commits mentioned
above, and again run tests, does anything now break? If it does (but
the first tests were OK), then that shows that those commits did
something useful on your platform.
I just tried this on my normal x86 workstation. I applied your lwlock
patch and ontop I removed most volatiles (there's a couple still
required) from xlog.c. Works for 100 seconds. Then I reverted the above
commits. Breaks within seconds:
master:
LOG: request to flush past end of generated WAL; request 2/E5EC3DE0, currpos
2/E5EC1E60
standby:
LOG: record with incorrect prev-link 4/684C3108 at 4/684C3108
and similar.
So at least for x86 the compiler barriers are obviously required and
seemingly working.
I've attached the very quickly written xlog.c de-volatizing patch.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training Services
From 2b68a134925e4b2fb6ff282f5ed83b8f57b10732 Mon Sep 17 00:00:00 2001
From: Andres Freund and...@anarazel.de
Date: Wed, 17 Sep 2014 13:21:20 +0200
Subject: [PATCH] xlog.c-remove-volatile
---
src/backend/access/transam/xlog.c | 473 ++
1 file changed, 176 insertions(+), 297 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 34f2fc0..103f077 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -1219,16 +1219,13 @@ begin:;
*/
if (StartPos / XLOG_BLCKSZ != EndPos / XLOG_BLCKSZ)
{
- /* use volatile pointer to prevent code rearrangement */
- volatile XLogCtlData *xlogctl = XLogCtl;
-
- SpinLockAcquire(xlogctl-info_lck);
+ SpinLockAcquire(XLogCtl-info_lck);
/* advance global request to include new block(s) */
- if (xlogctl-LogwrtRqst.Write EndPos)
- xlogctl-LogwrtRqst.Write = EndPos;
+ if (XLogCtl-LogwrtRqst.Write EndPos)
+ XLogCtl-LogwrtRqst.Write = EndPos;
/* update local result copy while I have the chance */
- LogwrtResult = xlogctl-LogwrtResult;
- SpinLockRelease(xlogctl-info_lck);
+ LogwrtResult = XLogCtl-LogwrtResult;
+ SpinLockRelease(XLogCtl-info_lck);
}
/*
@@ -1323,7 +1320,7 @@ static void
ReserveXLogInsertLocation(int size, XLogRecPtr *StartPos, XLogRecPtr *EndPos,
XLogRecPtr *PrevPtr)
{
- volatile XLogCtlInsert *Insert = XLogCtl-Insert;
+ XLogCtlInsert *Insert = XLogCtl-Insert;
uint64 startbytepos;
uint64 endbytepos;
uint64 prevbytepos;
@@ -1378,7 +1375,7 @@ ReserveXLogInsertLocation(int size, XLogRecPtr *StartPos, XLogRecPtr *EndPos,
static bool
ReserveXLogSwitch(XLogRecPtr *StartPos, XLogRecPtr *EndPos, XLogRecPtr *PrevPtr)
{
- volatile XLogCtlInsert *Insert = XLogCtl-Insert;
+ XLogCtlInsert *Insert = XLogCtl-Insert;
uint64 startbytepos;
uint64 endbytepos;
uint64 prevbytepos;
@@ -1696,7 +1693,7 @@ WaitXLogInsertionsToFinish(XLogRecPtr upto)
uint64 bytepos;
XLogRecPtr reservedUpto;
XLogRecPtr finishedUpto;
- volatile XLogCtlInsert *Insert = XLogCtl-Insert;
+ XLogCtlInsert *Insert = XLogCtl-Insert;
int i;
if (MyProc == NULL)
@@ -2131,16 +2128,11 @@ AdvanceXLInsertBuffer(XLogRecPtr upto, bool opportunistic)
break;
/* Before waiting, get info_lck and update LogwrtResult */
- {
-/* use volatile pointer
