From b33d9955a1c2e9840d16344e6adf5af148701e15 Mon Sep 17 00:00:00 2001 From: "Kulkarni, Ashwin Kumar" Date: Fri, 8 Dec 2023 15:22:42 +0000 Subject: [PATCH] fix(sysman): fixes multithread access issue with FSAccess Related-To: NEO-9720 Signed-off-by: Kulkarni, Ashwin Kumar --- .../linux/sysman_fs_access_interface.cpp | 6 +- .../shared/linux/sysman_fs_access_interface.h | 3 +- .../unit_tests/sources/linux/test_sysman.cpp | 62 +++++++++--------- .../tools/source/sysman/linux/fs_access.cpp | 6 +- .../tools/source/sysman/linux/fs_access.h | 3 +- .../sources/sysman/linux/test_sysman.cpp | 63 ++++++++++--------- 6 files changed, 81 insertions(+), 62 deletions(-) diff --git a/level_zero/sysman/source/shared/linux/sysman_fs_access_interface.cpp b/level_zero/sysman/source/shared/linux/sysman_fs_access_interface.cpp index ca8c1d9d5a..ff761b4060 100644 --- a/level_zero/sysman/source/shared/linux/sysman_fs_access_interface.cpp +++ b/level_zero/sysman/source/shared/linux/sysman_fs_access_interface.cpp @@ -45,7 +45,6 @@ void FdCacheInterface::eraseLeastUsedEntryFromCache() { } int FdCacheInterface::getFd(std::string file) { - std::unique_lock lock(fdMutex); int fd = -1; if (fdMap.find(file) == fdMap.end()) { fd = NEO::SysCalls::open(file.c_str(), O_RDONLY); @@ -72,6 +71,7 @@ FdCacheInterface::~FdCacheInterface() { template ze_result_t FsAccessInterface::readValue(const std::string file, T &val) { + auto lock = this->obtainMutex(); std::string readVal(64, '\0'); int fd = pFdCacheInterface->getFd(file); @@ -303,6 +303,10 @@ bool FsAccessInterface::directoryExists(const std::string path) { return true; } +std::unique_lock FsAccessInterface::obtainMutex() { + return std::unique_lock(this->fsMutex); +} + // Procfs Access ProcFsAccessInterface::ProcFsAccessInterface() = default; ProcFsAccessInterface::~ProcFsAccessInterface() = default; diff --git a/level_zero/sysman/source/shared/linux/sysman_fs_access_interface.h b/level_zero/sysman/source/shared/linux/sysman_fs_access_interface.h index 4269193fc7..d8286d5f84 100644 --- a/level_zero/sysman/source/shared/linux/sysman_fs_access_interface.h +++ b/level_zero/sysman/source/shared/linux/sysman_fs_access_interface.h @@ -32,7 +32,6 @@ class FdCacheInterface { std::map> fdMap = {}; private: - std::mutex fdMutex{}; void eraseLeastUsedEntryFromCache(); }; @@ -65,11 +64,13 @@ class FsAccessInterface { protected: FsAccessInterface(); + MOCKABLE_VIRTUAL std::unique_lock obtainMutex(); private: template ze_result_t readValue(const std::string file, T &val); std::unique_ptr pFdCacheInterface = nullptr; + std::mutex fsMutex{}; }; class ProcFsAccessInterface : private FsAccessInterface { diff --git a/level_zero/sysman/test/unit_tests/sources/linux/test_sysman.cpp b/level_zero/sysman/test/unit_tests/sources/linux/test_sysman.cpp index be2c5fe996..66c6488287 100644 --- a/level_zero/sysman/test/unit_tests/sources/linux/test_sysman.cpp +++ b/level_zero/sysman/test/unit_tests/sources/linux/test_sysman.cpp @@ -180,6 +180,39 @@ TEST_F(SysmanDeviceFixture, GivenSysfsAccessClassAndIntegerWhenCallingReadOnMult delete tempSysfsAccess; } +TEST_F(SysmanDeviceFixture, GivenValidMockMutexFsAccessWhenCallingReadThenMutexLockCounterMatchesNumberOfReadCalls) { + VariableBackup mockOpen(&NEO::SysCalls::sysCallsOpen, [](const char *pathname, int flags) -> int { + return 1; + }); + + VariableBackup mockPread(&NEO::SysCalls::sysCallsPread, [](int fd, void *buf, size_t count, off_t offset) -> ssize_t { + std::string value = "123"; + memcpy(buf, value.data(), value.size()); + return value.size(); + }); + + class MockMutexFsAccess : public L0::Sysman::FsAccessInterface { + public: + uint32_t mutexLockCounter = 0; + std::unique_lock obtainMutex() override { + mutexLockCounter++; + std::unique_lock mutexLock = L0::Sysman::FsAccessInterface::obtainMutex(); + EXPECT_TRUE(mutexLock.owns_lock()); + return mutexLock; + } + }; + MockMutexFsAccess *tempFsAccess = new MockMutexFsAccess(); + std::string fileName = {}; + uint32_t iVal32; + uint32_t testReadCount = 10; + for (uint32_t i = 0; i < testReadCount; i++) { + fileName = "mockfile" + std::to_string(i) + ".txt"; + EXPECT_EQ(ZE_RESULT_SUCCESS, tempFsAccess->read(fileName, iVal32)); + } + EXPECT_EQ(tempFsAccess->mutexLockCounter, testReadCount); + delete tempFsAccess; +} + TEST(FdCacheTest, GivenValidFdCacheWhenCallingGetFdOnSameFileThenVerifyCacheIsUpdatedProperly) { class MockFdCache : public FdCacheInterface { @@ -218,35 +251,6 @@ TEST(FdCacheTest, GivenValidFdCacheWhenCallingGetFdOnSameFileThenVerifyCacheIsUp EXPECT_EQ(pFdCache->fdMap.end(), pFdCache->fdMap.find("mockfile9.txt")); } -TEST(FdCacheTest, GivenValidFdCacheWhenCallingGetFdOnMultipleFilesManyTimesThenVerifyCacheIsUpdatedCorrectly) { - - class MockFdCache : public FdCacheInterface { - public: - using FdCacheInterface::fdMap; - }; - - VariableBackup mockOpen(&NEO::SysCalls::sysCallsOpen, [](const char *pathname, int flags) -> int { - return 1; - }); - - std::unique_ptr pFdCache = std::make_unique(); - std::string fileName = {}; - for (auto i = 0; i < L0::Sysman::FdCacheInterface::maxSize; i++) { - fileName = "mockfile" + std::to_string(i) + ".txt"; - int j = i + 1; - while (j--) { - EXPECT_LE(0, pFdCache->getFd(fileName)); - } - } - - // replace a least referred file and add new file - fileName = "mockfile100.txt"; - EXPECT_LE(0, pFdCache->getFd(fileName)); - - // Verify cache doesn't have an element that is accessed less number of times. - EXPECT_EQ(pFdCache->fdMap.end(), pFdCache->fdMap.find("mockfile0.txt")); -} - TEST(FdCacheTest, GivenValidFdCacheWhenClearingCacheThenVerifyProperFdsAreClosedAndCacheIsUpdatedProperly) { class MockFdCache : public FdCacheInterface { diff --git a/level_zero/tools/source/sysman/linux/fs_access.cpp b/level_zero/tools/source/sysman/linux/fs_access.cpp index e31439c416..4725d7ab54 100644 --- a/level_zero/tools/source/sysman/linux/fs_access.cpp +++ b/level_zero/tools/source/sysman/linux/fs_access.cpp @@ -46,7 +46,6 @@ void FdCache::eraseLeastUsedEntryFromCache() { } int FdCache::getFd(std::string file) { - std::unique_lock lock(fdMutex); int fd = -1; if (fdMap.find(file) == fdMap.end()) { fd = NEO::SysCalls::open(file.c_str(), O_RDONLY); @@ -73,6 +72,7 @@ FdCache::~FdCache() { template ze_result_t FsAccess::readValue(const std::string file, T &val) { + auto lock = this->obtainMutex(); std::string readVal(64, '\0'); int fd = pFdCache->getFd(file); @@ -308,6 +308,10 @@ bool FsAccess::directoryExists(const std::string path) { return true; } +std::unique_lock FsAccess::obtainMutex() { + return std::unique_lock(this->fsMutex); +} + // Procfs Access const std::string ProcfsAccess::procDir = "/proc/"; const std::string ProcfsAccess::fdDir = "/fd/"; diff --git a/level_zero/tools/source/sysman/linux/fs_access.h b/level_zero/tools/source/sysman/linux/fs_access.h index 0bfb44eb61..60697106dd 100644 --- a/level_zero/tools/source/sysman/linux/fs_access.h +++ b/level_zero/tools/source/sysman/linux/fs_access.h @@ -42,7 +42,6 @@ class FdCache { std::map> fdMap = {}; private: - std::mutex fdMutex{}; void eraseLeastUsedEntryFromCache(); }; @@ -79,11 +78,13 @@ class FsAccess { FsAccess(); decltype(&NEO::SysCalls::access) accessSyscall = NEO::SysCalls::access; decltype(&stat) statSyscall = stat; + MOCKABLE_VIRTUAL std::unique_lock obtainMutex(); private: template ze_result_t readValue(const std::string file, T &val); std::unique_ptr pFdCache = nullptr; + std::mutex fsMutex{}; }; class ProcfsAccess : private FsAccess { diff --git a/level_zero/tools/test/unit_tests/sources/sysman/linux/test_sysman.cpp b/level_zero/tools/test/unit_tests/sources/sysman/linux/test_sysman.cpp index 832b10f5eb..eaf7dde81b 100644 --- a/level_zero/tools/test/unit_tests/sources/sysman/linux/test_sysman.cpp +++ b/level_zero/tools/test/unit_tests/sources/sysman/linux/test_sysman.cpp @@ -414,6 +414,40 @@ TEST_F(SysmanDeviceFixture, GivenSysfsAccessClassAndIntegerWhenCallingReadThenSu delete tempSysfsAccess; } +TEST_F(SysmanDeviceFixture, GivenValidMockMutexFsAccessWhenCallingReadThenMutexLockCounterMatchesNumberOfReadCalls) { + VariableBackup mockOpen(&NEO::SysCalls::sysCallsOpen, [](const char *pathname, int flags) -> int { + return 1; + }); + + VariableBackup mockPread(&NEO::SysCalls::sysCallsPread, [](int fd, void *buf, size_t count, off_t offset) -> ssize_t { + std::string value = "123"; + memcpy(buf, value.data(), value.size()); + return value.size(); + }); + + class MockMutexFsAccess : public L0::FsAccess { + public: + uint32_t mutexLockCounter = 0; + std::unique_lock obtainMutex() override { + mutexLockCounter++; + std::unique_lock mutexLock = L0::FsAccess::obtainMutex(); + EXPECT_TRUE(mutexLock.owns_lock()); + return mutexLock; + } + }; + + MockMutexFsAccess *tempFsAccess = new MockMutexFsAccess(); + std::string fileName = {}; + uint32_t iVal32; + uint32_t testReadCount = 10; + for (uint32_t i = 0; i < testReadCount; i++) { + fileName = "mockfile" + std::to_string(i) + ".txt"; + EXPECT_EQ(ZE_RESULT_SUCCESS, tempFsAccess->read(fileName, iVal32)); + } + EXPECT_EQ(tempFsAccess->mutexLockCounter, testReadCount); + delete tempFsAccess; +} + TEST_F(SysmanDeviceFixture, GivenSysfsAccessClassAndOpenSysCallFailsWhenCallingReadThenFailureIsReturned) { VariableBackup mockOpen(&NEO::SysCalls::sysCallsOpen, [](const char *pathname, int flags) -> int { @@ -540,35 +574,6 @@ TEST(FdCacheTest, GivenValidFdCacheWhenClearingCacheThenVerifyProperFdsAreClosed delete pFdCache; } -TEST(FdCacheTest, GivenValidFdCacheWhenCallingGetFdOnMultipleFilesManyTimesThenVerifyCacheIsUpdatedCorrectly) { - - class MockFdCache : public FdCache { - public: - using FdCache::fdMap; - }; - - VariableBackup mockOpen(&NEO::SysCalls::sysCallsOpen, [](const char *pathname, int flags) -> int { - return 1; - }); - - std::unique_ptr pFdCache = std::make_unique(); - std::string fileName = {}; - for (auto i = 0; i < L0::FdCache::maxSize; i++) { - fileName = "mockfile" + std::to_string(i) + ".txt"; - int j = i + 1; - while (j--) { - EXPECT_LE(0, pFdCache->getFd(fileName)); - } - } - - // replace a least referred file and add new file - fileName = "mockfile100.txt"; - EXPECT_LE(0, pFdCache->getFd(fileName)); - - // Verify cache doesn't have an element that is accessed less number of times. - EXPECT_EQ(pFdCache->fdMap.end(), pFdCache->fdMap.find("mockfile0.txt")); -} - TEST_F(SysmanDeviceFixture, GivenSysfsAccessClassAndUnsignedIntegerWhenCallingReadThenSuccessIsReturned) { VariableBackup mockOpen(&NEO::SysCalls::sysCallsOpen, [](const char *pathname, int flags) -> int {