Re: Freezing without write I/O

On 2013-09-20 08:54:26 -0400, Robert Haas wrote:
> On Fri, Sep 20, 2013 at 8:40 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> > On 2013-09-20 08:32:29 -0400, Robert Haas wrote:
> >> Personally, I think the biggest change that would help here is to
> >> mandate that spinlock operations serve as compiler fences.  That would
> >> eliminate the need for scads of volatile references all over the
> >> place.
> >
> > The effectively already do, don't they? It's an external, no-inlineable
> > function call (s_lock, not the actual TAS). And even if it were to get
> > inlined via LTO optimization, the inline assembler we're using is doing
> > the __asm__ __volatile__ ("...", "memory") dance. That's a full compiler
> > barrier.
> > The non-asm implementations call to OS/compiler primitives that are also
> > fences.
> >
> > In the case I brougth up here there is no spinlock or something similar.
> 
> Well, that doesn't match my previous discussions with Tom Lane, or this comment:
> 
>  *      Another caution for users of these macros is that it is the caller's
>  *      responsibility to ensure that the compiler doesn't re-order accesses
>  *      to shared memory to precede the actual lock acquisition, or follow the
>  *      lock release.  Typically we handle this by using volatile-qualified
>  *      pointers to refer to both the spinlock itself and the shared data
>  *      structure being accessed within the spinlocked critical section.
>  *      That fixes it because compilers are not allowed to re-order accesses
>  *      to volatile objects relative to other such accesses.

To me that doesn't really make much sense to be honest.Note that the next paragraph tells us that
*    On platforms with weak memory ordering, the TAS(), TAS_SPIN(), and*    S_UNLOCK() macros must further include
hardware-levelmemory fence*    instructions to prevent similar re-ordering at the hardware level.*    TAS() and
TAS_SPIN()must guarantee that loads and stores issued after*    the macro are not executed until the lock has been
obtained. Conversely,*    S_UNLOCK() must guarantee that loads and stores issued before the macro*    have been
executedbefore the lock is released.
 

so, TAS has to work as a memory barrier if the architecture doesn't have
strong cache coherency guarantees itself. But memory barriers have to be
compiler barriers?

> That's not to disagree with your point.  If TAS is a compiler barrier,
> then we oughta be OK.  Unless something can migrate into the spot
> between a failed TAS and the call to s_lock?

We're talking compiler barriers right. In that case failure or success
do not play a role, or am I missing something?

> But I'm pretty sure that
> we've repeatedly had to change code to keep things from falling over
> in this area, see e.g. commits

> 584f818bef68450d23d1b75afbaf19febe38fd37 (the last apparently a live
> bug).
> d3fc362ec2ce1cf095950dddf24061915f836c22, and

I've quickly checked those out, and things looked mighty different back
then. And incidentally several of the inline assembly implementations
back then didn't specify that they clobber memory (which is what makes
it a compiler barrier).

> fa72121594534dda6cc010f0bf6b3e8d22987526,

Not sure here. Several of the inline assembly bits where changed to
clobber memory, but not all.

> 07eeb9d109c057854b20707562ce517efa591761,

Hm. Those mostly look to be overly cautios to me.

I think we should go through the various implementations and make sure
they are actual compiler barriers and then change the documented policy.


Greetings,

Andres Freund

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