[SYCL][Graph] Makes command graph functions thread-safe #265
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mfrancepillois
added
the
Graph Implementation
mfrancepillois
force-pushed
the
maxime/thread-safe
branch
from
2ac3537
to
a2c1b06
Compare
EwanC
reviewed
Jul 17, 2023
Bensuo
requested changes
Jul 17, 2023
Base automatically changed from
maxime/graph_debug_functions_print_check_sim
to
sycl-graph-develop
July 18, 2023 07:51
EwanC
reviewed
Jul 19, 2023
Bensuo
reviewed
Jul 20, 2023
mfrancepillois
force-pushed
the
maxime/thread-safe
branch
6 times, most recently
from
91bb734
to
487e668
Compare
Bensuo
approved these changes
Jul 24, 2023
EwanC
approved these changes
Aug 1, 2023
reble
reviewed
Aug 2, 2023
| using WriteLock = std::unique_lock<std::shared_mutex>; | ||
|
|
||
| /// Protects all the fields that can be changed by class' methods. | ||
| mutable std::shared_mutex MMutex; |
There was a problem hiding this comment.
Since each executable and modifiable graph has its own mutex, and there is a 1:n mapping of modifiable to executable graphs I see the potential for a deadlock. Because the order how we lock the graph becomes important. Do we have a test in place that would catch this?
There was a problem hiding this comment.
If I understand your comment correctly, you are asking if there is a potential risk for deadlock due to cross mutex locking due to multiple graph_exec referencing the same graph_impl, right?
Thread1: Thread2:
A.lock() B.lock()
B.lock() A.lock()
If so, given that graph_impl does not reference graph_exec, the only case where it could append is:
Thread1: Thread2:
graph_impl.begin_recording(queue)
graph_impl.add(ext_oneapi_graph(graph_exec)) queue.submit(ext_oneapi_graph(graph_exec))
However, since graph_exec is only locked with read permission (handle.cpp l.1054), this cannot create a deadlock.
Also, it is not allowed to use Explicit API and Record&Replay API at the same time for the same graph.
There was a problem hiding this comment.
Yes your understanding is correct. It might help to document somewhere that we'll have to take the locks always in the same order, e.g. modifiable first then executable. This could help reducing the risk of introducing a deadlock in the future.
There was a problem hiding this comment.
I added a comment.
reble
reviewed
Aug 2, 2023
Addresses comments made on the first PR commit. Mutexes are now added to Graph implementation entry points instead of end points as was the case in the previous commit. Adds "build_pthread_inc" lit test macro to facilitate the compilation of the threading tests. Removes std::barrier (std-20) dependency in threading tests. Addresses Issue: #85
Moves threading tests that do not require a device to run to unitests
Co-authored-by: Ben Tracy <ben.tracy@codeplay.com>
Adds some comments.
Co-authored-by: Pablo Reble <pablo.reble@intel.com>
Co-authored-by: Ewan Crawford <ewan@codeplay.com>
Adds dedidacted sub-class to unitests for multi-threading unitests
mfrancepillois
force-pushed
the
maxime/thread-safe
branch
from
727d6ef
to
1597546
Compare
mfrancepillois
added a commit
that referenced
this pull request
Aug 4, 2023
* [SYCL][Graph] Makes command graph functions thread-safe Addresses comments made on the first PR commit. Mutexes are now added to Graph implementation entry points instead of end points as was the case in the previous commit. Adds "build_pthread_inc" lit test macro to facilitate the compilation of the threading tests. Removes std::barrier (std-20) dependency in threading tests. Addresses Issue: #85 * [SYCL][Graph] Makes command graph functions thread-safe Moves threading tests that do not require a device to run to unitests * Update sycl/source/detail/graph_impl.cpp Co-authored-by: Ben Tracy <ben.tracy@codeplay.com> * [SYCL][Graph] Makes command graph functions thread-safe Adds some comments. * Update sycl/source/handler.cpp Co-authored-by: Pablo Reble <pablo.reble@intel.com> * Update sycl/source/detail/graph_impl.hpp Co-authored-by: Ewan Crawford <ewan@codeplay.com> * [SYCL][Graph] Makes command graph functions thread-safe Adds dedidacted sub-class to unitests for multi-threading unitests * [SYCL][Graph] Makes command graph functions thread-safe Adds comments * [SYCL][Graph] thread-safe: bug fix after rebase --------- Co-authored-by: Ben Tracy <ben.tracy@codeplay.com> Co-authored-by: Pablo Reble <pablo.reble@intel.com> Co-authored-by: Ewan Crawford <ewan@codeplay.com>
EwanC
added a commit
that referenced
this pull request
Aug 10, 2023
* [SYCL][Graph] Makes command graph functions thread-safe Addresses comments made on the first PR commit. Mutexes are now added to Graph implementation entry points instead of end points as was the case in the previous commit. Adds "build_pthread_inc" lit test macro to facilitate the compilation of the threading tests. Removes std::barrier (std-20) dependency in threading tests. Addresses Issue: #85 * [SYCL][Graph] Makes command graph functions thread-safe Moves threading tests that do not require a device to run to unitests * Update sycl/source/detail/graph_impl.cpp Co-authored-by: Ben Tracy <ben.tracy@codeplay.com> * [SYCL][Graph] Makes command graph functions thread-safe Adds some comments. * Update sycl/source/handler.cpp Co-authored-by: Pablo Reble <pablo.reble@intel.com> * Update sycl/source/detail/graph_impl.hpp Co-authored-by: Ewan Crawford <ewan@codeplay.com> * [SYCL][Graph] Makes command graph functions thread-safe Adds dedidacted sub-class to unitests for multi-threading unitests * [SYCL][Graph] Makes command graph functions thread-safe Adds comments * [SYCL][Graph] thread-safe: bug fix after rebase --------- Co-authored-by: Ben Tracy <ben.tracy@codeplay.com> Co-authored-by: Pablo Reble <pablo.reble@intel.com> Co-authored-by: Ewan Crawford <ewan@codeplay.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds mutual exclusions to make graph functions thread-safe.
Adds debugging functions to help check equivalence of two graphs
Improves threading tests to ensure implementation thread-safe property.
Addresses Issue: #85