Date: Tue, 1 Jul 2014 12:40:58 +0200
From: Andres Freund <andres@2ndquadrant.com>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: Robert Haas <robertmhaas@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>,
 Peter Geoghegan <pg@heroku.com>, Pg Hackers <pgsql-hackers@postgresql.org>,
 Ants Aasma <ants@cybertec.at>
Subject: Re: better atomics - v0.5
Message-ID: <20140701104058.GZ26930@awork2.anarazel.de>
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>
 <20140630092401.GJ21422@awork2.anarazel.de>
 <CAA4eK1Jygpgm-uCz4Hj+wQ8mfPP6rJ102326H5=OMgzfpktgoA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAA4eK1Jygpgm-uCz4Hj+wQ8mfPP6rJ102326H5=OMgzfpktgoA@mail.gmail.com>
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

On 2014-07-01 10:44:19 +0530, Amit Kapila wrote:
> 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, like
> if  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.

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.

> > > > > > 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.
> 
> 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.

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.

> > > 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

Hm? Since _Asm_sched_fence isn't for the intel compiler but for HP's
acc, I don't see why?

But they should go into atomics-generic-acc.h.

> -#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?


Greetings,

Andres Freund

-- 
 Andres Freund	                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers