Re: Spinlocks and compiler/memory barriers

On 2014-06-30 12:46:29 -0400, Robert Haas wrote:
> >> But trying to use a spinlock
> >> acquire-release to shore up problems with the spinlock release
> >> implementation makes my head explode.
> >
> > Well, it actually makes some sense. Nearly any TAS() implementation is
> > going to have some memory barrier semantics - so using a TAS() as
> > fallback makes sense. That's why we're relying on it for use in memory
> > barrier emulation after all.
>
> As far as I know, all of our TAS() implementations prevent CPU
> reordering in the acquire direction. It is not clear that they
> provide CPU-reordering guarantees adequate for the release direction,
> even when paired with whatever S_UNLOCK() implementation they're mated
> with.

> And it's quite clear that many of them aren't adequate to prevent
> compiler-reordering in any case.

I actually don't think there currently are tas() implementations that
aren't compiler barriers? Maybe UNIVEL/unixware. A bit of luck.

> > Also note that barrier.h assumes that a S_LOCK/S_UNLOCK acts as a
> > compiler barrier - which really isn't guaranteed by the current contract
> > of s_lock.h. Although the tas() implementations look safe.
>
> Ugh, you're right. That should really be moved out-of-line.

Good. Then we already loose the compile time interdependency from
barrier.h to s_lock.h - although the fallback will have a runtime
dependency.

> >> Do you want to propose a patch that does *only* that first part before
> >> we go further here? Do you want me to try to put together such a
> >> patch based on the details mentioned on this thread?
> >
> > I'm fine with either - we're both going to flying pretty blind :/.
> >
> > I think the way S_UNLOCK's release memory barrier semantics are fixed
> > might influence the way the compiler barrier issue can be solved.
>
> I think I'm starting to understand the terminological confusion: to
> me, a memory barrier means a fence against both the compiler and the
> CPU. I now think you're using it to mean a fence against the CPU, as
> distinct from a fence against the compiler. That had me really
> confused in some previous replies.

Oh. Lets henceforth define 'fence' as the cache coherency thingy and
read/write/release/acquire/memory barrier as the combination?

> > Fixing
> > the release semantics will involve going through all tas()
> > implementations and see whether the default release semantics are
> > ok. The ones with broken semantics will need to grow their own
> > S_UNLOCK. I am wondering if that commit shouldn't just remove the
> > default S_UNLOCK and move it to the tas() implementation sites.
>
> So, I think that here you are talking about CPU behavior rather than
> compiler behavior. Next paragraph is on that basis.

Yes, I am.

> The implementations that don't currently have their own S_UNLOCK() are
> i386
> x86_64

TSO, thus fine.

> Itanium

As a special case volatile stores emit release/acquire fences unless
specific compiler flags are used. Thus safe.

> ARM without GCC atomics

Borked.

> S/390 zSeries

Strongly ordered.

> Sparc
> Sun Studio

Borked. At least in some setups.

> Linux m68k

At least linux doesn't support SMP for m68k, so cache coherency
shouldn't be a problem.

> VAX

I don't really know, but I don't care. The NetBSD people statements about VAX
SMP support didn't increase my concern for VAX SMP.

> m32r

No idea.

> SuperH

Not offially supported (as it's not in installation.sgml), don't care :)

> non-Linux m68k

Couldn't figure out if anybody supports SMP here. Doesn't look like it.

> Univel CC

Don't care.

> HP/UX non-GCC

Itanics volatile semantics should work.

> and WIN32_ONLY_COMPILER

Should be fine due to TSO on x86 and itanic volatiles.

> Because most of those
> are older platforms, I'm betting that more of them than not are OK; I
> think we should confine ourselves to trying to fix the ones we're sure
> are wrong

Sounds sane.

>, which if I understand you correctly are ARM without GCC
> atomics, Sparc, and MIPS.

I've to revise my statement on MIPS, it actually looks safe. I seem to
have missed that it has its own S_UNLOCK.

Do I see it correctly that !(defined(__mips__) && !defined(__sgi)) isn't
supported?

> I think it'd be better to just add copies
> of S_UNLOCK to those three rather and NOT duplicate the definition in
> the other 12 places.

Ok.

Greetings,

Andres Freund

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