Re: [HACKERS] removing volatile qualifiers from lwlock.c

2014-09-19 Thread Robert Haas 
On Wed, Sep 17, 2014 at 7:45 AM, Andres Freund and...@2ndquadrant.com wrote:
 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.

Oh, that's great.  In that case I think I should go ahead and apply
that patch in the hopes of turning up any systems where the barriers
aren't working properly yet.  Although it would be nice to know
whether it breaks with *only* the lwlock.c patch.

 I've attached the very quickly written xlog.c de-volatizing patch.

I don't have time to go through this in detail, but I don't object to
you applying it if you're confident you've done it carefully enough.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] removing volatile qualifiers from lwlock.c

2014-09-19 Thread Andres Freund 
On 2014-09-19 13:58:17 -0400, Robert Haas wrote:
 On Wed, Sep 17, 2014 at 7:45 AM, Andres Freund and...@2ndquadrant.com wrote:
  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.
 
 Oh, that's great.  In that case I think I should go ahead and apply
 that patch in the hopes of turning up any systems where the barriers
 aren't working properly yet.

Agreed.

 Although it would be nice to know whether it breaks with *only* the lwlock.c 
 patch.

It didn't, at least not visibly within the 1000s I let pgbench run.

  I've attached the very quickly written xlog.c de-volatizing patch.
 
 I don't have time to go through this in detail, but I don't object to
 you applying it if you're confident you've done it carefully enough.

It's definitely not yet carefully enough checked. I wanted to get it
break fast and only made one pass through the file. But I think it
should be easy enough to get it into shape for that.

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] removing volatile qualifiers from lwlock.c

2014-09-17 Thread Andres Freund 
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

[HACKERS] removing volatile qualifiers from lwlock.c

2014-09-10 Thread Robert Haas 
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.

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.

The change to make spinlocks act as compiler barriers provides the
foundation for, hopefully, a cleaner code base and easier future work
on scalabilty and performance projects, so help is much appreciated.

Thanks,

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c
index 7c96da5..66fb2e4 100644
--- a/src/backend/storage/lmgr/lwlock.c
+++ b/src/backend/storage/lmgr/lwlock.c
@@ -112,7 +112,7 @@ static lwlock_stats lwlock_stats_dummy;
 bool		Trace_lwlocks = false;
 
 inline static void
-PRINT_LWDEBUG(const char *where, const volatile LWLock *lock)
+PRINT_LWDEBUG(const char *where, const LWLock *lock)
 {
 	if (Trace_lwlocks)
 		elog(LOG, %s(%s %d): excl %d shared %d head %p rOK %d,
@@ -406,9 +406,7 @@ LWLock *
 LWLockAssign(void)
 {
 	LWLock	   *result;
-
-	/* use volatile pointer to prevent code rearrangement */
-	volatile int *LWLockCounter;
+	int		   *LWLockCounter;
 
 	LWLockCounter = (int *) ((char *) MainLWLockArray - 3 * sizeof(int));
 	SpinLockAcquire(ShmemLock);
@@ -429,9 +427,7 @@ int
 LWLockNewTrancheId(void)
 {
 	int			result;
-
-	/* use volatile pointer to prevent code rearrangement */
-	volatile int *LWLockCounter;
+	int		   *LWLockCounter;
 
 	LWLockCounter = (int *) ((char *) MainLWLockArray - 3 * sizeof(int));
 	SpinLockAcquire(ShmemLock);
@@ -511,9 +507,8 @@ LWLockAcquireWithVar(LWLock *l, uint64 *valptr, uint64 val)
 
 /* internal function to implement LWLockAcquire and LWLockAcquireWithVar */
 static inline bool
-LWLockAcquireCommon(LWLock *l, LWLockMode mode, uint64 *valptr, uint64 val)
+LWLockAcquireCommon(LWLock *lock, LWLockMode mode, uint64 *valptr, uint64 val)
 {
-	volatile LWLock *lock = l;
 	PGPROC	   *proc = MyProc;
 	bool		retry = false;
 	bool		result = true;
@@ -525,7 +520,7 @@ LWLockAcquireCommon(LWLock *l, LWLockMode mode, uint64 *valptr, uint64 val)
 	PRINT_LWDEBUG(LWLockAcquire, lock);
 
 #ifdef LWLOCK_STATS
-	lwstats = get_lwlock_stats_entry(l);
+	lwstats = get_lwlock_stats_entry(lock);
 
 	/* Count lock acquisition attempts */
 	if (mode == LW_EXCLUSIVE)
@@ -642,13 +637,13 @@ LWLockAcquireCommon(LWLock *l, LWLockMode mode, uint64 *valptr, uint64 val)
 		 * so that the lock manager or signal manager will see the received
 		 * signal when it next waits.
 		 */
-		LOG_LWDEBUG(LWLockAcquire, T_NAME(l), T_ID(l), waiting);
+		LOG_LWDEBUG(LWLockAcquire, T_NAME(lock), T_ID(lock), waiting);
 
 #ifdef LWLOCK_STATS
 		lwstats-block_count++;
 #endif
 
-		TRACE_POSTGRESQL_LWLOCK_WAIT_START(T_NAME(l), T_ID(l), mode);
+		TRACE_POSTGRESQL_LWLOCK_WAIT_START(T_NAME(lock), T_ID(lock), mode);
 
 		for (;;)
 		{
@@ -659,9 +654,9 @@ LWLockAcquireCommon(LWLock *l, LWLockMode mode, uint64 *valptr, uint64 val)
 			extraWaits++;
 		}
 
-		TRACE_POSTGRESQL_LWLOCK_WAIT_DONE(T_NAME(l), T_ID(l), mode);
+		TRACE_POSTGRESQL_LWLOCK_WAIT_DONE(T_NAME(lock), T_ID(lock), mode);
 
-		LOG_LWDEBUG(LWLockAcquire, T_NAME(l), T_ID(l), awakened);
+		LOG_LWDEBUG(LWLockAcquire, T_NAME(lock), T_ID(lock), awakened);
 
 		/* Now loop back and try to acquire lock again. */
 		retry = true;
@@ -675,10 +670,10 @@ LWLockAcquireCommon(LWLock *l, LWLockMode mode, uint64 *valptr, uint64 val)
 	/* We are done updating shared state of the lock itself. */
 	SpinLockRelease(lock-mutex);
 
-	TRACE_POSTGRESQL_LWLOCK_ACQUIRE(T_NAME(l), T_ID(l), mode);
+