On 02/01/2017 01:07 PM, Konstantin Knizhnik wrote:
Attached please find my patch for XLC/AIX.
The most critical fix is adding __sync to pg_atomic_fetch_add_u32_impl.
The comment in this file says that:
* __fetch_and_add() emits a leading "sync" and trailing "isync",
thereby
* providing sequential consistency. This is undocumented.
But it is not true any more (I checked generated assembler code in
debugger).
This is why I have added __sync() to this function. Now pgbench working
normally.
Seems like it was not so much undocumented, but an implementation detail
that was not guaranteed after all..
Does __fetch_and_add emit a trailing isync there either? Seems odd if
__compare_and_swap requires it, but __fetch_and_add does not. Unless we
can find conclusive documentation on that, I think we should assume that
an __isync() is required, too.
Also there is mysterious disappearance of assembler section function
with sync instruction from pg_atomic_compare_exchange_u32_impl.
I have fixed it by using __sync() built-in function instead.
__sync() seems more appropriate there, anyway. We're using intrinsics
for all the other things in generic-xlc.h. But it sure is scary that the
"asm" sections just disappeared.
In arch-ppc.h, shouldn't we have #ifdef __IBMC__ guards for the __sync()
and __lwsync() intrinsics? Those are an xlc compiler-specific thing,
right? Or if they are expected to work on any ppc compiler, then we
should probably use them always, instead of the asm sections.
In summary, I came up with the attached. It's essentially your patch,
with tweaks for the above-mentioned things. I don't have a powerpc
system to test on, so there are probably some silly typos there.
- Heikki
diff --git a/src/include/port/atomics/arch-ppc.h
b/src/include/port/atomics/arch-ppc.h
index ed1cd9d1b9..7cf8c8ef97 100644
--- a/src/include/port/atomics/arch-ppc.h
+++ b/src/include/port/atomics/arch-ppc.h
@@ -23,4 +23,11 @@
#define pg_memory_barrier_impl() __asm__ __volatile__ ("sync" : : :
"memory")
#define pg_read_barrier_impl() __asm__ __volatile__ ("lwsync" : : :
"memory")
#define pg_write_barrier_impl() __asm__ __volatile__ ("lwsync"
: : : "memory")
+
+#if defined(__IBMC__) || defined(__IBMCPP__)
+
+#define pg_memory_barrier_impl() __sync()
+#define pg_read_barrier_impl() __lwsync()
+#define pg_write_barrier_impl() __lwsync()
+
#endif
diff --git a/src/include/port/atomics/generic-xlc.h
b/src/include/port/atomics/generic-xlc.h
index f854612d39..e1dd3310a5 100644
--- a/src/include/port/atomics/generic-xlc.h
+++ b/src/include/port/atomics/generic-xlc.h
@@ -48,7 +48,7 @@ pg_atomic_compare_exchange_u32_impl(volatile pg_atomic_uint32
*ptr,
* consistency only, do not use it here. GCC atomics observe the same
* restriction; see its rs6000_pre_atomic_barrier().
*/
- __asm__ __volatile__ (" sync \n" ::: "memory");
+ __sync();
/*
* XXX: __compare_and_swap is defined to take signed parameters, but
that
@@ -73,11 +73,19 @@ pg_atomic_compare_exchange_u32_impl(volatile
pg_atomic_uint32 *ptr,
static inline uint32
pg_atomic_fetch_add_u32_impl(volatile pg_atomic_uint32 *ptr, int32 add_)
{
+ uint32 ret;
+
/*
- * __fetch_and_add() emits a leading "sync" and trailing "isync",
thereby
- * providing sequential consistency. This is undocumented.
+ * Use __sync() before and __isync() after, like in compare-exchange
+ * above.
*/
- return __fetch_and_add((volatile int *)&ptr->value, add_);
+ __sync();
+
+ ret = __fetch_and_add((volatile int *)&ptr->value, add_);
+
+ __isync();
+
+ return ret;
}
#ifdef PG_HAVE_ATOMIC_U64_SUPPORT
@@ -89,7 +97,7 @@ pg_atomic_compare_exchange_u64_impl(volatile pg_atomic_uint64
*ptr,
{
bool ret;
- __asm__ __volatile__ (" sync \n" ::: "memory");
+ __sync();
ret = __compare_and_swaplp((volatile long*)&ptr->value,
(long *)expected,
(long)newval);
@@ -103,7 +111,15 @@ pg_atomic_compare_exchange_u64_impl(volatile
pg_atomic_uint64 *ptr,
static inline uint64
pg_atomic_fetch_add_u64_impl(volatile pg_atomic_uint64 *ptr, int64 add_)
{
- return __fetch_and_addlp((volatile long *)&ptr->value, add_);
+ uint64 ret;
+
+ __sync();
+
+ ret = __fetch_and_addlp((volatile long *)&ptr->value, add_);
+
+ __isync();
+
+ return ret;
}
#endif /* PG_HAVE_ATOMIC_U64_SUPPORT */
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
