On 2014-06-30 11:04:53 +0530, Amit Kapila wrote: > On Sun, Jun 29, 2014 at 2:54 PM, Andres Freund <and...@2ndquadrant.com> wrote: > > On 2014-06-29 11:53:28 +0530, Amit Kapila wrote: > > > On Sat, Jun 28, 2014 at 1:48 PM, Andres Freund <and...@2ndquadrant.com> > > > I think it is better to have some discussion about it. Apart from this, > > > today I noticed atomic read/write doesn't use any barrier semantics > > > and just rely on volatile. > > > > Yes, intentionally so. It's often important to get/set the current value > > without the cost of emitting a memory barrier. It just has to be a > > recent value and it actually has to come from memory. > > I agree with you that we don't want to incur the cost of memory barrier > for get/set, however it should not be at the cost of correctness.
I really can't follow here. A volatile read/store forces it to go to memory without barriers. The ABIs of all platforms we work with guarantee that 4bytes stores/reads are atomic - we've been relying on that for a long while. > > And that's actually > > enforced by the current definition - I think? > > Yeah, this is the only point which I am trying to ensure, thats why I sent > you few links in previous mail where I had got some suspicion that > just doing get/set with volatile might lead to correctness issue in some > cases. But this isn't something this patch started doing - we've been doing that for a long while? > Some another Note, I found today while reading more on it which suggests > that my previous observation is right: > > "Within a thread of execution, accesses (reads and writes) to all volatile > objects are guaranteed to not be reordered relative to each other, but this > order is not guaranteed to be observed by another thread, since volatile > access does not establish inter-thread synchronization." > http://en.cppreference.com/w/c/atomic/memory_order That's just saying there's no ordering guarantees with volatile stores/reads. I don't see a contradiction? > > > > b) It's only supported from vista onwards. Afaik we still support XP. > > > #ifndef pg_memory_barrier_impl > > > #define pg_memory_barrier_impl() MemoryBarrier() > > > #endif > > > > > > The MemoryBarrier() macro used also supports only on vista onwards, > > > so I think for reasons similar to using MemoryBarrier() can apply for > > > this as well. > > > > I think that's odd because barrier.h has been using MemoryBarrier() > > without a version test for a long time now. I guess everyone uses a new > > enough visual studio. > > Yeap or might be the people who even are not using new enough version > doesn't have a use case that can hit a problem due to MemoryBarrier() 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. Postgres supports building with with visual studio 2005 and MemoryBarrier() seems to be supported by it. I think we'd otherwise seen problems since 9.1 where barrier.h was introduced. > In this case, I have a question for you. > > Un-patched usage in barrier.h is as follows: > .. > #elif defined(__ia64__) || defined(__ia64) > #define pg_compiler_barrier() _Asm_sched_fence() > #define pg_memory_barrier() _Asm_mf() > > #elif defined(WIN32_ONLY_COMPILER) > /* Should work on both MSVC and Borland. */ > #include <intrin.h> > #pragma intrinsic(_ReadWriteBarrier) > #define pg_compiler_barrier() _ReadWriteBarrier() > #define pg_memory_barrier() MemoryBarrier() > #endif > > 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. > However the patch defines as below: > > #if defined(HAVE_GCC__ATOMIC_INT32_CAS) || defined(HAVE_GCC__SYNC_INT32_CAS) > # include "storage/atomics-generic-gcc.h" > #elif defined(WIN32_ONLY_COMPILER) > # include "storage/atomics-generic-msvc.h" > > What I can understand from above is that defines in > storage/atomics-generic-msvc.h, will override any previous defines > for compiler/memory barriers and _ReadWriteBarrier()/MemoryBarrier() > will be considered for Windows always. Well, the memory barrier is surrounded by #ifndef pg_memory_barrier_impl. The compiler barrier can't reasonably be defined earlier since it's a compiler not an architecture thing. > > > > > 6. > > > > > pg_atomic_compare_exchange_u32() > > > > > > > > > > It is better to have comments above this and all other related > > > functions. > > > Okay, generally code has such comments on top of function > > > definition rather than with declaration. > > > > I don't want to have it on the _impl functions because they're > > duplicated for the individual platforms and will just become out of > > date... > > Sure, I was also not asking for _impl functions. What I was asking > in this point was to have comments on top of definition of > pg_atomic_compare_exchange_u32() in atomics.h > In particular, on top of below and similar functions, rather than > at the place where they are declared. Hm, we can do that. Don't think it'll be clearer (because you need to go to the header anyway), but I don't feel strongly. I'd much rather get rid of the separated definition/declaration, but we'd need to rely on PG_USE_INLINE for it... Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (firstname.lastname@example.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers