On Tue, Jul 31, 2012 at 4:06 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> IMHO, the way we have it now is kind of a mess. SpinLockAcquire and
>> SpinLockRelease are required to be CPU barriers, but they are not
>> required to be compiler barriers. If we changed that so that they
>> were required to act as barriers of both flavors,
>
> Since they are macros, how do you propose to do that exactly?
Why does it matter that they are macros?
> I agree that volatile-izing everything in the vicinity is a sucky
> solution, but the last time we looked at this there did not seem to
> be a better one.
Well, Linux has a barrier() primitive which is defined as a
compiler-barrier, so I don't see why we shouldn't be able to manage
the same thing. In fact, we've already got it, though it's presently
unused; see storage/barrier.h.
Looking over s_lock.h, it looks like TAS is typically defined using
__asm__ __volatile__, and the __asm__ is marked as clobbering memory.
As the fine comments say "this prevents gcc from thinking it can cache
the values of shared-memory fields across the asm code", which is
another way of saying that it's a compiler barrier. However, there's
no similar guard in S_UNLOCK, which is simply declared as a volatile
store, and therefore compiler ordering is guaranteed only with respect
to other volatile pointer references. If we added something of the
form __asm__ __volatile__("" : : : "memory") in there, it should
serve as a full compiler barrier. That might have to go in a static
inline function as we do with TAS, but I think it should work.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company