Skip to content

[Hexagon][runtime] Make HexagonThreadManager::CheckSemaphore thread safe #13609

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Dec 17, 2022
Merged

[Hexagon][runtime] Make HexagonThreadManager::CheckSemaphore thread safe #13609

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c262c2e
Protect CheckSemaphore with mutex
janetsc Nov 30, 2022
559371f
Merge branch 'apache:main' into check_semaphore
janetsc Dec 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions src/runtime/hexagon/hexagon_thread_manager.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

@Lunderberg Lunderberg Jan 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

@janetsc janetsc Jan 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point - thank you! I'll file a task for this.

if (semaphores_.find(syncID) == semaphores_.end()) {
semaphores_[syncID] = reinterpret_cast<qurt_sem_t*>(malloc(sizeof(qurt_sem_t)));
qurt_sem_init_val(semaphores_[syncID], 0);
// If we don't find it, lock the mutex, make sure it hasn't
// been added by another thread before creating it.
std::lock_guard<std::mutex> lock(semaphores_mutex_);
if (semaphores_.find(syncID) == semaphores_.end()) {
semaphores_[syncID] = reinterpret_cast<qurt_sem_t*>(malloc(sizeof(qurt_sem_t)));
qurt_sem_init_val(semaphores_[syncID], 0);
}
}
}

Expand Down
3 changes: 3 additions & 0 deletions src/runtime/hexagon/hexagon_thread_manager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,9 @@ class HexagonThreadManager {
//! \brief Semaphores used by `Signal` and `Wait` mapped by ID.
std::unordered_map<unsigned, qurt_sem_t*> semaphores_;

//! \brief Protects updates to semaphores_
std::mutex semaphores_mutex_;

//! \brief Start semaphore created at time of construction; signled by `Start`.
qurt_sem_t start_semaphore_;

Expand Down