Re: [PATCH v2] Use CC atomic builtins as a fallback

Hello Tom, all,

Tom Lane [2011-12-20 21:14 -0500]:
> Getting this thread back to the original patch ... I'm afraid that if we
> apply this as-is, what will happen is that we fix ARMv7 and break older
> versions.

Right, I guess it's dependent on the compiler version, too. That's why
my original patch tested for this first and used it if it was
available, then we can have any code in the #else part further down.

> So I'm thinking that removing the swpb ASM option is not such a good
> idea. We could possibly test for __sync_lock_test_and_set first, and
> only use swpb if we're on ARM and don't have the builtin.

OK, that would be kind of a hybrid solution between the first and
second version of the patch. Can work on this (but only next year,
need to do some christmas prep, and then holidays/AFK).

> Another thing that is bothering me is that according to the gcc
> manual, eg here,
> http://gcc.gnu.org/onlinedocs/gcc-4.1.2/gcc/Atomic-Builtins.html
> __sync_lock_test_and_set is nominally provided for datatypes 1, 2,
> 4, or 8 bytes in length, but the underlying hardware doesn't
> necessarily support all those widths natively. If you pick the
> wrong width then you don't get an inline operation at all, but a
> call to some possibly inefficient library subroutine.

It's even worse. Our original version of the patch used a char, and
while that worked fine on the older Babbage board, it completely broke
at runtime on e. g. a Panda board. It wasn't just slow, it caused
hangs and test failures all over the place. With int it works
flawlessly.

> I see that your patch just assumes that "int" will be a good width
> for the lock type, but it's unclear to me what that choice is based
> on and whether or not it might be a really bad choice on some
> platforms.

Unfortunately the gcc documentation doesn't give any further
conditions. As an "int" is usually meant to fit the standard word size
of a processor, if that function call/machine op code supports just
one datatype, it will most likely be "int", not something which you
have to mask on read/write (char) or potentially involves multiple
words (long/long long).

The intel definition [1] (and also in above gcc doc) says "32 or 64
bit integer", which also matches "int" (unless we are looking at some
really small architectures like the old Atmels with their 16 bit
words, but these are three magnitudes too small for PostgreSQL anyway
:) ) So with the intel compiler "char" is clearly forbidden.

Beyond that I don't have any further data.

So, I'm happy with keeping this patch in Debian/Ubuntu only for the
time being, as there we have a pretty clear idea of the supported ARM
platforms, and can actually test the patches on all the platforms. I
just found it prudent to at least bring it up here.

Thanks, and happy end-of-year holidays!

Martin

[1] http://www.cs.fsu.edu/~engelen/courses/HPC-adv/intref_cls.pdf p. 198

--
Ubuntu Developer (www.ubuntu.com) | Debian Developer (www.debian.org)