You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by to...@apache.org on 2017/04/19 01:40:28 UTC
kudu git commit: gutil: stop supporting AMD Opteron K8 (take 2)
Repository: kudu
Updated Branches:
refs/heads/master e56f81b5d -> a342d6533
gutil: stop supporting AMD Opteron K8 (take 2)
This removes a workaround for a bug in very old (circa 2003) Opteron
chips. The workaround was producing an extra branch around most of our
atomic operations. This likely has a negligible perf impact, but it's
possible the slightly smaller code would make us run a bit faster or
reduce branch mispredictions.
Previously we also entirely forgot to call this function, so it was in
fact never applying the workaround. This patch marks the function as a
constructor, which ensures that it gets automatically called by any
binaries using atomic ops.
Impala made a similar change[1][2] a while back and seems to have had
no real issues.
[1] https://gerrit.cloudera.org/#/c/2516/
[2] https://github.com/apache/incubator-impala/commit/a7c3f301bb1da14d6bb35d37aab47450c67cca0c
Change-Id: I4eff19c62741e7433e378f7e0725079ecbcf1a97
Reviewed-on: http://gerrit.cloudera.org:8080/6680
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/a342d653
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/a342d653
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/a342d653
Branch: refs/heads/master
Commit: a342d6533df653cb5f946b78eab74b2abf7e6410
Parents: e56f81b
Author: Todd Lipcon <to...@apache.org>
Authored: Mon Apr 17 17:38:10 2017 -0700
Committer: Todd Lipcon <to...@apache.org>
Committed: Wed Apr 19 01:22:21 2017 +0000
----------------------------------------------------------------------
src/kudu/gutil/atomicops-internals-tsan.h | 2 --
src/kudu/gutil/atomicops-internals-x86.cc | 23 ++++++++----------
src/kudu/gutil/atomicops-internals-x86.h | 32 +-------------------------
3 files changed, 11 insertions(+), 46 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/kudu/blob/a342d653/src/kudu/gutil/atomicops-internals-tsan.h
----------------------------------------------------------------------
diff --git a/src/kudu/gutil/atomicops-internals-tsan.h b/src/kudu/gutil/atomicops-internals-tsan.h
index aecaefc..be9c144 100644
--- a/src/kudu/gutil/atomicops-internals-tsan.h
+++ b/src/kudu/gutil/atomicops-internals-tsan.h
@@ -19,8 +19,6 @@
// Features of this x86. Values may not be correct before main() 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.
};
BASE_EXPORT extern struct AtomicOps_x86CPUFeatureStruct
http://git-wip-us.apache.org/repos/asf/kudu/blob/a342d653/src/kudu/gutil/atomicops-internals-x86.cc
----------------------------------------------------------------------
diff --git a/src/kudu/gutil/atomicops-internals-x86.cc b/src/kudu/gutil/atomicops-internals-x86.cc
index 5d4529e..541429e 100644
--- a/src/kudu/gutil/atomicops-internals-x86.cc
+++ b/src/kudu/gutil/atomicops-internals-x86.cc
@@ -60,12 +60,13 @@
// 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
};
-// Initialize the AtomicOps_Internalx86CPUFeatures struct.
+// Initialize the AtomicOps_Internalx86CPUFeatures struct in any compilation
+// unit that links with this one.
+__attribute__((constructor))
static void AtomicOps_Internalx86CPUFeaturesInit() {
uint32 eax;
uint32 ebx;
@@ -95,12 +96,16 @@ static void AtomicOps_Internalx86CPUFeaturesInit() {
// 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.
+ //
+ // Opteron Rev E is the first-generation Opteron (models 1xx, 2xx, 8xx)
+ // also known as "K8", released in 2003. These processors are old enough
+ // that we can just drop support for them instead of trying to work around
+ // the above bug.
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;
+ LOG(FATAL) << "AMD Family 0fh model 32 through 63 not supported due to "
+ << "buggy atomic operations.";
}
// edx bit 26 is SSE2 which we use to tell use whether we can use mfence
@@ -108,21 +113,13 @@ static void AtomicOps_Internalx86CPUFeaturesInit() {
// ecx bit 13 indicates whether the cmpxchg16b instruction is supported
AtomicOps_Internalx86CPUFeatures.has_cmpxchg16b = ((ecx >> 13) & 1);
-
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;
}
-// AtomicOps initialisation routine for external use.
-void AtomicOps_x86CPUFeaturesInit() {
- AtomicOps_Internalx86CPUFeaturesInit();
-}
-
#endif
#endif // GUTIL_ATOMICOPS_INTERNALS_X86_H_
http://git-wip-us.apache.org/repos/asf/kudu/blob/a342d653/src/kudu/gutil/atomicops-internals-x86.h
----------------------------------------------------------------------
diff --git a/src/kudu/gutil/atomicops-internals-x86.h b/src/kudu/gutil/atomicops-internals-x86.h
index acbd2e3..1d2a5c4 100644
--- a/src/kudu/gutil/atomicops-internals-x86.h
+++ b/src/kudu/gutil/atomicops-internals-x86.h
@@ -40,11 +40,8 @@
// This struct is not part of the public API of this module; clients may not
// use it.
-// Features of this x86. Values may not be correct before InitGoogle() is run,
-// but are set conservatively.
+// Features of this x86.
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.
};
@@ -53,9 +50,6 @@ extern struct AtomicOps_x86CPUFeatureStruct AtomicOps_Internalx86CPUFeatures;
#define ATOMICOPS_COMPILER_BARRIER() __asm__ __volatile__("" : : : "memory")
-// AtomicOps initialisation for open source use.
-void AtomicOps_x86CPUFeaturesInit();
-
typedef int32_t Atomic32;
typedef int64_t Atomic64;
@@ -102,9 +96,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;
}
@@ -132,9 +123,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;
}
@@ -142,9 +130,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;
}
@@ -257,9 +242,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;
}
@@ -287,9 +269,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;
}
@@ -403,9 +382,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;
}
@@ -431,9 +407,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;
}
@@ -493,9 +466,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;
}