Re: [HACKERS] pg_memory_barrier() doesn't compile, let alone work, for me
Robert Haas robertmh...@gmail.com writes: On Sun, Jul 14, 2013 at 8:41 PM, Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_memory_barrier() doesn't compile, let alone work, for me
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_memory_barrier() doesn't compile, let alone work, for me
On Wed, Jul 17, 2013 at 4:57 PM, Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_memory_barrier() doesn't compile, let alone work, for me
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 klep...@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 signature.asc Description: Digital signature
[HACKERS] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_memory_barrier() doesn't compile, let alone work, for me
On Sun, Jul 14, 2013 at 8:41 PM, Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
