Skip to content

[SYCL][PL][L0] Fixes problem with possible deep recursion. #3093

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 2 commits into from
Jan 27, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
33 changes: 26 additions & 7 deletions sycl/plugins/level_zero/pi_level_zero.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -827,7 +827,8 @@ static void printZeEventList(_pi_ze_event_list_t &PiZeEventList) {
zePrint("\n");
}

pi_result _pi_ze_event_list_t::releaseAndDestroyPiZeEventList() {
pi_result _pi_ze_event_list_t::collectEventsForReleaseAndDestroyPiZeEventList(
std::list<pi_event> &EventsToBeReleased) {
// acquire a lock before reading the length and list fields.
// Acquire the lock, copy the needed data locally, and reset
// the fields, then release the lock.
Expand All @@ -854,7 +855,8 @@ pi_result _pi_ze_event_list_t::releaseAndDestroyPiZeEventList() {
}

for (pi_uint32 I = 0; I < LocLength; I++) {
piEventRelease(LocPiEventList[I]);
// Add the event to be released to the list
EventsToBeReleased.push_back(LocPiEventList[I]);
}

if (LocZeEventList != nullptr) {
Expand Down Expand Up @@ -3755,6 +3757,7 @@ pi_result piEventGetProfilingInfo(pi_event Event, pi_profiling_info ParamName,
// This currently recycles the associate command list, and also makes
// sure to release any kernel that may have been used by the event.
static void cleanupAfterEvent(pi_event Event) {
zePrint("cleanupAfterEvent entry\n");
// The implementation of this is slightly tricky. The same event
// can be referred to by multiple threads, so it is possible to
// have a race condition between the read of fields of the event,
Expand Down Expand Up @@ -3798,11 +3801,27 @@ static void cleanupAfterEvent(pi_event Event) {
}
}

// Had to make sure lock of Event's Queue has been released before
// the code starts to release the event wait list, as that could
// potentially cause recursive calls to cleanupAfterEvent, and
// run into a problem trying to do multiple locks on the same Queue.
Event->WaitList.releaseAndDestroyPiZeEventList();
// Make a list of all the dependent events that must have signalled
// because this event was dependent on them. This list will be appended
// to as we walk it so that this algorithm doesn't go recursive
// due to dependent events themselves being dependent on other events
// forming a potentially very deep tree, and deep recursion. That
// turned out to be a significant problem with the recursive code
// that preceded this implementation.

std::list<pi_event> EventsToBeReleased;

Event->WaitList.collectEventsForReleaseAndDestroyPiZeEventList(
EventsToBeReleased);

while (!EventsToBeReleased.empty()) {
pi_event DepEvent = EventsToBeReleased.front();
EventsToBeReleased.pop_front();

DepEvent->WaitList.collectEventsForReleaseAndDestroyPiZeEventList(
EventsToBeReleased);
piEventRelease(DepEvent);
}
zePrint("cleanupAfterEvent exit\n");
}

Expand Down
8 changes: 5 additions & 3 deletions sycl/plugins/level_zero/pi_level_zero.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -509,9 +509,11 @@ struct _pi_ze_event_list_t {
const pi_event *EventList,
pi_queue CurQueue);

// Release all the events in this object's PiEventList, and destroy
// the data structures it contains.
pi_result releaseAndDestroyPiZeEventList();
// Add all the events in this object's PiEventList to the end
// of the list EventsToBeReleased. Destroy pi_ze_event_list_t data
// structure fields making it look empty.
pi_result collectEventsForReleaseAndDestroyPiZeEventList(
std::list<pi_event> &EventsToBeReleased);

// Had to create custom assignment operator because the mutex is
// not assignment copyable. Just field by field copy of the other
Expand Down