On Tue, Oct 1, 2013 at 2:08 PM, Andres Freund <and...@2ndquadrant.com> wrote: >> As for the specific patch being discussed here. The read barrier is in >> the wrong place and with the wrong comment, and the write side is >> assuming that SpinLockAcquire() is a write barrier, which it isn't >> documented to do at the moment. > > Lots of things we do pretty much already assume it is one - and I have a > hard time imagining any spinlock implementation that isn't a write > barrier. In fact it's the only reason we use spinlocks in quite some > places. > What we are missing is that it's not guaranteed to be a compiler barrier > on all platforms :(. But that's imo a bug.
Agree on both counts. >> The correct way to think of this is >> that StartupXLOG() does a bunch of state modifications and then >> advertises the fact that it's done by setting >> xlogctl->SharedRecoveryInProgress = false; The state modifications >> should better be visible to anyone seeing that last write, so you need >> one write barrier between the state modifications and setting the >> flag. > > SpinLockAcquire() should provide that. Yes. It's notable that in this case it's a matter of correctness that the global state modifications do *not* share the critical section with the flag update. Otherwise the flag update may become visible before the state updates. >> So we need a read barrier somewhere *after* reading the flag in >> RecoveryInProgress() and reading the shared memory structures, and in >> theory a full barrier if we are going to be writing data. In practice >> x86 is covered thanks to it's memory model, Power is covered thanks to >> the control dependency and ARM would need a read barrier, but I don't >> think any real ARM CPU does speculative stores as that would be >> insane. > > Does there necessarily have to be a "visible" control dependency? Unfortunately no, the compiler is quite free to hoist loads and even stores out from the conditional block losing the control dependency. :( It's quite unlikely to do so as it would be a very large code motion and it probably has no reason to do it, but I don't see anything that would disallow it. I wonder if we should just emit a full fence there for platforms with weak memory models. Trying to enforce the control dependency seems quite fragile. Regards, Ants Aasma -- Cybertec Schönig & Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt Web: http://www.postgresql-support.de -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
