[SYCL] Add thread safety to host-task submit to in-order queue #4395
Conversation
Signed-off-by: Sergey Kanaev <sergey.kanaev@intel.com>
Signed-off-by: Sergey Kanaev <sergey.kanaev@intel.com>
s-kanaev
force-pushed
the
thread-safe-in-order-queue
branch
from
0ed5b4c to
9107613
Compare
Signed-off-by: Sergey Kanaev <sergey.kanaev@intel.com>
|
@smaslov-intel , a friendly ping |
| if (MIsInorder && (Type == CG::CGTYPE::CodeplayHostTask || | ||
| Type == CG::CGTYPE::CodeplayInteropTask)) | ||
| Handler.depends_on(MLastEvent); | ||
| bool NeedSeparateDependencyMgmt = |
There was a problem hiding this comment.
This is not used in this function other than passing it to finalizeHandler, Please just move it there.
| if (MIsInorder) { | ||
| // Accessing and changing of an event isn't atomic operation. |
There was a problem hiding this comment.
What does spec say about thread-safety of the in-order queue? If multiple threads concurrently submit commands to the same in-order queue then no order is guaranteed, right?
There was a problem hiding this comment.
The spec says:
The in_order property adds the requirement that the SYCL queue provides in-order semantics where tasks are executed in the order in which they are submitted. Tasks submitted in this fashion can be viewed as having an implicit dependence on the previously submitted operation.
I believe, the following can be considered as correct.
When several threads attempt to submit a command simultaneously, the implementation should provide ordering between them.
There was a problem hiding this comment.
the implementation should provide ordering between them.
But this would be implementation-defined ordering, right? So no clear in-order semantics.
Is this change fixing exactly the case where multiple threads submit simultaneously?
Was it just abnormally failing without this change?
There was a problem hiding this comment.
But this would be implementation-defined ordering, right? So no clear in-order semantics.
Correct. Although, I can't see other ordering than impl-defined for the case of simultaneous calls.
Is this change fixing exactly the case where multiple threads submit simultaneously?
Absolutely. Probably, there should be spin-lock instead of mutex in some cases.
Was it just abnormally failing without this change?
I didn't observe such failures yet. This issue was found by eyeballing. I expect the issue is hard to reproduce due small size of non-atomic change.
There was a problem hiding this comment.
NIT: I suggest adding a comment explaining that this lock doesn't provide any specific order for the in-order queue, but just to served competing requests safely (in arbitrary order).
No description provided.