Revert "fix(sysman): fixes multithread access issue with FSAccess"

Compute-Runtime-Automation · Compute-Runtime-Automation · commit 7988c4ba7535 · 2023-12-12T15:43:14.000+01:00
This reverts commit d14cefb. Signed-off-by: ocldev <ocldev@intel.com>
diff --git a/level_zero/sysman/source/linux/sysman_fs_access.cpp b/level_zero/sysman/source/linux/sysman_fs_access.cpp
@@ -74,7 +74,6 @@ FdCache::~FdCache() {
 
 template <typename T>
 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);
@@ -304,10 +303,6 @@ bool FsAccess::directoryExists(const std::string path) {
     return true;
 }
 
-std::unique_lock<std::mutex> FsAccess::obtainMutex() {
-    return std::unique_lock<std::mutex>(this->fsMutex);
-}
-
 // Procfs Access
 const std::string ProcfsAccess::procDir = "/proc/";
 const std::string ProcfsAccess::fdDir = "/fd/";
diff --git a/level_zero/sysman/source/linux/sysman_fs_access.h b/level_zero/sysman/source/linux/sysman_fs_access.h
@@ -17,7 +17,6 @@
 #include <list>
 #include <map>
 #include <memory>
-#include <mutex>
 #include <sstream>
 #include <string>
 #include <sys/stat.h>
@@ -78,13 +77,11 @@ class FsAccess {
     FsAccess();
     decltype(&NEO::SysCalls::access) accessSyscall = NEO::SysCalls::access;
     decltype(&stat) statSyscall = stat;
-    MOCKABLE_VIRTUAL std::unique_lock<std::mutex> obtainMutex();
 
   private:
     template <typename T>
     ze_result_t readValue(const std::string file, T &val);
     std::unique_ptr<FdCache> pFdCache = nullptr;
-    std::mutex fsMutex{};
 };
 
 class ProcfsAccess : private FsAccess {
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
@@ -45,6 +45,7 @@ void FdCacheInterface::eraseLeastUsedEntryFromCache() {
 }
 
 int FdCacheInterface::getFd(std::string file) {
+    std::unique_lock<std::mutex> lock(fdMutex);
     int fd = -1;
     if (fdMap.find(file) == fdMap.end()) {
         fd = NEO::SysCalls::open(file.c_str(), O_RDONLY);
@@ -71,7 +72,6 @@ FdCacheInterface::~FdCacheInterface() {
 
 template <typename T>
 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,10 +303,6 @@ bool FsAccessInterface::directoryExists(const std::string path) {
     return true;
 }
 
-std::unique_lock<std::mutex> FsAccessInterface::obtainMutex() {
-    return std::unique_lock<std::mutex>(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
@@ -32,6 +32,7 @@ class FdCacheInterface {
     std::map<std::string, std::pair<int, uint32_t>> fdMap = {};
 
   private:
+    std::mutex fdMutex{};
     void eraseLeastUsedEntryFromCache();
 };
 
@@ -66,13 +67,11 @@ class FsAccessInterface {
     FsAccessInterface();
     decltype(&NEO::SysCalls::access) accessSyscall = NEO::SysCalls::access;
     decltype(&stat) statSyscall = stat;
-    MOCKABLE_VIRTUAL std::unique_lock<std::mutex> obtainMutex();
 
   private:
     template <typename T>
     ze_result_t readValue(const std::string file, T &val);
     std::unique_ptr<FdCacheInterface> 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
@@ -219,39 +219,6 @@ TEST_F(SysmanDeviceFixture, GivenSysfsAccessClassAndIntegerWhenCallingReadOnMult
     delete tempSysfsAccess;
 }
 
-TEST_F(SysmanDeviceFixture, GivenValidMockMutexFsAccessWhenCallingReadThenMutexLockCounterMatchesNumberOfReadCalls) {
-    VariableBackup<decltype(NEO::SysCalls::sysCallsOpen)> mockOpen(&NEO::SysCalls::sysCallsOpen, [](const char *pathname, int flags) -> int {
-        return 1;
-    });
-
-    VariableBackup<decltype(NEO::SysCalls::sysCallsPread)> 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<std::mutex> obtainMutex() override {
-            mutexLockCounter++;
-            std::unique_lock<std::mutex> 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 FdCache {
@@ -290,6 +257,35 @@ 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<decltype(NEO::SysCalls::sysCallsOpen)> mockOpen(&NEO::SysCalls::sysCallsOpen, [](const char *pathname, int flags) -> int {
+        return 1;
+    });
+
+    std::unique_ptr<MockFdCache> pFdCache = std::make_unique<MockFdCache>();
+    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 FdCache {
diff --git a/level_zero/tools/source/sysman/linux/fs_access.cpp b/level_zero/tools/source/sysman/linux/fs_access.cpp
@@ -46,6 +46,7 @@ void FdCache::eraseLeastUsedEntryFromCache() {
 }
 
 int FdCache::getFd(std::string file) {
+    std::unique_lock<std::mutex> lock(fdMutex);
     int fd = -1;
     if (fdMap.find(file) == fdMap.end()) {
         fd = NEO::SysCalls::open(file.c_str(), O_RDONLY);
@@ -72,7 +73,6 @@ FdCache::~FdCache() {
 
 template <typename T>
 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,10 +308,6 @@ bool FsAccess::directoryExists(const std::string path) {
     return true;
 }
 
-std::unique_lock<std::mutex> FsAccess::obtainMutex() {
-    return std::unique_lock<std::mutex>(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
@@ -42,6 +42,7 @@ class FdCache {
     std::map<std::string, std::pair<int, uint32_t>> fdMap = {};
 
   private:
+    std::mutex fdMutex{};
     void eraseLeastUsedEntryFromCache();
 };
 
@@ -78,13 +79,11 @@ class FsAccess {
     FsAccess();
     decltype(&NEO::SysCalls::access) accessSyscall = NEO::SysCalls::access;
     decltype(&stat) statSyscall = stat;
-    MOCKABLE_VIRTUAL std::unique_lock<std::mutex> obtainMutex();
 
   private:
     template <typename T>
     ze_result_t readValue(const std::string file, T &val);
     std::unique_ptr<FdCache> 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
@@ -414,40 +414,6 @@ TEST_F(SysmanDeviceFixture, GivenSysfsAccessClassAndIntegerWhenCallingReadThenSu
     delete tempSysfsAccess;
 }
 
-TEST_F(SysmanDeviceFixture, GivenValidMockMutexFsAccessWhenCallingReadThenMutexLockCounterMatchesNumberOfReadCalls) {
-    VariableBackup<decltype(NEO::SysCalls::sysCallsOpen)> mockOpen(&NEO::SysCalls::sysCallsOpen, [](const char *pathname, int flags) -> int {
-        return 1;
-    });
-
-    VariableBackup<decltype(NEO::SysCalls::sysCallsPread)> 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<std::mutex> obtainMutex() override {
-            mutexLockCounter++;
-            std::unique_lock<std::mutex> 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<decltype(NEO::SysCalls::sysCallsOpen)> mockOpen(&NEO::SysCalls::sysCallsOpen, [](const char *pathname, int flags) -> int {
@@ -574,6 +540,35 @@ TEST(FdCacheTest, GivenValidFdCacheWhenClearingCacheThenVerifyProperFdsAreClosed
     delete pFdCache;
 }
 
+TEST(FdCacheTest, GivenValidFdCacheWhenCallingGetFdOnMultipleFilesManyTimesThenVerifyCacheIsUpdatedCorrectly) {
+
+    class MockFdCache : public FdCache {
+      public:
+        using FdCache::fdMap;
+    };
+
+    VariableBackup<decltype(NEO::SysCalls::sysCallsOpen)> mockOpen(&NEO::SysCalls::sysCallsOpen, [](const char *pathname, int flags) -> int {
+        return 1;
+    });
+
+    std::unique_ptr<MockFdCache> pFdCache = std::make_unique<MockFdCache>();
+    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<decltype(NEO::SysCalls::sysCallsOpen)> mockOpen(&NEO::SysCalls::sysCallsOpen, [](const char *pathname, int flags) -> int {
