fix(sysman): fixes multithread access issue with FSAccess

Related-To: NEO-9720

Signed-off-by: Kulkarni, Ashwin Kumar <[email protected]>
Source: b33d995

Author: AshwinKumarKulkarni

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<std::mutex> 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 <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,6 +303,10 @@ 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;

level_zero/sysman/source/shared/linux/sysman_fs_access_interface.h

@@ -32,7 +32,6 @@ class FdCacheInterface {
     std::map<std::string, std::pair<int, uint32_t>> fdMap = {};
 
   private:
-    std::mutex fdMutex{};
     void eraseLeastUsedEntryFromCache();
 };
 
@@ -65,11 +64,13 @@ class FsAccessInterface {
 
   protected:
     FsAccessInterface();
+    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 {

level_zero/sysman/test/unit_tests/sources/linux/test_sysman.cpp

@@ -219,6 +219,39 @@ 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 {
@@ -257,35 +290,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<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 {

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<std::mutex> 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 <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,6 +308,10 @@ 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/";

level_zero/tools/source/sysman/linux/fs_access.h

@@ -42,7 +42,6 @@ class FdCache {
     std::map<std::string, std::pair<int, uint32_t>> 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<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 {

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<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 {
@@ -540,35 +574,6 @@ 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 {