From cec700525cd65a4fa97cd325ffe26b52fce511f6 Mon Sep 17 00:00:00 2001
From: Alexander Alekhin <alexander.alekhin@intel.com>
Date: Tue, 16 Jan 2018 17:33:06 +0300
Subject: [PATCH] core(ocl): fix deadlock in UMatDataAutoLock

UMatData locks are not mapped on real locks (they are mapped to some "pre-initialized" pool).

Concurrent execution of these statements may lead to deadlock:
- a.copyTo(b) from thread 1
- c.copyTo(d) from thread 2
where:
- 'a' and 'd' are mapped to single lock "A".
- 'b' and 'c' are mapped to single lock "B".

Workaround is to process locks with strict order.
---
 modules/core/include/opencv2/core/mat.hpp     |  8 ---
 modules/core/include/opencv2/core/mat.inl.hpp |  3 -
 modules/core/src/ocl.cpp                      |  5 +-
 modules/core/src/umatrix.cpp                  | 57 ++++++++++++++++++-
 modules/core/src/umatrix.hpp                  | 20 +++++++
 5 files changed, 78 insertions(+), 15 deletions(-)
 create mode 100644 modules/core/src/umatrix.hpp

diff --git a/modules/core/include/opencv2/core/mat.hpp b/modules/core/include/opencv2/core/mat.hpp
index 65408d5e7c..7b103d09cc 100644
--- a/modules/core/include/opencv2/core/mat.hpp
+++ b/modules/core/include/opencv2/core/mat.hpp
@@ -545,14 +545,6 @@ struct CV_EXPORTS UMatData
 };
 
 
-struct CV_EXPORTS UMatDataAutoLock
-{
-    explicit UMatDataAutoLock(UMatData* u);
-    ~UMatDataAutoLock();
-    UMatData* u;
-};
-
-
 struct CV_EXPORTS MatSize
 {
     explicit MatSize(int* _p);
diff --git a/modules/core/include/opencv2/core/mat.inl.hpp b/modules/core/include/opencv2/core/mat.inl.hpp
index dbd9584a59..74cee2747c 100644
--- a/modules/core/include/opencv2/core/mat.inl.hpp
+++ b/modules/core/include/opencv2/core/mat.inl.hpp
@@ -3911,9 +3911,6 @@ inline void UMatData::markDeviceCopyObsolete(bool flag)
         flags &= ~DEVICE_COPY_OBSOLETE;
 }
 
-inline UMatDataAutoLock::UMatDataAutoLock(UMatData* _u) : u(_u) { u->lock(); }
-inline UMatDataAutoLock::~UMatDataAutoLock() { u->unlock(); }
-
 //! @endcond
 
 } //cv
diff --git a/modules/core/src/ocl.cpp b/modules/core/src/ocl.cpp
index 3f580f4dfc..a08ed88e74 100644
--- a/modules/core/src/ocl.cpp
+++ b/modules/core/src/ocl.cpp
@@ -123,6 +123,8 @@
 #include "opencv2/core/opencl/opencl_svm.hpp"
 #endif
 
+#include "umatrix.hpp"
+
 namespace cv { namespace ocl {
 
 #define IMPLEMENT_REFCOUNTABLE() \
@@ -5424,8 +5426,7 @@ public:
                                             srcrawofs, new_srcofs, new_srcstep,
                                             dstrawofs, new_dstofs, new_dststep);
 
-        UMatDataAutoLock src_autolock(src);
-        UMatDataAutoLock dst_autolock(dst);
+        UMatDataAutoLock src_autolock(src, dst);
 
         if( !src->handle || (src->data && src->hostCopyObsolete() < src->deviceCopyObsolete()) )
         {
diff --git a/modules/core/src/umatrix.cpp b/modules/core/src/umatrix.cpp
index 14b12585e7..02fc694ab4 100644
--- a/modules/core/src/umatrix.cpp
+++ b/modules/core/src/umatrix.cpp
@@ -41,6 +41,7 @@
 
 #include "precomp.hpp"
 #include "opencl_kernels_core.hpp"
+#include "umatrix.hpp"
 
 ///////////////////////////////// UMat implementation ///////////////////////////////
 
@@ -127,14 +128,66 @@ UMatData::~UMatData()
     }
 }
 
+static size_t getUMatDataLockIndex(const UMatData* u)
+{
+    size_t idx = ((size_t)(void*)u) % UMAT_NLOCKS;
+    return idx;
+}
+
 void UMatData::lock()
 {
-    umatLocks[(size_t)(void*)this % UMAT_NLOCKS].lock();
+    size_t idx = getUMatDataLockIndex(this);
+    //printf("%d lock(%d)\n", cv::utils::getThreadID(), (int)idx);
+    umatLocks[idx].lock();
 }
 
 void UMatData::unlock()
 {
-    umatLocks[(size_t)(void*)this % UMAT_NLOCKS].unlock();
+    size_t idx = getUMatDataLockIndex(this);
+    //printf("%d unlock(%d)\n", cv::utils::getThreadID(), (int)idx);
+    umatLocks[idx].unlock();
+}
+
+
+struct UMatDataAutoLockUsage
+{
+    int count;
+    UMatDataAutoLockUsage() : count(0) { }
+};
+static TLSData<UMatDataAutoLockUsage>& getUMatDataAutoLockUsageTLS()
+{
+    CV_SINGLETON_LAZY_INIT_REF(TLSData<UMatDataAutoLockUsage>, new TLSData<UMatDataAutoLockUsage>());
+}
+static int& getUMatDataAutoLockUsage() { return getUMatDataAutoLockUsageTLS().get()->count; }
+
+
+UMatDataAutoLock::UMatDataAutoLock(UMatData* u) : u1(u), u2(NULL)
+{
+    int& usage_count = getUMatDataAutoLockUsage();
+    CV_Assert(usage_count == 0);  // UMatDataAutoLock can't be used multiple times from the same thread
+    usage_count = 1;
+    u1->lock();
+}
+UMatDataAutoLock::UMatDataAutoLock(UMatData* u1_, UMatData* u2_) : u1(u1_), u2(u2_)
+{
+    int& usage_count = getUMatDataAutoLockUsage();
+    CV_Assert(usage_count == 0);  // UMatDataAutoLock can't be used multiple times from the same thread
+    usage_count = 1;
+    if (getUMatDataLockIndex(u1) > getUMatDataLockIndex(u2))
+    {
+        std::swap(u1, u2);
+    }
+    u1->lock();
+    u2->lock();
+}
+UMatDataAutoLock::~UMatDataAutoLock()
+{
+    int& usage_count = getUMatDataAutoLockUsage();
+    CV_Assert(usage_count == 1);
+    usage_count = 0;
+    u1->unlock();
+    if (u2)
+      u2->unlock();
 }
 
 
diff --git a/modules/core/src/umatrix.hpp b/modules/core/src/umatrix.hpp
new file mode 100644
index 0000000000..f480dc6b33
--- /dev/null
+++ b/modules/core/src/umatrix.hpp
@@ -0,0 +1,20 @@
+// This file is part of OpenCV project.
+// It is subject to the license terms in the LICENSE file found in the top-level directory
+// of this distribution and at http://opencv.org/license.html.
+#ifndef OPENCV_CORE_SRC_UMATRIX_HPP
+#define OPENCV_CORE_SRC_UMATRIX_HPP
+
+namespace cv {
+
+struct CV_EXPORTS UMatDataAutoLock
+{
+    explicit UMatDataAutoLock(UMatData* u);
+    UMatDataAutoLock(UMatData* u1, UMatData* u2);
+    ~UMatDataAutoLock();
+    UMatData* u1;
+    UMatData* u2;
+};
+
+}
+
+#endif // OPENCV_CORE_SRC_UMATRIX_HPP