Re: better atomics - v0.5 - Mailing list pgsql-hackers
| From | Amit Kapila |
|---|---|
| Subject | Re: better atomics - v0.5 |
| Date | |
| Msg-id | CAA4eK1KsUMTErTDf-ht3-Qb7NvkVPGd-=Uy7NrEi2oviJgk9tA@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
(Amit Kapila <amit.kapila16@gmail.com>)
|
| List | pgsql-hackers |
On Tue, Jul 1, 2014 at 4:10 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> On 2014-07-01 10:44:19 +0530, Amit Kapila wrote:
>
> > 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.
>
> Meh. It's quite common that read/load do not have barrier semantics. The
> majority of read/write users won't need barriers and it'll be expensive
> to provide them with them.
So in such a case they shouldn't use atomic API and rather access
+pg_atomic_read_u64_impl(volatile pg_atomic_uint64 *ptr)
+{
..
+ pg_atomic_compare_exchange_u64_impl(ptr, &old, 0);
> On 2014-07-01 10:44:19 +0530, Amit Kapila wrote:
>
> > 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.
>
> Meh. It's quite common that read/load do not have barrier semantics. The
> majority of read/write users won't need barriers and it'll be expensive
> to provide them with them.
So in such a case they shouldn't use atomic API and rather access
directly, I think part of the purpose to use atomic ops is also that they
should provide synchronisation for read/write among processes, earlier
we don't have any such facility, so wherever required we have no option
but to use barriers to achieve the same.
One more point here, in patch 64-bit atomic read/write are
implemented using *_exchange_* API, won't this automatically ensure
that 64-bit versions have barrier support?
+{
..
+ pg_atomic_compare_exchange_u64_impl(ptr, &old, 0);
..
> > > > In this case, I have a question for you.
>
> > > 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
>
> Hm? Since _Asm_sched_fence isn't for the intel compiler but for HP's
> acc, I don't see why?
Sorry, I don't understand what you mean by above?
+}
> > > > > > > b) It's only supported from vista onwards. Afaik we still support
> > XP.
>
> Well, we have quite good evidence for it working by knowing that
> postgres has in the past worked on xp? If you have a better idea, send
> in a patch for today's barrier.h and I'll adopt that.
> > > > > > > b) It's only supported from vista onwards. Afaik we still support
> > XP.
>
> Well, we have quite good evidence for it working by knowing that
> postgres has in the past worked on xp? If you have a better idea, send
> in a patch for today's barrier.h and I'll adopt that.
Here the only point was to check if it is better to use version of
InterLocked... API which is a recommended for the case of Itanium-
based processor and will behave same for others. If you have any
inclination/suggestion for the same, then I can try for it.
> > > > In this case, I have a question for you.
>
> > > 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
>
> Hm? Since _Asm_sched_fence isn't for the intel compiler but for HP's
> acc, I don't see why?
Sorry, I don't understand what you mean by above?
The point I wanted to clarify here was that is it possible that above
defines lead to different definitions for Itanium on windows?
> But they should go into atomics-generic-acc.h.
Okay, but in your current patch (0001-Basic-atomic-ops-implementation),
> But they should go into atomics-generic-acc.h.
Okay, but in your current patch (0001-Basic-atomic-ops-implementation),
they seem to be in different files.
> > -#define pg_compiler_barrier() __memory_barrier()
> >
> > 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?
>
> #if defined(__INTEL_COMPILER)
> #define pg_compiler_barrier_impl() __memory_barrier()
> #else
> #define pg_compiler_barrier_impl() __asm__ __volatile__("" ::: "memory")
> #endif
>
> There earlier was a typo defining pg_compiler_barrier instead of
> pg_compiler_barrier_impl for intel, maybe that's what you were referring to?
I have tried to search for this in patch file 0001-Basic-atomic-ops-implementation
> > -#define pg_compiler_barrier() __memory_barrier()
> >
> > 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?
>
> #if defined(__INTEL_COMPILER)
> #define pg_compiler_barrier_impl() __memory_barrier()
> #else
> #define pg_compiler_barrier_impl() __asm__ __volatile__("" ::: "memory")
> #endif
>
> There earlier was a typo defining pg_compiler_barrier instead of
> pg_compiler_barrier_impl for intel, maybe that's what you were referring to?
I have tried to search for this in patch file 0001-Basic-atomic-ops-implementation
and found that it has been removed from barrier.h but not added anywhere else.
pgsql-hackers by date: