Make sure that stopping Deferred Deleter is thread safe

Change-Id: Iaf4cf4f7291a5a87f7199ee86b4fc116acdc18c0

runtime/memory_manager/deferred_deleter.cpp

@@ -1,5 +1,5 @@
 /*
-* Copyright (c) 2017, Intel Corporation
+* Copyright (c) 2017 - 2018, Intel Corporation
 *
 * Permission is hereby granted, free of charge, to any person obtaining a
 * copy of this software and associated documentation files (the "Software"),
@@ -26,30 +26,41 @@
 namespace OCLRT {
 DeferredDeleter::DeferredDeleter() {
     doWorkInBackground = false;
-    threadLoaded = false;
     elementsToRelease = 0;
 }
 
 void DeferredDeleter::stop() {
-    if (worker) {
-        while (!threadLoaded)
-            ;
-        threadLoaded = false;
+    // Called with threadMutex acquired
+    if (worker != nullptr) {
+        // Working thread was created so we can safely stop it
+        std::unique_lock<std::mutex> lock(queueMutex);
+        // Make sure that working thread really started
+        while (!doWorkInBackground) {
+            lock.unlock();
+            lock.lock();
+        }
+        // Signal working thread to finish its job
         doWorkInBackground = false;
-        queueMutex.lock();
-        queueMutex.unlock();
+        lock.unlock();
         condition.notify_one();
+        // Wait for the working job to exit
         worker->join();
+        // Delete working thread
         delete worker;
         worker = nullptr;
     }
     drain(false);
 }
 
-DeferredDeleter::~DeferredDeleter() {
+void DeferredDeleter::safeStop() {
+    std::lock_guard<std::mutex> lock(threadMutex);
     stop();
 }
 
+DeferredDeleter::~DeferredDeleter() {
+    safeStop();
+}
+
 void DeferredDeleter::deferDeletion(DeferrableDeletion *deletion) {
     std::unique_lock<std::mutex> lock(queueMutex);
     elementsToRelease++;
@@ -73,8 +84,9 @@ void DeferredDeleter::removeClient() {
 }
 
 void DeferredDeleter::ensureThread() {
-    if (worker)
+    if (worker != nullptr) {
         return;
+    }
     worker = new std::thread(run, this);
 }
 
@@ -88,15 +100,18 @@ bool DeferredDeleter::shouldStop() {
 
 void DeferredDeleter::run(DeferredDeleter *self) {
     std::unique_lock<std::mutex> lock(self->queueMutex);
+    // Mark that working thread really started
     self->doWorkInBackground = true;
-    self->threadLoaded = true;
     do {
         if (self->queue.peekIsEmpty()) {
+            // Wait for signal that some items are ready to be deleted
             self->condition.wait(lock);
         }
         lock.unlock();
+        // Delete items placed into deferred delete queue
         self->clearQueue();
         lock.lock();
+        // Check whether working thread should be stopped
     } while (!self->shouldStop());
     lock.unlock();
 }

runtime/memory_manager/deferred_deleter.h

@@ -1,5 +1,5 @@
 /*
-* Copyright (c) 2017, Intel Corporation
+* Copyright (c) 2017 - 2018, Intel Corporation
 *
 * Permission is hereby granted, free of charge, to any person obtaining a
 * copy of this software and associated documentation files (the "Software"),
@@ -48,6 +48,7 @@ class DeferredDeleter {
 
   protected:
     void stop();
+    void safeStop();
     void ensureThread();
     MOCKABLE_VIRTUAL void clearQueue();
     MOCKABLE_VIRTUAL bool areElementsReleased();
@@ -56,7 +57,6 @@ class DeferredDeleter {
     static void run(DeferredDeleter *self);
 
     std::atomic<bool> doWorkInBackground;
-    std::atomic<bool> threadLoaded;
     std::atomic<int> elementsToRelease;
     std::thread *worker = nullptr;
     int32_t numClients = 0;

unit_tests/memory_manager/deferred_deleter_mt_tests.cpp

@@ -1,5 +1,5 @@
 /*
-* Copyright (c) 2017, Intel Corporation
+* Copyright (c) 2017 - 2018, Intel Corporation
 *
 * Permission is hereby granted, free of charge, to any person obtaining a
 * copy of this software and associated documentation files (the "Software"),
@@ -49,7 +49,7 @@ struct DeferredDeleterTest : public ::testing::Test {
     }
 
     void waitForAsyncThread() {
-        while (!deleter->isThreadLoaded()) {
+        while (!deleter->isWorking()) {
             std::this_thread::yield();
         }
     }
@@ -63,7 +63,6 @@ TEST_F(DeferredDeleterTest, initialValues) {
     EXPECT_EQ(0, deleter->getClientsNum());
     EXPECT_FALSE(deleter->isWorking());
     EXPECT_FALSE(deleter->isThreadRunning());
-    EXPECT_FALSE(deleter->isThreadLoaded());
     EXPECT_EQ(0, deleter->drainCalled);
     EXPECT_EQ(0, deleter->clearCalled);
     EXPECT_EQ(0, deleter->areElementsReleasedCalled);

unit_tests/mocks/mock_deferred_deleter.cpp

@@ -1,5 +1,5 @@
 /*
-* Copyright (c) 2017, Intel Corporation
+* Copyright (c) 2017 - 2018, Intel Corporation
 *
 * Permission is hereby granted, free of charge, to any person obtaining a
 * copy of this software and associated documentation files (the "Software"),
@@ -84,10 +84,6 @@ bool MockDeferredDeleter::isWorking() {
     return doWorkInBackground;
 }
 
-bool MockDeferredDeleter::isThreadLoaded() {
-    return threadLoaded;
-}
-
 bool MockDeferredDeleter::isThreadRunning() {
     return worker != nullptr;
 }

unit_tests/mocks/mock_deferred_deleter.h

@@ -1,5 +1,5 @@
 /*
-* Copyright (c) 2017, Intel Corporation
+* Copyright (c) 2017 - 2018, Intel Corporation
 *
 * Permission is hereby granted, free of charge, to any person obtaining a
 * copy of this software and associated documentation files (the "Software"),
@@ -50,8 +50,6 @@ class MockDeferredDeleter : public DeferredDeleter {
 
     bool isWorking();
 
-    bool isThreadLoaded();
-
     bool isThreadRunning();
 
     bool isQueueEmpty();

unit_tests/mt_tests/memory_manager/deferred_deleter_clear_queue_mt_tests.cpp

@@ -1,5 +1,5 @@
 /*
-* Copyright (c) 2017, Intel Corporation
+* Copyright (c) 2017 - 2018, Intel Corporation
 *
 * Permission is hereby granted, free of charge, to any person obtaining a
 * copy of this software and associated documentation files (the "Software"),
@@ -82,3 +82,69 @@ int paramsForClearQueueTest[] = {1, 10, 20, 50, 100};
 INSTANTIATE_TEST_CASE_P(DeferredDeleterMtTests,
                         ClearQueueTest,
                         ::testing::ValuesIn(paramsForClearQueueTest));
+
+class MyDeferredDeleter : public DeferredDeleter {
+  public:
+    bool isQueueEmpty() {
+        std::lock_guard<std::mutex> lock(queueMutex);
+        return queue.peekIsEmpty();
+    }
+    int getElementsToRelease() {
+        return elementsToRelease;
+    }
+    bool isWorking() {
+        return doWorkInBackground;
+    }
+    bool isThreadRunning() {
+        return worker != nullptr;
+    }
+    int getClientsNum() {
+        return numClients;
+    }
+    void forceSafeStop() {
+        safeStop();
+    }
+};
+
+struct DeferredDeleterMtTest : public ::testing::Test {
+
+    void SetUp() override {
+        deleter.reset(new MyDeferredDeleter());
+    }
+
+    void TearDown() override {
+        EXPECT_TRUE(deleter->isQueueEmpty());
+        EXPECT_EQ(0, deleter->getElementsToRelease());
+    }
+
+    void waitForAsyncThread() {
+        while (!deleter->isWorking()) {
+            std::this_thread::yield();
+        }
+    }
+    std::unique_ptr<MyDeferredDeleter> deleter;
+};
+
+TEST_F(DeferredDeleterMtTest, asyncThreadsStopDeferredDeleter) {
+    deleter->addClient();
+
+    waitForAsyncThread();
+    EXPECT_TRUE(deleter->isThreadRunning());
+    EXPECT_TRUE(deleter->isWorking());
+
+    // Start worker thread
+    std::thread t([&]() {
+        deleter->forceSafeStop();
+        EXPECT_FALSE(deleter->isThreadRunning());
+        EXPECT_FALSE(deleter->isWorking());
+    });
+
+    deleter->forceSafeStop();
+    EXPECT_FALSE(deleter->isThreadRunning());
+    EXPECT_FALSE(deleter->isWorking());
+
+    t.join();
+
+    deleter->removeClient();
+    EXPECT_EQ(0, deleter->getClientsNum());
+}