Skip to content

[SYCL][CUDA] Use context mutex in refcount inc/dec #6577

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

Merged
merged 1 commit into from
Aug 16, 2022

Conversation

JackAKirk
Copy link
Contributor

context wasn't using its mutex when incrementing/decrementing the ref count, even though multiple threads may attempt this at the same time in multithreaded applications such as in assert_in_simultaneously_multiple_tus.cpp. This may fix #6463 but I can't be sure of this because I can't reproduce flaky CI test failures in #6463 locally.

Signed-off-by: JackAKirk jack.kirk@codeplay.com

Signed-off-by: JackAKirk <jack.kirk@codeplay.com>
@JackAKirk JackAKirk requested a review from a team as a code owner August 12, 2022 17:22
@JackAKirk JackAKirk temporarily deployed to aws August 12, 2022 17:45 Inactive
@JackAKirk JackAKirk temporarily deployed to aws August 12, 2022 18:02 Inactive
@JackAKirk JackAKirk closed this Aug 12, 2022
@JackAKirk JackAKirk reopened this Aug 12, 2022
@JackAKirk JackAKirk temporarily deployed to aws August 12, 2022 18:08 Inactive
@JackAKirk JackAKirk closed this Aug 12, 2022
@JackAKirk JackAKirk reopened this Aug 12, 2022
@JackAKirk JackAKirk temporarily deployed to aws August 12, 2022 18:21 Inactive
@JackAKirk JackAKirk closed this Aug 12, 2022
@JackAKirk JackAKirk reopened this Aug 12, 2022
@JackAKirk JackAKirk closed this Aug 12, 2022
@JackAKirk JackAKirk temporarily deployed to aws August 12, 2022 18:33 Inactive
@JackAKirk JackAKirk reopened this Aug 12, 2022
@JackAKirk JackAKirk temporarily deployed to aws August 12, 2022 18:40 Inactive
@JackAKirk JackAKirk closed this Aug 12, 2022
@JackAKirk JackAKirk reopened this Aug 12, 2022
@JackAKirk JackAKirk closed this Aug 13, 2022
@JackAKirk JackAKirk reopened this Aug 13, 2022
@steffenlarsen steffenlarsen merged commit 7eee446 into intel:sycl Aug 16, 2022
@sergey-semenov
Copy link
Contributor

@JackAKirk On second thought, shouldn't this patch do effectively nothing except add some overhead since refCount_ is atomic anyway?

@JackAKirk
Copy link
Contributor Author

JackAKirk commented Aug 18, 2022

@JackAKirk On second thought, shouldn't this patch do effectively nothing except add some overhead since refCount_ is atomic anyway?

Yes you are right... sorry, can this commit be reverted? I was too quick with this, the problem appeared to start soon after a commit that added additional usage of incrementing the ref count. I'll continue investigating #6463

@sergey-semenov
Copy link
Contributor

@JackAKirk I think it should be, please open a PR with the revert.

@JackAKirk
Copy link
Contributor Author

@JackAKirk I think it should be, please open a PR with the revert.

OK sure

@JackAKirk
Copy link
Contributor Author

@JackAKirk I think it should be, please open a PR with the revert.

Here it is #6603

steffenlarsen pushed a commit that referenced this pull request Aug 19, 2022
reverted unnecessary mutex

Signed-off-by: JackAKirk <jack.kirk@codeplay.com>
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.

Sporadic llvm-test-suite failures on CUDA back-end.
3 participants