[SYCL][L0] Fix interop event leak

[bso-intel]

The retain call for Level Zero backend causes a memory leak.
We can avoid this unnecessary call by adding the condition.

[cperkinsintel]

LGTM

is there a matching test over on llvm-test-suite?

[bader]

@bso-intel, are these failures caused by the changes from your patch?

[bso-intel]

@bader ,
Yes. I am investigating....

[smaslov-intel]

Why is the event with "0" reference count is still flying around?

[bso-intel]

We call cleanup() when the RefCount of an Event reaches zero.

 if (--(Event->RefCount) == 0) {
    if (!Event->CleanedUp)
      Event->cleanup(LockedQueue);

[smaslov-intel]

Then do we call cleanup() when RefCount is > 0?

[bso-intel]

I am just answering to your question "Why is the event with "0" reference count is still flying around?".
We call cleanup() when RefCount becomes zero in piEventRelease(), and then We call EventRetain() in cleanup(), it is a circular dependency and causes a crash.
That's why we need to avoid calling piEventRelease() when cleanup() is called from piEventRelease().

[smaslov-intel]

If cleanup is only called on RefCount == 0 then why do we call EventRelease from it? I am not understanding the reason for the circular dependency now.

[bso-intel]

No, cleanup() is called even when RefCount is not zero.
That's why there is a call to piEventRelease() from cleanup().
For example, cleanup() is called from piEventsWait() and pi_queue::resetCommandList().
If cleanup() has been called only when RefCount is zero, this code should have been already broken.

[smaslov-intel]

OK, so event.cleanup() is called as long as the event is known to get completed. But the at cleanup() it will set the CleanedUp to true, so there should be no other cleanup() called on it (at destruction). There is a comment about it:

llvm/sycl/plugins/level_zero/pi_level_zero.cpp

Line 5330 in b01d820

// NOTE: that this needs to be done only once for an event so

Please explain why this check for RefCount == 0 is necessary (show the exact scenario).

[smaslov-intel]

@bso-intel : please follow up. I really don't want unnecessary RefCount checks which are indicative of some design misses if they are really needed.

[bso-intel]

Oops, I forgot to remove that.
Done now.

[againull]

I think we should also remove call to the cleanup from piEventRelease.
Event is retained explicitly when created in createEventAndAssociateQueue() to ensure that the event is not destroyed before it is really signaled. Ref count is decremented in Event->cleanup() after event is singaled. This
means that if reference count for an event is zero then cleanup was already called for this event and should not be called anymore. Interop events and user events don't have associated command list, queue,
kernel and waitlist, so we don't need to call cleanup() for interop and user
events.
I had an intention to do this earlier in 94943f3 but forgot about it.
Opened a PR for this small change: #6073.

[bso-intel]

@smaslov-intel
I added an additional piEventRetain() to match the reference count of events created by piEventCreate(). (line#5449).
Now all tests passed.

[bso-intel]

@smaslov-intel
I removed the condition.

[smaslov-intel]

Please put a more clear comment for this important change.
Where does this "retain" represent and where is the pairing "release"?

[smaslov-intel]

I realize the interop PI event created here would be released by the SYCL RT (at SYCL event's destructor) but what will wait until the event is completed?

[bso-intel]

We don't know. I think we should not make any assumption about interop event.

[smaslov-intel]

So we will not do the wait in SYCL RT, and hence the extra retain will cause this PI event to never be released.

[bso-intel]

In fact, I just checked SYCL_PI_TRACE and found that interop event is different than regular event.
Regular event created in piEventCreate() has ref count=2 at the beginning and then decremented twice before it is deallocated.
However, interop event is not created by createEventAndAssociateQueue(), so its ref count is 1, and piEventRelease() is called by RT once.
So, we don't have memory leak for interop event.

[smaslov-intel]

the Event is conctructed with RefCnt=1, so this retain should change it to 2.

[smaslov-intel]

Also, I wonder if we should really call to createEventAndAssociateQueue here as it may be having other wanted side-effects.

[bso-intel]

Sorry, I must have been too sick.
You are right. The refcount=2 after retain().
The current code is right because we actually release this interop event twice.
Once by SYCL RT and another time when we release the queue.
That's why we needed the retain here.

[smaslov-intel]

fixed in #6180