Spinlock assembly cleanup

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: pgsql-hackers(at)postgreSQL(dot)org
Subject: Spinlock assembly cleanup
Date: 2004-06-14 20:18:24
Message-ID: 5988.1087244304@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Greg Stark 2004-06-14 20:48:50 Re: Accelerating aggregates
Previous Message Ramanujam H S Iyengar 2004-06-14 19:34:49 Re: Coding question