Re: better atomics - v0.5 - Mailing list pgsql-hackers

On 2014-06-29 11:53:28 +0530, Amit Kapila wrote:
> On Sat, Jun 28, 2014 at 1:48 PM, Andres Freund <andres@2ndquadrant.com>
> > Two things:
> > a) compare_exchange is a read/write operator and so far I've defined it
> >    to have full barrier semantics (documented in atomics.h). I'd be
> >    happy to discuss which semantics we want for which operation.
> 
> 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. And that's actually
enforced by the current definition - I think?

> > 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. Those intrinsics aren't actually OS but just
compiler dependent.

Otherwise we could just define it as:

FORCEINLINE
VOID
MemoryBarrier (   VOID   )
{   LONG Barrier;   __asm {       xchg Barrier, eax   }
}


> > c) It doesn't make any difference on x86 ;)
> 
> What about processors like Itanium which care about acquire/release
> memory barrier semantics?

Well, it still will be correct? I don't think it makes much sense to
focus overly much on itanium here with the price of making anything more
complicated for others.

> > > I think this value is required for lwlock patch, but I am wondering why
> > > can't the same be achieved if we just return the *current* value and
> > > then let lwlock patch do the handling based on it.  This will have
> another
> > > advantage that our pg_* API will also have similar signature as native
> > > API's.
> >
> > Many usages of compare/exchange require that you'll get the old value
> > back in an atomic fashion. Unfortunately the Interlocked* API doesn't
> > provide a nice way to do that.
> 
> Yes, but I think the same cannot be accomplished even by using
> expected.

More complicatedly so, yes? I don't think we want those comparisons on
practically every callsite.

> >Note that this definition of
> > compare/exchange both maps nicely to C11's atomics and the actual x86
> > cmpxchg instruction.
> >
> > I've generally tried to mimick C11's API as far as it's
> > possible. There's some limitations because we can't do generic types and
> > such, but other than that.
> 
> If I am reading C11's spec for this API correctly, then it says as below:
> "Atomically compares the value pointed to by obj with the value pointed
> to by expected, and if those are equal, replaces the former with desired
> (performs read-modify-write operation). Otherwise, loads the actual value
> pointed to by obj into *expected (performs load operation)."
> 
> So it essentialy means that it loads actual value in expected only if
> operation is not successful.

Yes. But in the case it's successfull it will already contain the right
value.

> > > 4.
> ..
> > > There is a Caution notice in microsoft site indicating
> > > _ReadWriteBarrier/MemoryBarrier are deprected.
> >
> > It seemed to be the most widely available API, and it's what barrier.h
> > already used.
> > Do you have a different suggestion?
> 
> I am trying to think based on suggestion given by Microsoft, but
> not completely clear how to accomplish the same at this moment.

Well, they refer to C11 stuff. But I think it'll be a while before it
makes sense to use a fallback based on that.

> > > 6.
> > > pg_atomic_compare_exchange_u32()
> > >
> > > It is better to have comments above this and all other related
> functions.
> >
> > Check atomics.h, there's comments above it:
> > /*
> >  * pg_atomic_compare_exchange_u32 - CAS operation
> >  *
> >  * Atomically compare the current value of ptr with *expected and store
> newval
> >  * iff ptr and *expected have the same value. The current value of *ptr
> will
> >  * always be stored in *expected.
> >  *
> >  * Return whether the values have been exchanged.
> >  *
> >  * Full barrier semantics.
> >  */
> 
> 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...

Greetings,

Andres Freund