Re: Inconsistent use of "volatile" when accessing shared memory?

Hi,

On 2023-11-02 23:19:03 -0700, Jeff Davis wrote:
> I noticed that we occasionally use "volatile" to access shared memory,
> but usually not; and I'm not clear on the rules for doing so. For
> instance, AdvanceXLInsertBuffer() writes to XLogCtl->xlblocks[nextidx]
> through a volatile pointer; but then immediately writes to XLogCtl-
> >InitializedUpTo with a non-volatile pointer. There are also places in
> procarray.c that make use of volatile through UINT32_ACCESS_ONCE(), and
> of course there are atomics (which use volatile as well as guaranteeing
> atomicity).

> In theory, I think we're always supposed to access shared memory
> through a volatile pointer, right? Otherwise a sufficiently smart (and
> cruel) compiler could theoretically optimize away the load/store in
> some surprising cases, or hold a value in a register longer than we
> expect, and then any memory barriers would be useless.

I don't think so. We used to use volatile for most shared memory accesses, but
volatile doesn't provide particularly useful semantics - and generates
*vastly* slower code in a lot of circumstances. Most of that usage predates
spinlocks being proper compiler barriers, which was rectified in:

commit 0709b7ee72e
Author: Robert Haas <rhaas(at)postgresql(dot)org>
Date: 2014-09-09 17:45:20 -0400

Change the spinlock primitives to function as compiler barriers.

or the introduction of compiler/memory barriers in

commit 0c8eda62588
Author: Robert Haas <rhaas(at)postgresql(dot)org>
Date: 2011-09-23 17:52:43 -0400

Memory barrier support for PostgreSQL.

Most instances of volatile used for shared memory access should be replaced
with explicit compiler/memory barriers, as appropriate.

Note that use of volatile does *NOT* guarantee anything about memory ordering!

> But in practice we don't do that even for sensitive structures like the
> one referenced by XLogCtl. My intuition up until now was that if we
> access through a global pointer, then the compiler wouldn't completely
> optimize away the store/load.

That's not guaranteed at all - luckily, as it'd lead to code being more
bulky. It's not that the global variable will be optimized away entirely in
many situations, but repeated accesses can sometimes be merged and the access
can be moved around.

> What is the guidance here? Is the volatile pointer use in
> AdvanceXLInsertBuffer() required, and if so, why not other places?

I don't think it's required. The crucial part is to avoid memory reordering
between zeroing the block / initializing fields and changing that field - the
relevant part for that is the pg_write_barrier(); *not* the volatile.

The volatile does prevent the compiler from deferring the update of
xlblocks[idx] to the next loop iteration. Which I guess isn't a bad idea - but
it's not required for correctness.

When an individual value is read/written from memory, and all that's desired
is to prevent the compiler for eliding/moving that operation, it can lead to
better code to use volatile *on the individual access* compared to using
pg_compiler_barrier(), because it allows the compiler to keep other variables
in registers.

Greetings,

Andres Freund