Re: better atomics - v0.5

On 2014-06-30 11:04:53 +0530, Amit Kapila wrote:
> On Sun, Jun 29, 2014 at 2:54 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > On 2014-06-29 11:53:28 +0530, Amit Kapila wrote:
> > > On Sat, Jun 28, 2014 at 1:48 PM, Andres Freund <andres(at)2ndquadrant(dot)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