From 6d451ba7e1f3e51c79503f153a2b9fbbed07eb11 Mon Sep 17 00:00:00 2001 From: Sergey Semenov Date: Fri, 28 Feb 2020 09:55:49 +0300 Subject: [PATCH 1/6] [SYCL] Fix command cleanup invoked from multiple threads This patch fixes a sporadic bug where one thread attempted to clean up a command already deleted by another. Signed-off-by: Sergey Semenov --- sycl/source/detail/event_impl.cpp | 6 +-- sycl/source/detail/event_impl.hpp | 7 ++-- sycl/source/detail/scheduler/scheduler.cpp | 6 ++- sycl/source/detail/scheduler/scheduler.hpp | 2 +- .../scheduler/CommandCleanupThreadSafety.cpp | 38 +++++++++++++++++++ 5 files changed, 49 insertions(+), 10 deletions(-) create mode 100644 sycl/test/scheduler/CommandCleanupThreadSafety.cpp diff --git a/sycl/source/detail/event_impl.cpp b/sycl/source/detail/event_impl.cpp index a7b3b467b8988..398e0cc7bdc9a 100644 --- a/sycl/source/detail/event_impl.cpp +++ b/sycl/source/detail/event_impl.cpp @@ -95,16 +95,14 @@ event_impl::event_impl(QueueImplPtr Queue) : MQueue(Queue) { void event_impl::wait( std::shared_ptr Self) const { - if (MEvent) // presence of MEvent means the command has been enqueued, so no need to // go via the slow path event waiting in the scheduler waitInternal(); else if (MCommand) - detail::Scheduler::getInstance().waitForEvent(std::move(Self)); + detail::Scheduler::getInstance().waitForEvent(Self); if (MCommand && !SYCLConfig::get()) - detail::Scheduler::getInstance().cleanupFinishedCommands( - static_cast(MCommand)); + detail::Scheduler::getInstance().cleanupFinishedCommands(std::move(Self)); } void event_impl::wait_and_throw( diff --git a/sycl/source/detail/event_impl.hpp b/sycl/source/detail/event_impl.hpp index b9c8d4fa17de5..c3072c3250ff5 100644 --- a/sycl/source/detail/event_impl.hpp +++ b/sycl/source/detail/event_impl.hpp @@ -25,6 +25,7 @@ class context_impl; using ContextImplPtr = std::shared_ptr; class queue_impl; using QueueImplPtr = std::shared_ptr; +class Command; class event_impl { public: @@ -132,12 +133,12 @@ class event_impl { /// Returns command that is associated with the event. /// /// @return a generic pointer to Command object instance. - void *getCommand() { return MCommand; } + Command *getCommand() { return MCommand; } /// Associates this event with the command. /// /// @param Command is a generic pointer to Command object instance. - void setCommand(void *Command) { MCommand = Command; } + void setCommand(Command *Command) { MCommand = Command; } /// Returns host profiling information. /// @@ -151,7 +152,7 @@ class event_impl { bool MOpenCLInterop = false; bool MHostEvent = true; std::unique_ptr MHostProfilingInfo; - void *MCommand = nullptr; + Command *MCommand = nullptr; }; } // namespace detail diff --git a/sycl/source/detail/scheduler/scheduler.cpp b/sycl/source/detail/scheduler/scheduler.cpp index 4da29c0a23299..6cff0d9d1fbca 100644 --- a/sycl/source/detail/scheduler/scheduler.cpp +++ b/sycl/source/detail/scheduler/scheduler.cpp @@ -123,9 +123,11 @@ void Scheduler::waitForEvent(EventImplPtr Event) { GraphProcessor::waitForEvent(std::move(Event)); } -void Scheduler::cleanupFinishedCommands(Command *FinishedCmd) { +void Scheduler::cleanupFinishedCommands(EventImplPtr FinishedEvent) { std::lock_guard lock(MGraphLock); - MGraphBuilder.cleanupFinishedCommands(FinishedCmd); + Command *FinishedCmd = static_cast(FinishedEvent->getCommand()); + if (FinishedCmd) + MGraphBuilder.cleanupFinishedCommands(FinishedCmd); } void Scheduler::removeMemoryObject(detail::SYCLMemObjI *MemObj) { diff --git a/sycl/source/detail/scheduler/scheduler.hpp b/sycl/source/detail/scheduler/scheduler.hpp index e0429510eed1b..90000f6ab558c 100644 --- a/sycl/source/detail/scheduler/scheduler.hpp +++ b/sycl/source/detail/scheduler/scheduler.hpp @@ -79,7 +79,7 @@ class Scheduler { // Removes finished non-leaf non-alloca commands from the subgraph (assuming // that all its commands have been waited for). - void cleanupFinishedCommands(Command *FinishedCmd); + void cleanupFinishedCommands(EventImplPtr FinishedEvent); // Creates nodes in the graph, that update Req with the pointer to the host // memory which contains the latest data of the memory object. New diff --git a/sycl/test/scheduler/CommandCleanupThreadSafety.cpp b/sycl/test/scheduler/CommandCleanupThreadSafety.cpp new file mode 100644 index 0000000000000..a6d1bca4cfb6c --- /dev/null +++ b/sycl/test/scheduler/CommandCleanupThreadSafety.cpp @@ -0,0 +1,38 @@ +// RUN: %clangxx -fsycl %s -o %t.out -lpthread +// RUN: %CPU_RUN_PLACEHOLDER %t.out +#include + +#include +#include +#include +#include + +// This test checks that the command graph cleanup works properly when +// invoked from multiple threads. +using namespace cl::sycl; + +class Foo; + +event submitTask(queue &Q, buffer &Buf) { + return Q.submit([&](handler &Cgh) { + auto Acc = Buf.get_access(Cgh); + Cgh.single_task([=]() { Acc[0] = 42; }); + }); +} + +int main() { + queue Q; + buffer Buf(range<1>(1)); + + // Create multiple commands, each one dependent on the previous + std::vector Events; + for (std::size_t I = 0; I < 16; ++I) + Events.push_back(submitTask(Q, Buf)); + + // Initiate cleanup from multiple threads + std::vector Threads; + for (event &E : Events) + Threads.emplace_back([&]() { E.wait(); }); + for (std::thread &T : Threads) + T.join(); +} From d6a7b6bd1bce7f440ba823c5948b5e1cfa53ab9a Mon Sep 17 00:00:00 2001 From: Sergey Semenov Date: Fri, 28 Feb 2020 13:12:07 +0300 Subject: [PATCH 2/6] Revert event_impl::MCommand pointer type change Signed-off-by: Sergey Semenov --- sycl/source/detail/event_impl.hpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/sycl/source/detail/event_impl.hpp b/sycl/source/detail/event_impl.hpp index c3072c3250ff5..b558b633d1d8e 100644 --- a/sycl/source/detail/event_impl.hpp +++ b/sycl/source/detail/event_impl.hpp @@ -133,12 +133,12 @@ class event_impl { /// Returns command that is associated with the event. /// /// @return a generic pointer to Command object instance. - Command *getCommand() { return MCommand; } + void *getCommand() { return MCommand; } /// Associates this event with the command. /// /// @param Command is a generic pointer to Command object instance. - void setCommand(Command *Command) { MCommand = Command; } + void setCommand(void *Command) { MCommand = Command; } /// Returns host profiling information. /// @@ -152,7 +152,7 @@ class event_impl { bool MOpenCLInterop = false; bool MHostEvent = true; std::unique_ptr MHostProfilingInfo; - Command *MCommand = nullptr; + void *MCommand = nullptr; }; } // namespace detail From 578ab3db08bd587a4878d9f86e555af69c5593b4 Mon Sep 17 00:00:00 2001 From: Sergey Semenov Date: Fri, 28 Feb 2020 13:23:59 +0300 Subject: [PATCH 3/6] Fix FinishedCmdCleanup test Signed-off-by: Sergey Semenov --- sycl/test/scheduler/FinishedCmdCleanup.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/sycl/test/scheduler/FinishedCmdCleanup.cpp b/sycl/test/scheduler/FinishedCmdCleanup.cpp index e0f736886b040..cf036d81d65fa 100644 --- a/sycl/test/scheduler/FinishedCmdCleanup.cpp +++ b/sycl/test/scheduler/FinishedCmdCleanup.cpp @@ -1,6 +1,7 @@ // RUN: %clangxx -fsycl -I %sycl_source_dir %s -o %t.out // RUN: %t.out #include +#include #include #include @@ -76,7 +77,9 @@ int main() { addEdge(InnerA, &LeafA, &AllocaA); addEdge(InnerA, InnerB, &AllocaB); - TS.cleanupFinishedCommands(InnerA); + std::shared_ptr Event{new detail::event_impl{}}; + Event->setCommand(InnerA); + TS.cleanupFinishedCommands(Event); TS.removeRecordForMemObj(detail::getSyclObjImpl(BufC).get()); assert(NInnerCommandsAlive == 0); From 7927a36a581599dbc4c4e18b3eb3e643226ae089 Mon Sep 17 00:00:00 2001 From: Sergey Semenov Date: Fri, 28 Feb 2020 14:22:15 +0300 Subject: [PATCH 4/6] Remove leftover forward decl Signed-off-by: Sergey Semenov --- sycl/source/detail/event_impl.hpp | 1 - 1 file changed, 1 deletion(-) diff --git a/sycl/source/detail/event_impl.hpp b/sycl/source/detail/event_impl.hpp index b558b633d1d8e..b9c8d4fa17de5 100644 --- a/sycl/source/detail/event_impl.hpp +++ b/sycl/source/detail/event_impl.hpp @@ -25,7 +25,6 @@ class context_impl; using ContextImplPtr = std::shared_ptr; class queue_impl; using QueueImplPtr = std::shared_ptr; -class Command; class event_impl { public: From 772b99dcf2cfcea82f13d35942cfa956219421ef Mon Sep 17 00:00:00 2001 From: Sergey Semenov Date: Fri, 28 Feb 2020 16:15:31 +0300 Subject: [PATCH 5/6] Address comments Signed-off-by: Sergey Semenov --- sycl/source/detail/scheduler/scheduler.cpp | 2 ++ sycl/test/scheduler/CommandCleanupThreadSafety.cpp | 3 ++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/sycl/source/detail/scheduler/scheduler.cpp b/sycl/source/detail/scheduler/scheduler.cpp index 6cff0d9d1fbca..37c2529d44863 100644 --- a/sycl/source/detail/scheduler/scheduler.cpp +++ b/sycl/source/detail/scheduler/scheduler.cpp @@ -126,6 +126,8 @@ void Scheduler::waitForEvent(EventImplPtr Event) { void Scheduler::cleanupFinishedCommands(EventImplPtr FinishedEvent) { std::lock_guard lock(MGraphLock); Command *FinishedCmd = static_cast(FinishedEvent->getCommand()); + // The command might have been cleaned up (and set to nullptr) by another + // thread if (FinishedCmd) MGraphBuilder.cleanupFinishedCommands(FinishedCmd); } diff --git a/sycl/test/scheduler/CommandCleanupThreadSafety.cpp b/sycl/test/scheduler/CommandCleanupThreadSafety.cpp index a6d1bca4cfb6c..ed9e227321fdb 100644 --- a/sycl/test/scheduler/CommandCleanupThreadSafety.cpp +++ b/sycl/test/scheduler/CommandCleanupThreadSafety.cpp @@ -26,7 +26,8 @@ int main() { // Create multiple commands, each one dependent on the previous std::vector Events; - for (std::size_t I = 0; I < 16; ++I) + const std::size_t NTasks = 16; + for (std::size_t I = 0; I < NTasks; ++I) Events.push_back(submitTask(Q, Buf)); // Initiate cleanup from multiple threads From d15a735e59296e48ea23ca79c0b9f99126821b89 Mon Sep 17 00:00:00 2001 From: Sergey Semenov Date: Fri, 28 Feb 2020 17:17:11 +0300 Subject: [PATCH 6/6] Turn off the test on windows due to use of pthread Signed-off-by: Sergey Semenov --- sycl/test/scheduler/CommandCleanupThreadSafety.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/sycl/test/scheduler/CommandCleanupThreadSafety.cpp b/sycl/test/scheduler/CommandCleanupThreadSafety.cpp index ed9e227321fdb..f1d39db4e8995 100644 --- a/sycl/test/scheduler/CommandCleanupThreadSafety.cpp +++ b/sycl/test/scheduler/CommandCleanupThreadSafety.cpp @@ -1,3 +1,4 @@ +// UNSUPPORTED: windows // RUN: %clangxx -fsycl %s -o %t.out -lpthread // RUN: %CPU_RUN_PLACEHOLDER %t.out #include