Re: better atomics - v0.5

On Sat, Jun 28, 2014 at 1:48 PM, Andres Freund <andres(at)2ndquadrant(dot)com>
wrote:
> On 2014-06-28 11:58:41 +0530, Amit Kapila wrote:
> > 1.
> > +pg_atomic_compare_exchange_u32_impl(volatile pg_atomic_uint32 *ptr,
> > + uint32 *expected, uint32 newval)
> > +{
> > + bool ret;
> > + uint32 current;
> > + current = InterlockedCompareExchange(&ptr->value, newval, *expected);
> >
> > Is there a reason why using InterlockedCompareExchange() is better
> > than using InterlockedCompareExchangeAcquire() variant?
>
> 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. I think it might not be sane in all cases,
example with Visual Studio 2003, volatile to volatile references are
ordered; the compiler will not re-order volatile variable access. However,
these operations could be re-ordered by the processor.
For detailed example, refer below link:

http://msdn.microsoft.com/en-us/library/windows/desktop/ms686355(v=vs.85).aspx

Also if we see C11 specs both load and store uses memory order as
memory_order_seq_cst by default which is same as for compare and
exchange
I have referred below link:
http://en.cppreference.com/w/c/atomic/atomic_store

> 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.

> c) It doesn't make any difference on x86 ;)

What about processors like Itanium which care about acquire/release
memory barrier semantics?

> > 2.
> > +pg_atomic_compare_exchange_u32_impl()
> > {
> > ..
> > + *expected = current;
> > }
> >
> > Can we implement this API without changing expected value?
..
> > 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.

>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.

> > 3. Is there a overhead of using Add, instead of increment/decrement
> > version of Interlocked?
>
> There's a theoretical difference for IA64 which has a special
> increment/decrement operation which can only change the value by a
> rather limited number of values. I don't think it's worth to care.

Okay.

> > 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.

> > 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.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com