agora inbox for [email protected]
help / color / mirror / Atom feed[PATCH] Change fastgetattr and heap_getattr to inline functions
4+ messages / 2 participants
[nested] [flat]
* [PATCH] Change fastgetattr and heap_getattr to inline functions
@ 2022-03-24 09:40 Alvaro Herrera <[email protected]>
0 siblings, 0 replies; 4+ messages in thread
From: Alvaro Herrera @ 2022-03-24 09:40 UTC (permalink / raw)
They were macros previously, but recent callsite additions made Coverity
complain about one of the assertions being always true. This change
could have been made a long time ago, but the Coverity complain broke
the inertia.
---
src/backend/access/heap/heapam.c | 46 ---------
src/include/access/htup_details.h | 151 ++++++++++++++----------------
2 files changed, 69 insertions(+), 128 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 3746336a09..74ad445e59 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -1131,52 +1131,6 @@ heapgettup_pagemode(HeapScanDesc scan,
}
-#if defined(DISABLE_COMPLEX_MACRO)
-/*
- * This is formatted so oddly so that the correspondence to the macro
- * definition in access/htup_details.h is maintained.
- */
-Datum
-fastgetattr(HeapTuple tup, int attnum, TupleDesc tupleDesc,
- bool *isnull)
-{
- return (
- (attnum) > 0 ?
- (
- (*(isnull) = false),
- HeapTupleNoNulls(tup) ?
- (
- TupleDescAttr((tupleDesc), (attnum) - 1)->attcacheoff >= 0 ?
- (
- fetchatt(TupleDescAttr((tupleDesc), (attnum) - 1),
- (char *) (tup)->t_data + (tup)->t_data->t_hoff +
- TupleDescAttr((tupleDesc), (attnum) - 1)->attcacheoff)
- )
- :
- nocachegetattr((tup), (attnum), (tupleDesc))
- )
- :
- (
- att_isnull((attnum) - 1, (tup)->t_data->t_bits) ?
- (
- (*(isnull) = true),
- (Datum) NULL
- )
- :
- (
- nocachegetattr((tup), (attnum), (tupleDesc))
- )
- )
- )
- :
- (
- (Datum) NULL
- )
- );
-}
-#endif /* defined(DISABLE_COMPLEX_MACRO) */
-
-
/* ----------------------------------------------------------------
* heap access method interface
* ----------------------------------------------------------------
diff --git a/src/include/access/htup_details.h b/src/include/access/htup_details.h
index b2d52ed16c..de0b91e1fa 100644
--- a/src/include/access/htup_details.h
+++ b/src/include/access/htup_details.h
@@ -690,88 +690,6 @@ struct MinimalTupleData
#define HeapTupleClearHeapOnly(tuple) \
HeapTupleHeaderClearHeapOnly((tuple)->t_data)
-
-/* ----------------
- * fastgetattr
- *
- * Fetch a user attribute's value as a Datum (might be either a
- * value, or a pointer into the data area of the tuple).
- *
- * This must not be used when a system attribute might be requested.
- * Furthermore, the passed attnum MUST be valid. Use heap_getattr()
- * instead, if in doubt.
- *
- * This gets called many times, so we macro the cacheable and NULL
- * lookups, and call nocachegetattr() for the rest.
- * ----------------
- */
-
-#if !defined(DISABLE_COMPLEX_MACRO)
-
-#define fastgetattr(tup, attnum, tupleDesc, isnull) \
-( \
- AssertMacro((attnum) > 0), \
- (*(isnull) = false), \
- HeapTupleNoNulls(tup) ? \
- ( \
- TupleDescAttr((tupleDesc), (attnum)-1)->attcacheoff >= 0 ? \
- ( \
- fetchatt(TupleDescAttr((tupleDesc), (attnum)-1), \
- (char *) (tup)->t_data + (tup)->t_data->t_hoff + \
- TupleDescAttr((tupleDesc), (attnum)-1)->attcacheoff)\
- ) \
- : \
- nocachegetattr((tup), (attnum), (tupleDesc)) \
- ) \
- : \
- ( \
- att_isnull((attnum)-1, (tup)->t_data->t_bits) ? \
- ( \
- (*(isnull) = true), \
- (Datum)NULL \
- ) \
- : \
- ( \
- nocachegetattr((tup), (attnum), (tupleDesc)) \
- ) \
- ) \
-)
-#else /* defined(DISABLE_COMPLEX_MACRO) */
-
-extern Datum fastgetattr(HeapTuple tup, int attnum, TupleDesc tupleDesc,
- bool *isnull);
-#endif /* defined(DISABLE_COMPLEX_MACRO) */
-
-
-/* ----------------
- * heap_getattr
- *
- * Extract an attribute of a heap tuple and return it as a Datum.
- * This works for either system or user attributes. The given attnum
- * is properly range-checked.
- *
- * If the field in question has a NULL value, we return a zero Datum
- * and set *isnull == true. Otherwise, we set *isnull == false.
- *
- * <tup> is the pointer to the heap tuple. <attnum> is the attribute
- * number of the column (field) caller wants. <tupleDesc> is a
- * pointer to the structure describing the row and all its fields.
- * ----------------
- */
-#define heap_getattr(tup, attnum, tupleDesc, isnull) \
- ( \
- ((attnum) > 0) ? \
- ( \
- ((attnum) > (int) HeapTupleHeaderGetNatts((tup)->t_data)) ? \
- getmissingattr((tupleDesc), (attnum), (isnull)) \
- : \
- fastgetattr((tup), (attnum), (tupleDesc), (isnull)) \
- ) \
- : \
- heap_getsysattr((tup), (attnum), (tupleDesc), (isnull)) \
- )
-
-
/* prototypes for functions in common/heaptuple.c */
extern Size heap_compute_data_size(TupleDesc tupleDesc,
Datum *values, bool *isnull);
@@ -815,4 +733,73 @@ extern size_t varsize_any(void *p);
extern HeapTuple heap_expand_tuple(HeapTuple sourceTuple, TupleDesc tupleDesc);
extern MinimalTuple minimal_expand_tuple(HeapTuple sourceTuple, TupleDesc tupleDesc);
+/*
+ * fastgetattr
+ * Fetch a user attribute's value as a Datum (might be either a
+ * value, or a pointer into the data area of the tuple).
+ *
+ * This must not be used when a system attribute might be requested.
+ * Furthermore, the passed attnum MUST be valid. Use heap_getattr()
+ * instead, if in doubt.
+ *
+ * This gets called many times, so we macro the cacheable and NULL
+ * lookups, and call nocachegetattr() for the rest.
+ */
+static inline Datum
+fastgetattr(HeapTuple tup, int attnum, TupleDesc tupleDesc, bool *isnull)
+{
+ AssertMacro(attnum > 0);
+
+ *isnull = false;
+ if (HeapTupleNoNulls(tup))
+ {
+ Form_pg_attribute att;
+
+ att = TupleDescAttr(tupleDesc, attnum - 1);
+ if (att->attcacheoff >= 0)
+ return fetchatt(att, (char *) tup->t_data + tup->t_data->t_hoff +
+ att->attcacheoff);
+ else
+ return nocachegetattr(tup, attnum, tupleDesc);
+ }
+ else
+ {
+ if (att_isnull(attnum - 1, tup->t_data->t_bits))
+ {
+ *isnull = true;
+ return (Datum) NULL;
+ }
+ else
+ return nocachegetattr(tup, attnum, tupleDesc);
+ }
+}
+
+/*
+ * heap_getattr
+ * Extract an attribute of a heap tuple and return it as a Datum.
+ * This works for either system or user attributes. The given attnum
+ * is properly range-checked.
+ *
+ * If the field in question has a NULL value, we return a zero Datum
+ * and set *isnull == true. Otherwise, we set *isnull == false.
+ *
+ * <tup> is the pointer to the heap tuple. <attnum> is the attribute
+ * number of the column (field) caller wants. <tupleDesc> is a
+ * pointer to the structure describing the row and all its fields.
+ *
+ */
+static inline Datum
+heap_getattr(HeapTuple tup, int attnum, TupleDesc tupleDesc, bool *isnull)
+{
+ if (attnum > 0)
+ {
+ if (attnum > (int) HeapTupleHeaderGetNatts(tup->t_data))
+ return getmissingattr(tupleDesc, attnum, isnull);
+ else
+ return fastgetattr(tup, attnum, tupleDesc, isnull);
+ }
+ else
+ return heap_getsysattr(tup, attnum, tupleDesc, isnull);
+}
+
#endif /* HTUP_DETAILS_H */
--
2.30.2
--5zm5x4bc2wrbhwa7--
^ permalink raw reply [nested|flat] 4+ messages in thread
* [PATCH v2] Change fastgetattr and heap_getattr to inline functions
@ 2022-03-24 09:40 Alvaro Herrera <[email protected]>
0 siblings, 0 replies; 4+ messages in thread
From: Alvaro Herrera @ 2022-03-24 09:40 UTC (permalink / raw)
They were macros previously, but recent callsite additions made Coverity
complain about one of the assertions being always true. This change
could have been made a long time ago, but the Coverity complain broke
the inertia.
---
src/backend/access/heap/heapam.c | 46 ---------
src/include/access/htup_details.h | 155 ++++++++++++++----------------
2 files changed, 73 insertions(+), 128 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 3746336a09..74ad445e59 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -1131,52 +1131,6 @@ heapgettup_pagemode(HeapScanDesc scan,
}
-#if defined(DISABLE_COMPLEX_MACRO)
-/*
- * This is formatted so oddly so that the correspondence to the macro
- * definition in access/htup_details.h is maintained.
- */
-Datum
-fastgetattr(HeapTuple tup, int attnum, TupleDesc tupleDesc,
- bool *isnull)
-{
- return (
- (attnum) > 0 ?
- (
- (*(isnull) = false),
- HeapTupleNoNulls(tup) ?
- (
- TupleDescAttr((tupleDesc), (attnum) - 1)->attcacheoff >= 0 ?
- (
- fetchatt(TupleDescAttr((tupleDesc), (attnum) - 1),
- (char *) (tup)->t_data + (tup)->t_data->t_hoff +
- TupleDescAttr((tupleDesc), (attnum) - 1)->attcacheoff)
- )
- :
- nocachegetattr((tup), (attnum), (tupleDesc))
- )
- :
- (
- att_isnull((attnum) - 1, (tup)->t_data->t_bits) ?
- (
- (*(isnull) = true),
- (Datum) NULL
- )
- :
- (
- nocachegetattr((tup), (attnum), (tupleDesc))
- )
- )
- )
- :
- (
- (Datum) NULL
- )
- );
-}
-#endif /* defined(DISABLE_COMPLEX_MACRO) */
-
-
/* ----------------------------------------------------------------
* heap access method interface
* ----------------------------------------------------------------
diff --git a/src/include/access/htup_details.h b/src/include/access/htup_details.h
index b2d52ed16c..0c5c7b352f 100644
--- a/src/include/access/htup_details.h
+++ b/src/include/access/htup_details.h
@@ -690,88 +690,6 @@ struct MinimalTupleData
#define HeapTupleClearHeapOnly(tuple) \
HeapTupleHeaderClearHeapOnly((tuple)->t_data)
-
-/* ----------------
- * fastgetattr
- *
- * Fetch a user attribute's value as a Datum (might be either a
- * value, or a pointer into the data area of the tuple).
- *
- * This must not be used when a system attribute might be requested.
- * Furthermore, the passed attnum MUST be valid. Use heap_getattr()
- * instead, if in doubt.
- *
- * This gets called many times, so we macro the cacheable and NULL
- * lookups, and call nocachegetattr() for the rest.
- * ----------------
- */
-
-#if !defined(DISABLE_COMPLEX_MACRO)
-
-#define fastgetattr(tup, attnum, tupleDesc, isnull) \
-( \
- AssertMacro((attnum) > 0), \
- (*(isnull) = false), \
- HeapTupleNoNulls(tup) ? \
- ( \
- TupleDescAttr((tupleDesc), (attnum)-1)->attcacheoff >= 0 ? \
- ( \
- fetchatt(TupleDescAttr((tupleDesc), (attnum)-1), \
- (char *) (tup)->t_data + (tup)->t_data->t_hoff + \
- TupleDescAttr((tupleDesc), (attnum)-1)->attcacheoff)\
- ) \
- : \
- nocachegetattr((tup), (attnum), (tupleDesc)) \
- ) \
- : \
- ( \
- att_isnull((attnum)-1, (tup)->t_data->t_bits) ? \
- ( \
- (*(isnull) = true), \
- (Datum)NULL \
- ) \
- : \
- ( \
- nocachegetattr((tup), (attnum), (tupleDesc)) \
- ) \
- ) \
-)
-#else /* defined(DISABLE_COMPLEX_MACRO) */
-
-extern Datum fastgetattr(HeapTuple tup, int attnum, TupleDesc tupleDesc,
- bool *isnull);
-#endif /* defined(DISABLE_COMPLEX_MACRO) */
-
-
-/* ----------------
- * heap_getattr
- *
- * Extract an attribute of a heap tuple and return it as a Datum.
- * This works for either system or user attributes. The given attnum
- * is properly range-checked.
- *
- * If the field in question has a NULL value, we return a zero Datum
- * and set *isnull == true. Otherwise, we set *isnull == false.
- *
- * <tup> is the pointer to the heap tuple. <attnum> is the attribute
- * number of the column (field) caller wants. <tupleDesc> is a
- * pointer to the structure describing the row and all its fields.
- * ----------------
- */
-#define heap_getattr(tup, attnum, tupleDesc, isnull) \
- ( \
- ((attnum) > 0) ? \
- ( \
- ((attnum) > (int) HeapTupleHeaderGetNatts((tup)->t_data)) ? \
- getmissingattr((tupleDesc), (attnum), (isnull)) \
- : \
- fastgetattr((tup), (attnum), (tupleDesc), (isnull)) \
- ) \
- : \
- heap_getsysattr((tup), (attnum), (tupleDesc), (isnull)) \
- )
-
-
/* prototypes for functions in common/heaptuple.c */
extern Size heap_compute_data_size(TupleDesc tupleDesc,
Datum *values, bool *isnull);
@@ -815,4 +733,77 @@ extern size_t varsize_any(void *p);
extern HeapTuple heap_expand_tuple(HeapTuple sourceTuple, TupleDesc tupleDesc);
extern MinimalTuple minimal_expand_tuple(HeapTuple sourceTuple, TupleDesc tupleDesc);
+/*
+ * fastgetattr
+ * Fetch a user attribute's value as a Datum (might be either a
+ * value, or a pointer into the data area of the tuple).
+ *
+ * This must not be used when a system attribute might be requested.
+ * Furthermore, the passed attnum MUST be valid. Use heap_getattr()
+ * instead, if in doubt.
+ *
+ * This gets called many times, so we macro the cacheable and NULL
+ * lookups, and call nocachegetattr() for the rest.
+ */
+static inline Datum
+fastgetattr(HeapTuple tup, int attnum, TupleDesc tupleDesc, bool *isnull)
+{
+ AssertMacro(attnum > 0);
+
+ *isnull = false;
+ if (HeapTupleNoNulls(tup))
+ {
+ Form_pg_attribute att;
+
+ att = TupleDescAttr(tupleDesc, attnum - 1);
+ if (att->attcacheoff >= 0)
+ return fetchatt(att, (char *) tup->t_data + tup->t_data->t_hoff +
+ att->attcacheoff);
+ else
+ return nocachegetattr(tup, attnum, tupleDesc);
+ }
+ else
+ {
+ if (att_isnull(attnum - 1, tup->t_data->t_bits))
+ {
+ *isnull = true;
+ return (Datum) NULL;
+ }
+ else
+ return nocachegetattr(tup, attnum, tupleDesc);
+ }
+
+ pg_unreachable();
+}
+
+/*
+ * heap_getattr
+ * Extract an attribute of a heap tuple and return it as a Datum.
+ * This works for either system or user attributes. The given attnum
+ * is properly range-checked.
+ *
+ * If the field in question has a NULL value, we return a zero Datum
+ * and set *isnull == true. Otherwise, we set *isnull == false.
+ *
+ * <tup> is the pointer to the heap tuple. <attnum> is the attribute
+ * number of the column (field) caller wants. <tupleDesc> is a
+ * pointer to the structure describing the row and all its fields.
+ *
+ */
+static inline Datum
+heap_getattr(HeapTuple tup, int attnum, TupleDesc tupleDesc, bool *isnull)
+{
+ if (attnum > 0)
+ {
+ if (attnum > (int) HeapTupleHeaderGetNatts(tup->t_data))
+ return getmissingattr(tupleDesc, attnum, isnull);
+ else
+ return fastgetattr(tup, attnum, tupleDesc, isnull);
+ }
+ else
+ return heap_getsysattr(tup, attnum, tupleDesc, isnull);
+
+ pg_unreachable();
+}
+
#endif /* HTUP_DETAILS_H */
--
2.30.2
--r7zmaabpzc6r33ml--
^ permalink raw reply [nested|flat] 4+ messages in thread
* ARM/ARM64 SpinLockAcquire and SpinLockRelease are not full barriers
@ 2025-05-13 16:22 Yura Sokolov <[email protected]>
2025-06-02 18:28 ` Re: ARM/ARM64 SpinLockAcquire and SpinLockRelease are not full barriers Yura Sokolov <[email protected]>
0 siblings, 1 reply; 4+ messages in thread
From: Yura Sokolov @ 2025-05-13 16:22 UTC (permalink / raw)
To: [email protected] <[email protected]>; Andres Freund <[email protected]>; Alexander Korotkov <[email protected]>
Good day.
During conversation about other patch, Andres pointed out SpinLockAcquire
have semantic of full memory barrier [1] .
spin.h also states:
> Load and stores operation in calling code are guaranteed not to be
reordered with respect to these operations, because they include a compiler
barrier.
But on ARM/ARM64 platform tas in s_lock.h is implemented as simple call to
__sync_lock_test_and_set, and . And GCC's documentation states it has only
Acquire semantic, and __sync_lock_release (used as implementation of
SpinLockRelease) has only Release semantic [2].
Compiling and disassembling postgresql with -O2 optimization level on arm64
we can see that SpinLockAquire is compiled down to call to
__aarch64_swp4_sync (in optimistic case) which has disassemble:
0000000000662cc0 <__aarch64_swp4_sync>:
662cc0: d503245f bti c
662cc4: 90001bf0 adrp x16, 9de000 <hist_start+0x3d18>
662cc8: 39582210 ldrb w16, [x16, #1544]
662ccc: 34000070 cbz w16, 662cd8
662cd0: b8a08020 swpa w0, w0, [x1]
662cd4: d65f03c0 ret
662cd8: 2a0003f0 mov w16, w0
662cdc: 885f7c20 ldxr w0, [x1]
662ce0: 88117c30 stxr w17, w16, [x1]
662ce4: 35ffffd1 cbnz w17, 662cdc
662ce8: d5033bbf dmb ish
662cec: d65f03c0 ret
Here we see 'swpa' instruction which has only acquire semantic [3].
And SpinLockRelease generates 'stlr' instruction, which has release
semantic, iirc.
Godbolt shows noticable difference between __sync_lock_test_and_set vs
__atomic_exchange_n(ATOMIC_SEQ_CST) (and __sync_lock_release vs
__atomic_store_n(ATOMIC_SEQ_CST)) using GCC even with march=armv7 [4]:
- __atomic_exchange_n and __atomic_store_n has 'dmb ish' instruction both
before and after load/store operations,
- but __sync_lock_test_and_set has 'dmb ish' only after load/store
operations and __sync_lock_release only before store operation. (You can
see same pattern in __aarch64_swp4_sync above in case 'swpa' instruction is
not present).
Therefore I think neither SpinLockAcquire nor SpinLockRelease have no full
memory barrier semantic on ARM/ARM64 at the moment when compiled with GCC.
Surprisingly, Clang puts 'dmb ish' both before and after load/store at
__sync_lock_test_and_set, but does not the same for __sync_lock_release.
As a simple fix I suggest to put __sync_synchronize before
__sync_lock_test_and_set and after __sync_lock_release.
Initially I wanted to use __atomic_exchange_n and __atomic_store_n. But in
absence of support for atomic instructions, their inner loop doesn't look
to be safe for reordering since it uses ldar+stlr loop:
- looks like previous operations could be reordered after 'ldar'.
- similarly following operations could be reordered before 'stlr'.
There's very relevant discussion at GCC's bugzilla [5].
Probably this hypothesis is not valid. I believe we have to ask someone
closely familiar with ARM internals to be sure.
# Alternative.
As an alternative, we may fix comments about
SpinLockAcquire/SpinLockRelease to state they provide acquire and release
barrier semantics only. And then check all their uses and set appropriate
memory barriers where their usage as full memory barriers is detected (as
in [1]).
[1]
https://postgr.es/m/5ccgykypol3azijw2chqnpg3rhuwjtwsmbfs3pgcqm7fu6laus%40wppo6zcfszay
[2]
https://gcc.gnu.org/onlinedocs/gcc-15.1.0/gcc/_005f_005fsync-Builtins.html#index-_005f_005fsync_005f...
[3]
https://developer.arm.com/documentation/100069/0606/Data-Transfer-Instructions/SWPA--SWPAL--SWP--SWP...
[4] https://godbolt.org/z/h5P9fjzWd
[5] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65697
--
regards
Yura Sokolov aka funny-falcon
Attachments:
[text/x-patch] v1-0001-Make-SpinLockAcquire-and-SpinLockRelease-full-mem.patch (2.1K, ../../[email protected]/2-v1-0001-Make-SpinLockAcquire-and-SpinLockRelease-full-mem.patch)
download | inline diff:
From 1315faa9a01c8d768fd750ed643bae4f3eefc48a Mon Sep 17 00:00:00 2001
From: Yura Sokolov <[email protected]>
Date: Tue, 13 May 2025 18:57:46 +0300
Subject: [PATCH v1] Make SpinLockAcquire and SpinLockRelease full memory
barriers.
Andres Freund stated SpinLockAcquire is used as a full memory barrier at
least once [1].
Code comment in spin.h also states:
> Load and stores operation in calling code are guaranteed not to be
> reordered with respect to these operations, because they include a
> compiler barrier.
But ARM/ARM64 code used __spin_lock_test_and_set and __spin_lock_release
which only have acquire and release semantics respectively.
Fix it by adding __sync_synchronize before __sync_lock_test_and_set and
after __sync_lock_release.
---
src/include/storage/s_lock.h | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/src/include/storage/s_lock.h b/src/include/storage/s_lock.h
index 2f73f9fcf57..7ee94302df1 100644
--- a/src/include/storage/s_lock.h
+++ b/src/include/storage/s_lock.h
@@ -258,10 +258,14 @@ typedef int slock_t;
static __inline__ int
tas(volatile slock_t *lock)
{
+ __sync_synchronize();
return __sync_lock_test_and_set(lock, 1);
}
-#define S_UNLOCK(lock) __sync_lock_release(lock)
+#define S_UNLOCK(lock) do { \
+ __sync_lock_release(lock); \
+ __sync_synchronize(); \
+} while(0)
#if defined(__aarch64__)
@@ -543,10 +547,14 @@ typedef int slock_t;
static __inline__ int
tas(volatile slock_t *lock)
{
+ __sync_synchronize();
return __sync_lock_test_and_set(lock, 1);
}
-#define S_UNLOCK(lock) __sync_lock_release(lock)
+#define S_UNLOCK(lock) do { \
+ __sync_lock_release(lock); \
+ __sync_synchronize(); \
+} while(0)
#elif defined(HAVE_GCC__SYNC_CHAR_TAS)
#define HAS_TEST_AND_SET
@@ -558,10 +566,14 @@ typedef char slock_t;
static __inline__ int
tas(volatile slock_t *lock)
{
+ __sync_synchronize();
return __sync_lock_test_and_set(lock, 1);
}
-#define S_UNLOCK(lock) __sync_lock_release(lock)
+#define S_UNLOCK(lock) do { \
+ __sync_lock_release(lock); \
+ __sync_synchronize(); \
+} while(0)
#endif /* HAVE_GCC__SYNC_INT32_TAS */
--
2.43.0
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: ARM/ARM64 SpinLockAcquire and SpinLockRelease are not full barriers
2025-05-13 16:22 ARM/ARM64 SpinLockAcquire and SpinLockRelease are not full barriers Yura Sokolov <[email protected]>
@ 2025-06-02 18:28 ` Yura Sokolov <[email protected]>
0 siblings, 0 replies; 4+ messages in thread
From: Yura Sokolov @ 2025-06-02 18:28 UTC (permalink / raw)
To: [email protected] <[email protected]>; Andres Freund <[email protected]>; Alexander Korotkov <[email protected]>
Good day, hackers.
13.05.2025 19:22, Yura Sokolov wrote:
> Good day.
>
> During conversation about other patch, Andres pointed out SpinLockAcquire
> have semantic of full memory barrier [1] .
>
> spin.h also states:
>
>> Load and stores operation in calling code are guaranteed not to be
> reordered with respect to these operations, because they include a compiler
> barrier.
>
> But on ARM/ARM64 platform tas in s_lock.h is implemented as simple call to
> __sync_lock_test_and_set, and . And GCC's documentation states it has only
> Acquire semantic, and __sync_lock_release (used as implementation of
> SpinLockRelease) has only Release semantic [2].
>
> Compiling and disassembling postgresql with -O2 optimization level on arm64
> we can see that SpinLockAquire is compiled down to call to
> __aarch64_swp4_sync (in optimistic case) which has disassemble:
>
> 0000000000662cc0 <__aarch64_swp4_sync>:
> 662cc0: d503245f bti c
> 662cc4: 90001bf0 adrp x16, 9de000 <hist_start+0x3d18>
> 662cc8: 39582210 ldrb w16, [x16, #1544]
> 662ccc: 34000070 cbz w16, 662cd8
> 662cd0: b8a08020 swpa w0, w0, [x1]
> 662cd4: d65f03c0 ret
> 662cd8: 2a0003f0 mov w16, w0
> 662cdc: 885f7c20 ldxr w0, [x1]
> 662ce0: 88117c30 stxr w17, w16, [x1]
> 662ce4: 35ffffd1 cbnz w17, 662cdc
> 662ce8: d5033bbf dmb ish
> 662cec: d65f03c0 ret
>
> Here we see 'swpa' instruction which has only acquire semantic [3].
>
> And SpinLockRelease generates 'stlr' instruction, which has release
> semantic, iirc.
>
> Godbolt shows noticable difference between __sync_lock_test_and_set vs
> __atomic_exchange_n(ATOMIC_SEQ_CST) (and __sync_lock_release vs
> __atomic_store_n(ATOMIC_SEQ_CST)) using GCC even with march=armv7 [4]:
> - __atomic_exchange_n and __atomic_store_n has 'dmb ish' instruction both
> before and after load/store operations,
> - but __sync_lock_test_and_set has 'dmb ish' only after load/store
> operations and __sync_lock_release only before store operation. (You can
> see same pattern in __aarch64_swp4_sync above in case 'swpa' instruction is
> not present).
>
> Therefore I think neither SpinLockAcquire nor SpinLockRelease have no full
> memory barrier semantic on ARM/ARM64 at the moment when compiled with GCC.
>
> Surprisingly, Clang puts 'dmb ish' both before and after load/store at
> __sync_lock_test_and_set, but does not the same for __sync_lock_release.
>
> As a simple fix I suggest to put __sync_synchronize before
> __sync_lock_test_and_set and after __sync_lock_release.
>
> Initially I wanted to use __atomic_exchange_n and __atomic_store_n. But in
> absence of support for atomic instructions, their inner loop doesn't look
> to be safe for reordering since it uses ldar+stlr loop:
> - looks like previous operations could be reordered after 'ldar'.
> - similarly following operations could be reordered before 'stlr'.
> There's very relevant discussion at GCC's bugzilla [5].
> Probably this hypothesis is not valid. I believe we have to ask someone
> closely familiar with ARM internals to be sure.
>
> # Alternative.
>
> As an alternative, we may fix comments about
> SpinLockAcquire/SpinLockRelease to state they provide acquire and release
> barrier semantics only. And then check all their uses and set appropriate
> memory barriers where their usage as full memory barriers is detected (as
> in [1]).
>
> [1]
> https://postgr.es/m/5ccgykypol3azijw2chqnpg3rhuwjtwsmbfs3pgcqm7fu6laus%40wppo6zcfszay
> [2]
> https://gcc.gnu.org/onlinedocs/gcc-15.1.0/gcc/_005f_005fsync-Builtins.html#index-_005f_005fsync_005f...
> [3]
> https://developer.arm.com/documentation/100069/0606/Data-Transfer-Instructions/SWPA--SWPAL--SWP--SWP...
> [4] https://godbolt.org/z/h5P9fjzWd
> [5] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65697
I've recognized my mistake about SpinLockAcquire/SpinLockRelease
requirements. In [1] I've describe my thoughts and suggested to introduce
read-write spin lock for the cases when there are a lot of readers and few
(or one) writers.
[1] https://postgr.es/m/450dafae-b620-4726-abc2-53347c419394%40postgrespro.ru
--
regards
Yura Sokolov aka funny-falcon
^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2025-06-02 18:28 UTC | newest]
Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-03-24 09:40 [PATCH] Change fastgetattr and heap_getattr to inline functions Alvaro Herrera <[email protected]>
2022-03-24 09:40 [PATCH v2] Change fastgetattr and heap_getattr to inline functions Alvaro Herrera <[email protected]>
2025-05-13 16:22 ARM/ARM64 SpinLockAcquire and SpinLockRelease are not full barriers Yura Sokolov <[email protected]>
2025-06-02 18:28 ` Re: ARM/ARM64 SpinLockAcquire and SpinLockRelease are not full barriers Yura Sokolov <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox