Re: better atomics - v0.5

On 2014-06-28 11:58:41 +0530, Amit Kapila wrote:
> On Wed, Jun 25, 2014 at 10:44 PM, Andres Freund <andres(at)2ndquadrant(dot)com>
> wrote:
> >
> > Attached is a new version of the atomic operations patch. Lots has
> > changed since the last post:
> >
> > * gcc, msvc work. acc, xlc, sunpro have blindly written support which
> > should be relatively easy to fix up. All try to implement TAS, 32 bit
> > atomic add, 32 bit compare exchange; some do it for 64bit as well.
>
> I have reviewed msvc part of patch and below are my findings:

Thanks for looking at it - I pretty much wrote that part blindly.

> 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.
b) It's only supported from vista onwards. Afaik we still support XP.
c) It doesn't make any difference on x86 ;)

> *Acquire or *Release variants can provide acquire memory ordering
> semantics for processors which support the same else will be defined
> as InterlockedCompareExchange.

> 2.
> +pg_atomic_compare_exchange_u32_impl()
> {
> ..
> + *expected = current;
> }
>
> Can we implement this API without changing expected value?
> I mean if the InterlockedCompareExchange() is success, then it will
> store the newval in ptr->value, else *ret* will be false.
> I am not sure if there is anything implementation specific in changing
> *expected*.
> 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. 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.

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

> 4.
> #pragma intrinsic(_ReadWriteBarrier)
> #define pg_compiler_barrier_impl() _ReadWriteBarrier()
>
> #ifndef pg_memory_barrier_impl
> #define pg_memory_barrier_impl() MemoryBarrier()
> #endif
>
> 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?

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

Thanks,

Andres Freund