Improve the implementation of atomic operations on Linux/ARM (including Android/ARM).

base/atomicops_internals_arm_gcc.h

Index: base/atomicops_internals_arm_gcc.h
diff --git a/base/atomicops_internals_arm_gcc.h b/base/atomicops_internals_arm_gcc.h
new file mode 100644
index 0000000000000000000000000000000000000000..bf915b065d9c764f20f6afcf7e17dbf7013d0106
--- /dev/null
+++ b/base/atomicops_internals_arm_gcc.h
@@ -0,0 +1,272 @@
+// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// This file is an internal atomic implementation, use base/atomicops.h instead.
+//
+// LinuxKernelCmpxchg and Barrier_AtomicIncrement are from Google Gears.
+
+#ifndef BASE_ATOMICOPS_INTERNALS_ARM_GCC_H_
+#define BASE_ATOMICOPS_INTERNALS_ARM_GCC_H_
+
+namespace base {
+namespace subtle {
+
+// Memory barriers on ARM are funky, but the kernel is here to help:

[Mark Mentovai, 2013/06/05 13:34:31]

Thanks for this comment!

+//
+// * ARMv5 didn't support SMP, there is no memory barrier instruction at
+//   all on this architecture, or when targetting its machine code.

[Mark Mentovai, 2013/06/05 13:34:31]

targeting

[digit1, 2013/06/05 16:35:47]

Done.

+//
+// * Some ARMv6 CPUs support SMP. A full memory barrier can be produced by
+//   writing a random value to a very specific coprocessor register.
+//
+// * On ARMv7, the "dmb" instruction is used to perform a full memory
+//   barrier (though writing to the co-processor will still work).
+//   However, on single core devices (e.g. Nexus One, or Nexus S),
+//   this instruction will take up to 200 ns, which is huge, even though
+//   it's completely un-needed on these devices.
+//
+// * There is no easy way to determine at runtime if the device is
+//   single or multi-core. However, the kernel provide a useful helper

[Mark Mentovai, 2013/06/05 13:34:31]

provides

[digit1, 2013/06/05 16:35:47]

Done.

+//   function at a fixed memory address (0xffff0fa0), which will always
+//   perform a memory barrier in the most efficient way. I.e. on single
+//   core devices, this is an empty function that exits immediately.
+//   On multi-core devices, it implements a full memory barrier.
+//
+//   Note that this helper function doesn't modify any register or memory.
+//   See the comment in Barrier_AtomicIncrement() to see why it is

[Mark Mentovai, 2013/06/05 13:34:31]

There are two versions of Barrier_AtomicIncrement() and neither explains what
this says will be explained.

[digit1, 2013/06/05 16:35:47]

This comment is obsolete (it was only useful for a previous patch that called
the function through inlined assembly). I've removed it.

+//   important.
+//
+// * This source could be compiled to ARMv5 machine code that runs on a
+//   multi-core ARMv6 or ARMv7 device. In this case, memory barriers
+//   are needed for correct execution. Always call the kernel helper, even
+//   when targetting ARMv5TE.
+//
+
+#define LINUX_ARM_KERNEL_MEMORY_BARRIER 0xffff0fa0

[Mark Mentovai, 2013/06/05 13:34:31]

You can avoid the macro entirely (and thus the need for the #undef at the
bottom). Just write the number inline on line 51, the same as you did on line
167. That should be fine, since you don’t use this value anywhere else in the
file.

[digit1, 2013/06/05 16:35:47]

Yes, again, this came from the previous patch were the macro was needed in the
inline assembly. Removed.

+
+inline void MemoryBarrier() {

[Mark Mentovai, 2013/06/05 13:34:31]

Make this static or put it in an unnamed namespace.

[digit1, 2013/06/05 16:35:47]

Unfortunately, the function is declared in base/atomicops.h and thus must be in
this namespace.

+  // Note: This is a function call, which is also an implicit compiler
+  // barrier.
+  ((void (*)(void))LINUX_ARM_KERNEL_MEMORY_BARRIER)();

[Mark Mentovai, 2013/06/05 13:34:31]

This is C++, the second void is unnecessary, it can just be ().

[digit1, 2013/06/05 16:35:47]

Done.

+}
+
+#if defined(__ARM_ARCH_7A__) || defined(__ARM_ARCH_6__)
+
+// On ARMv6 and higher, it is possible to directly use ldrex/strex
+// instructions to implement fast atomic operations directly.
+inline Atomic32 NoBarrier_CompareAndSwap(volatile Atomic32* ptr,
+                                         Atomic32 old_value,
+                                         Atomic32 new_value) {
+  Atomic32 prev_value;
+  int reloop;
+  do {
+    // The following is equivalent to:
+    //
+    //   prev_value = LDREX(ptr)
+    //   reloop = 0
+    //   if (prev_value != old_value)
+    //      reloop = STREX(ptr, new_value)
+    __asm__ __volatile__("    ldrex %0, [%3]\n"
+                         "    mov %1, #0\n"
+                         "    teq %0, %4\n"
+#ifdef __thumb2__
+                         "    it eq\n"
+#endif
+                         "    strexeq %1, %5, [%3]\n"
+                         : "=&r"(prev_value), "=&r"(reloop), "+m"(*ptr)
+                         : "r"(ptr), "r"(old_value), "r"(new_value)
+                         : "cc", "memory");
+  } while (reloop != 0);
+  return prev_value;
+}
+
+inline Atomic32 Acquire_CompareAndSwap(volatile Atomic32* ptr,
+                                       Atomic32 old_value,
+                                       Atomic32 new_value) {
+  Atomic32 result = NoBarrier_CompareAndSwap(ptr, old_value, new_value);
+  MemoryBarrier();
+  return result;
+}
+
+inline Atomic32 Release_CompareAndSwap(volatile Atomic32* ptr,
+                                       Atomic32 old_value,
+                                       Atomic32 new_value) {
+  MemoryBarrier();
+  return NoBarrier_CompareAndSwap(ptr, old_value, new_value);
+}
+
+inline Atomic32 NoBarrier_AtomicIncrement(volatile Atomic32* ptr,
+                                          Atomic32 increment) {
+  Atomic32 value;
+  int reloop;
+  do {
+    // Equivalent to:
+    //
+    //  value = LDREX(ptr)
+    //  value += increment
+    //  reloop = STREX(ptr, value)
+    //
+    __asm__ __volatile__("    ldrex %0, [%3]\n"
+                         "    add %0, %0, %4\n"
+                         "    strex %1, %0, [%3]\n"
+                         : "=&r"(value), "=&r"(reloop), "+m"(*ptr)
+                         : "r"(ptr), "r"(increment)
+                         : "cc", "memory");
+  } while (reloop);
+  return value;
+}
+
+inline Atomic32 Barrier_AtomicIncrement(volatile Atomic32* ptr,
+                                        Atomic32 increment) {
+  // TODO(digit): Investigate if it's possible to implement this with
+  // a single MemoryBarrier() operation between the LDREX and STREX.
+  // See http://crbug.com/246514
+  MemoryBarrier();
+  Atomic32 result = NoBarrier_AtomicIncrement(ptr, increment);
+  MemoryBarrier();
+  return result;
+}
+
+inline Atomic32 NoBarrier_AtomicExchange(volatile Atomic32* ptr,
+                                         Atomic32 new_value) {
+  Atomic32 old_value;
+  int reloop;
+  do {
+    // old_value = LDREX(ptr)
+    // fail = STREX(ptr, new_value)
+    __asm__ __volatile__("   ldrex %0, [%3]\n"
+                         "   strex %1, %4, [%3]\n"
+                         : "=&r"(old_value), "=&r"(reloop), "+m"(*ptr)
+                         : "r"(ptr), "r"(new_value)
+                         : "cc", "memory");
+  } while (reloop != 0);
+  return old_value;
+}
+
+#else

[Mark Mentovai, 2013/06/05 13:34:31]

Is there a C preprocessor macro you can use to indicate that you’re targeting
ARMv5? If so, use it here

#elif defined(__ARM_ARCH_5__)

Then have an

#else
#error whatever
#endif

at the bottom of the sequence. That way, when ARMv15 or whatever comes out and
someone tries to compile this code targeting that architecture, they won’t fall
into the ARMv5 implementation that they probably don’t want to use.

[digit1, 2013/06/05 16:35:47]

I've done that. Note that the checks must be more complex because the toolchain
actually doesn't define __ARM_ARCH_5__ for Android/ARMv5, but __ARM_ARCH_5TE__,
I've updated it.

+
+// The kernel also provides a helper function to perform an atomic
+// compare-and-swap operation at the hard-wired address 0xffff0fc0.
+// On ARMv5, this is implemented by a special code path that the kernel
+// detects and treats specially when thread pre-emption happens.
+// On ARMv6 and higher, it uses LDREX/STREX instructions instead.
+//
+// Note that this always perform a full memory barrier, there is no
+// need to add calls MemoryBarrier() before or after it. It also
+// returns 0 on success, and 1 on exit.
+//
+// Available and reliable since Linux 2.6.24. Note that the first Android
+// releases used 2.6.29, and ChromeOS is currently at 3.4, iirc, so this

[Mark Mentovai, 2013/06/05 13:34:31]

“iirc” has no place in a comment. You have the luxury of being able to look this
up or ask someone.

“currently” also becomes really stale in comments. Future readers (or future
digits) shouldn’t have to resort to version history to figure out when
“currently” was written. If you write “currently,” also write a date or a Chrome
milestone or something else to identify when the statement was true.

+// should only be a concern for people running _really_ old custom
+// Linux/ARM distributions).

[Mark Mentovai, 2013/06/05 13:34:31]

You’re ending a parenthetical that you never started.

+typedef int (*LinuxKernelCmpxchgFunc)(Atomic32 old_value,
+                                      Atomic32 new_value,
+                                      volatile Atomic32* ptr);
+LinuxKernelCmpxchgFunc pLinuxKernelCmpxchg __attribute__((weak)) =

[Mark Mentovai, 2013/06/05 13:34:31]

Isn’t this saying that base::subtle::pLinuxKernelCmpxchg is weak? You don’t care
about that. You’re not expecting or wanting user code to override you.

[Mark Mentovai, 2013/06/05 13:34:31]

The p prefix is uncommon in Chrome code. variables_are_normally_named_like_this.

[digit1, 2013/06/05 16:35:47]

Yes, this is part of the original code that was recently removed, so I didn't
touch it too much. I've changed all this to a more Chrome-y format.

+    (LinuxKernelCmpxchgFunc)0xffff0fc0;

[Mark Mentovai, 2013/06/05 13:34:31]

The typedef and this pointer should both be in an unnamed namespace.

[Mark Mentovai, 2013/06/05 13:34:31]

Make this const? You don’t want someone accidentally changing the value.

[digit1, 2013/06/05 16:35:47]

Same here.

+
+inline Atomic32 NoBarrier_CompareAndSwap(volatile Atomic32* ptr,
+                                         Atomic32 old_value,
+                                         Atomic32 new_value) {
+  Atomic32 prev_value;
+  for (;;) {
+    prev_value = *ptr;
+    if (prev_value != old_value)
+      return prev_value;
+    if (!pLinuxKernelCmpxchg(old_value, new_value, ptr))
+      return old_value;
+  }
+}
+
+inline Atomic32 NoBarrier_AtomicExchange(volatile Atomic32* ptr,
+                                         Atomic32 new_value) {
+  Atomic32 old_value;
+  do {
+    old_value = *ptr;
+  } while (pLinuxKernelCmpxchg(old_value, new_value, ptr));
+  return old_value;
+}
+
+inline Atomic32 NoBarrier_AtomicIncrement(volatile Atomic32* ptr,
+                                          Atomic32 increment) {
+  return Barrier_AtomicIncrement(ptr, increment);
+}
+
+inline Atomic32 Barrier_AtomicIncrement(volatile Atomic32* ptr,
+                                        Atomic32 increment) {
+  for (;;) {
+    // Atomic exchange the old value with an incremented one.
+    Atomic32 old_value = *ptr;
+    Atomic32 new_value = old_value + increment;
+    if (!pLinuxKernelCmpxchg(old_value, new_value, ptr)) {
+      // The exchange took place as expected.
+      return new_value;
+    }
+    // Otherwise, *ptr changed mid-loop and we need to retry.
+  }
+}
+
+inline Atomic32 Acquire_CompareAndSwap(volatile Atomic32* ptr,
+                                       Atomic32 old_value,
+                                       Atomic32 new_value) {
+  Atomic32 prev_value;
+  for (;;) {
+    prev_value = *ptr;
+    if (prev_value != old_value) {
+      // Always ensure acquire semantics.
+      MemoryBarrier();
+      return prev_value;
+    }
+    if (!pLinuxKernelCmpxchg(old_value, new_value, ptr))
+      return old_value;
+  }
+}
+
+inline Atomic32 Release_CompareAndSwap(volatile Atomic32* ptr,
+                                       Atomic32 old_value,
+                                       Atomic32 new_value) {
+  // Use NoBarrier_CompareAndSwap(), because its implementation
+  // ensures that all stores happen through the kernel helper
+  // which always implement a full barrier.
+  return NoBarrier_CompareAndSwap(ptr, old_value, new_value);
+}
+
+#endif  // __ARM_ARCH_6__ || __ARM_ARCH_7A__
+
+// NOTE: Atomicity of the following load and store operations is only
+// guaranteed in case of 32-bit alignement of |ptr| values.
+
+inline void NoBarrier_Store(volatile Atomic32* ptr, Atomic32 value) {
+  *ptr = value;
+}
+
+inline void Acquire_Store(volatile Atomic32* ptr, Atomic32 value) {
+  *ptr = value;
+  MemoryBarrier();
+}
+
+inline void Release_Store(volatile Atomic32* ptr, Atomic32 value) {
+  MemoryBarrier();
+  *ptr = value;
+}
+
+inline Atomic32 NoBarrier_Load(volatile const Atomic32* ptr) { return *ptr; }
+
+inline Atomic32 Acquire_Load(volatile const Atomic32* ptr) {
+  Atomic32 value = *ptr;
+  MemoryBarrier();
+  return value;
+}
+
+inline Atomic32 Release_Load(volatile const Atomic32* ptr) {
+  MemoryBarrier();
+  return *ptr;
+}
+
+#undef LINUX_ARM_KERNEL_MEMORY_BARRIER
+
+}  // namespace base::subtle
+}  // namespace base
+
+#endif  // BASE_ATOMICOPS_INTERNALS_ARM_GCC_H_