Skip to content

[SYCL] Fix hang in ProgramManager class #2274

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.

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

Conversation

alexanderfle
Copy link
Contributor

This patch fixes sporadic hangs caused by data race in NativePrograms variable of the ProgramManager class.
The current mutex lock from the program cache is not thread-safe.
When multiple threads use their own queues then they use different contexts and then they use their own program caches.
And since multiple threads can queue own command of kernel execution in parallel,
then each thread will lock its own program cache mutex but the access into NativePrograms variable remains with a race between the threads.

  • Added separate mutex and lock guards when accessing NativePrograms variable

Signed-off-by: Alexander Flegontov [email protected]

Signed-off-by: Alexander Flegontov <[email protected]>
@alexanderfle alexanderfle requested a review from kbobrovs as a code owner August 6, 2020 15:53
@alexanderfle alexanderfle requested review from a team and rbegam and removed request for a team August 7, 2020 08:48
Signed-off-by: Alexander Flegontov <[email protected]>
dm-vodopyanov
dm-vodopyanov previously approved these changes Aug 10, 2020
Copy link
Contributor

@dm-vodopyanov dm-vodopyanov left a comment

Choose a reason for hiding this comment

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

LGTM.
Minor comment: all other lock_guards in ProgramManager class lock mutexes of std::mutex type instead of mutex_class, suggest to change MNativeProgramsMutex's type to std::mutex for consistency.

@alexanderfle
Copy link
Contributor Author

@kbobrovs, Could you please review this PR?

Copy link
Contributor

@dm-vodopyanov dm-vodopyanov left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@rbegam rbegam left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@kbobrovs kbobrovs left a comment

Choose a reason for hiding this comment

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

LGTM

@bader bader merged commit e6fd911 into intel:sycl Aug 12, 2020
jsji pushed a commit that referenced this pull request Dec 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants