Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 8c3debb

Browse files
committed
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-D00167F8EEB4@yesql.se https://postgr.es/m/20180406233854.uni2h3mbnveczl32@alap3.anarazel.de Backpatch: 9.5-, where the atomics abstraction was introduced.
1 parent eb2a0e0 commit 8c3debb

File tree

3 files changed

+21
-27
lines changed

3 files changed

+21
-27
lines changed

src/backend/port/atomics.c

+19-2
Original file line numberDiff line numberDiff line change
@@ -68,18 +68,35 @@ pg_atomic_init_flag_impl(volatile pg_atomic_flag *ptr)
6868
#else
6969
SpinLockInit((slock_t *) &ptr->sema);
7070
#endif
71+
72+
ptr->value = false;
7173
}
7274

7375
bool
7476
pg_atomic_test_set_flag_impl(volatile pg_atomic_flag *ptr)
7577
{
76-
return TAS((slock_t *) &ptr->sema);
78+
uint32 oldval;
79+
80+
SpinLockAcquire((slock_t *) &ptr->sema);
81+
oldval = ptr->value;
82+
ptr->value = true;
83+
SpinLockRelease((slock_t *) &ptr->sema);
84+
85+
return oldval == 0;
7786
}
7887

7988
void
8089
pg_atomic_clear_flag_impl(volatile pg_atomic_flag *ptr)
8190
{
82-
S_UNLOCK((slock_t *) &ptr->sema);
91+
SpinLockAcquire((slock_t *) &ptr->sema);
92+
ptr->value = false;
93+
SpinLockRelease((slock_t *) &ptr->sema);
94+
}
95+
96+
bool
97+
pg_atomic_unlocked_test_flag_impl(volatile pg_atomic_flag *ptr)
98+
{
99+
return ptr->value == 0;
83100
}
84101

85102
#endif /* PG_HAVE_ATOMIC_FLAG_SIMULATION */

src/include/port/atomics/fallback.h

+2-11
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,7 @@ typedef struct pg_atomic_flag
8080
#else
8181
int sema;
8282
#endif
83+
volatile bool value;
8384
} pg_atomic_flag;
8485

8586
#endif /* PG_HAVE_ATOMIC_FLAG_SUPPORT */
@@ -132,17 +133,7 @@ extern bool pg_atomic_test_set_flag_impl(volatile pg_atomic_flag *ptr);
132133
extern void pg_atomic_clear_flag_impl(volatile pg_atomic_flag *ptr);
133134

134135
#define PG_HAVE_ATOMIC_UNLOCKED_TEST_FLAG
135-
static inline bool
136-
pg_atomic_unlocked_test_flag_impl(volatile pg_atomic_flag *ptr)
137-
{
138-
/*
139-
* Can't do this efficiently in the semaphore based implementation - we'd
140-
* have to try to acquire the semaphore - so always return true. That's
141-
* correct, because this is only an unlocked test anyway. Do this in the
142-
* header so compilers can optimize the test away.
143-
*/
144-
return true;
145-
}
136+
extern bool pg_atomic_unlocked_test_flag_impl(volatile pg_atomic_flag *ptr);
146137

147138
#endif /* PG_HAVE_ATOMIC_FLAG_SIMULATION */
148139

src/test/regress/regress.c

-14
Original file line numberDiff line numberDiff line change
@@ -633,7 +633,6 @@ wait_pid(PG_FUNCTION_ARGS)
633633
PG_RETURN_VOID();
634634
}
635635

636-
#ifndef PG_HAVE_ATOMIC_FLAG_SIMULATION
637636
static void
638637
test_atomic_flag(void)
639638
{
@@ -663,7 +662,6 @@ test_atomic_flag(void)
663662

664663
pg_atomic_clear_flag(&flag);
665664
}
666-
#endif /* PG_HAVE_ATOMIC_FLAG_SIMULATION */
667665

668666
static void
669667
test_atomic_uint32(void)
@@ -846,19 +844,7 @@ PG_FUNCTION_INFO_V1(test_atomic_ops);
846844
Datum
847845
test_atomic_ops(PG_FUNCTION_ARGS)
848846
{
849-
/* ---
850-
* Can't run the test under the semaphore emulation, it doesn't handle
851-
* checking two edge cases well:
852-
* - pg_atomic_unlocked_test_flag() always returns true
853-
* - locking a already locked flag blocks
854-
* it seems better to not test the semaphore fallback here, than weaken
855-
* the checks for the other cases. The semaphore code will be the same
856-
* everywhere, whereas the efficient implementations wont.
857-
* ---
858-
*/
859-
#ifndef PG_HAVE_ATOMIC_FLAG_SIMULATION
860847
test_atomic_flag();
861-
#endif
862848

863849
test_atomic_uint32();
864850

0 commit comments

Comments
 (0)