On 2014-06-30 12:46:29 -0400, Robert Haas wrote: > >> But trying to use a spinlock > >> acquire-release to shore up problems with the spinlock release > >> implementation makes my head explode. > > > > Well, it actually makes some sense. Nearly any TAS() implementation is > > going to have some memory barrier semantics - so using a TAS() as > > fallback makes sense. That's why we're relying on it for use in memory > > barrier emulation after all. > > As far as I know, all of our TAS() implementations prevent CPU > reordering in the acquire direction. It is not clear that they > provide CPU-reordering guarantees adequate for the release direction, > even when paired with whatever S_UNLOCK() implementation they're mated > with.
> And it's quite clear that many of them aren't adequate to prevent > compiler-reordering in any case. I actually don't think there currently are tas() implementations that aren't compiler barriers? Maybe UNIVEL/unixware. A bit of luck. > > Also note that barrier.h assumes that a S_LOCK/S_UNLOCK acts as a > > compiler barrier - which really isn't guaranteed by the current contract > > of s_lock.h. Although the tas() implementations look safe. > > Ugh, you're right. That should really be moved out-of-line. Good. Then we already loose the compile time interdependency from barrier.h to s_lock.h - although the fallback will have a runtime dependency. > >> 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? > > > > I'm fine with either - we're both going to flying pretty blind :/. > > > > I think the way S_UNLOCK's release memory barrier semantics are fixed > > might influence the way the compiler barrier issue can be solved. > > I think I'm starting to understand the terminological confusion: to > me, a memory barrier means a fence against both the compiler and the > CPU. I now think you're using it to mean a fence against the CPU, as > distinct from a fence against the compiler. That had me really > confused in some previous replies. Oh. Lets henceforth define 'fence' as the cache coherency thingy and read/write/release/acquire/memory barrier as the combination? > > Fixing > > the release semantics will involve going through all tas() > > implementations and see whether the default release semantics are > > ok. The ones with broken semantics will need to grow their own > > S_UNLOCK. I am wondering if that commit shouldn't just remove the > > default S_UNLOCK and move it to the tas() implementation sites. > > So, I think that here you are talking about CPU behavior rather than > compiler behavior. Next paragraph is on that basis. Yes, I am. > The implementations that don't currently have their own S_UNLOCK() are > i386 > x86_64 TSO, thus fine. > Itanium As a special case volatile stores emit release/acquire fences unless specific compiler flags are used. Thus safe. > ARM without GCC atomics Borked. > S/390 zSeries Strongly ordered. > Sparc > Sun Studio Borked. At least in some setups. > Linux m68k At least linux doesn't support SMP for m68k, so cache coherency shouldn't be a problem. > VAX I don't really know, but I don't care. The NetBSD people statements about VAX SMP support didn't increase my concern for VAX SMP. > m32r No idea. > SuperH Not offially supported (as it's not in installation.sgml), don't care :) > non-Linux m68k Couldn't figure out if anybody supports SMP here. Doesn't look like it. > Univel CC Don't care. > HP/UX non-GCC Itanics volatile semantics should work. > and WIN32_ONLY_COMPILER Should be fine due to TSO on x86 and itanic volatiles. > Because most of those > are older platforms, I'm betting that more of them than not are OK; I > think we should confine ourselves to trying to fix the ones we're sure > are wrong Sounds sane. >, which if I understand you correctly are ARM without GCC > atomics, Sparc, and MIPS. I've to revise my statement on MIPS, it actually looks safe. I seem to have missed that it has its own S_UNLOCK. Do I see it correctly that !(defined(__mips__) && !defined(__sgi)) isn't supported? > I think it'd be better to just add copies > of S_UNLOCK to those three rather and NOT duplicate the definition in > the other 12 places. Ok. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
