On 2013-10-16 12:26:28 -0400, Robert Haas wrote: > On Tue, Oct 15, 2013 at 8:33 AM, Andres Freund <and...@2ndquadrant.com> wrote: > > On 2013-10-13 16:56:12 +0200, Tom Lane wrote: > >> More to the point for this specific case, it seems like our process > >> ought to be > >> (1) select a preferably-small set of gcc atomic intrinsics that we > >> want to use. > > > > I suggest: > > * pg_atomic_load_u32(uint32 *) > > * uint32 pg_atomic_store_u32(uint32 *) > > We currently assume simple assignment suffices for this.
Partially that only works because we sprinkle 'volatile's on lots of places. That can actually hurt performance... it'd usually be something like: #define pg_atomic_load(uint32) (*(volatile uint32 *)(atomic)) Even if not needed in some places because a variable is already volatile, it's very helpful in pointing out what happens and where you need to be careful. > > * uint32 pg_atomic_exchange_u32(uint32 *ptr, uint32 val) > > * bool pg_atomic_compare_exchange_u32(uint32 *ptr, uint32 *expected, uint32 > > newval) > > * uint32 pg_atomic_fetch_add_u32(uint32 *ptr, uint32 add) > > * uint32 pg_atomic_fetch_sub_u32(uint32 *ptr, uint32 add) > > * uint32 pg_atomic_fetch_and_u32(uint32 *ptr, uint32 add) > > * uint32 pg_atomic_fetch_or_u32(uint32 *ptr, uint32 add) > > Do we really need all of those? Compare-and-swap is clearly needed, > and fetch-and-add is also very useful. I'm not sure about the rest. > Not that I necessarily object to having them, but it will be a lot > more work. Well, _sub can clearly be implemented with _add generically. I find code using _sub much easier to read than _add(-whatever), but that's maybe just me. But _and, _or are really useful because they can be used to atomically clear and set flag bits. > > > * u64 variants of the above > > * bool pg_atomic_test_set(void *ptr) > > * void pg_atomic_clear(void *ptr) > > What are the intended semantics here? It's basically TAS() without defining whether setting a value that needs to be set is a 1 or a 0. Gcc provides this and I think we should make our spinlock implementation use it if there is no special cased implementation available. > > Ontop of that we can generically implement: > > * pg_atomic_add_until_u32(uint32 *ptr, uint32 val, uint32 limit) > > * pg_atomic_(add|sub|and|or)_fetch_u32() > > * u64 variants of the above > > Are these really generally needed? _add_until() is very useful for implementing thinks like usagecount where you don't want to increase values too high. The lwlock scaling thing needs the add_fetch variant because we need to know what the lockcount is *after* we've registered. I think lots of lockless algorithm have similar requirements. Since those are either wrappers around fetch_op or compare_swap and thus can be implemented generically I don't really see a problem with providing them. > I have a related problem, which is that some code I'm currently > working on vis-a-vis parallelism can run lock-free on platforms with > atomic 8 bit assignment but needs a spinlock or two elsewhere. So I'd > want to use pg_atomic_store_u64(), but I'd also need a clean way to > test, at compile time, whether it's available. Yes, definitely. There should be a couple of #defines that declare whether non-prerequisite options are supported. I'd guess we want at least: * 8byte math * 16byte compare_and_swap > More general question: how do we move the ball down the field in this > area? I'm willing to do some of the legwork, but I doubt I can do all > of it, and I really think we need to make some progress here soon, as > it seems that you and I and Ants are all running into the same > problems in slightly different ways. What's our strategy for getting > something done here? That's a pretty good question. I am willing to write the gcc implementation, plus the generic wrappers and provide the infrastructure to override it per-platform. I won't be able to do anything about non-linux, non-gcc platforms in that timeframe though. I was thinking of something like: include/storage/atomic.h include/storage/atomic-gcc.h include/storage/atomic-msvc.h include/storage/atomic-acc-ia64.h where atomic.h first has a list of #ifdefs including the specialized files and then lots of #ifndef providing generic variants if not already provided by the platorm specific file. We could provide not only per-compiler files but also compiler independent files for some arches so we could e.g. define the restrictions for arm once. I think whether that's useful will be visible when writing the stuff. Makes sense? Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers