On Mon, Jun 30, 2014 at 11:17 AM, Andres Freund <and...@2ndquadrant.com> wrote: >> > Solaris seems to run with TSO enabled for SPARC (whereas linux uses RMO, >> > relaxed memory ordering), so it's probably fine to just use the compiler >> > barrier. >> >> If it isn't, that's a change that has nothing to do with making >> spinlock operations compiler barriers and everything to do with fixing >> a bug in the existing implementation. > > Well, it actually has. If we start to remove volatiles from critical > sections the compiler will schedule stores closer to the spinlock > release and reads more freely - making externally visible ordering > violations more likely. Especially on itanic.
Granted. >> Now, we want to make these >> operations compiler fences as well, and AIUI your proposal for that is >> to make NEW_S_UNLOCK(lock) = out of line function call + S_LOCK(dummy) >> + S_UNLOCK(dummy) + S_UNLOCK(lock). > > My proposal was to use barrier.h provided barriers as long as it > provides a native implementation. If neither a compiler nor a memory > barrier is provided my proposal was to fall back to the memory barrier > emulation we already have, but move it out of line (to avoid reordering > issues). The reason for doing so is that the release *has* to be a > release barrier. What do you mean by "the memory barrier emulation we already have"? The only memory barrier emulation we have, to my knowledge, is a spinlock acquire-release cycle. But trying to use a spinlock acquire-release to shore up problems with the spinlock release implementation makes my head explode. >> On the other hand, the API contract change making >> these into compiler barriers is a master-only change. I think for >> purposes of this patch we should assume that the existing code is >> sufficient to prevent CPU reordering and just focus on fixing compiler >> ordering problems. Whatever needs to change on the CPU-reordering end >> of things is a separate commit. > > I'm not arguing against that it should be a separate commit. Just that > the proper memory barrier bit has to come first. I feel like you're speaking somewhat imprecisely in an area where very precise speech is needed to avoid confusion. If you're saying that you think we should fix the S_UNLOCK() implementations that fail to prevent CPU-ordering before we change all the S_UNLOCK() implementations to prevent compiler-reordering, then that is fine with me; otherwise, I don't understand what you're getting at here. Do you want to propose a patch that does *only* that first part before we go further here? Do you want me to try to put together such a patch based on the details mentioned on this thread? -- 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
