You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by al...@apache.org on 2017/04/18 21:00:16 UTC
[1/3] kudu git commit: gutil: stop supporting AMD Opteron K8
Repository: kudu
Updated Branches:
refs/heads/master 238249c96 -> 5132624f2
gutil: stop supporting AMD Opteron K8
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.
This also adds a missing call to init the feature flags in the first
place, which we seem to have forgotten the first time around.
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: I59b5aeaae56b5f310b5dd160b868fcb5d04f4da1
Reviewed-on: http://gerrit.cloudera.org:8080/6661
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/b5004e18
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/b5004e18
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/b5004e18
Branch: refs/heads/master
Commit: b5004e1886eb96839ec120da582f0a06ce869db2
Parents: 238249c
Author: Todd Lipcon <to...@apache.org>
Authored: Mon Apr 17 17:38:10 2017 -0700
Committer: Todd Lipcon <to...@apache.org>
Committed: Tue Apr 18 18:31:33 2017 +0000
----------------------------------------------------------------------
src/kudu/gutil/atomicops-internals-tsan.h | 2 --
src/kudu/gutil/atomicops-internals-x86.cc | 14 +++++++-------
src/kudu/gutil/atomicops-internals-x86.h | 26 --------------------------
src/kudu/util/init.cc | 4 ++++
4 files changed, 11 insertions(+), 35 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/kudu/blob/b5004e18/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/b5004e18/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..9650847 100644
--- a/src/kudu/gutil/atomicops-internals-x86.cc
+++ b/src/kudu/gutil/atomicops-internals-x86.cc
@@ -60,7 +60,6 @@
// 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
};
@@ -95,12 +94,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,12 +111,9 @@ 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;
}
http://git-wip-us.apache.org/repos/asf/kudu/blob/b5004e18/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..c24907e 100644
--- a/src/kudu/gutil/atomicops-internals-x86.h
+++ b/src/kudu/gutil/atomicops-internals-x86.h
@@ -43,8 +43,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.
};
@@ -102,9 +100,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 +127,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 +134,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 +246,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 +273,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 +386,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 +411,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 +470,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;
}
http://git-wip-us.apache.org/repos/asf/kudu/blob/b5004e18/src/kudu/util/init.cc
----------------------------------------------------------------------
diff --git a/src/kudu/util/init.cc b/src/kudu/util/init.cc
index 3fa634a..45640f4 100644
--- a/src/kudu/util/init.cc
+++ b/src/kudu/util/init.cc
@@ -22,6 +22,7 @@
#include <string>
+#include "kudu/gutil/atomicops.h"
#include "kudu/gutil/cpu.h"
#include "kudu/gutil/strings/substitute.h"
#include "kudu/util/status.h"
@@ -79,6 +80,9 @@ Status CheckCPUFlags() {
void InitKuduOrDie() {
CheckStandardFds();
CHECK_OK(CheckCPUFlags());
+#ifndef THREAD_SANITIZER // Thread Sanitizer uses its own special atomicops implementation.
+ AtomicOps_x86CPUFeaturesInit();
+#endif
// NOTE: this function is called before flags are parsed.
// Do not add anything in here which is flag-dependent.
}
[3/3] kudu git commit: [cmake] fixed 'latest' dir symlink on MacOS X
Posted by al...@apache.org.
[cmake] fixed 'latest' dir symlink on MacOS X
If the 'latest' symbolic link already exists, do not follow it, so the
'latest' link is updated to point to the new src directory. Prior to
this, the result symlink on OS X would be created in the directory
which the 'latest' points at.
The fix is in replacing Linux-specific '-T' option with '-n' option
which is compatible at least between BSD-style and Linux ln
implementations and adding that '-n' flag regardless of the platform.
Change-Id: Ica6314eb5274e853b7b39b7e6ede17305d0c646d
Reviewed-on: http://gerrit.cloudera.org:8080/6667
Reviewed-by: Dan Burkert <da...@apache.org>
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/5132624f
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/5132624f
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/5132624f
Branch: refs/heads/master
Commit: 5132624f260da959f0cb1d1c8944f33da886c08d
Parents: 3de172d
Author: Alexey Serbin <as...@cloudera.com>
Authored: Tue Apr 18 00:52:15 2017 -0700
Committer: Alexey Serbin <as...@cloudera.com>
Committed: Tue Apr 18 20:57:25 2017 +0000
----------------------------------------------------------------------
CMakeLists.txt | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/kudu/blob/5132624f/CMakeLists.txt
----------------------------------------------------------------------
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 05c719b..3aa6911 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -54,17 +54,13 @@ string(REPLACE "${CMAKE_CURRENT_SOURCE_DIR}/build/" ""
BLESSED_BUILD_SUBDIR "${CMAKE_CURRENT_BINARY_DIR}")
string(FIND "${BLESSED_BUILD_SUBDIR}" "/" SLASH_POS)
if (SLASH_POS EQUAL -1)
- if (NOT APPLE)
- set(MORE_ARGS "-T")
- endif()
-
# Create the symlink both during cmake invocation and when the default target
# ('all') is built. The former is useful for scripts that, after running
# cmake, only build a single target (i.e. "make lint").
- execute_process(COMMAND ln ${MORE_ARGS} -sf ${CMAKE_CURRENT_BINARY_DIR}
+ execute_process(COMMAND ln ${MORE_ARGS} -nsf ${CMAKE_CURRENT_BINARY_DIR}
${CMAKE_CURRENT_SOURCE_DIR}/build/latest)
add_custom_target(latest_symlink ALL
- ln ${MORE_ARGS} -sf ${CMAKE_CURRENT_BINARY_DIR}
+ ln ${MORE_ARGS} -nsf ${CMAKE_CURRENT_BINARY_DIR}
${CMAKE_CURRENT_SOURCE_DIR}/build/latest
COMMENT "Recreating ../build/latest symlink")
[2/3] kudu git commit: [rpc] close socket on AcceptorPool::Shutdown()
Posted by al...@apache.org.
[rpc] close socket on AcceptorPool::Shutdown()
Close the underlying socket upon AcceptorPool::Shutdown() call. It does
not make much sense keeping the socket open after calling shutdown()
for both send and receive.
Due to the typical ownership pattern of AcceptorPool two references to
the object is kept: one reference is kept by RpcService, another by
Messenger. So, to close the socket it's necessary to call both
{RpcService,Messenger}::Shutdown() methods.
This patch also fixes delete_tablet-itest on MacOS X.
Change-Id: Ided3643b07e642beb586e51efa442b39b3785916
Reviewed-on: http://gerrit.cloudera.org:8080/6665
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon <to...@apache.org>
Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/3de172d6
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/3de172d6
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/3de172d6
Branch: refs/heads/master
Commit: 3de172d6519a5ab0c74fbab75349eb17214502d9
Parents: b5004e1
Author: Alexey Serbin <as...@cloudera.com>
Authored: Mon Apr 17 23:08:49 2017 -0700
Committer: Alexey Serbin <as...@cloudera.com>
Committed: Tue Apr 18 18:41:12 2017 +0000
----------------------------------------------------------------------
src/kudu/rpc/acceptor_pool.cc | 22 +++++++++++++++++-----
src/kudu/rpc/messenger.cc | 5 +++--
src/kudu/tserver/tablet_server-test.cc | 6 +++---
src/kudu/tserver/tablet_server.cc | 13 +++----------
src/kudu/tserver/tablet_server.h | 1 -
src/kudu/util/net/socket.cc | 10 +++++-----
6 files changed, 31 insertions(+), 26 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/kudu/blob/3de172d6/src/kudu/rpc/acceptor_pool.cc
----------------------------------------------------------------------
diff --git a/src/kudu/rpc/acceptor_pool.cc b/src/kudu/rpc/acceptor_pool.cc
index 2bda162..f3c935c 100644
--- a/src/kudu/rpc/acceptor_pool.cc
+++ b/src/kudu/rpc/acceptor_pool.cc
@@ -17,15 +17,17 @@
#include "kudu/rpc/acceptor_pool.h"
-#include <gflags/gflags.h>
-#include <glog/logging.h>
-#include <inttypes.h>
-#include <iostream>
#include <pthread.h>
-#include <stdint.h>
+
+#include <cinttypes>
+#include <cstdint>
+#include <iostream>
#include <string>
#include <vector>
+#include <gflags/gflags.h>
+#include <glog/logging.h>
+
#include "kudu/gutil/ref_counted.h"
#include "kudu/gutil/strings/substitute.h"
#include "kudu/rpc/messenger.h"
@@ -112,6 +114,16 @@ void AcceptorPool::Shutdown() {
CHECK_OK(ThreadJoiner(thread.get()).Join());
}
threads_.clear();
+
+ // Close the socket: keeping the descriptor open and, possibly, receiving late
+ // not-to-be-read messages from the peer does not make much sense. The
+ // Socket::Close() method is called upon destruction of the aggregated socket_
+ // object as well. However, the typical ownership pattern of an AcceptorPool
+ // object includes two references wrapped via a shared_ptr smart pointer: one
+ // is held by Messenger, another by RpcServer. If not calling Socket::Close()
+ // here, it would necessary to wait until Messenger::Shutdown() is called for
+ // the corresponding messenger object to close this socket.
+ ignore_result(socket_.Close());
}
Sockaddr AcceptorPool::bind_address() const {
http://git-wip-us.apache.org/repos/asf/kudu/blob/3de172d6/src/kudu/rpc/messenger.cc
----------------------------------------------------------------------
diff --git a/src/kudu/rpc/messenger.cc b/src/kudu/rpc/messenger.cc
index 09ccbbe..bb3746f 100644
--- a/src/kudu/rpc/messenger.cc
+++ b/src/kudu/rpc/messenger.cc
@@ -58,6 +58,7 @@
using std::string;
using std::shared_ptr;
+using std::make_shared;
using strings::Substitute;
DEFINE_string(rpc_authentication, "optional",
@@ -322,11 +323,11 @@ Status Messenger::AddAcceptorPool(const Sockaddr &accept_addr,
RETURN_NOT_OK(sock.Bind(accept_addr));
Sockaddr remote;
RETURN_NOT_OK(sock.GetSocketAddress(&remote));
- shared_ptr<AcceptorPool> acceptor_pool(new AcceptorPool(this, &sock, remote));
+ auto acceptor_pool(make_shared<AcceptorPool>(this, &sock, remote));
std::lock_guard<percpu_rwlock> guard(lock_);
acceptor_pools_.push_back(acceptor_pool);
- *pool = acceptor_pool;
+ pool->swap(acceptor_pool);
return Status::OK();
}
http://git-wip-us.apache.org/repos/asf/kudu/blob/3de172d6/src/kudu/tserver/tablet_server-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tserver/tablet_server-test.cc b/src/kudu/tserver/tablet_server-test.cc
index 971a76c..2b41729 100644
--- a/src/kudu/tserver/tablet_server-test.cc
+++ b/src/kudu/tserver/tablet_server-test.cc
@@ -2124,14 +2124,14 @@ TEST_F(TabletServerTest, TestInsertLatencyMicroBenchmark) {
TEST_F(TabletServerTest, TestRpcServerCreateDestroy) {
RpcServerOptions opts;
{
- RpcServer server1(opts);
+ RpcServer server(opts);
}
{
- RpcServer server2(opts);
+ RpcServer server(opts);
MessengerBuilder mb("foo");
shared_ptr<Messenger> messenger;
ASSERT_OK(mb.Build(&messenger));
- ASSERT_OK(server2.Init(messenger));
+ ASSERT_OK(server.Init(messenger));
}
}
http://git-wip-us.apache.org/repos/asf/kudu/blob/3de172d6/src/kudu/tserver/tablet_server.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tserver/tablet_server.cc b/src/kudu/tserver/tablet_server.cc
index 27a0c1d..20bde34 100644
--- a/src/kudu/tserver/tablet_server.cc
+++ b/src/kudu/tserver/tablet_server.cc
@@ -18,8 +18,6 @@
#include "kudu/tserver/tablet_server.h"
#include <glog/logging.h>
-#include <list>
-#include <vector>
#include "kudu/cfile/block_cache.h"
#include "kudu/fs/fs_manager.h"
@@ -29,19 +27,16 @@
#include "kudu/server/webserver.h"
#include "kudu/tserver/heartbeater.h"
#include "kudu/tserver/scanners.h"
+#include "kudu/tserver/tablet_copy_service.h"
#include "kudu/tserver/tablet_service.h"
#include "kudu/tserver/ts_tablet_manager.h"
#include "kudu/tserver/tserver-path-handlers.h"
-#include "kudu/tserver/tablet_copy_service.h"
#include "kudu/util/maintenance_manager.h"
#include "kudu/util/net/net_util.h"
#include "kudu/util/net/sockaddr.h"
#include "kudu/util/status.h"
using kudu::rpc::ServiceIf;
-using kudu::tablet::TabletPeer;
-using std::shared_ptr;
-using std::vector;
namespace kudu {
namespace tserver {
@@ -132,16 +127,14 @@ Status TabletServer::Start() {
}
void TabletServer::Shutdown() {
- LOG(INFO) << "TabletServer shutting down...";
-
if (initted_) {
+ LOG(INFO) << "TabletServer shutting down...";
maintenance_manager_->Shutdown();
WARN_NOT_OK(heartbeater_->Stop(), "Failed to stop TS Heartbeat thread");
ServerBase::Shutdown();
tablet_manager_->Shutdown();
+ LOG(INFO) << "TabletServer shut down complete. Bye!";
}
-
- LOG(INFO) << "TabletServer shut down complete. Bye!";
}
} // namespace tserver
http://git-wip-us.apache.org/repos/asf/kudu/blob/3de172d6/src/kudu/tserver/tablet_server.h
----------------------------------------------------------------------
diff --git a/src/kudu/tserver/tablet_server.h b/src/kudu/tserver/tablet_server.h
index b8f3b77..e353c6d 100644
--- a/src/kudu/tserver/tablet_server.h
+++ b/src/kudu/tserver/tablet_server.h
@@ -19,7 +19,6 @@
#include <memory>
#include <string>
-#include <vector>
#include "kudu/consensus/metadata.pb.h"
#include "kudu/gutil/atomicops.h"
http://git-wip-us.apache.org/repos/asf/kudu/blob/3de172d6/src/kudu/util/net/socket.cc
----------------------------------------------------------------------
diff --git a/src/kudu/util/net/socket.cc b/src/kudu/util/net/socket.cc
index c0945ca..e0bea14 100644
--- a/src/kudu/util/net/socket.cc
+++ b/src/kudu/util/net/socket.cc
@@ -81,16 +81,16 @@ Socket::~Socket() {
}
Status Socket::Close() {
- if (fd_ < 0)
+ if (fd_ < 0) {
return Status::OK();
- int err, fd = fd_;
- fd_ = -1;
+ }
+ int fd = fd_;
if (::close(fd) < 0) {
- err = errno;
+ int err = errno;
return Status::NetworkError(std::string("close error: ") +
ErrnoToString(err), Slice(), err);
}
- fd = -1;
+ fd_ = -1;
return Status::OK();
}