You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by ta...@apache.org on 2018/01/05 23:19:54 UTC
[2/2] impala git commit: IMPALA-6362: avoid Reservation/MemTracker
deadlock
IMPALA-6362: avoid Reservation/MemTracker deadlock
Avoid the circular dependency between ReservationTracker::lock_ and
MemTracker::child_trackers_lock_ by not acquiring
ReservationTracker::lock_ in GetReservation(), where an atomic
operation is sufficient.
Testing:
Added a unit test that reproed the deadlock.
Change-Id: Id7adbe961a925075422c685690dd3d1609779ced
Reviewed-on: http://gerrit.cloudera.org:8080/8933
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Tested-by: Impala Public Jenkins
Project: http://git-wip-us.apache.org/repos/asf/impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/d25607d0
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/d25607d0
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/d25607d0
Branch: refs/heads/master
Commit: d25607d01b90abe6fdb7a422278688876fa54594
Parents: d1a0510
Author: Tim Armstrong <ta...@cloudera.com>
Authored: Wed Jan 3 15:38:21 2018 -0800
Committer: Impala Public Jenkins <im...@gerrit.cloudera.org>
Committed: Fri Jan 5 22:57:21 2018 +0000
----------------------------------------------------------------------
.../bufferpool/reservation-tracker-test.cc | 44 ++++++++++++++++++++
.../runtime/bufferpool/reservation-tracker.cc | 25 ++++++-----
be/src/runtime/bufferpool/reservation-tracker.h | 15 ++++---
be/src/util/memory-metrics.cc | 4 ++
be/src/util/memory-metrics.h | 2 +
5 files changed, 75 insertions(+), 15 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/impala/blob/d25607d0/be/src/runtime/bufferpool/reservation-tracker-test.cc
----------------------------------------------------------------------
diff --git a/be/src/runtime/bufferpool/reservation-tracker-test.cc b/be/src/runtime/bufferpool/reservation-tracker-test.cc
index b6717f4..c46c5ea 100644
--- a/be/src/runtime/bufferpool/reservation-tracker-test.cc
+++ b/be/src/runtime/bufferpool/reservation-tracker-test.cc
@@ -26,6 +26,7 @@
#include "common/init.h"
#include "common/object-pool.h"
#include "runtime/mem-tracker.h"
+#include "util/memory-metrics.h"
#include "testutil/gtest-util.h"
#include "common/names.h"
@@ -55,6 +56,12 @@ class ReservationTrackerTest : public ::testing::Test {
return RuntimeProfile::Create(&obj_pool_, "test profile");
}
+ BufferPoolMetric* CreateReservationMetric(ReservationTracker* tracker) {
+ return obj_pool_.Add(new BufferPoolMetric(MetricDefs::Get("buffer-pool.reserved"),
+ BufferPoolMetric::BufferPoolMetricType::RESERVED, tracker,
+ nullptr));
+ }
+
ObjectPool obj_pool_;
ReservationTracker root_;
@@ -533,6 +540,43 @@ TEST_F(ReservationTrackerTest, ReservationUtil) {
ReservationUtil::GetMinMemLimitFromReservation(4 * GIG)));
}
+static void LogUsageThread(MemTracker* mem_tracker, AtomicInt32* done) {
+ while (done->Load() == 0) {
+ int64_t logged_consumption;
+ mem_tracker->LogUsage(10, " ", &logged_consumption);
+ }
+}
+
+// IMPALA-6362: regression test for deadlock between ReservationTracker and MemTracker.
+TEST_F(ReservationTrackerTest, MemTrackerDeadlock) {
+ const int64_t RESERVATION_LIMIT = 1024;
+ root_.InitRootTracker(nullptr, numeric_limits<int64_t>::max());
+ MemTracker* mem_tracker = obj_pool_.Add(new MemTracker);
+ ReservationTracker* reservation = obj_pool_.Add(new ReservationTracker());
+ reservation->InitChildTracker(nullptr, &root_, mem_tracker, RESERVATION_LIMIT);
+
+ // Create a child MemTracker with a buffer pool consumption metric, that calls
+ // reservation->GetReservation() when its usage is logged.
+ obj_pool_.Add(new MemTracker(CreateReservationMetric(reservation),
+ -1, "Reservation", mem_tracker));
+ // Start background thread that repeatededly logs the 'mem_tracker' tree.
+ AtomicInt32 done(0);
+ thread log_usage_thread(&LogUsageThread, mem_tracker, &done);
+
+ // Retry enough times to reproduce the deadlock with LogUsageThread().
+ for (int i = 0; i < 100; ++i) {
+ // Fail to increase reservation, hitting limit of 'reservation'. This will try
+ // to log the 'mem_tracker' tree while holding reservation->lock_.
+ Status err;
+ ASSERT_FALSE(reservation->IncreaseReservation(RESERVATION_LIMIT + 1, &err));
+ ASSERT_FALSE(err.ok());
+ }
+
+ done.Store(1);
+ log_usage_thread.join();
+ reservation->Close();
+ mem_tracker->Close();
+}
}
int main(int argc, char **argv) {
http://git-wip-us.apache.org/repos/asf/impala/blob/d25607d0/be/src/runtime/bufferpool/reservation-tracker.cc
----------------------------------------------------------------------
diff --git a/be/src/runtime/bufferpool/reservation-tracker.cc b/be/src/runtime/bufferpool/reservation-tracker.cc
index 1c84912..aba5dce 100644
--- a/be/src/runtime/bufferpool/reservation-tracker.cc
+++ b/be/src/runtime/bufferpool/reservation-tracker.cc
@@ -21,6 +21,7 @@
#include <cstdlib>
#include "common/object-pool.h"
+#include "gutil/atomicops.h"
#include "gutil/strings/substitute.h"
#include "runtime/exec-env.h"
#include "runtime/mem-tracker.h"
@@ -189,10 +190,12 @@ bool ReservationTracker::IncreaseReservationInternalLocked(int64_t bytes,
}
*error_status = Status::Expected(error_msg);
}
+ } else if (parent_ == nullptr) {
+ // No parent and no linked MemTracker - increase can be granted.
+ DCHECK(mem_tracker_ == nullptr) << "Root cannot have linked MemTracker";
+ granted = true;
} else {
- if (parent_ == nullptr) {
- granted = true;
- } else {
+ {
lock_guard<SpinLock> l(parent_->lock_);
granted = parent_->IncreaseReservationInternalLocked(
reservation_increase, true, true, error_status);
@@ -208,7 +211,6 @@ bool ReservationTracker::IncreaseReservationInternalLocked(int64_t bytes,
parent_->DecreaseReservation(reservation_increase, true);
}
}
-
if (granted) {
// The reservation was granted and state updated in all ancestors: we can modify
// this tracker's state now.
@@ -374,15 +376,17 @@ void ReservationTracker::ReleaseTo(int64_t bytes) {
}
int64_t ReservationTracker::GetReservation() {
- lock_guard<SpinLock> l(lock_);
+ // Don't acquire lock - there is no point in holding it for this function only since
+ // the value read can change as soon as we release it.
DCHECK(initialized_);
- return reservation_;
+ return base::subtle::Acquire_Load(&reservation_);
}
int64_t ReservationTracker::GetUsedReservation() {
- lock_guard<SpinLock> l(lock_);
+ // Don't acquire lock - there is no point in holding it for this function only since
+ // the value read can change as soon as we release it.
DCHECK(initialized_);
- return used_reservation_;
+ return base::subtle::Acquire_Load(&used_reservation_);
}
int64_t ReservationTracker::GetUnusedReservation() {
@@ -392,9 +396,10 @@ int64_t ReservationTracker::GetUnusedReservation() {
}
int64_t ReservationTracker::GetChildReservations() {
- lock_guard<SpinLock> l(lock_);
+ // Don't acquire lock - there is no point in holding it for this function only since
+ // the value read can change as soon as we release it.
DCHECK(initialized_);
- return child_reservations_;
+ return base::subtle::Acquire_Load(&child_reservations_);
}
void ReservationTracker::CheckConsistency() const {
http://git-wip-us.apache.org/repos/asf/impala/blob/d25607d0/be/src/runtime/bufferpool/reservation-tracker.h
----------------------------------------------------------------------
diff --git a/be/src/runtime/bufferpool/reservation-tracker.h b/be/src/runtime/bufferpool/reservation-tracker.h
index 337e12b..ff4b77e 100644
--- a/be/src/runtime/bufferpool/reservation-tracker.h
+++ b/be/src/runtime/bufferpool/reservation-tracker.h
@@ -154,18 +154,19 @@ class ReservationTracker {
/// 'bytes' before calling this method.
void ReleaseTo(int64_t bytes);
- /// Returns the amount of the reservation in bytes.
+ /// Returns the amount of the reservation in bytes. Does not acquire the internal lock.
int64_t GetReservation();
/// Returns the current amount of the reservation used at this tracker, not including
- /// reservations of children in bytes.
+ /// reservations of children in bytes. Does not acquire the internal lock.
int64_t GetUsedReservation();
/// Returns the amount of the reservation neither used nor given to childrens'
- /// reservations at this tracker in bytes.
+ /// reservations at this tracker in bytes. Acquires the internal lock.
int64_t GetUnusedReservation();
- /// Returns the total reservations of children in bytes.
+ /// Returns the total reservations of children in bytes. Does not acquire the
+ /// internal lock.
int64_t GetChildReservations();
/// Support for debug actions: deny reservation increase with probability 'probability'.
@@ -292,18 +293,22 @@ class ReservationTracker {
/// TODO: remove once all memory is accounted via ReservationTrackers.
MemTracker* mem_tracker_ = nullptr;
- /// The maximum reservation in bytes that this tracker can have.
+ /// The maximum reservation in bytes that this tracker can have. Can be read with an
+ /// atomic load without holding lock.
int64_t reservation_limit_;
/// This tracker's current reservation in bytes. 'reservation_' <= 'reservation_limit_'.
+ /// Can be read with an atomic load without holding lock.
int64_t reservation_;
/// Total reservation of children in bytes. This is included in 'reservation_'.
/// 'used_reservation_' + 'child_reservations_' <= 'reservation_'.
+ /// Can be read with an atomic load without holding lock.
int64_t child_reservations_;
/// The amount of the reservation currently used by this tracker in bytes.
/// 'used_reservation_' + 'child_reservations_' <= 'reservation_'.
+ /// Can be read with an atomic load without holding lock.
int64_t used_reservation_;
};
}
http://git-wip-us.apache.org/repos/asf/impala/blob/d25607d0/be/src/util/memory-metrics.cc
----------------------------------------------------------------------
diff --git a/be/src/util/memory-metrics.cc b/be/src/util/memory-metrics.cc
index d1e7a16..3308bf4 100644
--- a/be/src/util/memory-metrics.cc
+++ b/be/src/util/memory-metrics.cc
@@ -264,6 +264,10 @@ BufferPoolMetric::BufferPoolMetric(const TMetricDef& def, BufferPoolMetricType t
buffer_pool_(buffer_pool) {}
void BufferPoolMetric::CalculateValue() {
+ // IMPALA-6362: we have to be careful that none of the below calls to ReservationTracker
+ // methods acquire ReservationTracker::lock_ to avoid a potential circular dependency
+ // with MemTracker::child_trackers_lock_, which may be held when refreshing MemTracker
+ // consumption.
switch (type_) {
case BufferPoolMetricType::LIMIT:
value_ = buffer_pool_->GetSystemBytesLimit();
http://git-wip-us.apache.org/repos/asf/impala/blob/d25607d0/be/src/util/memory-metrics.h
----------------------------------------------------------------------
diff --git a/be/src/util/memory-metrics.h b/be/src/util/memory-metrics.h
index 5491f8c..3294c30 100644
--- a/be/src/util/memory-metrics.h
+++ b/be/src/util/memory-metrics.h
@@ -210,6 +210,8 @@ class BufferPoolMetric : public IntGauge {
virtual void CalculateValue();
private:
+ friend class ReservationTrackerTest;
+
enum class BufferPoolMetricType {
LIMIT, // Limit on memory allocated to buffers.
// Total amount of buffer memory allocated from the system. Always <= LIMIT.