Re: Spinlock assembly cleanup

Sounds good to me. Consistencyis important because it lets us fix
problems across all cpu types. I am not 100% excited about the memory
part because it invalidates all register memory values, not just the
shared memory location. 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.

---------------------------------------------------------------------------

Tom Lane wrote:
> Pursuant to the gripes raised by Martin Pitt ---
>
> I've consulted some gcc experts within Red Hat and come to the following
> conclusions:
>
> * We should consistently refer to the spinlock contents via a
> read/write operand declared like "+m"(*lock). This is consistent
> with longstanding practice in the Linux kernel and therefore is unlikely
> to get broken in future gcc releases. The existing ports that use
> matched input and output parameters will break, or at least draw nasty
> warnings, in upcoming gcc releases.
>
> * Not all of the ports currently declare *lock as an input operand,
> but this seems rather dangerous to me; I think all should use "+m".
>
> * Some but not all the ports list "memory" as a clobbered operand.
> The gcc manual saith
>
> : If your assembler instruction modifies memory in an unpredictable
> : fashion, add `memory' to the list of clobbered registers. This will
> : cause GNU CC to not keep memory values cached in registers across the
> : assembler instruction.
>
> Now as far as I can see, none of the spinlock sequences directly clobber
> any memory other than the spinlock itself, and so (as long as the lock
> is stated to be an output operand) one might think the memory clobber
> marking to be excessive. However, I am thinking it is actually a good
> idea and we ought to add the marking to all ports, not remove it. The
> thought is that what we are actually using the spinlock for is to guard
> access to values in shared memory, and therefore the act of waiting for
> a spinlock can be seen as waiting for other memory variables to assume
> values they didn't necessarily have last time we looked. If gcc caches
> shared variables in registers across a spinlock acquisition, the code is
> broken.
>
> The alternative to doing this would be to always use volatile pointers
> to access shared memory, but I don't want to do that --- in the first
> place it's notationally cumbersome, and in the second place it would
> hurt performance unnecessarily. Within straight-line code that holds a
> spinlock there is no reason to treat shared memory as volatile. It's
> only when crossing a spinlock boundary that you must reload from memory,
> and that seems to be exactly what the "memory" modifier declares for us.
>
> (I am assuming here that marking the asm fragment "volatile" does not
> necessarily do what the "memory" modifier does; I can't see anything in
> the gcc docs that claims "volatile" includes the effects of "memory".)
>
> So I'd like to make all the gcc-asm fragments for spinlocks follow these
> rules. Comments?
>
> regards, tom lane
>
> ---------------------------(end of broadcast)---------------------------
> TIP 5: Have you checked our extensive FAQ?
>
> http://www.postgresql.org/docs/faqs/FAQ.html
>

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073