Thread: pg_memory_barrier() doesn't compile, let alone work, for me
Commit 9a20a9b2 breaks the build for me, using gcc on HPPA:
xlog.c:2182: macro `pg_memory_barrier' used without args
xlog.c:2546: macro `pg_memory_barrier' used without args
make[4]: *** [xlog.o] Error 1
The reason for this is that the "fallback" definition of
pg_memory_barrier has been wrong since day one; it needs this fix:
-#define pg_memory_barrier(x) \
+#define pg_memory_barrier() \
However, fixing that doesn't yield much joy; initdb stalls and then
crashes with
PANIC: stuck spinlock (40054a88) detected at xlog.c:2182
The reason for that is that the code does not bother to initialize
"dummy_spinlock" anywhere. It might accidentally fail to fail
on machines where the unlocked state of a spinlock is all-zeroes
(given a compiler that's not picky about the incorrect macro usage)
... but HPPA is not such a machine.
Rather than trying to think of a fix for that, I'm of the opinion that
we should rip this out. The fallback implementation of pg_memory_barrier
ought to be pg_compiler_barrier(), on the theory that non-mainstream
architectures don't have weak memory ordering anyway, or if they do you
need to do some work to get PG to work on them. Or maybe we ought to
stop pretending that the code is likely to work on arbitrary machines,
and just #error if there's not a supplied machine-specific macro.
regards, tom lane
On Sun, Jul 14, 2013 at 8:41 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Commit 9a20a9b2 breaks the build for me, using gcc on HPPA:
>
> xlog.c:2182: macro `pg_memory_barrier' used without args
> xlog.c:2546: macro `pg_memory_barrier' used without args
> make[4]: *** [xlog.o] Error 1
>
> The reason for this is that the "fallback" definition of
> pg_memory_barrier has been wrong since day one; it needs this fix:
>
> -#define pg_memory_barrier(x) \
> +#define pg_memory_barrier() \
Uggh, sorry.
> However, fixing that doesn't yield much joy; initdb stalls and then
> crashes with
>
> PANIC: stuck spinlock (40054a88) detected at xlog.c:2182
>
> The reason for that is that the code does not bother to initialize
> "dummy_spinlock" anywhere. It might accidentally fail to fail
> on machines where the unlocked state of a spinlock is all-zeroes
> (given a compiler that's not picky about the incorrect macro usage)
> ... but HPPA is not such a machine.
This would not be hard to fix, I think.
> Rather than trying to think of a fix for that, I'm of the opinion that
> we should rip this out. The fallback implementation of pg_memory_barrier
> ought to be pg_compiler_barrier(), on the theory that non-mainstream
> architectures don't have weak memory ordering anyway, or if they do you
> need to do some work to get PG to work on them. Or maybe we ought to
> stop pretending that the code is likely to work on arbitrary machines,
> and just #error if there's not a supplied machine-specific macro.
Well, pg_memory_barrier() isn't even equivalent to
pg_compiler_barrier() on x86, which has among the strongest memory
orderings out there, so I think your first idea is a non-starter. A
compiler barrier on x86 will fence reads from reads and writes from
writes, but it will not prevent a read from being speculated before a
subsequent write, which a full barrier must do.
I'm pretty sure we've got latent memory-ordering risks in our existing
code which we just haven't detected and fixed yet. Consider, for
example, this exciting code from GetNewTransactionId:
myproc->subxids.xids[nxids] = xid; mypgxact->nxids = nxids
+1;
I don't believe that's technically safe even on an architecture like
x86, because the compiler could decide to reorder those assignments.
Of course there is probably no reason to do so, and even if it does
you'd have to get really unlucky to see a user-visible failure, and if
you did you'd probably misguess the cause.
However, I'm guessing that as we start using memory barriers more, and
as we speed up the system generally, we're going to see bugs like this
that are currently hidden start to become visible from time to time.
Especially in view of that, I think it's wise to be pessimistic rather
than optimistic about what barriers are needed, when we don't know for
certain. That way, when we get a memory-ordering-related bug report,
we can at least hope that the problem must be something specific to
the problem area rather than a general failure to use the right memory
barrier primitives.
My preference would be to fix this in a narrow way, by initializing
dummy_spinlock somewhere. But if not, then I think #error is the only
safe way to go.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Sun, Jul 14, 2013 at 09:26:38PM -0400, Robert Haas wrote: > I'm pretty sure we've got latent memory-ordering risks in our existing > code which we just haven't detected and fixed yet. Consider, for > example, this exciting code from GetNewTransactionId: > > myproc->subxids.xids[nxids] = xid; > mypgxact->nxids = nxids + 1; > > I don't believe that's technically safe even on an architecture like > x86, because the compiler could decide to reorder those assignments. > Of course there is probably no reason to do so, and even if it does > you'd have to get really unlucky to see a user-visible failure, and if > you did you'd probably misguess the cause. You're probably right. Note that it's not even just the compiler that might reorder them, the CPU/cache subsystem/memory bus all play their part in memory reordering. x86 is pretty forgiving, which is why it works. I found this to be a really good explanation of all the things that can go wrong with memory ordering. It also explains why, in the long run, memory barriers are not optimal. http://herbsutter.com/2013/02/11/atomic-weapons-the-c-memory-model-and-modern-hardware/ That talk discusses how the hardware world is converging on SC [1] as the memory model to use. And C11/C++11 atomics will implement this for the programmer. With these you can actually make guarentees. For example, by marking mypgxact->nxids as an atomic type the compiler will emit all the necessary markings to let the CPU know what you want, so everything works the way you expect it to. Even on arcane architechtures. No explicit barriers needed. Unfortunatly, it won't help on compilers that don't support it. [1] http://en.wikipedia.org/wiki/Sequential_consistency There are places where you put code in and verify it does what you want. With this one you can put test programs in and it can tell you all possibly results due to memory reordering. http://svr-pes20-cppmem.cl.cam.ac.uk/cppmem/help.html Have a nice day, -- Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/ > He who writes carelessly confesses thereby at the very outset that he does > not attach much importance to his own thoughts. -- Arthur Schopenhauer
Robert Haas <robertmhaas@gmail.com> writes:
> On Sun, Jul 14, 2013 at 8:41 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> However, fixing that doesn't yield much joy; initdb stalls and then
>> crashes with
>>
>> PANIC: stuck spinlock (40054a88) detected at xlog.c:2182
>>
>> The reason for that is that the code does not bother to initialize
>> "dummy_spinlock" anywhere. It might accidentally fail to fail
>> on machines where the unlocked state of a spinlock is all-zeroes
>> (given a compiler that's not picky about the incorrect macro usage)
>> ... but HPPA is not such a machine.
> This would not be hard to fix, I think.
Really? Given that the memory barrier primitives are supposed to be,
well, primitive, I don't think this is exactly a trivial problem.
There's no good place to initialize such a variable, and there's even
less of a place to make sure that fork or exec leaves it in an
appropriate state in the child process.
>> Rather than trying to think of a fix for that, I'm of the opinion that
>> we should rip this out. The fallback implementation of pg_memory_barrier
>> ought to be pg_compiler_barrier(), on the theory that non-mainstream
>> architectures don't have weak memory ordering anyway, or if they do you
>> need to do some work to get PG to work on them. Or maybe we ought to
>> stop pretending that the code is likely to work on arbitrary machines,
>> and just #error if there's not a supplied machine-specific macro.
> Well, pg_memory_barrier() isn't even equivalent to
> pg_compiler_barrier() on x86, which has among the strongest memory
> orderings out there, so I think your first idea is a non-starter.
Among the strongest memory orderings compared to what? Since what we're
discussing here is non-mainstream architectures, I think this claim is
unfounded. Most of the ones I can think of offhand are old enough to
not even have multiprocessor support, so that the issue is vacuous.
> I'm pretty sure we've got latent memory-ordering risks in our existing
> code which we just haven't detected and fixed yet. Consider, for
> example, this exciting code from GetNewTransactionId:
> myproc->subxids.xids[nxids] = xid;
> mypgxact->nxids = nxids + 1;
> I don't believe that's technically safe even on an architecture like
> x86, because the compiler could decide to reorder those assignments.
Wrong, because both pointers are marked volatile. If the compiler does
reorder the stores, it's broken. Admittedly, this doesn't say anything
about hardware reordering :-(
> My preference would be to fix this in a narrow way, by initializing
> dummy_spinlock somewhere. But if not, then I think #error is the only
> safe way to go.
I'm inclined to agree that #error is the only realistic answer in
general, though we could probably go ahead with equating
pg_memory_barrier to pg_compiler_barrier on specific architectures we
know are single-processor-only. Unfortunately, that means we just
raised the bar for porting efforts significantly. And in particular,
it means somebody had better go through s_lock.h and make sure we have a
credible barrier implementation for every single arch+compiler supported
therein.
regards, tom lane
I wrote:
> I'm inclined to agree that #error is the only realistic answer in
> general, though we could probably go ahead with equating
> pg_memory_barrier to pg_compiler_barrier on specific architectures we
> know are single-processor-only. Unfortunately, that means we just
> raised the bar for porting efforts significantly. And in particular,
> it means somebody had better go through s_lock.h and make sure we have a
> credible barrier implementation for every single arch+compiler supported
> therein.
After going through s_lock.h another time, I can't help noticing that
a large majority of the non-mainstream architectures make use of the
default version of S_UNLOCK(), which is just
#define S_UNLOCK(lock) (*((volatile slock_t *) (lock)) = 0)
I assert that if this is a correct implementation, then the platform
does not reorder writes, since correctness requires that any writes to
shared memory variables protected by the lock occur before the lock is
released.
Generally speaking, I'm not seeing any memory-barrier-ish instructions
on the locking side either, meaning there's also no risk of read
reordering. It's possible that some of these arches do read reordering
except for not hoisting reads before instructions that can be used to
take locks ... but I'll bet that most of them simply don't have weak
memory ordering.
So I'm back to the position that pg_compiler_barrier() is a perfectly
credible default implementation. More so than an incorrect usage of
spinlocks, anyway. In particular, I'm going to go fix HPPA that way
so I can get my build working again.
BTW, the only arches for which we seem to have any barrier instructions
in S_UNLOCK are ARM, PPC, Alpha, and MIPS. Alpha, at least, is probably
dead, and I'm not seeing any MIPS machines in the buildfarm either;
I wouldn't feel bad about desupporting both of those arches.
Also, a comparison to s_lock.h says that the PPC code in barrier.h is a
few bricks shy of a load: it's not honoring USE_PPC_LWSYNC. And while
I'm bitching, the #ifdef structure in barrier.h is impossible to follow,
not least because none of the #endifs are labeled, contrary to project
style.
regards, tom lane
On Wed, Jul 17, 2013 at 4:57 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> This would not be hard to fix, I think. > > Really? Given that the memory barrier primitives are supposed to be, > well, primitive, I don't think this is exactly a trivial problem. > There's no good place to initialize such a variable, and there's even > less of a place to make sure that fork or exec leaves it in an > appropriate state in the child process. Well, I admit that I don't really know how spinlocks work on every obscure platform out there, but I would have thought we could initialize this in, say, main() and call it good. For that to be not OK, we'd have to be running on a non-EXEC_BACKEND platform where a previously initialized spinlock is no longer in a good state after fork(). Unless you know of a case where that happens, I'd be inclined to assume it's a non-problem. If we find a counterexample later, then I'd insert an architecture-specific hack for that platform only, with a comment along the lines of /* YBGTBFKM */. >> Well, pg_memory_barrier() isn't even equivalent to >> pg_compiler_barrier() on x86, which has among the strongest memory >> orderings out there, so I think your first idea is a non-starter. > > Among the strongest memory orderings compared to what? Since what we're > discussing here is non-mainstream architectures, I think this claim is > unfounded. Most of the ones I can think of offhand are old enough to > not even have multiprocessor support, so that the issue is vacuous. Compared to other multi-processor architectures. I agree that the barriers are all reducible to compiler barriers on single-processor architectures, but I think new ports of PostgreSQL are much more likely to be to multi-processor systems rather than uniprocessor systems. There are very, very few multi-processor systems where pg_memory_barrier() is reducible to pg_compiler_barrier(). >> I'm pretty sure we've got latent memory-ordering risks in our existing >> code which we just haven't detected and fixed yet. Consider, for >> example, this exciting code from GetNewTransactionId: > >> myproc->subxids.xids[nxids] = xid; >> mypgxact->nxids = nxids + 1; > >> I don't believe that's technically safe even on an architecture like >> x86, because the compiler could decide to reorder those assignments. > > Wrong, because both pointers are marked volatile. If the compiler does > reorder the stores, it's broken. Admittedly, this doesn't say anything > about hardware reordering :-( OK, natch. So it's safe on x86, but not on POWER. >> My preference would be to fix this in a narrow way, by initializing >> dummy_spinlock somewhere. But if not, then I think #error is the only >> safe way to go. > > I'm inclined to agree that #error is the only realistic answer in > general, though we could probably go ahead with equating > pg_memory_barrier to pg_compiler_barrier on specific architectures we > know are single-processor-only. I'd be fine with that. > Unfortunately, that means we just > raised the bar for porting efforts significantly. And in particular, > it means somebody had better go through s_lock.h and make sure we have a > credible barrier implementation for every single arch+compiler supported > therein. I tried, but the evidence shows that I have not entirely succeeded. :-( -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company