New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Move nativeFree calls outside of the memory_mutex to avoid deadlock #2124

Merged

umar456 merged 1 commit into arrayfire:master from umar456:sync_without_lock

Apr 13, 2018

Member

umar456 commented Apr 12, 2018

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.

Fixes #2115

umar456 added this to the v3.6.0 milestone

umar456 requested a review from pavanky

April 12, 2018 06:46

umar456 force-pushed the sync_without_lock branch from 34259d2 to 0f95fc4 Compare

April 12, 2018 06:47

umar456 added the backport label

FloopCZ reviewed

View reviewed changes

Contributor

FloopCZ left a comment •

edited

Loading

Thank you for the code.

This is a little bit off-topic, sorry, but I have a design pattern question. Backend-specific MemoryManagers inherit from the common MemoryManager<T> and pass it their own type as the template parameter. This is done because you prefer to call backend-specific methods like nativeFree using static_cast:

inline void nativeFree(void *ptr)
{
    static_cast<T*>(this)->nativeFree(ptr);
}

instead of using a virtual nativeFree function.

I thought you wanted to avoid virtual table lookup for performance (I may be wrong, though, considering that in this PR you e.g., store the call to nativeFree in std::function that is performing a similar type of type erasure anyway.). However, the common MemoryManager has a virtual destructor, is there a reason for this? Because you cannot use it purely polymorphically anyway, it always has to be parametrized using the backend-specific manager type.

I am asking mostly out of curiosity.

src/backend/common/MemoryManager.hpp Outdated

+                          for (auto &kv : current.free_map) {
+                              size_t num_ptrs = kv.second.size();
+                              //Free memory by popping the last element

Contributor

FloopCZ Apr 12, 2018

This comment is not true anymore.

Member Author

umar456 Apr 13, 2018

Addressed

src/backend/common/MemoryManager.hpp Outdated

+                              for(auto& p : kv.second) {
+                                  free_ptrs.push_back(p);
+                              }
+                              current.total_bytes -=  num_ptrs * kv.first;

Contributor

FloopCZ Apr 12, 2018

(OCD - two spaces after -=)

Member Author

umar456 Apr 13, 2018

Addressed

src/backend/common/MemoryManager.hpp Outdated

+                      // Frees the pointer outside the lock.
+                      std::unique_ptr<void, std::function<void(void*)>> freed_ptr(nullptr, [this](void* p) {
+                                                        this->nativeFree(p);
+                                                      });

Contributor

FloopCZ Apr 12, 2018

(OCD - this lambda seems misindented)

Member Author

umar456 Apr 13, 2018

Addressed

src/backend/common/MemoryManager.hpp

+                          // Pointer not found in locked map
+                          if (iter == current.locked_map.end()) {
+                              // Probably came from user, just free it
+                              freed_ptr.reset(ptr);

Contributor

FloopCZ Apr 12, 2018

This is a nice trick with unique_ptr. 👍

Member

pavanky Apr 12, 2018

Agreed 👍

src/backend/common/MemoryManager.hpp Outdated

+                                  std::vector<void *> ptrs;
+                                  ptrs.push_back(ptr);
+                                  current.free_map[bytes] = ptrs;
+                              }

Contributor

FloopCZ Apr 12, 2018 •

edited

Loading

I believe this whole "regular mode" branch (L291-L301) can be replaced with this line:

current.free_map[bytes].push_back(ptr);

because operator[] on mapping containers constructs the empty vector (default constructor) for you if the key does not exist.

Member

pavanky Apr 12, 2018

I agree with this ^ I was not aware of this quirk of C++ standard containers when I wrote this code.

Member Author

umar456 Apr 13, 2018

Done

umar456 force-pushed the sync_without_lock branch from 0f95fc4 to d04d0e0 Compare

April 12, 2018 15:49

pavanky reviewed

View reviewed changes

src/backend/common/MemoryManager.hpp Outdated

+                      unique_ptr<void, function<void(void*)>> freed_ptr(nullptr,
+                                                                        [this](void* p) {
+                                                                            this->nativeFree(p);
+                                                                        });

Member

pavanky Apr 12, 2018

Move this into the class definition of MemoryManager ? It could be useful elsewhere.

Member Author

umar456 Apr 13, 2018

Done

src/backend/common/MemoryManager.hpp

+                          // Pointer not found in locked map
+                          if (iter == current.locked_map.end()) {
+                              // Probably came from user, just free it
+                              freed_ptr.reset(ptr);

Member

pavanky Apr 12, 2018

Agreed 👍

src/backend/common/MemoryManager.hpp Outdated

+                                  std::vector<void *> ptrs;
+                                  ptrs.push_back(ptr);
+                                  current.free_map[bytes] = ptrs;
+                              }

Member

pavanky Apr 12, 2018

I agree with this ^ I was not aware of this quirk of C++ standard containers when I wrote this code.

src/backend/common/MemoryManager.hpp Outdated

@@ @@ -424,6 +445,7 @@ class MemoryManager @@
                   bool checkMemoryLimit()
                   {
                       const memory_info& current = this->getCurrentMemoryInfo();
+                      lock_guard_t lock(this->memory_mutex);

Member

pavanky Apr 12, 2018

Why lock here ? Was getCurrentMemoryInfo locking previously ?

Member Author

umar456 Apr 13, 2018

It wasn't locking but you are accessing the memory_info struct and some of the tools were throwing warnings for these accesses. I can remove it if you think its unnecessary but I don't think it hurts.

Member

pavanky commented Apr 12, 2018

I thought you wanted to avoid virtual table lookup for performance (I may be wrong, though, considering that in this PR you e.g., store the call to nativeFree in std::function that is performing a similar type of type erasure anyway.). However, the common MemoryManager has a virtual destructor, is there a reason for this? Because you cannot use it purely polymorphically anyway, it always has to be parametrized using the backend-specific manager type.

This is a result of MemoryManager starting off as a polymorphic base class and then being converted to the templated version because we could not guarantee the order of destruction of certain objects which resulted in sgefaults on exits (I don't recall the specifics).

I guess the virtual destructor is something that was missed when the conversion was done.

umar456 force-pushed the sync_without_lock branch 2 times, most recently from 1810a93 to 031b3e2 Compare

April 13, 2018 04:45

Member

9prady9 commented Apr 13, 2018

@FloopCZ Yes, it was a miss as @pavanky pointed out and I missed removing that. virtual qualifier is not needed there and in fact we can just remove that destructor and let the compiler handle it since it doesn't anything specific over there. @umar456 Can you please remove it.

Contributor

FloopCZ commented Apr 13, 2018

@pavanky @9prady9 Thank you for your answers and @umar456 for the code fixing this tricky bug.

pavanky previously approved these changes

View reviewed changes


          Move nativeFree calls outside of the memory_mutex to avoid deadlock

6105b16

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.

umar456 dismissed pavanky’s stale review via

6105b16

April 13, 2018 17:45

umar456 force-pushed the sync_without_lock branch from 031b3e2 to 6105b16 Compare

April 13, 2018 17:45

pavanky approved these changes

View reviewed changes

umar456 merged commit 1dfbbe3 into arrayfire:master

umar456 deleted the sync_without_lock branch

May 13, 2018 09:36

syurkevi pushed a commit to syurkevi/arrayfire that referenced this pull request


          Move nativeFree calls outside of the memory_mutex to avoid deadlock (a…

…rrayfire#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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment