Re: better atomics - v0.5 - Mailing list pgsql-hackers
| From | Amit Kapila |
|---|---|
| Subject | Re: better atomics - v0.5 |
| Date | |
| Msg-id | CAA4eK1Jygpgm-uCz4Hj+wQ8mfPP6rJ102326H5=OMgzfpktgoA@mail.gmail.com Whole thread Raw |
| In response to | Re: better atomics - v0.5 (Andres Freund <andres@2ndquadrant.com>) |
| Responses |
Re: better atomics - v0.5
|
| List | pgsql-hackers |
On Mon, Jun 30, 2014 at 2:54 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> 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:
> > > 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.
I think for such usage, we need to rely on barriers wherever there is a
if somewhere we need to use pg_atomic_compare_exchange_u32(),
> 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:
> > > 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.
I think for such usage, we need to rely on barriers wherever there is a
need for synchronisation, recent example I have noticed is in your patch
where we have to use pg_write_barrier() during wakeup. However if we
go by atomic ops definition, then no such dependency is required, likeif somewhere we need to use pg_atomic_compare_exchange_u32(),
after that we don't need to ensure about barriers, because this atomic
API adheres to Full barrier semantics.
I think defining barrier support for some atomic ops and not for others
will lead to inconsistency with specs and we need to additionally
define rules for such atomic ops usage.
I think we can still rely on volatile read/store for ordering guarantee
within same thread of execution and where ever we need synchronisation
of usage among different processes, we can directly use atomic ops
(get/set) without the need to use barriers.
> > In this case, I have a question for you.
> >
> > Un-patched usage in barrier.h is as follows:
> > ..
> >
> > 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.
Hmm.. Do you think in such a case, it would have gone in below
> > > > > > 6.
> > 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 think this depends on individual's perspective, so do the way
> > > > > b) It's only supported from vista onwards. Afaik we still support XP.
>
> 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.
>
> 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.
Okay, but why would they remove for MemoryBarrier() and not
for InterlockedCompareExchange(). I think it is bit difficult to predict
the reason and if we want to use anything which is not as msft docs,
we shall do that carefully and have some way to ensure that it will
work fine.
> > In this case, I have a question for you.
> >
> > Un-patched usage in barrier.h is as follows:
> > ..
> >
> > 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.
Hmm.. Do you think in such a case, it would have gone in below
define:
#elif defined(__INTEL_COMPILER)
/*
* icc defines __GNUC__, but doesn't support gcc's inline asm syntax
*/
#if defined(__ia64__) || defined(__ia64)
#define pg_memory_barrier() __mf()
#elif defined(__i386__) || defined(__x86_64__)
#define pg_memory_barrier() _mm_mfence()
#endif
-#define pg_compiler_barrier() __memory_barrier()
> > However the patch defines as below:
/*
* icc defines __GNUC__, but doesn't support gcc's inline asm syntax
*/
#if defined(__ia64__) || defined(__ia64)
#define pg_memory_barrier() __mf()
#elif defined(__i386__) || defined(__x86_64__)
#define pg_memory_barrier() _mm_mfence()
#endif
Currently this will be considered as compiler barrier for
__INTEL_COMPILER, but after patch, I don't see this define. I think
after patch, it will be compiler_impl specific, but why such a change?
> > However the patch defines as below:
..
> > 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.
I think as per your new compiler specific implementation stuff, this holds
> > 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.
I think as per your new compiler specific implementation stuff, this holds
true.
> > > > > > 6.
> > 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 think this depends on individual's perspective, so do the way
you feel better, the intention of this point was lets not deviate from
existing coding ways.
> I'd much rather get rid of the separated definition/declaration, but
> we'd need to rely on PG_USE_INLINE for it...
> I'd much rather get rid of the separated definition/declaration, but
> we'd need to rely on PG_USE_INLINE for it...
Okay.
pgsql-hackers by date: