Re: Spinlock assembly cleanup

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> I am not 100% excited about the memory
> part because it invalidates all register memory values, not just the
> shared memory location.

That's exactly the point.

> We are specifically accessing a memory address
> as part of the ASM, so I don't see how it could pull that memory value
> from a register behind our back.

We need to prevent gcc from caching values of *other* memory locations
behind our backs. Consider code on the order of

spinlockacquire(lock);
while (sharedvar == 0)
{
spinlockrelease(lock);
// we expect someone else to acquire lock and
// set sharedvar here...
spinlockacquire(lock);
}

If this is all inline code, and we didn't declare sharedvar as volatile,
then the compiler would be within its rights to assume that sharedvar
doesn't change, hence load it into a register once and not reload it
from shared memory after reacquiring the spinlock. This will of course
fail to do what we want it to.

We haven't seen failures of this kind because our direct use of
spinlocks is pretty constricted, and (for example) LWLockAcquire is
careful to use a volatile pointer for all accesses to the LWLock
fields. However this is inefficient: while it owns the spinlock,
LWLockAcquire doesn't really need to treat all the other fields
as volatile, so there are probably a few wasted loads in there.
And the requirement for using volatile pointers is something that is
likely to bite us if we start using spinlocks directly in more parts
of the code. Not to mention that if anyone cranks up the optimization
level to the point where LWLockAcquire can get inlined into other
functions, those functions will break immediately, because they are
not saying "volatile" for every shared memory access.

So I think it's best to fix it as part of the TAS asm definition.

As things stand at the moment, there's not going to be any efficiency
loss, because LWLockAcquire brute-forces the same result with a volatile
pointer, and its callers aren't going to expect to be able to cache
global variables across a function call anyway. In the long run when
you consider global inlining of functions, the possibility is there for
the efficiency to be better not worse if we do things this way.

regards, tom lane