Lunderberg commented on code in PR #13609:
URL: https://github.com/apache/tvm/pull/13609#discussion_r1060817672
##########
src/runtime/hexagon/hexagon_thread_manager.cc:
##########
@@ -265,9 +265,15 @@ void HexagonThreadManager::WaitOnThreads() {
}
void HexagonThreadManager::CheckSemaphore(unsigned syncID) {
+ // We want the success case to be fast, so do not lock the mutex
Review Comment:
I think there's still a case where this could result in a race condition,
though it's much less likely than before. The race condition would occur
between one thread that is reading `semaphores_[0]` inside
`HexagonThreadManager::Signal`, while another thread is writing to
`semaphores_[1]` inside `HexagonThreadManager::CheckSemaphore`. The const
methods of `std::unordered_map` are safe to run at the same time as other const
methods, but aren't safe to run at the same time as non-const methods.
I don't think we need to change anything now, but if we have mystery
multi-threading bugs in the future, it may be worth revisiting. The changes
that we could make would be as follows:
1. Change signature from `void CheckSemaphor(unsigned syncID)`, the function
could instead be `qurt_sem_t* GetSemaphore(unsigned syncID)`. This way, we
don't need the call to `semaphores_[syncID]` inside
`HexagonThreadManager::Signal` and `HexagonThreadManager::Wait`.
2. Thread-local cache of the map from syncID to `qurt_sem_t*`. The
thread-local cache can be checked without a mutex, preserving the lockfree fast
path. If the thread-local cache doesn't have the desired semaphore, then a
mutex can protect the global semaphore map, to either read from it (if already
created on another thread) or write to it (if not already found).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
