From 526ba1bde50d9f043b916bbded4d35ca81c9b806 Mon Sep 17 00:00:00 2001 From: Dominik Dabek Date: Thu, 27 Oct 2022 10:40:20 +0000 Subject: [PATCH] Fix l0 kernel set arg buffer caching Fix for incorrect cache hit if alloc id was uninitialized and allocations counter was the same. Signed-off-by: Dominik Dabek --- level_zero/core/source/kernel/kernel_imp.cpp | 4 +- .../unit_tests/sources/kernel/test_kernel.cpp | 38 +++++++++++++++++-- .../memory_manager/unified_memory_manager.h | 4 +- 3 files changed, 41 insertions(+), 5 deletions(-) diff --git a/level_zero/core/source/kernel/kernel_imp.cpp b/level_zero/core/source/kernel/kernel_imp.cpp index 1929537b91..c77e2253bf 100644 --- a/level_zero/core/source/kernel/kernel_imp.cpp +++ b/level_zero/core/source/kernel/kernel_imp.cpp @@ -536,7 +536,9 @@ ze_result_t KernelImp::setArgBuffer(uint32_t argIndex, size_t argSize, const voi NEO::SvmAllocationData *allocData = nullptr; if (argVal != nullptr) { const auto requestedAddress = *reinterpret_cast(argVal); - if (argInfo.allocId > 0 && requestedAddress == argInfo.value) { + if (argInfo.allocId > 0 && + argInfo.allocId < NEO::SvmAllocationData::uninitializedAllocId && + requestedAddress == argInfo.value) { bool reuseFromCache = false; if (allocationsCounter > 0) { if (allocationsCounter == argInfo.allocIdMemoryManagerCounter) { diff --git a/level_zero/core/test/unit_tests/sources/kernel/test_kernel.cpp b/level_zero/core/test/unit_tests/sources/kernel/test_kernel.cpp index 618e64f9df..4d1aa95df2 100644 --- a/level_zero/core/test/unit_tests/sources/kernel/test_kernel.cpp +++ b/level_zero/core/test/unit_tests/sources/kernel/test_kernel.cpp @@ -114,6 +114,7 @@ TEST_F(SetKernelArgCacheTest, givenValidBufferArgumentWhenSetMultipleTimesThenSe auto svmAllocsManager = device->getDriverHandle()->getSvmAllocsManager(); auto allocationProperties = NEO::SVMAllocsManager::SvmAllocationProperties{}; auto svmAllocation = svmAllocsManager->createSVMAlloc(4096, allocationProperties, context->rootDeviceIndices, context->deviceBitfields); + auto allocData = svmAllocsManager->getSVMAlloc(svmAllocation); size_t callCounter = 0u; @@ -122,12 +123,28 @@ TEST_F(SetKernelArgCacheTest, givenValidBufferArgumentWhenSetMultipleTimesThenSe EXPECT_EQ(++callCounter, mockKernel.setArgBufferWithAllocCalled); // same setArg but allocationCounter == 0 - called + ASSERT_EQ(svmAllocsManager->allocationsCounter, 0u); EXPECT_EQ(ZE_RESULT_SUCCESS, mockKernel.setArgBuffer(0, sizeof(svmAllocation), &svmAllocation)); EXPECT_EQ(++callCounter, mockKernel.setArgBufferWithAllocCalled); - // same setArg - not called and argInfo.allocationCounter is updated + // update allocationsCounter to 1 ++svmAllocsManager->allocationsCounter; - EXPECT_EQ(0u, mockKernel.kernelArgInfos[0].allocIdMemoryManagerCounter); + ASSERT_EQ(mockKernel.kernelArgInfos[0].allocIdMemoryManagerCounter, 0u); + EXPECT_EQ(ZE_RESULT_SUCCESS, mockKernel.setArgBuffer(0, sizeof(svmAllocation), &svmAllocation)); + EXPECT_EQ(++callCounter, mockKernel.setArgBufferWithAllocCalled); + EXPECT_EQ(mockKernel.kernelArgInfos[0].allocIdMemoryManagerCounter, 1u); + + allocData->setAllocId(1u); + // same setArg but allocId is uninitialized - called + ASSERT_EQ(mockKernel.kernelArgInfos[0].allocId, SvmAllocationData::uninitializedAllocId); + ASSERT_EQ(mockKernel.kernelArgInfos[0].allocIdMemoryManagerCounter, svmAllocsManager->allocationsCounter); + EXPECT_EQ(ZE_RESULT_SUCCESS, mockKernel.setArgBuffer(0, sizeof(svmAllocation), &svmAllocation)); + EXPECT_EQ(++callCounter, mockKernel.setArgBufferWithAllocCalled); + EXPECT_EQ(mockKernel.kernelArgInfos[0].allocId, 1u); + + ++svmAllocsManager->allocationsCounter; + // same setArg - not called and argInfo.allocationCounter is updated + EXPECT_EQ(1u, mockKernel.kernelArgInfos[0].allocIdMemoryManagerCounter); EXPECT_EQ(ZE_RESULT_SUCCESS, mockKernel.setArgBuffer(0, sizeof(svmAllocation), &svmAllocation)); EXPECT_EQ(callCounter, mockKernel.setArgBufferWithAllocCalled); EXPECT_EQ(svmAllocsManager->allocationsCounter, mockKernel.kernelArgInfos[0].allocIdMemoryManagerCounter); @@ -137,13 +154,18 @@ TEST_F(SetKernelArgCacheTest, givenValidBufferArgumentWhenSetMultipleTimesThenSe EXPECT_EQ(callCounter, mockKernel.setArgBufferWithAllocCalled); // same setArg but different allocId - called - svmAllocsManager->getSVMAlloc(svmAllocation)->setAllocId(1u); + allocData->setAllocId(2u); ++svmAllocsManager->allocationsCounter; + ASSERT_NE(mockKernel.kernelArgInfos[0].allocIdMemoryManagerCounter, svmAllocsManager->allocationsCounter); + ASSERT_NE(mockKernel.kernelArgInfos[0].allocId, allocData->getAllocId()); EXPECT_EQ(ZE_RESULT_SUCCESS, mockKernel.setArgBuffer(0, sizeof(svmAllocation), &svmAllocation)); EXPECT_EQ(++callCounter, mockKernel.setArgBufferWithAllocCalled); + EXPECT_EQ(mockKernel.kernelArgInfos[0].allocIdMemoryManagerCounter, svmAllocsManager->allocationsCounter); + EXPECT_EQ(mockKernel.kernelArgInfos[0].allocId, allocData->getAllocId()); // different value - called auto secondSvmAllocation = svmAllocsManager->createSVMAlloc(4096, allocationProperties, context->rootDeviceIndices, context->deviceBitfields); + svmAllocsManager->getSVMAlloc(secondSvmAllocation)->setAllocId(3u); EXPECT_EQ(ZE_RESULT_SUCCESS, mockKernel.setArgBuffer(0, sizeof(secondSvmAllocation), &secondSvmAllocation)); EXPECT_EQ(++callCounter, mockKernel.setArgBufferWithAllocCalled); @@ -163,9 +185,19 @@ TEST_F(SetKernelArgCacheTest, givenValidBufferArgumentWhenSetMultipleTimesThenSe EXPECT_EQ(++callCounter, mockKernel.setArgBufferWithAllocCalled); EXPECT_FALSE(mockKernel.kernelArgInfos[0].isSetToNullptr); + // allocations counter == 0 called + svmAllocsManager->allocationsCounter = 0; + EXPECT_EQ(ZE_RESULT_SUCCESS, mockKernel.setArgBuffer(0, sizeof(secondSvmAllocation), &secondSvmAllocation)); + EXPECT_EQ(++callCounter, mockKernel.setArgBufferWithAllocCalled); + // same value but no svmData - ZE_RESULT_ERROR_INVALID_ARGUMENT svmAllocsManager->freeSVMAlloc(secondSvmAllocation); ++svmAllocsManager->allocationsCounter; + ASSERT_GT(mockKernel.kernelArgInfos[0].allocId, 0u); + ASSERT_LT(mockKernel.kernelArgInfos[0].allocId, SvmAllocationData::uninitializedAllocId); + ASSERT_EQ(mockKernel.kernelArgInfos[0].value, secondSvmAllocation); + ASSERT_GT(svmAllocsManager->allocationsCounter, 0u); + ASSERT_NE(mockKernel.kernelArgInfos[0].allocIdMemoryManagerCounter, svmAllocsManager->allocationsCounter); EXPECT_EQ(ZE_RESULT_ERROR_INVALID_ARGUMENT, mockKernel.setArgBuffer(0, sizeof(secondSvmAllocation), &secondSvmAllocation)); EXPECT_EQ(callCounter, mockKernel.setArgBufferWithAllocCalled); diff --git a/shared/source/memory_manager/unified_memory_manager.h b/shared/source/memory_manager/unified_memory_manager.h index 0b55920349..7d27778eb3 100644 --- a/shared/source/memory_manager/unified_memory_manager.h +++ b/shared/source/memory_manager/unified_memory_manager.h @@ -60,9 +60,11 @@ struct SvmAllocationData { return allocId; } + static constexpr uint32_t uninitializedAllocId = std::numeric_limits::max(); + protected: const uint32_t maxRootDeviceIndex; - uint32_t allocId = std::numeric_limits::max(); + uint32_t allocId = uninitializedAllocId; }; struct SvmMapOperation {