[aarch64-port-dev ] RFR: aarch64: minor improvements of atomic operations

[Yangfei (Felix)]

> -----Original Message-----
> From: Andrew Haley [mailto:aph at redhat.com]
> Sent: Wednesday, November 13, 2019 5:39 PM
> To: Yangfei (Felix) <felix.yang at huawei.com>; Erik Österlund
> <erik.osterlund at oracle.com>; Andrew Dinn <adinn at redhat.com>;
> aarch64-port-dev at openjdk.java.net
> Cc: hotspot-dev at openjdk.java.net
> Subject: Re: [aarch64-port-dev ] RFR: aarch64: minor improvements of atomic
> operations
> 
> On 11/13/19 9:26 AM, Yangfei (Felix) wrote:
> > Yes, the cmpxchg case is different here.
> > So the v2 patch in my previous mail approved?
> > Will create a bug and do necessary testing.
> 
> I don't know which patch is v2, but for the reasons carefully laid out in the
> kernel-dev thread we don't need two full barriers. The first version of
> Atomic::PlatformCmpxchg you posted is OK.
> 

Well, I think the cmpxchg case is different: the compare in the loop may fail and then we don't got a change to execute the stlxr instruction.  
This is explicitedly discussed in that thread: https://patchwork.kernel.org/patch/3575821/
As a result, aarch64 Linux plants two barriers in that patch: 

@@ -112,17 +114,20 @@  static inline int atomic_cmpxchg(atomic_t *ptr, int old, int new)
 	unsigned long tmp;
 	int oldval;
 
+	smp_mb();              < ========
+
 	asm volatile("// atomic_cmpxchg\n"
-"1:	ldaxr	%w1, %2\n"
+"1:	ldxr	%w1, %2\n"
 "	cmp	%w1, %w3\n"
 "	b.ne	2f\n"
-"	stlxr	%w0, %w4, %2\n"
+"	stxr	%w0, %w4, %2\n"
 "	cbnz	%w0, 1b\n"
 "2:"
 	: "=&r" (tmp), "=&r" (oldval), "+Q" (ptr->counter)
 	: "Ir" (old), "r" (new)
 	: "cc", "memory");
 
+	smp_mb();             < ========
 	return oldval;
 }


That's why I switched to the V2 patch:

diff -r 846fee5ea75e src/hotspot/os_cpu/linux_aarch64/atomic_linux_aarch64.hpp
--- a/src/hotspot/os_cpu/linux_aarch64/atomic_linux_aarch64.hpp Wed Nov 13 10:27:06 2019 +0900
+++ b/src/hotspot/os_cpu/linux_aarch64/atomic_linux_aarch64.hpp Wed Nov 13 16:33:16 2019 +0800
@@ -52,7 +52,7 @@
                                                      T volatile* dest,
                                                      atomic_memory_order order) const {
   STATIC_ASSERT(byte_size == sizeof(T));
-  T res = __sync_lock_test_and_set(dest, exchange_value);
+  T res = __atomic_exchange_n(dest, exchange_value, __ATOMIC_RELEASE);
   FULL_MEM_BARRIER;
   return res;
 }
@@ -70,7 +70,12 @@
                               __ATOMIC_RELAXED, __ATOMIC_RELAXED);
     return value;
   } else {
-    return __sync_val_compare_and_swap(dest, compare_value, exchange_value);
+    T value = compare_value;
+    FULL_MEM_BARRIER;
+    __atomic_compare_exchange(dest, &value, &exchange_value, /*weak*/false,
+                              __ATOMIC_RELAXED, __ATOMIC_RELAXED);
+    FULL_MEM_BARRIER;
+    return value;
   }
 }