Skip to content

Commit ec6fbe1

Browse files
[SYCL][L0] Fix problem with possible deep recursion. (#3093)
Fixes an issue where cleanupAfterEvent could potentially very deeply recurse releasing events from dependent wait lists. This algorithm does not recurse at all, but adds to a list of events to release, and iterates until the list is empty.
1 parent 7ed4f58 commit ec6fbe1

File tree

2 files changed

+31
-10
lines changed

2 files changed

+31
-10
lines changed

sycl/plugins/level_zero/pi_level_zero.cpp

Lines changed: 26 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -827,7 +827,8 @@ static void printZeEventList(_pi_ze_event_list_t &PiZeEventList) {
827827
zePrint("\n");
828828
}
829829

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

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

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

3801-
// Had to make sure lock of Event's Queue has been released before
3802-
// the code starts to release the event wait list, as that could
3803-
// potentially cause recursive calls to cleanupAfterEvent, and
3804-
// run into a problem trying to do multiple locks on the same Queue.
3805-
Event->WaitList.releaseAndDestroyPiZeEventList();
3804+
// Make a list of all the dependent events that must have signalled
3805+
// because this event was dependent on them. This list will be appended
3806+
// to as we walk it so that this algorithm doesn't go recursive
3807+
// due to dependent events themselves being dependent on other events
3808+
// forming a potentially very deep tree, and deep recursion. That
3809+
// turned out to be a significant problem with the recursive code
3810+
// that preceded this implementation.
3811+
3812+
std::list<pi_event> EventsToBeReleased;
3813+
3814+
Event->WaitList.collectEventsForReleaseAndDestroyPiZeEventList(
3815+
EventsToBeReleased);
3816+
3817+
while (!EventsToBeReleased.empty()) {
3818+
pi_event DepEvent = EventsToBeReleased.front();
3819+
EventsToBeReleased.pop_front();
3820+
3821+
DepEvent->WaitList.collectEventsForReleaseAndDestroyPiZeEventList(
3822+
EventsToBeReleased);
3823+
piEventRelease(DepEvent);
3824+
}
38063825
zePrint("cleanupAfterEvent exit\n");
38073826
}
38083827

sycl/plugins/level_zero/pi_level_zero.hpp

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -509,9 +509,11 @@ struct _pi_ze_event_list_t {
509509
const pi_event *EventList,
510510
pi_queue CurQueue);
511511

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

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

0 commit comments

Comments
 (0)