From a010fb36346fdeaf3e2492171dd7c5c8969ea4fa Mon Sep 17 00:00:00 2001 From: Jaime Arteaga Date: Wed, 2 Mar 2022 03:43:59 +0000 Subject: [PATCH] Dont close shared handle on imported allocations Related-To: LOCI-2272 Signed-off-by: Jaime Arteaga --- .../core/source/driver/driver_handle_imp.cpp | 1 + .../unit_tests/fixtures/memory_ipc_fixture.h | 1 + .../unit_tests/sources/event/test_event.cpp | 1 + .../linux/drm_memory_manager_tests.cpp | 30 +++++++++++++++++++ .../windows/wddm_memory_manager_tests.cpp | 16 ++++++++++ .../source/memory_manager/memory_manager.cpp | 6 +++- shared/source/memory_manager/memory_manager.h | 2 ++ .../os_agnostic_memory_manager.cpp | 4 +++ .../os_agnostic_memory_manager.h | 1 + .../memory_manager/unified_memory_manager.cpp | 5 ++-- .../memory_manager/unified_memory_manager.h | 2 ++ .../os_interface/linux/drm_memory_manager.cpp | 8 ++++- .../os_interface/linux/drm_memory_manager.h | 1 + .../windows/wddm_memory_manager.cpp | 4 +++ .../windows/wddm_memory_manager.h | 1 + .../mocks/linux/mock_drm_memory_manager.cpp | 7 +++++ .../mocks/linux/mock_drm_memory_manager.h | 3 ++ .../windows/mock_wddm_memory_manager.h | 4 +++ .../unit_test/device/neo_device_tests.cpp | 1 + 19 files changed, 94 insertions(+), 4 deletions(-) diff --git a/level_zero/core/source/driver/driver_handle_imp.cpp b/level_zero/core/source/driver/driver_handle_imp.cpp index 3ea26a8569..3e5b4edabc 100644 --- a/level_zero/core/source/driver/driver_handle_imp.cpp +++ b/level_zero/core/source/driver/driver_handle_imp.cpp @@ -439,6 +439,7 @@ void *DriverHandleImp::importFdHandle(ze_device_handle_t hDevice, ze_ipc_memory_ allocData.size = alloc->getUnderlyingBufferSize(); allocData.memoryType = InternalMemoryType::DEVICE_UNIFIED_MEMORY; allocData.device = neoDevice; + allocData.isImportedAllocation = true; if (flags & ZE_DEVICE_MEM_ALLOC_FLAG_BIAS_UNCACHED) { allocData.allocationFlagsProperty.flags.locallyUncachedResource = 1; } diff --git a/level_zero/core/test/unit_tests/fixtures/memory_ipc_fixture.h b/level_zero/core/test/unit_tests/fixtures/memory_ipc_fixture.h index 28181df79d..2b9d521cb0 100644 --- a/level_zero/core/test/unit_tests/fixtures/memory_ipc_fixture.h +++ b/level_zero/core/test/unit_tests/fixtures/memory_ipc_fixture.h @@ -335,6 +335,7 @@ class MemoryManagerIpcMock : public NEO::MemoryManager { AllocationStatus populateOsHandles(NEO::OsHandleStorage &handleStorage, uint32_t rootDeviceIndex) override { return AllocationStatus::Success; }; void cleanOsHandles(NEO::OsHandleStorage &handleStorage, uint32_t rootDeviceIndex) override{}; void freeGraphicsMemoryImpl(NEO::GraphicsAllocation *gfxAllocation) override{}; + void freeGraphicsMemoryImpl(GraphicsAllocation *gfxAllocation, bool isImportedAllocation) override{}; uint64_t getSystemSharedMemory(uint32_t rootDeviceIndex) override { return 0; }; uint64_t getLocalMemorySize(uint32_t rootDeviceIndex, uint32_t deviceBitfield) override { return 0; }; double getPercentOfGlobalMemoryAvailable(uint32_t rootDeviceIndex) override { return 0; } diff --git a/level_zero/core/test/unit_tests/sources/event/test_event.cpp b/level_zero/core/test/unit_tests/sources/event/test_event.cpp index 1d38e38ed0..02b06f130b 100644 --- a/level_zero/core/test/unit_tests/sources/event/test_event.cpp +++ b/level_zero/core/test/unit_tests/sources/event/test_event.cpp @@ -51,6 +51,7 @@ class MemoryManagerEventPoolFailMock : public NEO::MemoryManager { AllocationStatus populateOsHandles(NEO::OsHandleStorage &handleStorage, uint32_t rootDeviceIndex) override { return AllocationStatus::Success; }; void cleanOsHandles(NEO::OsHandleStorage &handleStorage, uint32_t rootDeviceIndex) override{}; void freeGraphicsMemoryImpl(NEO::GraphicsAllocation *gfxAllocation) override{}; + void freeGraphicsMemoryImpl(GraphicsAllocation *gfxAllocation, bool isImportedAllocation) override{}; uint64_t getSystemSharedMemory(uint32_t rootDeviceIndex) override { return 0; }; uint64_t getLocalMemorySize(uint32_t rootDeviceIndex, uint32_t deviceBitfield) override { return 0; }; double getPercentOfGlobalMemoryAvailable(uint32_t rootDeviceIndex) override { return 0; } diff --git a/opencl/test/unit_test/os_interface/linux/drm_memory_manager_tests.cpp b/opencl/test/unit_test/os_interface/linux/drm_memory_manager_tests.cpp index 5f8af25447..114091a6f9 100644 --- a/opencl/test/unit_test/os_interface/linux/drm_memory_manager_tests.cpp +++ b/opencl/test/unit_test/os_interface/linux/drm_memory_manager_tests.cpp @@ -4891,6 +4891,36 @@ TEST_F(DrmMemoryManagerTest, givenDrmMemoryManagerWhenCopyMemoryToAllocationThen memoryManager->freeGraphicsMemory(allocation); } +TEST_F(DrmMemoryManagerTest, givenDrmMemoryManagerWhenFreeingImportedMemoryThenCloseSharedHandleIsNotCalled) { + mock->ioctl_expected.gemUserptr = 1; + mock->ioctl_expected.gemWait = 1; + mock->ioctl_expected.gemClose = 1; + + std::vector dataToCopy(MemoryConstants::pageSize, 1u); + + auto allocation = memoryManager->allocateGraphicsMemoryWithProperties({rootDeviceIndex, dataToCopy.size(), AllocationType::BUFFER, device->getDeviceBitfield()}); + ASSERT_NE(nullptr, allocation); + + memoryManager->freeGraphicsMemory(allocation, true); + + EXPECT_EQ(memoryManager->callsToCloseSharedHandle, 0u); +} + +TEST_F(DrmMemoryManagerTest, givenDrmMemoryManagerWhenFreeingNonImportedMemoryThenCloseSharedHandleIsCalled) { + mock->ioctl_expected.gemUserptr = 1; + mock->ioctl_expected.gemWait = 1; + mock->ioctl_expected.gemClose = 1; + + std::vector dataToCopy(MemoryConstants::pageSize, 1u); + + auto allocation = memoryManager->allocateGraphicsMemoryWithProperties({rootDeviceIndex, dataToCopy.size(), AllocationType::BUFFER, device->getDeviceBitfield()}); + ASSERT_NE(nullptr, allocation); + + memoryManager->freeGraphicsMemory(allocation); + + EXPECT_EQ(memoryManager->callsToCloseSharedHandle, 1u); +} + TEST_F(DrmMemoryManagerTest, givenDrmMemoryManagerWhenGetLocalMemoryIsCalledThenSizeOfLocalMemoryIsReturned) { EXPECT_EQ(0 * GB, memoryManager->getLocalMemorySize(rootDeviceIndex, 0xF)); } diff --git a/opencl/test/unit_test/os_interface/windows/wddm_memory_manager_tests.cpp b/opencl/test/unit_test/os_interface/windows/wddm_memory_manager_tests.cpp index 194cd331a8..ab3325a114 100644 --- a/opencl/test/unit_test/os_interface/windows/wddm_memory_manager_tests.cpp +++ b/opencl/test/unit_test/os_interface/windows/wddm_memory_manager_tests.cpp @@ -383,6 +383,22 @@ TEST_F(WddmMemoryManagerSimpleTest, givenAllocateGraphicsMemoryForNonSvmHostPtrI memoryManager->freeGraphicsMemory(allocation); } +TEST_F(WddmMemoryManagerSimpleTest, givenAllocateGraphicsMemoryForNonSvmHostPtrIsCalledWhenNotAlignedPtrIsPassedAndImportedAllocationIsFalseThenAlignedGraphicsAllocationIsFreed) { + memoryManager.reset(new MockWddmMemoryManager(false, false, *executionEnvironment)); + auto size = 13u; + auto hostPtr = reinterpret_cast(0x10001); + + AllocationData allocationData; + allocationData.size = size; + allocationData.hostPtr = hostPtr; + auto allocation = memoryManager->allocateGraphicsMemoryForNonSvmHostPtr(allocationData); + EXPECT_NE(nullptr, allocation); + EXPECT_EQ(hostPtr, allocation->getUnderlyingBuffer()); + EXPECT_EQ(size, allocation->getUnderlyingBufferSize()); + EXPECT_EQ(1u, allocation->getAllocationOffset()); + memoryManager->freeGraphicsMemoryImpl(allocation, false); +} + TEST_F(WddmMemoryManagerTest, givenAllocateGraphicsMemoryForNonSvmHostPtrIsCalledWhencreateWddmAllocationFailsThenGraphicsAllocationIsNotCreated) { char hostPtr[64]; memoryManager->setDeferredDeleter(nullptr); diff --git a/shared/source/memory_manager/memory_manager.cpp b/shared/source/memory_manager/memory_manager.cpp index 5ffc8488c1..d5f4ca72ec 100644 --- a/shared/source/memory_manager/memory_manager.cpp +++ b/shared/source/memory_manager/memory_manager.cpp @@ -195,6 +195,10 @@ void MemoryManager::freeSystemMemory(void *ptr) { } void MemoryManager::freeGraphicsMemory(GraphicsAllocation *gfxAllocation) { + return freeGraphicsMemory(gfxAllocation, false); +} + +void MemoryManager::freeGraphicsMemory(GraphicsAllocation *gfxAllocation, bool isImportedAllocation) { if (!gfxAllocation) { return; } @@ -213,7 +217,7 @@ void MemoryManager::freeGraphicsMemory(GraphicsAllocation *gfxAllocation) { } getLocalMemoryUsageBankSelector(gfxAllocation->getAllocationType(), gfxAllocation->getRootDeviceIndex())->freeOnBanks(gfxAllocation->storageInfo.getMemoryBanks(), gfxAllocation->getUnderlyingBufferSize()); - freeGraphicsMemoryImpl(gfxAllocation); + freeGraphicsMemoryImpl(gfxAllocation, isImportedAllocation); } //if not in use destroy in place //if in use pass to temporary allocation list that is cleaned on blocking calls diff --git a/shared/source/memory_manager/memory_manager.h b/shared/source/memory_manager/memory_manager.h index a096652b25..c75450322d 100644 --- a/shared/source/memory_manager/memory_manager.h +++ b/shared/source/memory_manager/memory_manager.h @@ -128,7 +128,9 @@ class MemoryManager { void freeSystemMemory(void *ptr); virtual void freeGraphicsMemoryImpl(GraphicsAllocation *gfxAllocation) = 0; + virtual void freeGraphicsMemoryImpl(GraphicsAllocation *gfxAllocation, bool isImportedAllocation) = 0; MOCKABLE_VIRTUAL void freeGraphicsMemory(GraphicsAllocation *gfxAllocation); + MOCKABLE_VIRTUAL void freeGraphicsMemory(GraphicsAllocation *gfxAllocation, bool isImportedAllocation); virtual void handleFenceCompletion(GraphicsAllocation *allocation){}; void checkGpuUsageAndDestroyGraphicsAllocations(GraphicsAllocation *gfxAllocation); diff --git a/shared/source/memory_manager/os_agnostic_memory_manager.cpp b/shared/source/memory_manager/os_agnostic_memory_manager.cpp index 8af72d058f..f0f9851cb7 100644 --- a/shared/source/memory_manager/os_agnostic_memory_manager.cpp +++ b/shared/source/memory_manager/os_agnostic_memory_manager.cpp @@ -260,6 +260,10 @@ void OsAgnosticMemoryManager::removeAllocationFromHostPtrManager(GraphicsAllocat } } +void OsAgnosticMemoryManager::freeGraphicsMemoryImpl(GraphicsAllocation *gfxAllocation, bool isImportedAllocation) { + return freeGraphicsMemoryImpl(gfxAllocation); +} + void OsAgnosticMemoryManager::freeGraphicsMemoryImpl(GraphicsAllocation *gfxAllocation) { for (auto handleId = 0u; handleId < gfxAllocation->getNumGmms(); handleId++) { delete gfxAllocation->getGmm(handleId); diff --git a/shared/source/memory_manager/os_agnostic_memory_manager.h b/shared/source/memory_manager/os_agnostic_memory_manager.h index 7319a6ef22..ae991754de 100644 --- a/shared/source/memory_manager/os_agnostic_memory_manager.h +++ b/shared/source/memory_manager/os_agnostic_memory_manager.h @@ -75,6 +75,7 @@ class OsAgnosticMemoryManager : public MemoryManager { void addAllocationToHostPtrManager(GraphicsAllocation *gfxAllocation) override; void removeAllocationFromHostPtrManager(GraphicsAllocation *gfxAllocation) override; void freeGraphicsMemoryImpl(GraphicsAllocation *gfxAllocation) override; + void freeGraphicsMemoryImpl(GraphicsAllocation *gfxAllocation, bool isImportedAllocation) override; AllocationStatus populateOsHandles(OsHandleStorage &handleStorage, uint32_t rootDeviceIndex) override; void cleanOsHandles(OsHandleStorage &handleStorage, uint32_t rootDeviceIndex) override; diff --git a/shared/source/memory_manager/unified_memory_manager.cpp b/shared/source/memory_manager/unified_memory_manager.cpp index 8825b0b4bf..8546e2a615 100644 --- a/shared/source/memory_manager/unified_memory_manager.cpp +++ b/shared/source/memory_manager/unified_memory_manager.cpp @@ -477,12 +477,13 @@ void SVMAllocsManager::freeZeroCopySvmAllocation(SvmAllocationData *svmData) { void SVMAllocsManager::freeSvmAllocationWithDeviceStorage(SvmAllocationData *svmData) { auto graphicsAllocations = svmData->gpuAllocations.getGraphicsAllocations(); GraphicsAllocation *cpuAllocation = svmData->cpuAllocation; + bool isImportedAllocation = svmData->isImportedAllocation; SVMAllocs.remove(*svmData); for (auto gpuAllocation : graphicsAllocations) { - memoryManager->freeGraphicsMemory(gpuAllocation); + memoryManager->freeGraphicsMemory(gpuAllocation, isImportedAllocation); } - memoryManager->freeGraphicsMemory(cpuAllocation); + memoryManager->freeGraphicsMemory(cpuAllocation, isImportedAllocation); } bool SVMAllocsManager::hasHostAllocations() { diff --git a/shared/source/memory_manager/unified_memory_manager.h b/shared/source/memory_manager/unified_memory_manager.h index 97fccf4321..2a6284a38d 100644 --- a/shared/source/memory_manager/unified_memory_manager.h +++ b/shared/source/memory_manager/unified_memory_manager.h @@ -36,6 +36,7 @@ struct SvmAllocationData { this->memoryType = svmAllocData.memoryType; this->allocId = svmAllocData.allocId; this->pageSizeForAlignment = svmAllocData.pageSizeForAlignment; + this->isImportedAllocation = svmAllocData.isImportedAllocation; for (auto allocation : svmAllocData.gpuAllocations.getGraphicsAllocations()) { if (allocation) { this->gpuAllocations.addAllocation(allocation); @@ -50,6 +51,7 @@ struct SvmAllocationData { InternalMemoryType memoryType = InternalMemoryType::SVM; MemoryProperties allocationFlagsProperty; Device *device = nullptr; + bool isImportedAllocation = false; void setAllocId(uint32_t id) { allocId = id; } diff --git a/shared/source/os_interface/linux/drm_memory_manager.cpp b/shared/source/os_interface/linux/drm_memory_manager.cpp index 34ee9d48fa..962dc29963 100644 --- a/shared/source/os_interface/linux/drm_memory_manager.cpp +++ b/shared/source/os_interface/linux/drm_memory_manager.cpp @@ -774,6 +774,10 @@ void DrmMemoryManager::removeAllocationFromHostPtrManager(GraphicsAllocation *gf } void DrmMemoryManager::freeGraphicsMemoryImpl(GraphicsAllocation *gfxAllocation) { + return freeGraphicsMemoryImpl(gfxAllocation, false); +} + +void DrmMemoryManager::freeGraphicsMemoryImpl(GraphicsAllocation *gfxAllocation, bool isImported) { if (DebugManager.flags.DoNotFreeResources.get()) { return; } @@ -800,7 +804,9 @@ void DrmMemoryManager::freeGraphicsMemoryImpl(GraphicsAllocation *gfxAllocation) for (auto bo : bos) { unreference(bo, bo && bo->peekIsReusableAllocation() ? false : true); } - closeSharedHandle(gfxAllocation); + if (isImported == false) { + closeSharedHandle(gfxAllocation); + } } releaseGpuRange(gfxAllocation->getReservedAddressPtr(), gfxAllocation->getReservedAddressSize(), gfxAllocation->getRootDeviceIndex()); diff --git a/shared/source/os_interface/linux/drm_memory_manager.h b/shared/source/os_interface/linux/drm_memory_manager.h index 64e07b97ee..6af62170b6 100644 --- a/shared/source/os_interface/linux/drm_memory_manager.h +++ b/shared/source/os_interface/linux/drm_memory_manager.h @@ -33,6 +33,7 @@ class DrmMemoryManager : public MemoryManager { void addAllocationToHostPtrManager(GraphicsAllocation *gfxAllocation) override; void removeAllocationFromHostPtrManager(GraphicsAllocation *gfxAllocation) override; void freeGraphicsMemoryImpl(GraphicsAllocation *gfxAllocation) override; + void freeGraphicsMemoryImpl(GraphicsAllocation *gfxAllocation, bool isImportedAllocation) override; void handleFenceCompletion(GraphicsAllocation *allocation) override; GraphicsAllocation *createGraphicsAllocationFromExistingStorage(AllocationProperties &properties, void *ptr, MultiGraphicsAllocation &multiGraphicsAllocation) override; GraphicsAllocation *createGraphicsAllocationFromSharedHandle(osHandle handle, const AllocationProperties &properties, bool requireSpecificBitness, bool isHostIpcAllocation) override; diff --git a/shared/source/os_interface/windows/wddm_memory_manager.cpp b/shared/source/os_interface/windows/wddm_memory_manager.cpp index 7adbf1cff3..bb9868cca2 100644 --- a/shared/source/os_interface/windows/wddm_memory_manager.cpp +++ b/shared/source/os_interface/windows/wddm_memory_manager.cpp @@ -508,6 +508,10 @@ void WddmMemoryManager::freeAssociatedResourceImpl(GraphicsAllocation &graphicsA } } +void WddmMemoryManager::freeGraphicsMemoryImpl(GraphicsAllocation *gfxAllocation, bool isImportedAllocation) { + return freeGraphicsMemoryImpl(gfxAllocation); +} + void WddmMemoryManager::freeGraphicsMemoryImpl(GraphicsAllocation *gfxAllocation) { WddmAllocation *input = static_cast(gfxAllocation); DEBUG_BREAK_IF(!validateAllocation(input)); diff --git a/shared/source/os_interface/windows/wddm_memory_manager.h b/shared/source/os_interface/windows/wddm_memory_manager.h index fdc46107ce..6f2fa3fba0 100644 --- a/shared/source/os_interface/windows/wddm_memory_manager.h +++ b/shared/source/os_interface/windows/wddm_memory_manager.h @@ -41,6 +41,7 @@ class WddmMemoryManager : public MemoryManager { WddmMemoryManager &operator=(const WddmMemoryManager &) = delete; void freeGraphicsMemoryImpl(GraphicsAllocation *gfxAllocation) override; + void freeGraphicsMemoryImpl(GraphicsAllocation *gfxAllocation, bool isImportedAllocation) override; void handleFenceCompletion(GraphicsAllocation *allocation) override; GraphicsAllocation *createGraphicsAllocationFromSharedHandle(osHandle handle, const AllocationProperties &properties, bool requireSpecificBitness, bool isHostIpcAllocation) override; diff --git a/shared/test/common/mocks/linux/mock_drm_memory_manager.cpp b/shared/test/common/mocks/linux/mock_drm_memory_manager.cpp index 245ed80660..a909df8ffe 100644 --- a/shared/test/common/mocks/linux/mock_drm_memory_manager.cpp +++ b/shared/test/common/mocks/linux/mock_drm_memory_manager.cpp @@ -78,6 +78,13 @@ DrmAllocation *TestedDrmMemoryManager::allocate32BitGraphicsMemory(uint32_t root bool useLocalMemory = !allocationData.flags.useSystemMemory && this->localMemorySupported[rootDeviceIndex]; return allocate32BitGraphicsMemoryImpl(allocationData, useLocalMemory); } + +void TestedDrmMemoryManager::closeSharedHandle(GraphicsAllocation *gfxAllocation) { + std::unique_lock lock(callsToCloseSharedHandleMtx); + DrmMemoryManager::closeSharedHandle(gfxAllocation); + callsToCloseSharedHandle++; +} + TestedDrmMemoryManager::~TestedDrmMemoryManager() { DrmMemoryManager::commonCleanup(); } diff --git a/shared/test/common/mocks/linux/mock_drm_memory_manager.h b/shared/test/common/mocks/linux/mock_drm_memory_manager.h index 2bd54aebf4..fc4615f29f 100644 --- a/shared/test/common/mocks/linux/mock_drm_memory_manager.h +++ b/shared/test/common/mocks/linux/mock_drm_memory_manager.h @@ -149,12 +149,15 @@ class TestedDrmMemoryManager : public MemoryManagerCreate { alignedFreeWrapperCalled++; DrmMemoryManager::alignedFreeWrapper(ptr); } + void closeSharedHandle(GraphicsAllocation *gfxAllocation) override; uint32_t alignedFreeWrapperCalled = 0u; + uint32_t callsToCloseSharedHandle = 0; protected: std::mutex unreferenceMtx; std::mutex releaseGpuRangeMtx; std::mutex alignedFreeWrapperMtx; + std::mutex callsToCloseSharedHandleMtx; }; struct MockDrmGemCloseWorker : DrmGemCloseWorker { diff --git a/shared/test/common/os_interface/windows/mock_wddm_memory_manager.h b/shared/test/common/os_interface/windows/mock_wddm_memory_manager.h index 64d0f95082..e80e1151ab 100644 --- a/shared/test/common/os_interface/windows/mock_wddm_memory_manager.h +++ b/shared/test/common/os_interface/windows/mock_wddm_memory_manager.h @@ -73,6 +73,10 @@ class MockWddmMemoryManager : public MemoryManagerCreate { BaseClass::freeGraphicsMemoryImpl(gfxAllocation); } + void freeGraphicsMemoryImpl(GraphicsAllocation *gfxAllocation, bool isImportedAllocation) override { + BaseClass::freeGraphicsMemoryImpl(gfxAllocation, isImportedAllocation); + } + GraphicsAllocation *allocateHugeGraphicsMemory(const AllocationData &allocationData, bool sharedVirtualAddress) override { allocateHugeGraphicsMemoryCalled = true; return BaseClass::allocateHugeGraphicsMemory(allocationData, sharedVirtualAddress); diff --git a/shared/test/unit_test/device/neo_device_tests.cpp b/shared/test/unit_test/device/neo_device_tests.cpp index e94f45aeff..9a2d1e8732 100644 --- a/shared/test/unit_test/device/neo_device_tests.cpp +++ b/shared/test/unit_test/device/neo_device_tests.cpp @@ -255,6 +255,7 @@ TEST_F(DeviceGetCapsTest, givenFlagEnabled64kbPagesWhenCallConstructorMemoryMana AllocationStatus populateOsHandles(OsHandleStorage &handleStorage, uint32_t rootDeviceIndex) override { return AllocationStatus::Success; }; void cleanOsHandles(OsHandleStorage &handleStorage, uint32_t rootDeviceIndex) override{}; void freeGraphicsMemoryImpl(GraphicsAllocation *gfxAllocation) override{}; + void freeGraphicsMemoryImpl(GraphicsAllocation *gfxAllocation, bool isImportedAllocation) override{}; uint64_t getSystemSharedMemory(uint32_t rootDeviceIndex) override { return 0; };