From: Andres Freund <andres@2ndquadrant.com>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Peter Geoghegan <pg@heroku.com>
Cc: Pg Hackers <pgsql-hackers@postgresql.org>
Cc: Ants Aasma <ants@cybertec.at>
Subject: Re: better atomics - v0.5
Date: Mon, 30 Jun 2014 11:24:01 +0200
Message-ID: <20140630092401.GJ21422@awork2.anarazel.de> (raw)
In-Reply-To: <CAA4eK1JqqqZuqvKzfUB-XX381xxfGYHSCVmtYA307GKf5X1uyQ@mail.gmail.com>
References: <CA+TgmoYBW+ux5-8Ja=Mcyuy8=VXAnVRHp3Kess6Pn3DMXAPAEA@mail.gmail.com>
<20140625171434.GG24114@awork2.anarazel.de>
<CAA4eK1JwFe1qOMVGRH6TdWEKjX_LbgZzz=OeDg1GO=BVw9QJbA@mail.gmail.com>
<20140628081818.GA6450@awork2.anarazel.de>
<CAA4eK1+j2xp-xGLh2M8xATnv_X=0j1678Pw-wuGy1O8Kw-qWnw@mail.gmail.com>
<20140629092421.GH6450@awork2.anarazel.de>
<CAA4eK1JqqqZuqvKzfUB-XX381xxfGYHSCVmtYA307GKf5X1uyQ@mail.gmail.com>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
On 2014-06-30 11:04:53 +0530, Amit Kapila wrote:
> On Sun, Jun 29, 2014 at 2:54 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> > On 2014-06-29 11:53:28 +0530, Amit Kapila wrote:
> > > On Sat, Jun 28, 2014 at 1:48 PM, Andres Freund <andres@2ndquadrant.com>
> > > 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.
> >
> > Yes, intentionally so. It's often important to get/set the current value
> > without the cost of emitting a memory barrier. It just has to be a
> > recent value and it actually has to come from memory.
>
> I agree with you that we don't want to incur the cost of memory barrier
> for get/set, however it should not be at the cost of correctness.
I really can't follow here. A volatile read/store forces it to go to
memory without barriers. The ABIs of all platforms we work with
guarantee that 4bytes stores/reads are atomic - we've been relying on
that for a long while.
> > And that's actually
> > enforced by the current definition - I think?
>
> Yeah, this is the only point which I am trying to ensure, thats why I sent
> you few links in previous mail where I had got some suspicion that
> just doing get/set with volatile might lead to correctness issue in some
> cases.
But this isn't something this patch started doing - we've been doing
that for a long while?
> Some another Note, I found today while reading more on it which suggests
> that my previous observation is right:
>
> "Within a thread of execution, accesses (reads and writes) to all volatile
> objects are guaranteed to not be reordered relative to each other, but this
> order is not guaranteed to be observed by another thread, since volatile
> access does not establish inter-thread synchronization."
> http://en.cppreference.com/w/c/atomic/memory_order
That's just saying there's no ordering guarantees with volatile
stores/reads. I don't see a contradiction?
> > > > 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.
> >
> > I think that's odd because barrier.h has been using MemoryBarrier()
> > without a version test for a long time now. I guess everyone uses a new
> > enough visual studio.
>
> Yeap or might be the people who even are not using new enough version
> doesn't have a use case that can hit a problem due to MemoryBarrier()
Well, with barrier.h as it stands they'd get a compiler error if it
indeed is unsupported? But I think there's something else going on -
msft might just be removing XP from it's documentation. Postgres supports
building with with visual studio 2005 and MemoryBarrier() seems to be
supported by it.
I think we'd otherwise seen problems since 9.1 where barrier.h was introduced.
> In this case, I have a question for you.
>
> Un-patched usage in barrier.h is as follows:
> ..
> #elif defined(__ia64__) || defined(__ia64)
> #define pg_compiler_barrier() _Asm_sched_fence()
> #define pg_memory_barrier() _Asm_mf()
>
> #elif defined(WIN32_ONLY_COMPILER)
> /* Should work on both MSVC and Borland. */
> #include <intrin.h>
> #pragma intrinsic(_ReadWriteBarrier)
> #define pg_compiler_barrier() _ReadWriteBarrier()
> #define pg_memory_barrier() MemoryBarrier()
> #endif
>
> If I understand correctly the current define mechanism in barrier.h,
> it will have different definition for Itanium processors even for windows.
Either noone has ever tested postgres on itanium windows (quite
possible), because afaik _Asm_sched_fence() doesn't work on
windows/msvc, or windows doesn't define __ia64__/__ia64. Those macros
arefor HP's acc on HPUX.
> However the patch defines as below:
>
> #if defined(HAVE_GCC__ATOMIC_INT32_CAS) || defined(HAVE_GCC__SYNC_INT32_CAS)
> # include "storage/atomics-generic-gcc.h"
> #elif defined(WIN32_ONLY_COMPILER)
> # include "storage/atomics-generic-msvc.h"
>
> What I can understand from above is that defines in
> storage/atomics-generic-msvc.h, will override any previous defines
> for compiler/memory barriers and _ReadWriteBarrier()/MemoryBarrier()
> will be considered for Windows always.
Well, the memory barrier is surrounded by #ifndef
pg_memory_barrier_impl. The compiler barrier can't reasonably be defined
earlier since it's a compiler not an architecture thing.
> > > > > 6.
> > > > > pg_atomic_compare_exchange_u32()
> > > > >
> > > > > It is better to have comments above this and all other related
> > > functions.
> > > Okay, generally code has such comments on top of function
> > > definition rather than with declaration.
> >
> > I don't want to have it on the _impl functions because they're
> > duplicated for the individual platforms and will just become out of
> > date...
>
> Sure, I was also not asking for _impl functions. What I was asking
> in this point was to have comments on top of definition of
> pg_atomic_compare_exchange_u32() in atomics.h
> In particular, on top of below and similar functions, rather than
> at the place where they are declared.
Hm, we can do that. Don't think it'll be clearer (because you need to go
to the header anyway), but I don't feel strongly.
I'd much rather get rid of the separated definition/declaration, but
we'd need to rely on PG_USE_INLINE for it...
Greetings,
Andres Freund
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: andres@2ndquadrant.com, amit.kapila16@gmail.com, robertmhaas@gmail.com, tgl@sss.pgh.pa.us, pg@heroku.com, ants@cybertec.at
Subject: Re: better atomics - v0.5
In-Reply-To: <20140630092401.GJ21422@awork2.anarazel.de>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox