Fix bug in VA sharing multithreaded scenario

- createGraphicsAllocationFromSharedHandle was not threadsafe,
instead of reusing BufferObject for a single handle when
multiple threads were creating memory objects from a single VASurface,
new BO could be created and placed in container with BOs for reuse.
This was leading to errors in ioctl calls.

- add lock for following set of operations:
 1. find BufferObject with a given handle in container
 2. create shared BO when not found
 3. add shared BO to container
 prevents creating multiple BOs for a single handle

- replace recursive mutex with regular mutex as mutex shouldn't
be locked recursively

Change-Id: I0937e2abf3bf1c672c6d77422d46e441f7216a68

runtime/os_interface/linux/drm_memory_manager.cpp

@@ -79,8 +79,6 @@ DrmMemoryManager::~DrmMemoryManager() {
 }
 
 void DrmMemoryManager::eraseSharedBufferObject(OCLRT::BufferObject *bo) {
-    std::lock_guard<decltype(mtx)> lock(mtx);
-
     auto it = std::find(sharingBufferObjects.begin(), sharingBufferObjects.end(), bo);
     //If an object isReused = true, it must be in the vector
     DEBUG_BREAK_IF(it == sharingBufferObjects.end());
@@ -88,8 +86,6 @@ void DrmMemoryManager::eraseSharedBufferObject(OCLRT::BufferObject *bo) {
 }
 
 void DrmMemoryManager::pushSharedBufferObject(OCLRT::BufferObject *bo) {
-    std::lock_guard<decltype(mtx)> lock(mtx);
-
     bo->isReused = true;
     sharingBufferObjects.push_back(bo);
 }
@@ -111,6 +107,7 @@ uint32_t DrmMemoryManager::unreference(OCLRT::BufferObject *bo, bool synchronous
         auto allocatorType = bo->peekAllocationType();
 
         if (bo->isReused) {
+            std::lock_guard<decltype(mtx)> lock(mtx);
             eraseSharedBufferObject(bo);
         }
 
@@ -314,8 +311,6 @@ DrmAllocation *DrmMemoryManager::allocate32BitGraphicsMemory(size_t size, const
 
 BufferObject *DrmMemoryManager::findAndReferenceSharedBufferObject(int boHandle) {
     BufferObject *bo = nullptr;
-
-    std::lock_guard<decltype(mtx)> lock(mtx);
     for (const auto &i : sharingBufferObjects) {
         if (i->handle == static_cast<int>(boHandle)) {
             bo = i;
@@ -365,7 +360,10 @@ GraphicsAllocation *DrmMemoryManager::createGraphicsAllocationFromSharedHandle(o
     auto boHandle = openFd.handle;
     BufferObject *bo = nullptr;
 
+    std::unique_lock<std::mutex> lock(mtx, std::defer_lock);
+
     if (reuseBO) {
+        lock.lock();
         bo = findAndReferenceSharedBufferObject(boHandle);
     }
 

runtime/os_interface/linux/drm_memory_manager.h

@@ -94,7 +94,7 @@ class DrmMemoryManager : public MemoryManager {
     decltype(&munmap) munmapFunction = munmap;
     decltype(&close) closeFunction = close;
     std::vector<BufferObject *> sharingBufferObjects;
-    std::recursive_mutex mtx;
+    std::mutex mtx;
     std::unique_ptr<Allocator32bit> internal32bitAllocator;
 };
 } // namespace OCLRT

unit_tests/mocks/CMakeLists.txt

@@ -81,6 +81,10 @@ if(WIN32)
     ${CMAKE_CURRENT_SOURCE_DIR}/mock_gmm_page_table_mngr.cpp
     ${IGDRCL_SRC_tests_mock_wddm}
   )
+else()
+  list(APPEND IGDRCL_SRCS_tests_mocks
+    ${CMAKE_CURRENT_SOURCE_DIR}/linux/mock_drm_memory_manager.h
+  )
 endif()
 
 add_subdirectories()

unit_tests/mocks/linux/mock_drm_memory_manager.h

@@ -0,0 +1,92 @@
+/*
+* Copyright (c) 2018, Intel Corporation
+*
+* Permission is hereby granted, free of charge, to any person obtaining a
+* copy of this software and associated documentation files (the "Software"),
+* to deal in the Software without restriction, including without limitation
+* the rights to use, copy, modify, merge, publish, distribute, sublicense,
+* and/or sell copies of the Software, and to permit persons to whom the
+* Software is furnished to do so, subject to the following conditions:
+*
+* The above copyright notice and this permission notice shall be included
+* in all copies or substantial portions of the Software.
+*
+* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+* OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+* THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
+* OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+* ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+* OTHER DEALINGS IN THE SOFTWARE.
+*/
+
+#include "runtime/os_interface/linux/drm_memory_manager.h"
+
+namespace OCLRT {
+static off_t lseekReturn = 4096u;
+static int lseekCalledCount = 0;
+static int mmapMockCallCount = 0;
+static int munmapMockCallCount = 0;
+
+off_t lseekMock(int fd, off_t offset, int whence) noexcept {
+    lseekCalledCount++;
+    return lseekReturn;
+}
+void *mmapMock(void *addr, size_t length, int prot, int flags,
+               int fd, long offset) noexcept {
+    mmapMockCallCount++;
+    return reinterpret_cast<void *>(0x1000);
+}
+
+int munmapMock(void *addr, size_t length) noexcept {
+    munmapMockCallCount++;
+    return 0;
+}
+
+int closeMock(int) {
+    return 0;
+}
+
+class TestedDrmMemoryManager : public DrmMemoryManager {
+  public:
+    using DrmMemoryManager::allocUserptr;
+    using DrmMemoryManager::setDomainCpu;
+    using DrmMemoryManager::sharingBufferObjects;
+
+    TestedDrmMemoryManager(Drm *drm) : DrmMemoryManager(drm, gemCloseWorkerMode::gemCloseWorkerInactive, false, false) {
+        this->lseekFunction = &lseekMock;
+        this->mmapFunction = &mmapMock;
+        this->munmapFunction = &munmapMock;
+        this->closeFunction = &closeMock;
+        lseekReturn = 4096;
+        lseekCalledCount = 0;
+        mmapMockCallCount = 0;
+        munmapMockCallCount = 0;
+    };
+    TestedDrmMemoryManager(Drm *drm, bool allowForcePin, bool validateHostPtrMemory) : DrmMemoryManager(drm, gemCloseWorkerMode::gemCloseWorkerInactive, allowForcePin, validateHostPtrMemory) {
+        this->lseekFunction = &lseekMock;
+        this->mmapFunction = &mmapMock;
+        this->munmapFunction = &munmapMock;
+        this->closeFunction = &closeMock;
+        lseekReturn = 4096;
+        lseekCalledCount = 0;
+        mmapMockCallCount = 0;
+        munmapMockCallCount = 0;
+    }
+
+    void unreference(BufferObject *bo) {
+        DrmMemoryManager::unreference(bo);
+    }
+
+    void injectPinBB(BufferObject *newPinBB) {
+        BufferObject *currentPinBB = pinBB;
+        pinBB = nullptr;
+        DrmMemoryManager::unreference(currentPinBB);
+        pinBB = newPinBB;
+    }
+
+    DrmGemCloseWorker *getgemCloseWorker() { return this->gemCloseWorker.get(); }
+
+    Allocator32bit *getDrmInternal32BitAllocator() { return internal32bitAllocator.get(); }
+};
+} // namespace OCLRT

unit_tests/mt_tests/os_interface/CMakeLists.txt

@@ -0,0 +1,25 @@
+# Copyright (c) 2018, Intel Corporation
+#
+# Permission is hereby granted, free of charge, to any person obtaining a
+# copy of this software and associated documentation files (the "Software"),
+# to deal in the Software without restriction, including without limitation
+# the rights to use, copy, modify, merge, publish, distribute, sublicense,
+# and/or sell copies of the Software, and to permit persons to whom the
+# Software is furnished to do so, subject to the following conditions:
+#
+# The above copyright notice and this permission notice shall be included
+# in all copies or substantial portions of the Software.
+#
+# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+# OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+# THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
+# OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+# ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+# OTHER DEALINGS IN THE SOFTWARE.
+
+if(UNIX)
+  target_sources(igdrcl_mt_tests PRIVATE
+    ${CMAKE_CURRENT_SOURCE_DIR}/linux/drm_memory_manager_mt_tests.cpp
+  )
+endif()

unit_tests/mt_tests/os_interface/linux/drm_memory_manager_mt_tests.cpp

@@ -0,0 +1,77 @@
+/*
+* Copyright (c) 2018, Intel Corporation
+*
+* Permission is hereby granted, free of charge, to any person obtaining a
+* copy of this software and associated documentation files (the "Software"),
+* to deal in the Software without restriction, including without limitation
+* the rights to use, copy, modify, merge, publish, distribute, sublicense,
+* and/or sell copies of the Software, and to permit persons to whom the
+* Software is furnished to do so, subject to the following conditions:
+*
+* The above copyright notice and this permission notice shall be included
+* in all copies or substantial portions of the Software.
+*
+* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+* OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+* THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
+* OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+* ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+* OTHER DEALINGS IN THE SOFTWARE.
+*/
+
+#include "runtime/os_interface/linux/drm_memory_manager.h"
+#include "unit_tests/mocks/linux/mock_drm_memory_manager.h"
+#include "unit_tests/os_interface/linux/device_command_stream_fixture.h"
+#include "gtest/gtest.h"
+
+#include <atomic>
+#include <memory>
+#include <thread>
+
+using namespace OCLRT;
+using namespace std;
+
+TEST(DrmMemoryManagerTest, givenDrmMemoryManagerWhenSharedAllocationIsCreatedFromMultipleThreadsThenSingleBoIsReused) {
+    class MockDrm : public Drm {
+      public:
+        MockDrm(int fd) : Drm(fd) {}
+
+        int ioctl(unsigned long request, void *arg) override {
+            if (request == DRM_IOCTL_PRIME_FD_TO_HANDLE) {
+                auto *primeToHandleParams = (drm_prime_handle *)arg;
+                primeToHandleParams->handle = 10;
+            }
+            return 0;
+        }
+    };
+
+    auto mock = make_unique<MockDrm>(0);
+    auto memoryManager = make_unique<TestedDrmMemoryManager>(mock.get());
+
+    osHandle handle = 3;
+    constexpr size_t maxThreads = 10;
+
+    GraphicsAllocation *createdAllocations[maxThreads];
+    thread threads[maxThreads];
+    atomic<size_t> index(0);
+
+    auto createFunction = [&]() {
+        size_t indexFree = index++;
+        createdAllocations[indexFree] = memoryManager->createGraphicsAllocationFromSharedHandle(handle, false, true);
+        EXPECT_NE(nullptr, createdAllocations[indexFree]);
+    };
+
+    for (size_t i = 0; i < maxThreads; i++) {
+        threads[i] = std::thread(createFunction);
+    }
+
+    while (index < maxThreads) {
+        EXPECT_GE(1u, memoryManager->sharingBufferObjects.size());
+    }
+
+    for (size_t i = 0; i < maxThreads; i++) {
+        threads[i].join();
+        memoryManager->freeGraphicsMemory(createdAllocations[i]);
+    }
+}

unit_tests/os_interface/linux/drm_memory_manager_tests.cpp

@@ -20,9 +20,9 @@
  * OTHER DEALINGS IN THE SOFTWARE.
  */
 
+#include "hw_cmds.h"
 #include "runtime/command_stream/linear_stream.h"
 #include "runtime/command_stream/device_command_stream.h"
-#include "hw_cmds.h"
 #include "runtime/event/event.h"
 #include "runtime/helpers/aligned_memory.h"
 #include "runtime/helpers/ptr_math.h"
@@ -33,92 +33,29 @@
 #include "runtime/os_interface/linux/drm_buffer_object.h"
 #include "runtime/os_interface/linux/drm_command_stream.h"
 #include "runtime/os_interface/linux/drm_memory_manager.h"
+#include "runtime/os_interface/32bit_memory.h"
 #include "runtime/utilities/tag_allocator.h"
-#include "runtime/mem_obj/buffer.h"
-#include "unit_tests/mocks/mock_context.h"
+
 #include "unit_tests/fixtures/memory_management_fixture.h"
 #include "unit_tests/helpers/debug_manager_state_restore.h"
 #include "unit_tests/helpers/memory_management.h"
-#include "test.h"
-#include "unit_tests/os_interface/linux/device_command_stream_fixture.h"
-#include "runtime/os_interface/32bit_memory.h"
 #include "unit_tests/mocks/mock_32bitAllocator.h"
+#include "unit_tests/mocks/mock_context.h"
 #include "unit_tests/mocks/mock_device.h"
 #include "unit_tests/mocks/mock_gmm.h"
+#include "unit_tests/mocks/linux/mock_drm_memory_manager.h"
+#include "unit_tests/os_interface/linux/device_command_stream_fixture.h"
+
+#include "test.h"
 #include "drm/i915_drm.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+
 #include <iostream>
 #include <memory>
 
 using namespace OCLRT;
 
-static off_t lseekReturn = 4096u;
-static int lseekCalledCount = 0;
-static int mmapMockCallCount = 0;
-static int munmapMockCallCount = 0;
-
-off_t lseekMock(int fd, off_t offset, int whence) noexcept {
-    lseekCalledCount++;
-    return lseekReturn;
-}
-void *mmapMock(void *addr, size_t length, int prot, int flags,
-               int fd, long offset) noexcept {
-    mmapMockCallCount++;
-    return reinterpret_cast<void *>(0x1000);
-}
-
-int munmapMock(void *addr, size_t length) noexcept {
-    munmapMockCallCount++;
-    return 0;
-}
-
-int closeMock(int) {
-    return 0;
-}
-
-class TestedDrmMemoryManager : public DrmMemoryManager {
-  public:
-    using DrmMemoryManager::allocUserptr;
-    using DrmMemoryManager::setDomainCpu;
-
-    TestedDrmMemoryManager(Drm *drm) : DrmMemoryManager(drm, gemCloseWorkerMode::gemCloseWorkerInactive, false, false) {
-        this->lseekFunction = &lseekMock;
-        this->mmapFunction = &mmapMock;
-        this->munmapFunction = &munmapMock;
-        this->closeFunction = &closeMock;
-        lseekReturn = 4096;
-        lseekCalledCount = 0;
-        mmapMockCallCount = 0;
-        munmapMockCallCount = 0;
-    };
-    TestedDrmMemoryManager(Drm *drm, bool allowForcePin, bool validateHostPtrMemory) : DrmMemoryManager(drm, gemCloseWorkerMode::gemCloseWorkerInactive, allowForcePin, validateHostPtrMemory) {
-        this->lseekFunction = &lseekMock;
-        this->mmapFunction = &mmapMock;
-        this->munmapFunction = &munmapMock;
-        this->closeFunction = &closeMock;
-        lseekReturn = 4096;
-        lseekCalledCount = 0;
-        mmapMockCallCount = 0;
-        munmapMockCallCount = 0;
-    }
-
-    void unreference(BufferObject *bo) {
-        DrmMemoryManager::unreference(bo);
-    }
-
-    void injectPinBB(BufferObject *newPinBB) {
-        BufferObject *currentPinBB = pinBB;
-        pinBB = nullptr;
-        DrmMemoryManager::unreference(currentPinBB);
-        pinBB = newPinBB;
-    }
-
-    DrmGemCloseWorker *getgemCloseWorker() { return this->gemCloseWorker.get(); }
-
-    Allocator32bit *getDrmInternal32BitAllocator() { return internal32bitAllocator.get(); }
-};
-
 class DrmMemoryManagerFixture : public MemoryManagementFixture {
   public:
     TestedDrmMemoryManager *memoryManager = nullptr;