Skip to content

[SYCL] Add thread safety to host-task submit to in-order queue #4395

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 6 commits into from
Oct 7, 2021
Merged

[SYCL] Add thread safety to host-task submit to in-order queue #4395

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
92305e5
[SYCL] Add thread safety to host-task submit to in-order queue
Aug 24, 2021
9107613
Fixes and comments
Aug 25, 2021
cd0f0ee
Update queue_impl.hpp
Aug 25, 2021
8553620
Merge remote-tracking branch 'public/sycl' into thread-safe-in-order-…
Sep 9, 2021
0fe5f96
Merge remote-tracking branch 'public/sycl' into thread-safe-in-order-…
Sep 20, 2021
a8f52ab
Fix merge conflict resolution glitch
Sep 20, 2021
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
34 changes: 26 additions & 8 deletions sycl/source/detail/queue_impl.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,6 +419,23 @@ class queue_impl {
}

private:
void finalizeHandler(handler &Handler, bool NeedSeparateDependencyMgmt,
event &EventRet) {
if (MIsInorder) {
// Accessing and changing of an event isn't atomic operation.
Comment on lines +424 to +425
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

// Hence, here is the lock for thread-safety.
std::lock_guard<std::mutex> Lock{MLastEventMtx};

if (NeedSeparateDependencyMgmt)
Handler.depends_on(MLastEvent);

EventRet = Handler.finalize();

MLastEvent = EventRet;
} else
EventRet = Handler.finalize();
}

/// Performs command group submission to the queue.
///
/// \param CGF is a function object containing command group.
Expand All @@ -437,9 +454,9 @@ class queue_impl {
// Host and interop tasks, however, are not submitted to low-level runtimes
// and require separate dependency management.
const CG::CGTYPE Type = Handler.getType();
if (MIsInorder && (Type == CG::CGTYPE::CodeplayHostTask ||
Type == CG::CGTYPE::CodeplayInteropTask))
Handler.depends_on(MLastEvent);
bool NeedSeparateDependencyMgmt =
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not used in this function other than passing it to finalizeHandler, Please just move it there.

MIsInorder && (Type == CG::CGTYPE::CodeplayHostTask ||
Type == CG::CGTYPE::CodeplayInteropTask);

event Event;

Expand All @@ -452,14 +469,11 @@ class queue_impl {
: ProgramManager::getInstance().kernelUsesAssert(
Handler.MOSModuleHandle, Handler.MKernelName);

Event = Handler.finalize();
finalizeHandler(Handler, NeedSeparateDependencyMgmt, Event);

(*PostProcess)(IsKernel, KernelUsesAssert, Event);
} else
Event = Handler.finalize();

if (MIsInorder)
MLastEvent = Event;
finalizeHandler(Handler, NeedSeparateDependencyMgmt, Event);

addEvent(Event);
return Event;
Expand Down Expand Up @@ -521,7 +535,11 @@ class queue_impl {
// Buffer to store assert failure descriptor
buffer<AssertHappened, 1> MAssertHappenedBuffer;

// This event is employed for enhanced dependency tracking with in-order queue
// Access to the event should be guarded with MLastEventMtx
event MLastEvent;
std::mutex MLastEventMtx;

const bool MIsInorder;
};

Expand Down