Re: removing old ports and architectures

On 2013-10-16 12:26:28 -0400, Robert Haas wrote:
> On Tue, Oct 15, 2013 at 8:33 AM, Andres Freund <andres(at)2ndquadrant(dot)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