On Wed, Jun 25, 2014 at 10:44 PM, Andres Freund <and...@2ndquadrant.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: 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? *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. 3. Is there a overhead of using Add, instead of increment/decrement version of Interlocked? I could not find any evidence which can clearly indicate, if one is better than other except some recommendation in below link which suggests to *maintain reference count*, use Increment/Decrement Refer *The Locking Cookbook* in below link: http://blogs.technet.com/b/winserverperformance/archive/2008/05/21/designing-applications-for-high-performance-part-ii.aspx However, when I tried to check the instructions each function generates, then I don't find anything which suggests that *Add variant can be slow. Refer Instructions for both functions: cPublicProc _InterlockedIncrement,1 cPublicFpo 1,0 mov ecx,Addend ; get pointer to addend variable mov eax,1 ; set increment value Lock1: lock xadd [ecx],eax ; interlocked increment inc eax ; adjust return value stdRET _InterlockedIncrement ; stdENDP _InterlockedIncrement cPublicProc _InterlockedExchangeAdd, 2 cPublicFpo 2,0 mov ecx, [esp + 4] ; get addend address mov eax, [esp + 8] ; get increment value Lock4: lock xadd [ecx], eax ; exchange add stdRET _InterlockedExchangeAdd stdENDP _InterlockedExchangeAdd For details, refer link: http://read.pudn.com/downloads3/sourcecode/windows/248345/win2k/private/windows/base/client/i386/critsect.asm__.htm I don't think there is any need of change in current implementation, however I just wanted to share my analysis, so that if any one else can see any point in preferring one or the other way of implementation. 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. Please refer below link: http://msdn.microsoft.com/en-us/library/f20w0x5e(v=vs.110).aspx When I checked previous versions of MSVC, I noticed that these are supported on x86, IPF, x64 till VS2008 and supported only on x86, x64 for VS2012 onwards. Link for VS2010 support: http://msdn.microsoft.com/en-us/library/f20w0x5e(v=vs.100).aspx 5. + * atomics-generic-msvc.h .. + * Portions Copyright (c) 2013, PostgreSQL Global Development Group Shouldn't copyright notice be 2014? 6. pg_atomic_compare_exchange_u32() It is better to have comments above this and all other related functions. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com