From 227eb39b1401a279dca33b0b3b5adad4f0cbb998 Mon Sep 17 00:00:00 2001
From: Nadav Har'El <nyh@cloudius-systems.com>
Date: Tue, 27 Aug 2013 12:50:42 +0300
Subject: [PATCH] Fix deadlock in leak detector
Commit 65afd0754fe419302efca95599b19e53ffca8dbd that fixed mincore()
exposed a deadlock in the leak detector, caused by two threads taking
two locks in opposite order:
Thread 1: malloc() does alloc_tracker::remember(). This takes the tracker
lock and calls backtrace() calling mincore() which takes the
vma_list_mutex.
Thread 2: mmap() does mmu::allocate() which takes the vma_list_mutex and
then through mmu::populate::small_page calls memory::alloc_page() which
calls alloc_tracker::remember() and takes the tracker lock.
This patch fixes this deadlock: alloc_tracker::remember() will now drop its
lock while running backtrace(), as the lock is only needed to protect the
allocations[] array. We need to retake the lock after backtrace() completes,
to copy the backtrace back to the allocations[] array.
Previously, the lock's depth was also (ab)used for avoiding nested
allocation tracking (e.g., tracking of memory allocation done inside
backtrace() itself), but now that backtrace() is run without the lock,
we need a different mechanism - a per-thread "in_tracker" flag, which
is turned on inside the alloc_tracker::remember()/forget() methods.
---
core/alloctracker.cc | 90 ++++++++++++++++++++++-------------------
include/alloctracker.hh | 22 ++++++++++
2 files changed, 70 insertions(+), 42 deletions(-)
diff --git a/core/alloctracker.cc b/core/alloctracker.cc
index 7ff2270b6..b2033f678 100644
--- a/core/alloctracker.cc
+++ b/core/alloctracker.cc
@@ -6,49 +6,49 @@
namespace memory {
+__thread alloc_tracker::in_tracker_t alloc_tracker::in_tracker;
+
void alloc_tracker::remember(void *addr, int size)
{
- std::lock_guard<mutex> guard(lock);
- // If the code in this method causes new allocations, they will in turn
- // cause a nested call to this function. To avoid endless recursion, we
- // do not track these deeper memory allocations. Remember that the
- // purpose of the allocation tracking is finding memory leaks in the
- // application code, not in the code in this file.
- if (lock.getdepth() > 1) {
+ if (in_tracker)
return;
- }
+ std::lock_guard<in_tracker_t> intracker(in_tracker);
- if (!allocations || first_free < 0) {
- // Grow the vector to make room for more allocation records.
- int old_size = size_allocations;
- size_allocations = size_allocations ? 2*size_allocations : 1024;
- struct alloc_info *old_allocations = allocations;
- allocations = (struct alloc_info *)
- malloc(size_allocations * sizeof(struct alloc_info));
- if (old_allocations) {
- memcpy(allocations, old_allocations,
- size_allocations * sizeof(struct alloc_info));
- } else {
- first_free = -1;
- newest_allocation = -1;
- }
- for (size_t i = old_size; i < size_allocations; ++i) {
- allocations[i].addr = 0;
- allocations[i].next = first_free;
- first_free = i;
+ int index;
+
+ WITH_LOCK(lock) {
+ if (!allocations || first_free < 0) {
+ // Grow the vector to make room for more allocation records.
+ int old_size = size_allocations;
+ size_allocations = size_allocations ? 2*size_allocations : 1024;
+ struct alloc_info *old_allocations = allocations;
+ allocations = (struct alloc_info *)
+ malloc(size_allocations * sizeof(struct alloc_info));
+ if (old_allocations) {
+ memcpy(allocations, old_allocations,
+ size_allocations * sizeof(struct alloc_info));
+ } else {
+ first_free = -1;
+ newest_allocation = -1;
+ }
+ for (size_t i = old_size; i < size_allocations; ++i) {
+ allocations[i].addr = 0;
+ allocations[i].next = first_free;
+ first_free = i;
+ }
}
- }
- // Free node available, reuse it
- int index = first_free;
- struct alloc_info *a = &allocations[index];
- first_free = a->next;
- a->next = newest_allocation;
- newest_allocation = index;
+ // Free node available, reuse it
+ index = first_free;
+ struct alloc_info *a = &allocations[index];
+ first_free = a->next;
+ a->next = newest_allocation;
+ newest_allocation = index;
- a->seq = current_seq++;
- a->addr = addr;
- a->size = size;
+ a->seq = current_seq++;
+ a->addr = addr;
+ a->size = size;
+ }
// Do the backtrace. If we ask for only a small number of call levels
// we'll get only the deepest (most recent) levels, but when
@@ -71,9 +71,15 @@ void alloc_tracker::remember(void *addr, int size)
bt_from += n - MAX_BACKTRACE;
n = MAX_BACKTRACE;
}
- a->nbacktrace = n < 0 ? 0 : n;
- for (int i = 0; i < n; i++) {
- a->backtrace[i] = bt_from[i];
+
+ // We need the lock back, to prevent a concurrent allocation from moving
+ // allocations[] while we use it.
+ WITH_LOCK(lock) {
+ struct alloc_info *a = &allocations[index];
+ a->nbacktrace = n < 0 ? 0 : n;
+ for (int i = 0; i < n; i++) {
+ a->backtrace[i] = bt_from[i];
+ }
}
}
@@ -81,10 +87,10 @@ void alloc_tracker::forget(void *addr)
{
if (!addr)
return;
- std::lock_guard<mutex> guard(lock);
- if (lock.getdepth() > 1) {
+ if (in_tracker)
return;
- }
+ std::lock_guard<in_tracker_t> intracker(in_tracker);
+ std::lock_guard<mutex> guard(lock);
if (!allocations) {
return;
}
diff --git a/include/alloctracker.hh b/include/alloctracker.hh
index d4d19b7af..99487003e 100644
--- a/include/alloctracker.hh
+++ b/include/alloctracker.hh
@@ -68,6 +68,28 @@ private:
size_t size_allocations = 0;
unsigned long current_seq = 0;
mutex lock;
+
+ // If the allocation tracking code causes new allocations, they will in
+ // turn cause a nested call to the tracking functions. To avoid endless
+ // recursion, we do not track these deeper memory allocations. Remember
+ // that the purpose of the allocation tracking is finding memory leaks in
+ // the application code, not allocation tracker code.
+ class in_tracker_t {
+ private:
+ bool flag;
+ public:
+ void lock() {
+ flag = true;
+ }
+ void unlock() {
+ flag = false;
+ }
+ operator bool() {
+ return flag;
+ }
+ };
+ static __thread class in_tracker_t in_tracker;
+
};
#endif
--
GitLab