Remove AMD Opteron Rev E workaround from atomicops

Impala doesn't run on Opteron Rev E because those CPUs don't support
SSSE3.  So let's not pay the price of this cmp/branch on the atomics
path (which is used by our SpinLock, and I plan to make our Atomic class
use these functions in a future change).

Note that gperfutil also removed this RevE workaround since these CPUs
are getting old and apparently many kernels don't have the workaround
anyway.

Let's call the init function for atomicops, even though it's basically a
no-op for 64-bit mode (since these features are always available). But
this future proofs the code a bit better.

Change-Id: I3639dcf86c14778967c0079b8dbc222a4516cf05
Reviewed-on: http://gerrit.cloudera.org:8080/2516
Reviewed-by: Dan Hecht <dhecht@cloudera.com>
Tested-by: Internal Jenkins

be/src/common/init.cc

@@ -20,6 +20,7 @@
 #include "common/logging.h"
 #include "common/status.h"
 #include "exprs/expr.h"
+#include "gutil/atomicops.h"
 #include "util/cpu-info.h"
 #include "util/debug-util.h"
 #include "util/disk-info.h"
@@ -147,6 +148,7 @@ void impala::InitCommonRuntime(int argc, char** argv, bool init_jvm,
     if (!error_message.empty()) EXIT_WITH_ERROR(error_message);
   }
   impala::InitGoogleLoggingSafe(argv[0]);
+  AtomicOps_x86CPUFeaturesInit();
   impala::InitThreading();
   impala::TimestampParser::Init();
   EXIT_IF_ERROR(impala::InitAuth(argv[0]));

be/src/gutil/atomicops-internals-x86.cc

@@ -55,9 +55,8 @@
 // Set the flags so that code will run correctly and conservatively
 // until InitGoogle() is called.
 struct AtomicOps_x86CPUFeatureStruct AtomicOps_Internalx86CPUFeatures = {
-  false,          // bug can't exist before process spawns multiple threads
   false,          // no SSE2
-  false,          // no cmpxchg16b
+  false           // no cmpxchg16b
 };
 
 // Initialize the AtomicOps_Internalx86CPUFeatures struct.
@@ -85,19 +84,6 @@ static void AtomicOps_Internalx86CPUFeaturesInit() {
     model += ((eax >> 16) & 0xf) << 4;
   }
 
-  // Opteron Rev E has a bug in which on very rare occasions a locked
-  // instruction doesn't act as a read-acquire barrier if followed by a
-  // non-locked read-modify-write instruction.  Rev F has this bug in
-  // pre-release versions, but not in versions released to customers,
-  // so we test only for Rev E, which is family 15, model 32..63 inclusive.
-  if (strcmp(vendor, "AuthenticAMD") == 0 &&       // AMD
-      family == 15 &&
-      32 <= model && model <= 63) {
-    AtomicOps_Internalx86CPUFeatures.has_amd_lock_mb_bug = true;
-  } else {
-    AtomicOps_Internalx86CPUFeatures.has_amd_lock_mb_bug = false;
-  }
-
   // edx bit 26 is SSE2 which we use to tell use whether we can use mfence
   AtomicOps_Internalx86CPUFeatures.has_sse2 = ((edx >> 26) & 1);
 
@@ -107,8 +93,6 @@ static void AtomicOps_Internalx86CPUFeaturesInit() {
   VLOG(1) << "vendor " << vendor <<
              "  family " << family <<
              "  model " << model <<
-             "  amd_lock_mb_bug " <<
-                   AtomicOps_Internalx86CPUFeatures.has_amd_lock_mb_bug <<
              "  sse2 " << AtomicOps_Internalx86CPUFeatures.has_sse2 <<
              "  cmpxchg16b " << AtomicOps_Internalx86CPUFeatures.has_cmpxchg16b;
 }

be/src/gutil/atomicops-internals-x86.h

@@ -38,8 +38,6 @@
 // Features of this x86.  Values may not be correct before InitGoogle() is run,
 // but are set conservatively.
 struct AtomicOps_x86CPUFeatureStruct {
-  bool has_amd_lock_mb_bug;  // Processor has AMD memory-barrier bug; do lfence
-                             // after acquire compare-and-swap.
   bool has_sse2;             // Processor has SSE2.
   bool has_cmpxchg16b;       // Processor supports cmpxchg16b instruction.
 };
@@ -97,9 +95,6 @@ inline Atomic32 Acquire_AtomicExchange(volatile Atomic32* ptr,
                                        Atomic32 new_value) {
   CheckNaturalAlignment(ptr);
   Atomic32 old_val = NoBarrier_AtomicExchange(ptr, new_value);
-  if (AtomicOps_Internalx86CPUFeatures.has_amd_lock_mb_bug) {
-    __asm__ __volatile__("lfence" : : : "memory");
-  }
   return old_val;
 }
 
@@ -127,9 +122,6 @@ inline Atomic32 Barrier_AtomicIncrement(volatile Atomic32* ptr,
                        : "+r" (temp), "+m" (*ptr)
                        : : "memory");
   // temp now holds the old value of *ptr
-  if (AtomicOps_Internalx86CPUFeatures.has_amd_lock_mb_bug) {
-    __asm__ __volatile__("lfence" : : : "memory");
-  }
   return temp + increment;
 }
 
@@ -137,9 +129,6 @@ inline Atomic32 Acquire_CompareAndSwap(volatile Atomic32* ptr,
                                        Atomic32 old_value,
                                        Atomic32 new_value) {
   Atomic32 x = NoBarrier_CompareAndSwap(ptr, old_value, new_value);
-  if (AtomicOps_Internalx86CPUFeatures.has_amd_lock_mb_bug) {
-    __asm__ __volatile__("lfence" : : : "memory");
-  }
   return x;
 }
 
@@ -252,9 +241,6 @@ inline Atomic64 NoBarrier_AtomicExchange(volatile Atomic64* ptr,
 inline Atomic64 Acquire_AtomicExchange(volatile Atomic64* ptr,
                                        Atomic64 new_value) {
   Atomic64 old_val = NoBarrier_AtomicExchange(ptr, new_value);
-  if (AtomicOps_Internalx86CPUFeatures.has_amd_lock_mb_bug) {
-    __asm__ __volatile__("lfence" : : : "memory");
-  }
   return old_val;
 }
 
@@ -282,9 +268,6 @@ inline Atomic64 Barrier_AtomicIncrement(volatile Atomic64* ptr,
                        : "+r" (temp), "+m" (*ptr)
                        : : "memory");
   // temp now contains the previous value of *ptr
-  if (AtomicOps_Internalx86CPUFeatures.has_amd_lock_mb_bug) {
-    __asm__ __volatile__("lfence" : : : "memory");
-  }
   return temp + increment;
 }
 
@@ -398,9 +381,6 @@ inline Atomic64 Acquire_AtomicExchange(volatile Atomic64* ptr,
                                        Atomic64 new_val) {
   CheckNaturalAlignment(ptr);
   Atomic64 old_val = NoBarrier_AtomicExchange(ptr, new_val);
-  if (AtomicOps_Internalx86CPUFeatures.has_amd_lock_mb_bug) {
-    __asm__ __volatile__("lfence" : : : "memory");
-  }
   return old_val;
 }
 
@@ -426,9 +406,6 @@ inline Atomic64 Barrier_AtomicIncrement(volatile Atomic64* ptr,
                                         Atomic64 increment) {
   CheckNaturalAlignment(ptr);
   Atomic64 new_val = NoBarrier_AtomicIncrement(ptr, increment);
-  if (AtomicOps_Internalx86CPUFeatures.has_amd_lock_mb_bug) {
-    __asm__ __volatile__("lfence" : : : "memory");
-  }
   return new_val;
 }
 
@@ -488,9 +465,6 @@ inline Atomic64 Acquire_CompareAndSwap(volatile Atomic64* ptr,
                                        Atomic64 old_value,
                                        Atomic64 new_value) {
   Atomic64 x = NoBarrier_CompareAndSwap(ptr, old_value, new_value);
-  if (AtomicOps_Internalx86CPUFeatures.has_amd_lock_mb_bug) {
-    __asm__ __volatile__("lfence" : : : "memory");
-  }
   return x;
 }