Hi,

As Daniel pointed out in:
https://postgr.es/m/fb948276-7b32-4b77-83e6-d00167f8e...@yesql.se the
pg_atomic_flag fallback implementation is broken.  That has gone
unnoticed because the fallback implementation wasn't testable until now:
-   /* ---
-    * Can't run the test under the semaphore emulation, it doesn't handle
-    * checking two edge cases well:
-    * - pg_atomic_unlocked_test_flag() always returns true
-    * - locking a already locked flag blocks
-    * it seems better to not test the semaphore fallback here, than weaken
-    * the checks for the other cases. The semaphore code will be the same
-    * everywhere, whereas the efficient implementations wont.
-    * ---
-    */
and flags weren't used until recently.

The attached fixes the bug and removes the edge-cases by storing a value
separate from the semaphore. I should have done that from the start.
This is an ABI break, but given the fallback didn't work at all, I don't
think that's a problem for backporting.

Fix attached. Comments?

Greetings,

Andres Freund
>From 3b98662adf5e0f82375b50833bc618403614a461 Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
Date: Fri, 6 Apr 2018 16:17:01 -0700
Subject: [PATCH 1/2] Fix and improve pg_atomic_flag fallback implementation.

The atomics fallback implementation for pg_atomic_flag was broken,
returning the inverted value from pg_atomic_test_set_flag().  This was
unnoticed because a) atomic flags were unused until recently b) the
test code wasn't run when the fallback implementation was in
use (because it didn't allow to test for some edge cases).

Fix the bug, and improve the fallback so it has the same behaviour as
the non-fallback implementation in the problematic edge cases. That
breaks ABI compatibility in the back branches when fallbacks are in
use, but given they were broken until now...

Author: Andres Freund
Reported-by: Daniel Gustafsson
Discussion: https://postgr.es/m/fb948276-7b32-4b77-83e6-d00167f8e...@yesql.se
Backpatch: 9.5-, where the atomics abstraction was introduced.
---
 src/backend/port/atomics.c          | 21 +++++++++++++++++++--
 src/include/port/atomics/fallback.h | 13 ++-----------
 src/test/regress/regress.c          | 14 --------------
 3 files changed, 21 insertions(+), 27 deletions(-)

diff --git a/src/backend/port/atomics.c b/src/backend/port/atomics.c
index e4e4734dd23..caa84bf2b62 100644
--- a/src/backend/port/atomics.c
+++ b/src/backend/port/atomics.c
@@ -68,18 +68,35 @@ pg_atomic_init_flag_impl(volatile pg_atomic_flag *ptr)
 #else
 	SpinLockInit((slock_t *) &ptr->sema);
 #endif
+
+	ptr->value = false;
 }
 
 bool
 pg_atomic_test_set_flag_impl(volatile pg_atomic_flag *ptr)
 {
-	return TAS((slock_t *) &ptr->sema);
+	uint32		oldval;
+
+	SpinLockAcquire((slock_t *) &ptr->sema);
+	oldval = ptr->value;
+	ptr->value = true;
+	SpinLockRelease((slock_t *) &ptr->sema);
+
+	return oldval == 0;
 }
 
 void
 pg_atomic_clear_flag_impl(volatile pg_atomic_flag *ptr)
 {
-	S_UNLOCK((slock_t *) &ptr->sema);
+	SpinLockAcquire((slock_t *) &ptr->sema);
+	ptr->value = false;
+	SpinLockRelease((slock_t *) &ptr->sema);
+}
+
+bool
+pg_atomic_unlocked_test_flag_impl(volatile pg_atomic_flag *ptr)
+{
+	return ptr->value == 0;
 }
 
 #endif							/* PG_HAVE_ATOMIC_FLAG_SIMULATION */
diff --git a/src/include/port/atomics/fallback.h b/src/include/port/atomics/fallback.h
index 7b9dcad8073..88a967ad5b9 100644
--- a/src/include/port/atomics/fallback.h
+++ b/src/include/port/atomics/fallback.h
@@ -80,6 +80,7 @@ typedef struct pg_atomic_flag
 #else
 	int			sema;
 #endif
+	volatile bool value;
 } pg_atomic_flag;
 
 #endif /* PG_HAVE_ATOMIC_FLAG_SUPPORT */
@@ -132,17 +133,7 @@ extern bool pg_atomic_test_set_flag_impl(volatile pg_atomic_flag *ptr);
 extern void pg_atomic_clear_flag_impl(volatile pg_atomic_flag *ptr);
 
 #define PG_HAVE_ATOMIC_UNLOCKED_TEST_FLAG
-static inline bool
-pg_atomic_unlocked_test_flag_impl(volatile pg_atomic_flag *ptr)
-{
-	/*
-	 * Can't do this efficiently in the semaphore based implementation - we'd
-	 * have to try to acquire the semaphore - so always return true. That's
-	 * correct, because this is only an unlocked test anyway. Do this in the
-	 * header so compilers can optimize the test away.
-	 */
-	return true;
-}
+extern bool pg_atomic_unlocked_test_flag_impl(volatile pg_atomic_flag *ptr);
 
 #endif /* PG_HAVE_ATOMIC_FLAG_SIMULATION */
 
diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c
index e14322c798a..8bc562ee4f0 100644
--- a/src/test/regress/regress.c
+++ b/src/test/regress/regress.c
@@ -633,7 +633,6 @@ wait_pid(PG_FUNCTION_ARGS)
 	PG_RETURN_VOID();
 }
 
-#ifndef PG_HAVE_ATOMIC_FLAG_SIMULATION
 static void
 test_atomic_flag(void)
 {
@@ -663,7 +662,6 @@ test_atomic_flag(void)
 
 	pg_atomic_clear_flag(&flag);
 }
-#endif							/* PG_HAVE_ATOMIC_FLAG_SIMULATION */
 
 static void
 test_atomic_uint32(void)
@@ -846,19 +844,7 @@ PG_FUNCTION_INFO_V1(test_atomic_ops);
 Datum
 test_atomic_ops(PG_FUNCTION_ARGS)
 {
-	/* ---
-	 * Can't run the test under the semaphore emulation, it doesn't handle
-	 * checking two edge cases well:
-	 * - pg_atomic_unlocked_test_flag() always returns true
-	 * - locking a already locked flag blocks
-	 * it seems better to not test the semaphore fallback here, than weaken
-	 * the checks for the other cases. The semaphore code will be the same
-	 * everywhere, whereas the efficient implementations wont.
-	 * ---
-	 */
-#ifndef PG_HAVE_ATOMIC_FLAG_SIMULATION
 	test_atomic_flag();
-#endif
 
 	test_atomic_uint32();
 
-- 
2.17.0.rc1.dirty

Reply via email to