On Sat, Jun 28, 2014 at 1:48 PM, Andres Freund <and...@2ndquadrant.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