Re: better atomics - v0.5 - Mailing list pgsql-hackers
| From | Amit Kapila |
|---|---|
| Subject | Re: better atomics - v0.5 |
| Date | |
| Msg-id | CAA4eK1JwFe1qOMVGRH6TdWEKjX_LbgZzz=OeDg1GO=BVw9QJbA@mail.gmail.com Whole thread Raw |
| In response to | better atomics - v0.5 (Andres Freund <andres@2ndquadrant.com>) |
| Responses |
Re: better atomics - v0.5
(Andres Freund <andres@2ndquadrant.com>)
|
| List | pgsql-hackers |
On Wed, Jun 25, 2014 at 10:44 PM, Andres Freund <andres@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.
>
> 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:
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:
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:
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:
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.
pgsql-hackers by date: