[SYCL][L0] Fix memory leak when tracking indirect accesses #7105
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
On the plugin API boundaries extra piKernelRetain/piKernelRelease can be called in arbitrary moments (from other threads) include *before* the kernel was submitted to the actual GPU HW. As such, piKernelRelease has nothing to do with submissions and indirect memory tracking updates should be initiated from the points where the kernel actually finishes - CleanupCompletedEvent.
againull
reviewed
Oct 20, 2022
Comment on lines
5819
to
5842
| if (AssociatedKernel) { | ||
| if (IndirectAccessTrackingEnabled) { | ||
| auto Kernel = AssociatedKernel; | ||
| // piKernelRelease is called by CleanupCompletedEvent(Event) as soon as | ||
| // kernel execution has finished. This is the place where we need to | ||
| // release memory allocations. If kernel is not in use (not submitted by | ||
| // some other thread) then release referenced memory allocations. As a | ||
| // result, memory can be deallocated and context can be removed from | ||
| // container in the platform. That's why we need to lock a mutex here. | ||
| pi_platform Plt = Kernel->Program->Context->getPlatform(); | ||
| std::scoped_lock<pi_shared_mutex> ContextsLock(Plt->ContextsMutex); | ||
|
|
||
| if (--Kernel->SubmissionsCount == 0) { | ||
| // Kernel is not submitted for execution, release referenced memory | ||
| // allocations. | ||
| for (auto &MemAlloc : Kernel->MemAllocs) { | ||
| // std::pair<void *const, MemAllocRecord> *, Hash | ||
| USMFreeHelper(MemAlloc->second.Context, MemAlloc->first, | ||
| MemAlloc->second.OwnZeMemHandle); | ||
| } | ||
| Kernel->MemAllocs.clear(); | ||
| } | ||
| } | ||
| PI_CALL(piKernelRelease(AssociatedKernel)); |
There was a problem hiding this comment.
We call piKernelRelease for dependent events below in this function - https://github.com/intel/llvm/pull/7105/files#diff-15dd1eb076d2164bd9e87d9057f05f652a716498e8cdf5975e564c65309a0985L5895-L5896
It looks like we need to do the same for them as well.
againull
approved these changes
Oct 24, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
On the plugin API boundaries extra piKernelRetain/piKernelRelease can be called in arbitrary moments (from other threads) include before the kernel was submitted to the actual GPU HW. As such, piKernelRelease has nothing to do with submissions and indirect memory tracking updates should be initiated from the points where the kernel actually finishes - CleanupCompletedEvent.