You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by da...@apache.org on 2016/10/03 19:30:43 UTC
[2/3] kudu git commit: Replace some TSAN suppressions with annotations
Replace some TSAN suppressions with annotations
In the case that there is a known race, we can either use an annotation or a
suppression to avoid it causing issues. It turns out that using suppressions
is rather inefficient: TSAN still needs to take the stack trace and feed
it to the external symbolizer program in order to compare the resulting
symbols against the suppression list.
This is particularly bad since the symbolizer does IO to read the symbols
and debug info, and thus can be pretty slow. TSAN does all of this work
while holding some global locks which prevent new threads from starting.
These thread creation delays have caused a lot of test flakiness in
TSAN builds.
This fixes the issue by switching out the most common suppressions to
use annotations instead.
Change-Id: I22b2f80e18cf84f58a30797bf2a355d3832a31b2
Reviewed-on: http://gerrit.cloudera.org:8080/4593
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Reviewed-by: Mike Percy <mp...@apache.org>
Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/407f95da
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/407f95da
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/407f95da
Branch: refs/heads/master
Commit: 407f95da12540de00d23c32fb9cdf853f36021c7
Parents: 3cb16ff
Author: Todd Lipcon <to...@apache.org>
Authored: Sun Oct 2 23:54:42 2016 -0700
Committer: Todd Lipcon <to...@apache.org>
Committed: Mon Oct 3 18:00:47 2016 +0000
----------------------------------------------------------------------
build-support/tsan-suppressions.txt | 15 ---------------
src/kudu/rpc/reactor.cc | 5 +++++
src/kudu/tablet/cbtree-test.cc | 2 ++
src/kudu/tablet/concurrent_btree.h | 13 +++++++++++++
src/kudu/util/debug-util.cc | 4 ++++
5 files changed, 24 insertions(+), 15 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/kudu/blob/407f95da/build-support/tsan-suppressions.txt
----------------------------------------------------------------------
diff --git a/build-support/tsan-suppressions.txt b/build-support/tsan-suppressions.txt
index a31b723..4fa7f41 100644
--- a/build-support/tsan-suppressions.txt
+++ b/build-support/tsan-suppressions.txt
@@ -26,25 +26,10 @@
race:_ULx86_64_init
race:_ULx86_64_local_addr_space_init
-# libev uses some lock-free synchronization, but doesn't have TSAN annotations.
-# See http://lists.schmorp.de/pipermail/libev/2013q2/002178.html or KUDU-366
-# for examples.
-race:evpipe_write
-race:evpipe_init
-race:epoll_ctl
-race:ev_async_send
-race:ev_run
-race:pipecb
-
# TSAN complains about data races on the global signals variable in
# ev_feed_signal and spoiled errno in ev_sighandler. Both are probably noise.
race:ev_sighandler
-# concurrent btree uses optimistic concurrency, needs to be annotated a bunch
-# more before it would pass. Relatively confident that it is correct based on
-# a lot of stress testing.
-race:concurrent_btree.h
-
# See https://github.com/google/glog/issues/80 for a general list of TSAN
# issues in glog.
# 1. glog's fatal signal handler isn't signal-safe -- it allocates memory.
http://git-wip-us.apache.org/repos/asf/kudu/blob/407f95da/src/kudu/rpc/reactor.cc
----------------------------------------------------------------------
diff --git a/src/kudu/rpc/reactor.cc b/src/kudu/rpc/reactor.cc
index 19a3dde..e8e0054 100644
--- a/src/kudu/rpc/reactor.cc
+++ b/src/kudu/rpc/reactor.cc
@@ -40,6 +40,7 @@
#include "kudu/rpc/sasl_server.h"
#include "kudu/rpc/transfer.h"
#include "kudu/util/countdown_latch.h"
+#include "kudu/util/debug/sanitizer_scopes.h"
#include "kudu/util/errno.h"
#include "kudu/util/flag_tags.h"
#include "kudu/util/monotime.h"
@@ -174,6 +175,10 @@ Status ReactorThread::DumpRunningRpcs(const DumpRunningRpcsRequestPB& req,
}
void ReactorThread::WakeThread() {
+ // libev uses some lock-free synchronization, but doesn't have TSAN annotations.
+ // See http://lists.schmorp.de/pipermail/libev/2013q2/002178.html or KUDU-366
+ // for examples.
+ debug::ScopedTSANIgnoreReadsAndWrites ignore_tsan;
async_.send();
}
http://git-wip-us.apache.org/repos/asf/kudu/blob/407f95da/src/kudu/tablet/cbtree-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tablet/cbtree-test.cc b/src/kudu/tablet/cbtree-test.cc
index 5750cfa..5f5fb9d 100644
--- a/src/kudu/tablet/cbtree-test.cc
+++ b/src/kudu/tablet/cbtree-test.cc
@@ -23,6 +23,7 @@
#include "kudu/gutil/gscoped_ptr.h"
#include "kudu/tablet/concurrent_btree.h"
#include "kudu/util/barrier.h"
+#include "kudu/util/debug/sanitizer_scopes.h"
#include "kudu/util/hexdump.h"
#include "kudu/util/memory/memory.h"
#include "kudu/util/memory/overwrite.h"
@@ -342,6 +343,7 @@ TEST_F(TestCBTree, TestInsertAndVerifyRandom) {
// - either mark it splitting or inserting (alternatingly)
// - unlock it
void LockCycleThread(AtomicVersion *v, int count_split, int count_insert) {
+ debug::ScopedTSANIgnoreReadsAndWrites ignore_tsan;
int i = 0;
while (count_split > 0 || count_insert > 0) {
i++;
http://git-wip-us.apache.org/repos/asf/kudu/blob/407f95da/src/kudu/tablet/concurrent_btree.h
----------------------------------------------------------------------
diff --git a/src/kudu/tablet/concurrent_btree.h b/src/kudu/tablet/concurrent_btree.h
index 1d3be26..795b5a4 100644
--- a/src/kudu/tablet/concurrent_btree.h
+++ b/src/kudu/tablet/concurrent_btree.h
@@ -35,6 +35,11 @@
// - The leaf nodes are linked together with a "next" pointer. This makes
// scanning simpler (the Masstree implementation avoids this because it
// complicates the removal operation)
+//
+// NOTE: this code disables TSAN for the most part. This is because it uses
+// some "clever" concurrency mechanisms which are difficult to model in TSAN.
+// We instead ensure correctness by heavy stress-testing.
+
#ifndef KUDU_TABLET_CONCURRENT_BTREE_H
#define KUDU_TABLET_CONCURRENT_BTREE_H
@@ -44,6 +49,7 @@
#include <memory>
#include <string>
+#include "kudu/util/debug/sanitizer_scopes.h"
#include "kudu/util/inline_slice.h"
#include "kudu/util/memory/arena.h"
#include "kudu/util/status.h"
@@ -849,6 +855,7 @@ class PreparedMutation {
// If the returned PreparedMutation object is not used with
// Insert(), it will be automatically unlocked by its destructor.
void Prepare(CBTree<Traits> *tree) {
+ debug::ScopedTSANIgnoreReadsAndWrites ignore_tsan;
CHECK(!prepared());
this->tree_ = tree;
this->arena_ = tree->arena_.get();
@@ -965,6 +972,7 @@ class CBTree {
//
// More advanced users can use the PreparedMutation class instead.
bool Insert(const Slice &key, const Slice &val) {
+ debug::ScopedTSANIgnoreReadsAndWrites ignore_tsan;
PreparedMutation<Traits> mutation(key);
mutation.Prepare(this);
return mutation.Insert(val);
@@ -992,6 +1000,7 @@ class CBTree {
//
// TODO: this call probably won't be necessary in the final implementation
GetResult GetCopy(const Slice &key, char *buf, size_t *buf_len) const {
+ debug::ScopedTSANIgnoreReadsAndWrites ignore_tsan;
size_t in_buf_len = *buf_len;
retry_from_root:
@@ -1047,6 +1056,7 @@ class CBTree {
// Returns true if the given key is contained in the tree.
// TODO: unit test
bool ContainsKey(const Slice &key) const {
+ debug::ScopedTSANIgnoreReadsAndWrites ignore_tsan;
bool ret;
retry_from_root:
@@ -1290,6 +1300,7 @@ class CBTree {
// 'node' is unlocked
bool Insert(PreparedMutation<Traits> *mutation,
const Slice &val) {
+ debug::ScopedTSANIgnoreReadsAndWrites ignore_tsan;
CHECK(!frozen_);
CHECK_NOTNULL(mutation);
DCHECK_EQ(mutation->tree(), this);
@@ -1712,6 +1723,7 @@ class CBTreeIterator {
void SeekToLeaf(const Slice &key) {
+ debug::ScopedTSANIgnoreReadsAndWrites ignore_tsan;
retry_from_root:
{
AtomicVersion version;
@@ -1747,6 +1759,7 @@ class CBTreeIterator {
}
bool SeekNextLeaf() {
+ debug::ScopedTSANIgnoreReadsAndWrites ignore_tsan;
DCHECK(seeked_);
LeafNode<Traits> *next = leaf_to_scan_->next_;
if (PREDICT_FALSE(next == NULL)) {
http://git-wip-us.apache.org/repos/asf/kudu/blob/407f95da/src/kudu/util/debug-util.cc
----------------------------------------------------------------------
diff --git a/src/kudu/util/debug-util.cc b/src/kudu/util/debug-util.cc
index 61866cc..9cf2aa4 100644
--- a/src/kudu/util/debug-util.cc
+++ b/src/kudu/util/debug-util.cc
@@ -28,6 +28,7 @@
#include "kudu/gutil/spinlock.h"
#include "kudu/gutil/stringprintf.h"
#include "kudu/gutil/strings/numbers.h"
+#include "kudu/util/debug/sanitizer_scopes.h"
#include "kudu/util/env.h"
#include "kudu/util/errno.h"
#include "kudu/util/monotime.h"
@@ -330,6 +331,9 @@ string GetLogFormatStackTraceHex() {
}
void StackTrace::Collect(int skip_frames) {
+ // google::GetStackTrace has a data race. This is called frequently, so better
+ // to ignore it with an annotation rather than use a suppression.
+ debug::ScopedTSANIgnoreReadsAndWrites ignore_tsan;
num_frames_ = google::GetStackTrace(frames_, arraysize(frames_), skip_frames);
}