On Wed, Feb 01, 2017 at 02:39:25PM +0200, Heikki Linnakangas wrote: > 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.
Konstantin, does "make -C src/bin/pg_bench check" fail >10% of the time in the bad build? > Seems like it was not so much undocumented, but an implementation detail > that was not guaranteed after all.. Seems so. > There was a long thread on these things the last time this was changed: > https://www.postgresql.org/message-id/20160425185204.jrvlghn3jxulsb7i%40alap3.anarazel.de. > I couldn't find an explanation there of why we thought that fetch_and_add > implicitly performs sync and isync. It was in the generated code, for AIX xlc 12.1.0.0. > >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. That is a problem, but it's a stretch to conclude that asm sections are generally prone to removal, while intrinsics are generally durable. > @@ -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; > } Since this emits double syncs with older xlc, I recommend instead replacing the whole thing with inline asm. As I opined in the last message of the thread you linked above, the intrinsics provide little value as abstractions if one checks the generated code to deduce how to use them. Now that the generated code is xlc-version-dependent, the port is better off with compiler-independent asm like we have for ppc in s_lock.h. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
