pgsql: Do all accesses to shared buffer headers through

On Wed, 2005-12-10 at 13:45 -0300, Tom Lane wrote:

Do all accesses to shared buffer headers through volatile-qualified
pointers, to ensure that compilers won't rearrange accesses to occur
while we're not holding the buffer header spinlock.

That is fairly error prone :-( Would it be possible to hide this in a
typedef?

-Neil

Neil Conway <neilc@samurai.com> writes:

On Wed, 2005-12-10 at 13:45 -0300, Tom Lane wrote:

Do all accesses to shared buffer headers through volatile-qualified
pointers, to ensure that compilers won't rearrange accesses to occur
while we're not holding the buffer header spinlock.

That is fairly error prone :-( Would it be possible to hide this in a
typedef?

How would a typedef make it safer? I see no particular difference
between omitting the "volatile" and choosing the wrong typedef.
Neither will happen if you follow the coding style of surrounding
routines (which is one reason I chose to make *all* the bufmgr routines
declare BufferDesc pointers as volatile, even though some of them
arguably didn't need to). If you don't follow the coding style,
neither convention will catch you at it.

We do however have here a New Coding Rule that's good for all parts
of the backend: if you are accessing a spinlock-protected data structure
then you should be using a volatile-qualified pointer for it. I had
at one time thought that volatile-qualifying the spinlock pointer should
be enough, on the grounds that compilers shouldn't move other stores
across a store to volatile. But apparently gcc only promises not to
rearrange volatile stores with respect to each other.

regards, tom lane

On Wed, 2005-12-10 at 18:54 -0400, Tom Lane wrote:

How would a typedef make it safer? I see no particular difference
between omitting the "volatile" and choosing the wrong typedef.

IMHO it is notationally clearer to define a "BufferDescPtr" that
contains the "volatile" qualifier than to make sure that "volatile" is
used everywhere that it is needed -- obviously, neither approach is
fool-proof. But perhaps that's just me...

We do however have here a New Coding Rule that's good for all parts
of the backend: if you are accessing a spinlock-protected data structure
then you should be using a volatile-qualified pointer for it.

I think this is worth documenting more clearly (I realize you added a
note in buf_internals.h, but perhaps a note in the spinlock headers
would be appropriate as well? The comment circa line 49 of s_lock.h
seems to need updating, for example.)

-Neil

Neil Conway <neilc@samurai.com> writes:

IMHO it is notationally clearer to define a "BufferDescPtr" that
contains the "volatile" qualifier than to make sure that "volatile" is
used everywhere that it is needed -- obviously, neither approach is
fool-proof. But perhaps that's just me...

Personally I dislike hiding type qualifiers like const and volatile
inside typedefs. I agree it's a judgment call.

regards, tom lane

Does any of this need to be backpatched?

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

Tom Lane wrote:

Log Message:
-----------
Do all accesses to shared buffer headers through volatile-qualified
pointers, to ensure that compilers won't rearrange accesses to occur
while we're not holding the buffer header spinlock. It's probably
not necessary to mark volatile in every single place in bufmgr.c,
but better safe than sorry. Per trouble report from Kevin Grittner.

Modified Files:
--------------
pgsql/contrib/pg_buffercache:
pg_buffercache_pages.c (r1.4 -> r1.5)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/contrib/pg_buffercache/pg_buffercache_pages.c.diff?r1=1.4&amp;r2=1.5)
pgsql/src/backend/storage/buffer:
bufmgr.c (r1.195 -> r1.196)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/storage/buffer/bufmgr.c.diff?r1=1.195&amp;r2=1.196)
freelist.c (r1.52 -> r1.53)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/storage/buffer/freelist.c.diff?r1=1.52&amp;r2=1.53)
pgsql/src/include/storage:
buf_internals.h (r1.79 -> r1.80)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/include/storage/buf_internals.h.diff?r1=1.79&amp;r2=1.80)

---------------------------(end of broadcast)---------------------------
TIP 9: In versions below 8.0, the planner will ignore your desire to
choose an index scan if your joining column's datatypes do not
match

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

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Does any of this need to be backpatched?

No --- we didn't have any per-buffer spinlocks before 8.1.

It's possible that at some point we'll need to start thinking about
applying volatile-pointer coding rules to data structures protected by
LWLocks. This could only become an issue if the compiler (a) inlines
LWLockAcquire/Release, and (b) tries to rearrange loads and stores
around the LWLock code. I would like to think that the latter is
impossible even with inlining, principally because the compiler can't
ignore the kernel calls that may occur within the LWLock routines;
those should be treated as external function calls and hence sequence
points, no matter how aggressive the compiler gets. But we'll see.

regards, tom lane

On Wed, 2005-12-10 at 22:46 -0400, Tom Lane wrote:

No --- we didn't have any per-buffer spinlocks before 8.1.

Right. To summarize the problem as I understand it: we need to force $CC
not to move loads and stores around spinlock acquire/release. This can't
be done by just marking the spinlock variables themselves "volatile", as
the volatile qualifier only affects the rearrangement of loads and
stores WRT other volatile variables.

Before 8.1, all the places that use spinlocks directly also write to
shared memory through a volatile pointer (code that just uses LWLocks
*should* be okay, as Tom says). That ensures loads and stores aren't
rearranged outside spinlock acquire/release -- the problem with the 8.1
bufmgr changes is that this was not done. Tom's fix was to add the
necessary volatile qualifiers.

Another way to fix the problem would be to have S_LOCK() and S_UNLOCK()
force $CC to not rearrange loads and stores by themselves, without
relying upon volatile pointers. If this is possible, I think it would be
better: forgetting a volatile qualifier *once* means we are vulnerable
to corruption of shared memory, depending on the vagaries of the
compiler. Does anyone know how this is done?

From talking with Andrew @ Supernews on IRC, he suggested the asm

"volatile" keyword as a possible solution ("volatile" is used for some
platform's S_LOCK/S_UNLOCK, but not the default S_UNLOCK(), which is
used by x86 and x86_64). [1]http://www.ibiblio.org/gferg/ldp/GCC-Inline-Assembly-HOWTO.html#ss5.4 suggests that this keyword prevents
rearrangement of code around the inline ASM, but the GCC docs ([2]http://gcc.gnu.org/onlinedocs/gcc-4.0.2/gcc/Extended-Asm.html)
don't actually state this: "The volatile keyword indicates that the
instruction has important side-effects ... Note that even a volatile asm
instruction can be moved relative to other code, including across jump
instructions." Per [3]http://gcc.gnu.org/bugzilla/show_bug.cgi?id=17884#c7, it seems [1]http://www.ibiblio.org/gferg/ldp/GCC-Inline-Assembly-HOWTO.html#ss5.4's understanding of the semantics of
"asm volatile" are incorrect -- so perhaps "asm volatile" isn't the
right tool, after all.

-Neil

[1]: http://www.ibiblio.org/gferg/ldp/GCC-Inline-Assembly-HOWTO.html#ss5.4
http://www.ibiblio.org/gferg/ldp/GCC-Inline-Assembly-HOWTO.html#ss5.4
[2]: http://gcc.gnu.org/onlinedocs/gcc-4.0.2/gcc/Extended-Asm.html
[3]: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=17884#c7

Neil Conway <neilc@samurai.com> writes:

Another way to fix the problem would be to have S_LOCK() and S_UNLOCK()
force $CC to not rearrange loads and stores by themselves, without
relying upon volatile pointers.

That would certainly be better if possible, but AFAIK it's not.
(Perhaps there is a gcc-specific hack, but certainly not one that's
portable to all compilers. "volatile" is the only tool the C standard
gives us.)

Note that the S_LOCK and S_UNLOCK macros *do* incorporate commands to
prevent hardware-level reorderings, on platforms where this is
necessary. The problem at hand is basically that those optimization-
fence instructions are not visible to the compiler...

From talking with Andrew @ Supernews on IRC, he suggested the asm
"volatile" keyword as a possible solution ("volatile" is used for some
platform's S_LOCK/S_UNLOCK, but not the default S_UNLOCK(), which is
used by x86 and x86_64). [1] suggests that this keyword prevents
rearrangement of code around the inline ASM, but the GCC docs ([2])
don't actually state this:

Interesting point. The bug case at hand involved rearrangement of code
around an S_UNLOCK, which on x86 doesn't use any asm block, only a store
through a volatile pointer. Maybe if it had used such a block we'd not
have seen the bug. Still, I think we have to do the volatile pointers
in order to guarantee correct results on non-gcc compilers, so it's not
clear that there's any point in pursuing the question of whether gcc by
itself could offer a nicer solution.

regards, tom lane

Neil Conway <neilc@samurai.com> writes:

On Wed, 2005-12-10 at 18:54 -0400, Tom Lane wrote:

We do however have here a New Coding Rule that's good for all parts
of the backend: if you are accessing a spinlock-protected data structure
then you should be using a volatile-qualified pointer for it.

I think this is worth documenting more clearly

BTW, forgot to reply to this part, but: have at it.

regards, tom lane

On Thu, 2005-13-10 at 00:11 -0400, Tom Lane wrote:

BTW, forgot to reply to this part, but: have at it.

I've committed the attached patch; feel free to improve it if you're not
happy.

-Neil

On Wed, Oct 12, 2005 at 11:49:47PM -0400, Tom Lane wrote:

That would certainly be better if possible, but AFAIK it's not.
(Perhaps there is a gcc-specific hack, but certainly not one that's
portable to all compilers. "volatile" is the only tool the C standard
gives us.)

Indeed. The linux kernel defines the following:

/* Optimization barrier */
/* The "volatile" is due to gcc bugs */
#define barrier() __asm__ __volatile__("": : :"memory")

The memory keyword (as the gcc docs state):

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.

They use this bit in all the spinlock and other locking code
specifically for this purpose. You can do things like:

do { barrier(); } while( condition );

where condition uses any memory variable and it will reread it
everytime, just as if the variable was volatile.

have seen the bug. Still, I think we have to do the volatile pointers
in order to guarantee correct results on non-gcc compilers, so it's not
clear that there's any point in pursuing the question of whether gcc by
itself could offer a nicer solution.

Yes, we need to look for solutions for other compilers. We just need to
be careful and have people check the spinlock code carefully when they
use other compilers. Maybe in the porting guide?

--
Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/

On Wed, Oct 12, 2005 at 10:46:14PM -0400, Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Does any of this need to be backpatched?

No --- we didn't have any per-buffer spinlocks before 8.1.

It's possible that at some point we'll need to start thinking about
applying volatile-pointer coding rules to data structures protected by
LWLocks. This could only become an issue if the compiler (a) inlines
LWLockAcquire/Release, and (b) tries to rearrange loads and stores
around the LWLock code. I would like to think that the latter is
impossible even with inlining, principally because the compiler can't
ignore the kernel calls that may occur within the LWLock routines;
those should be treated as external function calls and hence sequence
points, no matter how aggressive the compiler gets. But we'll see.

Sorry if I'm just confused here, but don't LWLocks protect data
structures susceptible to corruption? And if that's the case don't we
need to be sure that the compiler can't optimize around them? Or will
this only result in things like buffers not getting un-pinned?
--
Jim C. Nasby, Sr. Engineering Consultant jnasby@pervasive.com
Pervasive Software http://pervasive.com work: 512-231-6117
vcard: http://jim.nasby.net/pervasive.vcf cell: 512-569-9461

On Mon, 2005-17-10 at 16:48 -0500, Jim C. Nasby wrote:

Sorry if I'm just confused here, but don't LWLocks protect data
structures susceptible to corruption? And if that's the case don't we
need to be sure that the compiler can't optimize around them?

LWLocks certainly do protect shared data, and if the compiler rearranged
loads and stores around LWLocks acquire/release, it would result in
corruption. Tom was arguing it is unlikely the compiler will actually do
this (because LWLockAcquire is an out-of-line function call that might
invoke a system call, unlike SpinLockAcquire).

-Neil

Neil Conway <neilc@samurai.com> writes:

LWLocks certainly do protect shared data, and if the compiler rearranged
loads and stores around LWLocks acquire/release, it would result in
corruption. Tom was arguing it is unlikely the compiler will actually do
this (because LWLockAcquire is an out-of-line function call that might
invoke a system call, unlike SpinLockAcquire).

Actually it seems the sore spot is more likely to be SpinLockRelease,
which on many architectures expands to nothing more than an inlined

*(lockptr) = 0;

The lock pointer is declared as pointer to volatile, which should
discourage the compiler from removing the store altogether, but as
we've seen the compiler may be willing to rearrange the store with
respect to adjacent loads/stores that aren't marked volatile.

SpinLockAcquire contains an out-of-line function call, so although
the compiler could theoretically misoptimize things in the fast path,
it's probably much less risky than SpinLockRelease.

BTW, we may be perfectly safe on architectures like PPC, where
S_UNLOCK includes an __asm__ __volatile__ section for a hardware-level
optimization fence instruction. I wonder though if it'd be a good idea
to be marking those fence instructions with the "clobbers memory"
qualifier to ensure this?

regards, tom lane

On Fri, Oct 21, 2005 at 06:09:00PM -0400, Tom Lane wrote:

BTW, we may be perfectly safe on architectures like PPC, where
S_UNLOCK includes an __asm__ __volatile__ section for a hardware-level
optimization fence instruction. I wonder though if it'd be a good idea
to be marking those fence instructions with the "clobbers memory"
qualifier to ensure this?

Judging by the comments in the linux kernel w.r.t their barrier()
instruction, there are certain versions of gcc that (incorrectly) do
strange things with the "volatile" tag of asm statements. Cloberring
memory is the way to guarentee what you want...

Have a nice day,
--
Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/