[4/5] incubator-kudu git commit: Fix stray memory writes due to tcmalloc profiling

[to...@apache.org]

Fix stray memory writes due to tcmalloc profiling

This fixes an issue that has been causing frequent crashes in
JD.com's production cluster as well as various Cloudera test clusters.
The crashes would be in various different places, but the key signature
was that offset 120 in some data structure or array (eg the 16th element
of a vector) would be corrupted.

After doing a git bisect using an integration testing cluster running
an ITBLL workload, I found that this was a regression caused by the
introduction of tcmalloc contention profiling[1]. The short explanation
is that, if we experienced contention while freeing a Trace object, we
could in some cases increment offset 120 of some other allocation
which occurred soon after the deallocation of the Trace.

The issue is described in more detail in a new comment in trace.h.

With this patch, I was unable to reproduce the issue on the test
cluster. No new test is added since this is quite timing-dependent
and not amenable to unit testing or even stress testing.

[1] commit f6691e744b9cb796e1bbc6e07953f21f387c9a88

Change-Id: I9afca83d9cc24585960f6bf68d8996c4736ce6cb
Reviewed-on: http://gerrit.cloudera.org:8080/3445
Reviewed-by: David Ribeiro Alves <dr...@apache.org>
Reviewed-by: Mike Percy <mp...@apache.org>
Tested-by: Kudu Jenkins
(cherry picked from commit 0bb59836fb3e4b92e2d88674e8b88546faac5c8b)
Reviewed-on: http://gerrit.cloudera.org:8080/3456
Reviewed-by: Todd Lipcon <to...@apache.org>


Project: http://git-wip-us.apache.org/repos/asf/incubator-kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-kudu/commit/65422651
Tree: http://git-wip-us.apache.org/repos/asf/incubator-kudu/tree/65422651
Diff: http://git-wip-us.apache.org/repos/asf/incubator-kudu/diff/65422651

Branch: refs/heads/branch-0.9.x
Commit: 65422651b5fb838209914e2328db85751e2a82d0
Parents: 3585637
Author: Todd Lipcon <to...@cloudera.com>
Authored: Tue Jun 21 23:02:13 2016 -0700
Committer: Todd Lipcon <to...@apache.org>
Committed: Thu Jun 23 02:06:52 2016 +0000

----------------------------------------------------------------------
 src/kudu/util/trace.h | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-kudu/blob/65422651/src/kudu/util/trace.h
----------------------------------------------------------------------
diff --git a/src/kudu/util/trace.h b/src/kudu/util/trace.h
index 324c1c4..c594461 100644
--- a/src/kudu/util/trace.h
+++ b/src/kudu/util/trace.h
@@ -237,10 +237,30 @@ class ScopedAdoptTrace {
   }
 
   ~ScopedAdoptTrace() {
-    if (Trace::threadlocal_trace_) {
-      Trace::threadlocal_trace_->Release();
-    }
+    auto t = Trace::threadlocal_trace_;
     Trace::threadlocal_trace_ = old_trace_;
+
+    // It's critical that we Release() the reference count on 't' only
+    // after we've unset the thread-local variable. Otherwise, we can hit
+    // a nasty interaction with tcmalloc contention profiling. Consider
+    // the following sequence:
+    //
+    //   1. threadlocal_trace_ has refcount = 1
+    //   2. we call threadlocal_trace_->Release() which decrements refcount to 0
+    //   3. this calls 'delete' on the Trace object
+    //   3a. this calls tcmalloc free() on the Trace and various sub-objects
+    //   3b. the free() calls may end up experiencing contention in tcmalloc
+    //   3c. we try to account the contention in threadlocal_trace_'s TraceMetrics,
+    //       but it has already been freed.
+    //
+    // In the best case, we just scribble into some free tcmalloc memory. In the
+    // worst case, tcmalloc would have already re-used this memory for a new
+    // allocation on another thread, and we end up overwriting someone else's memory.
+    //
+    // Waiting to Release() only after 'unpublishing' the trace solves this.
+    if (t) {
+      t->Release();
+    }
     DFAKE_SCOPED_LOCK_THREAD_LOCKED(ctor_dtor_);
   }