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

> >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:

Reply via email to