Skip to content

[SYCL] Fix race that occurs when submitting to single queue in parallel #1872

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

Jump to bottom
Merged
merged 14 commits into from
Jul 2, 2020
Merged

[SYCL] Fix race that occurs when submitting to single queue in parallel #1872

merged 14 commits into from
Jul 2, 2020

Conversation

alexanderfle
Copy link
Contributor

@alexanderfle alexanderfle commented Jun 11, 2020 •
edited
Loading

This PR aims to fix the race when setting kernel parameters in parallel. That occurs when submitting to single queue by multiple threads.

  • In cases when we use cacheable kernel we must use mutex, as we share the same instance by threads.
  • In cases when we don't use cacheable kernel each thread has its own instance of the kernel and so race-condition doesn't occur.

Regression: queue submit. single queue

Signed-off-by: Alexander Flegontov alexander.flegontov@intel.com

@alexanderfle
[SYCL] Fix race that occurs when submitting to single queue in parallel
3ec79ba 
Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>
@alexanderfle alexanderfle requested a review from a team as a code owner June 11, 2020 10:03
@alexanderfle alexanderfle requested a review from v-klochkov June 11, 2020 10:03
s-kanaev
s-kanaev reviewed Jun 11, 2020
View reviewed changes
Copy link
Contributor

@s-kanaev s-kanaev left a comment

Choose a reason for hiding this comment

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

I believe we need a test for this case also.

romanovvlad
romanovvlad reviewed Jun 11, 2020
View reviewed changes
@romanovvlad
Copy link
Contributor

Also, could you please develop a test for this?

@alexanderfle
[SYCL] Apply comment from review about NewCmd
939dc81 
Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>
@alexanderfle
[SYCL] Added distinct lock for enqueue new command
edbbcf4 
Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>
s-kanaev
s-kanaev reviewed Jun 11, 2020
View reviewed changes
@alexanderfle
[SYCL] Apply clang-format
f1faabe 
Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>
@alexanderfle
[SYCL] Apply a single lock in Scheduler::addCG(), change locking in o…
301a0f1 
…ther places where enqueue takes place

Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>
@alexanderfle
[SYCL] Fix hang in Lit tests
2870269 
Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>
@alexanderfle
[SYCL] Fix hang in Lit with Cuda
2e4b1d3 
Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>
@alexanderfle
Copy link
Contributor Author

@s-kanaev , @romanovvlad I addressed your comments and applied as much as possible to pass all the Lit tests without fails and hangs. Could you please take a look and approve the current version of the patch?

s-kanaev
s-kanaev reviewed Jun 15, 2020
View reviewed changes
s-kanaev
s-kanaev reviewed Jun 15, 2020
View reviewed changes
Copy link
Contributor

@s-kanaev s-kanaev left a comment

Choose a reason for hiding this comment

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

Also we need a test for the case.
Just a reminder.

@alexanderfle
Copy link
Contributor Author

We already have a test for this, at the moment, which regresses on Linux and Windows and this patch fixes this problem.

@smaslov-intel smaslov-intel mentioned this pull request Jun 15, 2020
Merged
@alexanderfle
[SYCL] Add per-kernel mutex to fix the race when setting kernel param…
0bd1bf9 
…eters in parallel

Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>
@alexanderfle alexanderfle requested a review from kbobrovs as a code owner June 22, 2020 10:18
@alexanderfle alexanderfle requested a review from s-kanaev June 22, 2020 16:48
s-kanaev
s-kanaev reviewed Jun 22, 2020
View reviewed changes
@alexanderfle
[SYCL] Use lock_guard to lock mutex
fcb4e6c 
Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>
@alexanderfle
[SYCL] Use per-kernel mutex for all cases when the kernel is cacheable.
0dca88f 
Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>
s-kanaev
s-kanaev reviewed Jun 24, 2020
View reviewed changes
@alexanderfle
Copy link
Contributor Author

@s-kanaev , @kbobrovs , in the scope of the PR, I have done all. Could you please review it?

s-kanaev
s-kanaev previously approved these changes Jun 24, 2020
View reviewed changes
Copy link
Contributor

@s-kanaev s-kanaev left a comment

Choose a reason for hiding this comment

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

LGTM

kbobrovs
kbobrovs reviewed Jun 24, 2020
View reviewed changes
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.

Can a test be added?

@alexanderfle
Copy link
Contributor Author

@kbobrovs , We already have one regression test fails in our tests suit without this fix, and also I added an additional regression test in separate PR in our test suit, where I covered one case mentioned above as nullptr != ExecKernel->MSyclKernel.
So, I believe that here I have done all.

@alexanderfle alexanderfle requested a review from kbobrovs June 25, 2020 07:34
kbobrovs
kbobrovs reviewed Jun 25, 2020
View reviewed changes
@kbobrovs
Copy link
Contributor

@kbobrovs , We already have one regression test fails in our tests suit without this fix, and also I added an additional regression test in separate PR in our test suit, where I covered one case mentioned above as nullptr != ExecKernel->MSyclKernel.
So, I believe that here I have done all.

I could not find which test was failing in this PR. Would be good to mention then in the description.

kbobrovs
kbobrovs previously approved these changes Jun 25, 2020
View reviewed changes
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.

program_manager-related part LGTM.

@alexanderfle
[SYCL] Refactoring of lambda into a separate function
12502ae 
Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>
@alexanderfle alexanderfle dismissed stale reviews from kbobrovs and s-kanaev via 12502ae June 26, 2020 09:17
s-kanaev
s-kanaev previously approved these changes Jun 29, 2020
View reviewed changes
@alexanderfle
Copy link
Contributor Author

@kbobrovs , Could you please take a look again and approve?

@alexanderfle
Copy link
Contributor Author

@kbobrovs , ping

kbobrovs
kbobrovs previously approved these changes Jun 30, 2020
View reviewed changes
@alexanderfle
Copy link
Contributor Author

@bader , Could you please merge?

@alexanderfle alexanderfle dismissed stale reviews from kbobrovs and s-kanaev via 615ba25 July 2, 2020 06:58
@alexanderfle alexanderfle requested a review from kbobrovs July 2, 2020 07:40
@bader
Copy link
Contributor

bader commented Jul 2, 2020

@alexanderfle, please, fix CI failures.

@alexanderfle
Copy link
Contributor Author

@kbobrovs , ping

kbobrovs
kbobrovs approved these changes Jul 2, 2020
View reviewed changes
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

@alexanderfle
Copy link
Contributor Author

@bader , ping

@bader bader merged commit 95d3ec6 into intel:sycl Jul 2, 2020
@alexanderfle alexanderfle mentioned this pull request Aug 7, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sergey-semenov sergey-semenov sergey-semenov left review comments

@kbobrovs kbobrovs kbobrovs approved these changes

@s-kanaev s-kanaev s-kanaev approved these changes

@v-klochkov v-klochkov Awaiting requested review from v-klochkov

@romanovvlad romanovvlad Awaiting requested review from romanovvlad

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@alexanderfle @romanovvlad @kbobrovs @bader @sergey-semenov @s-kanaev