Re: lock support for aarch64

I used the following patch to add lock support aarch64. It is just a
copy of the arm support based on gcc builtins. Postgresql built with
this patch passes the various tests.

diff --git a/src/include/storage/s_lock.h b/src/include/storage/s_lock.h
index d4a783f..624a73b 100644
--- a/src/include/storage/s_lock.h
+++ b/src/include/storage/s_lock.h
@@ -335,6 +335,25 @@ tas(volatile slock_t *lock)
#endif /* __arm__ */


+/*
+ * Use gcc builtins for AArch64.
+ */
+#if defined(__aarch64__)
+#define HAS_TEST_AND_SET
+
+#define TAS(lock) tas(lock)
+
+typedef int slock_t;
+
+static __inline__ int
+tas(volatile slock_t *lock)
+{
+ return __sync_lock_test_and_set(lock, 1);
+}
+
+#define S_UNLOCK(lock) __sync_lock_release(lock)
+#endif /* __aarch64__ */
+
/* S/390 and S/390x Linux (32- and 64-bit zSeries) */
#if defined(__s390__) || defined(__s390x__)
#define HAS_TEST_AND_SET

On 13.05.2013 15:39, Mark Salter wrote:
> I used the following patch to add lock support aarch64. It is just a
> copy of the arm support based on gcc builtins. Postgresql built with
> this patch passes the various tests.

I think this needs an "#ifdef HAVE_GCC_INT_ATOMICS", like the ARM codepath.

If we are to support aarch64, it would be good to have an aarch64
machine on the buildfarm. Could you arrange that? See
http://buildfarm.postgresql.org/

- Heikki

On Mon, 2013-05-13 at 16:15 +0300, Heikki Linnakangas wrote:
> On 13.05.2013 15:39, Mark Salter wrote:
> > I used the following patch to add lock support aarch64. It is just a
> > copy of the arm support based on gcc builtins. Postgresql built with
> > this patch passes the various tests.
>
> I think this needs an "#ifdef HAVE_GCC_INT_ATOMICS", like the ARM codepath.

Yes. Would it be better to do something like:

+/*
+ * Use gcc builtins if available.
+ */
+#if !defined(HAS_TEST_AND_SET) && defined(HAVE_GCC_INT_ATOMICS)
+#define HAS_TEST_AND_SET
+
+#define TAS(lock) tas(lock)
+
+typedef int slock_t;
+
+static __inline__ int
+tas(volatile slock_t *lock)
+{
+ return __sync_lock_test_and_set(lock, 1);
+}
+
+#define S_UNLOCK(lock) __sync_lock_release(lock)
+#endif
+
/* Blow up if we didn't have any way to do spinlocks */
#ifndef HAS_TEST_AND_SET
#error PostgreSQL does not have native spinlock support on this platform. To continue the compilation, rerun configure using --disable-spinlocks. However, performance will be poor. Please report this to pgsql-bugs(at)postgresql(dot)org(dot)

>
> If we are to support aarch64, it would be good to have an aarch64
> machine on the buildfarm. Could you arrange that? See
> http://buildfarm.postgresql.org/
>

Right now, all we have is a simulator. It takes about 24hrs to build and
test the Fedora RPM. Hardware will start to be available soon. But yes,
I think we could arrange to add to the buildfarm.

--Mark

On Mon, May 13, 2013 at 8:15 AM, Heikki Linnakangas
<hlinnakangas(at)vmware(dot)com> wrote:
> On 13.05.2013 15:39, Mark Salter wrote:
>>
>> I used the following patch to add lock support aarch64. It is just a
>> copy of the arm support based on gcc builtins. Postgresql built with
>> this patch passes the various tests.
>
>
> I think this needs an "#ifdef HAVE_GCC_INT_ATOMICS", like the ARM codepath.

I'm starting to wonder why we don't always use gcc atomics if they are
available and assembly implementation is not (any maybe, even if it
is).

merlin

Mark Salter <msalter(at)redhat(dot)com> writes:
> I used the following patch to add lock support aarch64. It is just a
> copy of the arm support based on gcc builtins. Postgresql built with
> this patch passes the various tests.

Couldn't we just do

-#if defined(__arm__) || defined(__arm)
+#if defined(__arm__) || defined(__arm) || defined(__aarch64__)

in the existing ARM code block?

regards, tom lane

On 13.05.2013 17:26, Merlin Moncure wrote:
> I'm starting to wonder why we don't always use gcc atomics if they are
> available and assembly implementation is not (any maybe, even if it
> is).

That was discussed a while ago, but there were a lot of claims that
__sync_lock_test_and_set is broken on many various platforms:
http://www.postgresql.org/message-id/flat/5642(dot)1324482916(at)sss(dot)pgh(dot)pa(dot)us#5642(dot)1324482916@sss.pgh.pa.us.
The situation might've improved since, but I guess we should proceed
cautiously.

- Heikki

On 13.05.2013 18:14, Tom Lane wrote:
> Mark Salter<msalter(at)redhat(dot)com> writes:
>> I used the following patch to add lock support aarch64. It is just a
>> copy of the arm support based on gcc builtins. Postgresql built with
>> this patch passes the various tests.
>
> Couldn't we just do
>
> -#if defined(__arm__) || defined(__arm)
> +#if defined(__arm__) || defined(__arm) || defined(__aarch64__)
>
> in the existing ARM code block?

That would imply falling back to swpb instruction also on aarch64, which
won't work.

- Heikki

Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> writes:
> On 13.05.2013 18:14, Tom Lane wrote:
>> Couldn't we just do
>> -#if defined(__arm__) || defined(__arm)
>> +#if defined(__arm__) || defined(__arm) || defined(__aarch64__)

> That would imply falling back to swpb instruction also on aarch64, which
> won't work.

It doesn't work on current ARM32 chips either, but no one has complained
about the existing coding. At least on ARM64 there'd be a likelihood
that you'd get an assembler error rather than an executable that crashes
at launch.

I suppose we could consider adding

#ifdef __aarch64__
#error ...
#endif

in the section for not-HAVE_GCC_INT_ATOMICS, but I'm doubtful it's
worth the trouble.

regards, tom lane