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
compilerdoesn't re-order accesses* to shared memory to precede the actual lock acquisition, or follow the*
lockrelease. Typically we handle this by using volatile-qualified* pointers to refer to both the spinlock itself
andthe shared data* structure being accessed within the spinlocked critical section.* That fixes it because
compilersare not allowed to re-order accesses* to volatile objects relative to other such accesses.
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? 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
fa72121594534dda6cc010f0bf6b3e8d22987526,
07eeb9d109c057854b20707562ce517efa591761,
d3fc362ec2ce1cf095950dddf24061915f836c22, and
584f818bef68450d23d1b75afbaf19febe38fd37 (the last apparently a live
bug).
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company