Add barriers to the latch code.

Since their introduction latches have required barriers in SetLatch
and ResetLatch - but when they were introduced there wasn't any
barrier abstraction. Instead latches were documented to rely on the
callsites to provide barrier semantics.

Now that the barrier support looks halfway complete, add the necessary
barriers to both latch implementations.

Also remove a now superflous lock acquisition from syncrep.c and a
superflous (and insufficient) barrier from freelist.c. There might be
other cases that can now be simplified, but those are the only ones
I've seen on a quick scan.

We might want to backpatch this at some later point, but right now the
barrier infrastructure in the backbranches isn't totally on par with
master.

Discussion: 20150112154026.GB2092@awork2.anarazel.de

diff --git a/src/backend/port/unix_latch.c b/src/backend/port/unix_latch.c
index 92ae78015832b69f5f8250e3790c65c3ab50e90d..147e22cee4eee16e9b51b39476d8128e14927365 100644 (file)
--- a/src/backend/port/unix_latch.c
+++ b/src/backend/port/unix_latch.c
@@ -51,6 +51,7 @@
 #include "miscadmin.h"
 #include "portability/instr_time.h"
 #include "postmaster/postmaster.h"
+#include "storage/barrier.h"
 #include "storage/latch.h"
 #include "storage/pmsignal.h"
 #include "storage/shmem.h"
@@ -515,12 +516,11 @@ SetLatch(volatile Latch *latch)
        pid_t           owner_pid;
 
        /*
-        * XXX there really ought to be a memory barrier operation right here, to
-        * ensure that any flag variables we might have changed get flushed to
-        * main memory before we check/set is_set.  Without that, we have to
-        * require that callers provide their own synchronization for machines
-        * with weak memory ordering (see latch.h).
+        * The memory barrier has be to be placed here to ensure that any flag
+        * variables possibly changed by this process have been flushed to main
+        * memory, before we check/set is_set.
         */
+       pg_memory_barrier();
 
        /* Quick exit if already set */
        if (latch->is_set)
@@ -574,14 +574,12 @@ ResetLatch(volatile Latch *latch)
        latch->is_set = false;
 
        /*
-        * XXX there really ought to be a memory barrier operation right here, to
-        * ensure that the write to is_set gets flushed to main memory before we
+        * Ensure that the write to is_set gets flushed to main memory before we
         * examine any flag variables.  Otherwise a concurrent SetLatch might
         * falsely conclude that it needn't signal us, even though we have missed
         * seeing some flag updates that SetLatch was supposed to inform us of.
-        * For the moment, callers must supply their own synchronization of flag
-        * variables (see latch.h).
         */
+       pg_memory_barrier();
 }
 
 /*

diff --git a/src/backend/port/win32_latch.c b/src/backend/port/win32_latch.c
index da0657c915a01402ce3eaee1d4d61ca6097d0332..c7d4bdddc219dc7e45d8c80d2d9889c54fd532d5 100644 (file)
--- a/src/backend/port/win32_latch.c
+++ b/src/backend/port/win32_latch.c
@@ -27,6 +27,7 @@
 #include "miscadmin.h"
 #include "portability/instr_time.h"
 #include "postmaster/postmaster.h"
+#include "storage/barrier.h"
 #include "storage/latch.h"
 #include "storage/pmsignal.h"
 #include "storage/shmem.h"
@@ -293,6 +294,13 @@ SetLatch(volatile Latch *latch)
 {
        HANDLE          handle;
 
+       /*
+        * The memory barrier has be to be placed here to ensure that any flag
+        * variables possibly changed by this process have been flushed to main
+        * memory, before we check/set is_set.
+        */
+       pg_memory_barrier();
+
        /* Quick exit if already set */
        if (latch->is_set)
                return;
@@ -325,4 +333,12 @@ ResetLatch(volatile Latch *latch)
        Assert(latch->owner_pid == MyProcPid);
 
        latch->is_set = false;
+
+       /*
+        * Ensure that the write to is_set gets flushed to main memory before we
+        * examine any flag variables.  Otherwise a concurrent SetLatch might
+        * falsely conclude that it needn't signal us, even though we have missed
+        * seeing some flag updates that SetLatch was supposed to inform us of.
+        */
+       pg_memory_barrier();
 }

diff --git a/src/backend/replication/syncrep.c b/src/backend/replication/syncrep.c
index c99c270a7ca57aadce98a56ab07c358649b0fa7f..b2b3a81f54c7c289b89d259bf2ecda74f853497f 100644 (file)
--- a/src/backend/replication/syncrep.c
+++ b/src/backend/replication/syncrep.c
@@ -172,20 +172,10 @@ SyncRepWaitForLSN(XLogRecPtr XactCommitLSN)
                 * walsender changes the state to SYNC_REP_WAIT_COMPLETE, it will
                 * never update it again, so we can't be seeing a stale value in that
                 * case.
-                *
-                * Note: on machines with weak memory ordering, the acquisition of the
-                * lock is essential to avoid race conditions: we cannot be sure the
-                * sender's state update has reached main memory until we acquire the
-                * lock.  We could get rid of this dance if SetLatch/ResetLatch
-                * contained memory barriers.
                 */
                syncRepState = MyProc->syncRepState;
                if (syncRepState == SYNC_REP_WAITING)
-               {
-                       LWLockAcquire(SyncRepLock, LW_SHARED);
                        syncRepState = MyProc->syncRepState;
-                       LWLockRelease(SyncRepLock);
-               }
                if (syncRepState == SYNC_REP_WAIT_COMPLETE)
                        break;
 

diff --git a/src/backend/storage/buffer/freelist.c b/src/backend/storage/buffer/freelist.c
index ac647d65458175ad7b6942a98d30745df6f1c25f..3add619b5da7117afee16f8c5e421cb1de78bb49 100644 (file)
--- a/src/backend/storage/buffer/freelist.c
+++ b/src/backend/storage/buffer/freelist.c
@@ -214,7 +214,6 @@ StrategyGetBuffer(BufferAccessStrategy strategy)
        {
                /* reset bgwprocno first, before setting the latch */
                StrategyControl->bgwprocno = -1;
-               pg_write_barrier();
 
                /*
                 * Not acquiring ProcArrayLock here which is slightly icky. It's

diff --git a/src/include/storage/latch.h b/src/include/storage/latch.h
index 138a748f624fdf605ba2c745a6105ae3d4d37705..f1577b009557655139998cfdccb2ad0864c54f14 100644 (file)
--- a/src/include/storage/latch.h
+++ b/src/include/storage/latch.h
@@ -57,13 +57,6 @@
  * SetLatch *after* that. SetLatch is designed to return quickly if the
  * latch is already set.
  *
- * Presently, when using a shared latch for interprocess signalling, the
- * flag variable(s) set by senders and inspected by the wait loop must
- * be protected by spinlocks or LWLocks, else it is possible to miss events
- * on machines with weak memory ordering (such as PPC).  This restriction
- * will be lifted in future by inserting suitable memory barriers into
- * SetLatch and ResetLatch.
- *
  * On some platforms, signals will not interrupt the latch wait primitive
  * by themselves.  Therefore, it is critical that any signal handler that
  * is meant to terminate a WaitLatch wait calls SetLatch.