Re: better atomics - v0.5

On 2014-07-01 10:44:19 +0530, Amit Kapila wrote:
> I think for such usage, we need to rely on barriers wherever there is a
> need for synchronisation, recent example I have noticed is in your patch
> where we have to use pg_write_barrier() during wakeup. However if we
> go by atomic ops definition, then no such dependency is required, like
> if somewhere we need to use pg_atomic_compare_exchange_u32(),
> after that we don't need to ensure about barriers, because this atomic
> API adheres to Full barrier semantics.

> I think defining barrier support for some atomic ops and not for others
> will lead to inconsistency with specs and we need to additionally
> define rules for such atomic ops usage.

Meh. It's quite common that read/load do not have barrier semantics. The
majority of read/write users won't need barriers and it'll be expensive
to provide them with them.

> > > > > > b) It's only supported from vista onwards. Afaik we still support
> XP.
> >
> > Well, with barrier.h as it stands they'd get a compiler error if it
> > indeed is unsupported? But I think there's something else going on -
> > msft might just be removing XP from it's documentation.
>
> Okay, but why would they remove for MemoryBarrier() and not
> for InterlockedCompareExchange(). I think it is bit difficult to predict
> the reason and if we want to use anything which is not as msft docs,
> we shall do that carefully and have some way to ensure that it will
> work fine.

Well, we have quite good evidence for it working by knowing that
postgres has in the past worked on xp? If you have a better idea, send
in a patch for today's barrier.h and I'll adopt that.

> > > In this case, I have a question for you.
> > >
> > > Un-patched usage in barrier.h is as follows:
> > > ..
> > >
> > > If I understand correctly the current define mechanism in barrier.h,
> > > it will have different definition for Itanium processors even for windows.
> >
> > Either noone has ever tested postgres on itanium windows (quite
> > possible), because afaik _Asm_sched_fence() doesn't work on
> > windows/msvc, or windows doesn't define __ia64__/__ia64. Those macros
> > arefor HP's acc on HPUX.
>
> Hmm.. Do you think in such a case, it would have gone in below
> define:

> #elif defined(__INTEL_COMPILER)
> /*
> * icc defines __GNUC__, but doesn't support gcc's inline asm syntax
> */
> #if defined(__ia64__) || defined(__ia64)
> #define pg_memory_barrier() __mf()
> #elif defined(__i386__) || defined(__x86_64__)
> #define pg_memory_barrier() _mm_mfence()
> #endif

Hm? Since _Asm_sched_fence isn't for the intel compiler but for HP's
acc, I don't see why?

But they should go into atomics-generic-acc.h.

> -#define pg_compiler_barrier() __memory_barrier()
>
> Currently this will be considered as compiler barrier for
> __INTEL_COMPILER, but after patch, I don't see this define. I think
> after patch, it will be compiler_impl specific, but why such a change?

#if defined(__INTEL_COMPILER)
#define pg_compiler_barrier_impl() __memory_barrier()
#else
#define pg_compiler_barrier_impl() __asm__ __volatile__("" ::: "memory")
#endif

There earlier was a typo defining pg_compiler_barrier instead of
pg_compiler_barrier_impl for intel, maybe that's what you were referring to?

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services