From 94167132d831e27de519c87ffa6ee5ca7923b7cb Mon Sep 17 00:00:00 2001 From: Umar Arshad Date: Fri, 13 Apr 2018 19:00:37 -0400 Subject: [PATCH] Move nativeFree calls outside of the memory_mutex to avoid deadlock (#2124) Narrows the scope of the memory_mutex locks to just around the modification and reading of the internal memory data members and elements. This means that actual allocations happen outside of this mutex's lock. This was done because the CPU backend requires that the free operation performs a sync before the pointer is freed. This caused deadlocks when the main thread was waiting on the worker thread and the worker thread called nativeFree. This scenario happens when a buffer node is only referenced in the worker thread and it is removed. --- src/backend/common/MemoryManager.hpp | 158 ++++++++++++++------------- 1 file changed, 83 insertions(+), 75 deletions(-) diff --git a/src/backend/common/MemoryManager.hpp b/src/backend/common/MemoryManager.hpp index 711afe3562..b1ae911433 100644 --- a/src/backend/common/MemoryManager.hpp +++ b/src/backend/common/MemoryManager.hpp @@ -9,12 +9,14 @@ #pragma once -#include #include +#include #include #include +#include #include +#include #include #include #include @@ -22,8 +24,8 @@ namespace common { -typedef std::recursive_mutex mutex_t; -typedef std::lock_guard lock_guard_t; +using mutex_t = std::recursive_mutex; +using lock_guard_t = std::lock_guard; const unsigned MAX_BUFFERS = 1000; const size_t ONE_GB = 1 << 30; @@ -38,11 +40,13 @@ class MemoryManager size_t bytes; } locked_info; - using locked_t = typename std::unordered_map; + using locked_t = typename std::unordered_map; using locked_iter = typename locked_t::iterator; - typedef std::unordered_map >free_t; - typedef free_t::iterator free_iter; + using free_t = std::unordered_map >; + using free_iter = free_t::iterator; + + using uptr_t = std::unique_ptr>; typedef struct memory_info { @@ -91,23 +95,33 @@ class MemoryManager { if (this->debug_mode) return; - lock_guard_t lock(this->memory_mutex); + // This vector is used to store the pointers which will be deleted by + // the memory manager. We are using this to avoid calling free while + // the lock is being held becasue the CPU backend calls sync. + std::vector free_ptrs; memory_info& current = memory[device]; - - // Return if all buffers are locked - if (current.total_buffers == current.lock_buffers) return; - - for (auto &kv : current.free_map) { - size_t num_ptrs = kv.second.size(); - //Free memory by popping the last element - for (int n = num_ptrs-1; n >= 0; n--) { - this->nativeFree(kv.second[n]); - current.total_bytes -= kv.first; - current.total_buffers--; - kv.second.pop_back(); + { + lock_guard_t lock(this->memory_mutex); + // Return if all buffers are locked + if (current.total_buffers == current.lock_buffers) return; + free_ptrs.reserve(32); + + for (auto &kv : current.free_map) { + size_t num_ptrs = kv.second.size(); + // Free memory by pushing the last element into the free_ptrs + // vector which will be freed once outside of the lock + for(auto p : kv.second) { + free_ptrs.push_back(p); + } + current.total_bytes -= num_ptrs * kv.first; + current.total_buffers -= num_ptrs; } + current.free_map.clear(); + } + // Free memory outside of the lock + for(auto ptr : free_ptrs) { + this->nativeFree(ptr); } - current.free_map.clear(); } public: @@ -171,13 +185,14 @@ class MemoryManager void *alloc(const size_t bytes, bool user_lock) { - lock_guard_t lock(this->memory_mutex); - - void *ptr = NULL; - size_t alloc_bytes = this->debug_mode ? bytes : (divup(bytes, mem_step_size) * mem_step_size); + void *ptr = nullptr; + size_t alloc_bytes = + this->debug_mode ? bytes : + (divup(bytes, mem_step_size) * mem_step_size); if (bytes > 0) { memory_info& current = this->getCurrentMemoryInfo(); + locked_info info = {!user_lock, user_lock, alloc_bytes}; // There is no memory cache in debug mode if (!this->debug_mode) { @@ -188,36 +203,38 @@ class MemoryManager this->garbageCollect(); } + lock_guard_t lock(this->memory_mutex); free_iter iter = current.free_map.find(alloc_bytes); if (iter != current.free_map.end() && !iter->second.empty()) { ptr = iter->second.back(); iter->second.pop_back(); + current.locked_map[ptr] = info; + current.lock_bytes += alloc_bytes; + current.lock_buffers++; } - } // Only comes here if buffer size not found or in debug mode - if (ptr == NULL) { + if (ptr == nullptr) { // Perform garbage collection if memory can not be allocated try { ptr = this->nativeAlloc(alloc_bytes); - } catch (AfError &ex) { + } catch (const AfError &ex) { // If out of memory, run garbage collect and try again if (ex.getError() != AF_ERR_NO_MEM) throw; this->garbageCollect(); ptr = this->nativeAlloc(alloc_bytes); } + + lock_guard_t lock(this->memory_mutex); // Increment these two only when it succeeds to come here. current.total_bytes += alloc_bytes; current.total_buffers += 1; + current.locked_map[ptr] = info; + current.lock_bytes += alloc_bytes; + current.lock_buffers++; } - - - locked_info info = {!user_lock, user_lock, alloc_bytes}; - current.locked_map[ptr] = info; - current.lock_bytes += alloc_bytes; - current.lock_buffers++; } return ptr; } @@ -236,53 +253,46 @@ class MemoryManager // Shortcut for empty arrays if (!ptr) return; - lock_guard_t lock(this->memory_mutex); - memory_info& current = this->getCurrentMemoryInfo(); + // Frees the pointer outside the lock. + uptr_t freed_ptr(nullptr, [this](void* p) { this->nativeFree(p); }); + { + lock_guard_t lock(this->memory_mutex); + memory_info& current = this->getCurrentMemoryInfo(); - locked_iter iter = current.locked_map.find((void *)ptr); + locked_iter iter = current.locked_map.find((void *)ptr); - // Pointer not found in locked map - if (iter == current.locked_map.end()) { - // Probably came from user, just free it - this->nativeFree(ptr); - return; - } + // Pointer not found in locked map + if (iter == current.locked_map.end()) { + // Probably came from user, just free it + freed_ptr.reset(ptr); + return; + } - if (user_unlock) { - (iter->second).user_lock = false; - } else { - (iter->second).manager_lock = false; - } + if (user_unlock) { + (iter->second).user_lock = false; + } else { + (iter->second).manager_lock = false; + } - // Return early if either one is locked - if ((iter->second).user_lock || (iter->second).manager_lock) return; + // Return early if either one is locked + if ((iter->second).user_lock || (iter->second).manager_lock) return; - size_t bytes = iter->second.bytes; - current.lock_bytes -= iter->second.bytes; - current.lock_buffers--; + size_t bytes = iter->second.bytes; + current.lock_bytes -= iter->second.bytes; + current.lock_buffers--; - if (this->debug_mode) { - // Just free memory in debug mode - if ((iter->second).bytes > 0) { - this->nativeFree(iter->first); - current.total_buffers--; - current.total_bytes -= iter->second.bytes; - } - } else { - // In regular mode, move buffer to free map - free_iter fiter = current.free_map.find(bytes); - if (fiter != current.free_map.end()) { - // If found, push back - fiter->second.push_back(ptr); + if (this->debug_mode) { + // Just free memory in debug mode + if ((iter->second).bytes > 0) { + freed_ptr.reset(iter->first); + current.total_buffers--; + current.total_bytes -= iter->second.bytes; + } } else { - // If not found, create new vector for this size - std::vector ptrs; - ptrs.push_back(ptr); - current.free_map[bytes] = ptrs; + current.free_map[bytes].push_back(ptr); } + current.locked_map.erase(iter); } - - current.locked_map.erase(iter); } void garbageCollect() @@ -290,10 +300,8 @@ class MemoryManager cleanDeviceMemoryManager(this->getActiveDeviceId()); } - void printInfo(const char *msg, const int device) { - lock_guard_t lock(this->memory_mutex); const memory_info& current = this->getCurrentMemoryInfo(); printf("%s\n", msg); @@ -301,6 +309,7 @@ class MemoryManager "| POINTER | SIZE | AF LOCK | USER LOCK |\n" "---------------------------------------------------------\n"); + lock_guard_t lock(this->memory_mutex); for(auto& kv : current.locked_map) { const char* status_mngr = "Yes"; const char* status_user = "Unknown"; @@ -342,8 +351,8 @@ class MemoryManager void bufferInfo(size_t *alloc_bytes, size_t *alloc_buffers, size_t *lock_bytes, size_t *lock_buffers) { - lock_guard_t lock(this->memory_mutex); const memory_info& current = this->getCurrentMemoryInfo(); + lock_guard_t lock(this->memory_mutex); if (alloc_bytes ) *alloc_bytes = current.total_bytes; if (alloc_buffers ) *alloc_buffers = current.total_buffers; if (lock_bytes ) *lock_bytes = current.lock_bytes; @@ -357,7 +366,6 @@ class MemoryManager lock_guard_t lock(this->memory_mutex); locked_iter iter = current.locked_map.find(const_cast(ptr)); - if (iter != current.locked_map.end()) { iter->second.user_lock = true; } else {