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 -- Sent via pgsql-hackers mailing list (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers