Обсуждение: Re: [COMMITTERS] pgsql: Do all accesses to shared buffer headers through

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&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&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&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&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,
Pennsylvania19073
 

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] suggests that this keyword prevents
rearrangement of code around the inline ASM, but the GCC docs ([2])
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], it seems [1]'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
[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

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/
> Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a
> tool for doing 5% of the work and then sitting around waiting for someone
> else to do the other 95% so you can sue them.

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/
> Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a
> tool for doing 5% of the work and then sitting around waiting for someone
> else to do the other 95% so you can sue them.