On 2016-10-07 17:12:45 -0400, Tom Lane wrote:
> Andres Freund <and...@anarazel.de> writes:
> > It's not quite there yet, unfortunately. At the moment
> > pg_atomic_write_u32() is used for local buffers - and we explicitly
> > don't want that to be locking for temp buffers
> > (c.f. 6b93fcd149329d4ee7319561b30fc15a573c6307).
>
> Hm.
>
> > Don't really have a great idea about addressing this, besides either
> > just living with the lock for temp buffers on fallback platforms (which
> > don't have much of a practical relevance), or introduce
> > pg_atomic_unlocked_write_u32() or something. Neither seems great.
>
> Maybe we could hack it with some macro magic that would cause
> pg_atomic_write_u32() to be expanded into a simple assignment in
> localbuf.c only?
I think it's just as well to add a variant that's globally documented to
have no locking, there might be further uses of it. It's already in two
files (bufmgr.c/localbuf.c), and I don't think it's impossible that
further usages will crop up.
Patch that I intend to push soon-ish attached.
Andres
>From b0779abb3add11d4dd745779dd81ea8ecdd00a1d Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
Date: Fri, 7 Oct 2016 16:55:15 -0700
Subject: [PATCH] Fix fallback implementation of pg_atomic_write_u32().
I somehow had assumed that in the spinlock (in turn possibly using
semaphores) based fallback atomics implementation 32 bit writes could be
done without a lock. As far as the write goes that's correct, since
postgres supports only platforms with single-copy atomicity for aligned
32bit writes. But writing without holding the spinlock breaks
read-modify-write operations like pg_atomic_compare_exchange_u32(),
since they'll potentially "miss" a concurrent write, which can't happen
in actual hardware implementations.
In 9.6+ when using the fallback atomics implementation this could lead
to buffer header locks not being properly marked as released, and
potentially some related state corruption. I don't see a related danger
in 9.5 (earliest release with the API), because pg_atomic_write_u32()
wasn't used in a concurrent manner there.
The state variable of local buffers, before this change, were
manipulated using pg_atomic_write_u32(), to avoid unnecessary
synchronization overhead. As that'd not be the case anymore, introduce
and use pg_atomic_unlocked_write_u32(), which does not correctly
interact with RMW operations.
This bug only caused issues when postgres is compiled on platforms
without atomics support (i.e. no common new platform), or when compiled
with --disable-atomics, which explains why this wasn't noticed in
testing.
Reported-By: Tom Lane
Discussion: <14947.1475690...@sss.pgh.pa.us>
Backpatch: 9.5-, where the atomic operations API was introduced.
---
src/backend/port/atomics.c | 13 +++++++++++++
src/backend/storage/buffer/bufmgr.c | 6 +++---
src/backend/storage/buffer/localbuf.c | 16 ++++++++--------
src/include/port/atomics.h | 25 +++++++++++++++++++++++--
src/include/port/atomics/fallback.h | 3 +++
src/include/port/atomics/generic.h | 9 +++++++++
src/include/storage/buf_internals.h | 3 ++-
7 files changed, 61 insertions(+), 14 deletions(-)
diff --git a/src/backend/port/atomics.c b/src/backend/port/atomics.c
index 42169a3..d5970e4 100644
--- a/src/backend/port/atomics.c
+++ b/src/backend/port/atomics.c
@@ -104,6 +104,19 @@ pg_atomic_init_u32_impl(volatile pg_atomic_uint32 *ptr, uint32 val_)
ptr->value = val_;
}
+void
+pg_atomic_write_u32_impl(volatile pg_atomic_uint32 *ptr, uint32 val)
+{
+ /*
+ * One might think that an unlocked write doesn't need to acquire the
+ * spinlock, but one would be wrong. Even an unlocked write has to cause a
+ * concurrent pg_atomic_compare_exchange_u32() (et al) to fail.
+ */
+ SpinLockAcquire((slock_t *) &ptr->sema);
+ ptr->value = val;
+ SpinLockRelease((slock_t *) &ptr->sema);
+}
+
bool
pg_atomic_compare_exchange_u32_impl(volatile pg_atomic_uint32 *ptr,
uint32 *expected, uint32 newval)
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 2b63cd3..df4c9d7 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -821,7 +821,7 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
Assert(buf_state & BM_VALID);
buf_state &= ~BM_VALID;
- pg_atomic_write_u32(&bufHdr->state, buf_state);
+ pg_atomic_unlocked_write_u32(&bufHdr->state, buf_state);
}
else
{
@@ -941,7 +941,7 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
uint32 buf_state = pg_atomic_read_u32(&bufHdr->state);
buf_state |= BM_VALID;
- pg_atomic_write_u32(&bufHdr->state, buf_state);
+ pg_atomic_unlocked_write_u32(&bufHdr->state, buf_state);
}
else
{
@@ -3167,7 +3167,7 @@ FlushRelationBuffers(Relation rel)
false);
buf_state &= ~(BM_DIRTY | BM_JUST_DIRTIED);
- pg_atomic_write_u32(&bufHdr->state, buf_state);
+ pg_atomic_unlocked_write_u32(&bufHdr->state, buf_state);
/* Pop the error context stack */
error_context_stack = errcallback.previous;
diff --git a/src/backend/storage/buffer/localbuf.c b/src/backend/storage/buffer/localbuf.c
index ca23887..fa961ad 100644
--- a/src/backend/storage/buffer/localbuf.c
+++ b/src/backend/storage/buffer/localbuf.c
@@ -138,7 +138,7 @@ LocalBufferAlloc(SMgrRelation smgr, ForkNumber forkNum, BlockNumber blockNum,
if (BUF_STATE_GET_USAGECOUNT(buf_state) < BM_MAX_USAGE_COUNT)
{
buf_state += BUF_USAGECOUNT_ONE;
- pg_atomic_write_u32(&bufHdr->state, buf_state);
+ pg_atomic_unlocked_write_u32(&bufHdr->state, buf_state);
}
}
LocalRefCount[b]++;
@@ -181,7 +181,7 @@ LocalBufferAlloc(SMgrRelation smgr, ForkNumber forkNum, BlockNumber blockNum,
if (BUF_STATE_GET_USAGECOUNT(buf_state) > 0)
{
buf_state -= BUF_USAGECOUNT_ONE;
- pg_atomic_write_u32(&bufHdr->state, buf_state);
+ pg_atomic_unlocked_write_u32(&bufHdr->state, buf_state);
trycounter = NLocBuffer;
}
else
@@ -222,7 +222,7 @@ LocalBufferAlloc(SMgrRelation smgr, ForkNumber forkNum, BlockNumber blockNum,
/* Mark not-dirty now in case we error out below */
buf_state &= ~BM_DIRTY;
- pg_atomic_write_u32(&bufHdr->state, buf_state);
+ pg_atomic_unlocked_write_u32(&bufHdr->state, buf_state);
pgBufferUsage.local_blks_written++;
}
@@ -249,7 +249,7 @@ LocalBufferAlloc(SMgrRelation smgr, ForkNumber forkNum, BlockNumber blockNum,
/* mark buffer invalid just in case hash insert fails */
CLEAR_BUFFERTAG(bufHdr->tag);
buf_state &= ~(BM_VALID | BM_TAG_VALID);
- pg_atomic_write_u32(&bufHdr->state, buf_state);
+ pg_atomic_unlocked_write_u32(&bufHdr->state, buf_state);
}
hresult = (LocalBufferLookupEnt *)
@@ -266,7 +266,7 @@ LocalBufferAlloc(SMgrRelation smgr, ForkNumber forkNum, BlockNumber blockNum,
buf_state |= BM_TAG_VALID;
buf_state &= ~BUF_USAGECOUNT_MASK;
buf_state += BUF_USAGECOUNT_ONE;
- pg_atomic_write_u32(&bufHdr->state, buf_state);
+ pg_atomic_unlocked_write_u32(&bufHdr->state, buf_state);
*foundPtr = FALSE;
return bufHdr;
@@ -302,7 +302,7 @@ MarkLocalBufferDirty(Buffer buffer)
buf_state |= BM_DIRTY;
- pg_atomic_write_u32(&bufHdr->state, buf_state);
+ pg_atomic_unlocked_write_u32(&bufHdr->state, buf_state);
}
/*
@@ -351,7 +351,7 @@ DropRelFileNodeLocalBuffers(RelFileNode rnode, ForkNumber forkNum,
CLEAR_BUFFERTAG(bufHdr->tag);
buf_state &= ~BUF_FLAG_MASK;
buf_state &= ~BUF_USAGECOUNT_MASK;
- pg_atomic_write_u32(&bufHdr->state, buf_state);
+ pg_atomic_unlocked_write_u32(&bufHdr->state, buf_state);
}
}
}
@@ -395,7 +395,7 @@ DropRelFileNodeAllLocalBuffers(RelFileNode rnode)
CLEAR_BUFFERTAG(bufHdr->tag);
buf_state &= ~BUF_FLAG_MASK;
buf_state &= ~BUF_USAGECOUNT_MASK;
- pg_atomic_write_u32(&bufHdr->state, buf_state);
+ pg_atomic_unlocked_write_u32(&bufHdr->state, buf_state);
}
}
}
diff --git a/src/include/port/atomics.h b/src/include/port/atomics.h
index f7884d7..4e15ff8 100644
--- a/src/include/port/atomics.h
+++ b/src/include/port/atomics.h
@@ -255,10 +255,12 @@ pg_atomic_read_u32(volatile pg_atomic_uint32 *ptr)
}
/*
- * pg_atomic_write_u32 - unlocked write to atomic variable.
+ * pg_atomic_write_u32 - write to atomic variable.
*
* The write is guaranteed to succeed as a whole, i.e. it's not possible to
- * observe a partial write for any reader.
+ * observe a partial write for any reader. Note that this correctly interacts
+ * with pg_atomic_compare_exchange_u32, in contrast to
+ * pg_atomic_unlocked_write_u32().
*
* No barrier semantics.
*/
@@ -271,6 +273,25 @@ pg_atomic_write_u32(volatile pg_atomic_uint32 *ptr, uint32 val)
}
/*
+ * pg_atomic_unlocked_write_u32 - unlocked write to atomic variable.
+ *
+ * The write is guaranteed to succeed as a whole, i.e. it's not possible to
+ * observe a partial write for any reader. But note that writing this way is
+ * not guaranteed to correctly interact with read-modify-write operations like
+ * pg_atomic_compare_exchange_u32. This should only be used in cases where
+ * minor performance regressions due to atomics emulation are unacceptable.
+ *
+ * No barrier semantics.
+ */
+static inline void
+pg_atomic_unlocked_write_u32(volatile pg_atomic_uint32 *ptr, uint32 val)
+{
+ AssertPointerAlignment(ptr, 4);
+
+ pg_atomic_unlocked_write_u32_impl(ptr, val);
+}
+
+/*
* pg_atomic_exchange_u32 - exchange newval with current value
*
* Returns the old value of 'ptr' before the swap.
diff --git a/src/include/port/atomics/fallback.h b/src/include/port/atomics/fallback.h
index bdaa795..2290fff 100644
--- a/src/include/port/atomics/fallback.h
+++ b/src/include/port/atomics/fallback.h
@@ -133,6 +133,9 @@ pg_atomic_unlocked_test_flag_impl(volatile pg_atomic_flag *ptr)
#define PG_HAVE_ATOMIC_INIT_U32
extern void pg_atomic_init_u32_impl(volatile pg_atomic_uint32 *ptr, uint32 val_);
+#define PG_HAVE_ATOMIC_WRITE_U32
+extern void pg_atomic_write_u32_impl(volatile pg_atomic_uint32 *ptr, uint32 val);
+
#define PG_HAVE_ATOMIC_COMPARE_EXCHANGE_U32
extern bool pg_atomic_compare_exchange_u32_impl(volatile pg_atomic_uint32 *ptr,
uint32 *expected, uint32 newval);
diff --git a/src/include/port/atomics/generic.h b/src/include/port/atomics/generic.h
index 32a0113..1acd192 100644
--- a/src/include/port/atomics/generic.h
+++ b/src/include/port/atomics/generic.h
@@ -58,6 +58,15 @@ pg_atomic_write_u32_impl(volatile pg_atomic_uint32 *ptr, uint32 val)
}
#endif
+#ifndef PG_HAVE_ATOMIC_UNLOCKED_WRITE_U32
+#define PG_HAVE_ATOMIC_UNLOCKED_WRITE_U32
+static inline void
+pg_atomic_unlocked_write_u32_impl(volatile pg_atomic_uint32 *ptr, uint32 val)
+{
+ ptr->value = val;
+}
+#endif
+
/*
* provide fallback for test_and_set using atomic_exchange if available
*/
diff --git a/src/include/storage/buf_internals.h b/src/include/storage/buf_internals.h
index e0dfb2f..c7da9f6 100644
--- a/src/include/storage/buf_internals.h
+++ b/src/include/storage/buf_internals.h
@@ -168,7 +168,8 @@ typedef struct buftag
* We use this same struct for local buffer headers, but the locks are not
* used and not all of the flag bits are useful either. To avoid unnecessary
* overhead, manipulations of the state field should be done without actual
- * atomic operations (i.e. only pg_atomic_read/write).
+ * atomic operations (i.e. only pg_atomic_read_u32() and
+ * pg_atomic_unlocked_write_u32()).
*
* Be careful to avoid increasing the size of the struct when adding or
* reordering members. Keeping it below 64 bytes (the most common CPU
--
2.9.3
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
