Re: Spinlocks and compiler/memory barriers

On 2014-06-30 11:38:31 -0400, Robert Haas wrote:
> On Mon, Jun 30, 2014 at 11:17 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> >> Now, we want to make these
> >> operations compiler fences as well, and AIUI your proposal for that is
> >> to make NEW_S_UNLOCK(lock) = out of line function call + S_LOCK(dummy)
> >> + S_UNLOCK(dummy) + S_UNLOCK(lock).
> >
> > My proposal was to use barrier.h provided barriers as long as it
> > provides a native implementation. If neither a compiler nor a memory
> > barrier is provided my proposal was to fall back to the memory barrier
> > emulation we already have, but move it out of line (to avoid reordering
> > issues). The reason for doing so is that the release *has* to be a
> > release barrier.
>
> What do you mean by "the memory barrier emulation we already have"?
> The only memory barrier emulation we have, to my knowledge, is a
> spinlock acquire-release cycle.

Yes.

Unrelated, but why haven't we defined it as if (TAS(dummy))
S_UNLOCK(dummy);? That's just about as strong and less of a performance
drain?
Hm, I guess platforms that do an unlocked test first would be
problematic :/

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

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.

> >> On the other hand, the API contract change making
> >> these into compiler barriers is a master-only change. I think for
> >> purposes of this patch we should assume that the existing code is
> >> sufficient to prevent CPU reordering and just focus on fixing compiler
> >> ordering problems. Whatever needs to change on the CPU-reordering end
> >> of things is a separate commit.
> >
> > I'm not arguing against that it should be a separate commit. Just that
> > the proper memory barrier bit has to come first.
>
> I feel like you're speaking somewhat imprecisely in an area where very
> precise speech is needed to avoid confusion. If you're saying that
> you think we should fix the S_UNLOCK() implementations that fail to
> prevent CPU-ordering before we change all the S_UNLOCK()
> implementations to prevent compiler-reordering, then that is fine with
> me;

Yes, that's what I think is needed.

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

Please don't forget that I started this thread because I found the
current implementation lacking because s_lock neither has sane memory
release nor compiler release semantics... So it's not surprising that I
want to solve both :)

Greetings,

Andres Freund

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