From da5f1a6146a938310bed701971b58df5d892a5cf Mon Sep 17 00:00:00 2001 From: Vladana Stojiljkovic Date: Mon, 9 Sep 2024 15:52:15 +0200 Subject: [PATCH] SWDEV-482086 - Fix hipGraphInstantiate leak * In a scenario where kernel is launched with hipExtLaunchKernelGGL and stop event is used, hipGraphInstantiate leaks. Since stop event is used, profiling is enabled and Timestamp (ReferencedCountedObject) is created, but it doesn't get released. * The idea behind this solution is that profiling should be disabled when command is captured, hence the timestamp should not be created. Because information about capturing isn't available when kernel command is created, packet capturing state is used to determine whether to create a timestamp or not. Change-Id: Ia23adac4592ded4fb5e236acf99e12e729f63692 --- hipamd/src/hip_graph_internal.hpp | 4 ++-- rocclr/device/rocm/rocvirtual.cpp | 16 +++++++++------- rocclr/platform/command.hpp | 8 ++++---- 3 files changed, 15 insertions(+), 13 deletions(-) diff --git a/hipamd/src/hip_graph_internal.hpp b/hipamd/src/hip_graph_internal.hpp index 67fdfd1066..12fb04e29d 100644 --- a/hipamd/src/hip_graph_internal.hpp +++ b/hipamd/src/hip_graph_internal.hpp @@ -250,7 +250,7 @@ struct GraphNode : public hipGraphNodeDOTAttribute { node->RemoveEdge(this); } for (auto packet : gpuPackets_) { - delete packet; + delete[] packet; } amd::ScopedLock lock(nodeSetLock_); nodeSet_.erase(this); @@ -275,7 +275,7 @@ struct GraphNode : public hipGraphNodeDOTAttribute { hipError_t status = CreateCommand(capture_stream); gpuPackets_.clear(); for (auto& command : commands_) { - command->setCapturingState(true, &gpuPackets_, kernArgMgr, &capturedKernelName_); + command->setPktCapturingState(true, &gpuPackets_, kernArgMgr, &capturedKernelName_); // Enqueue command to capture GPU Packet. The packet is not submitted to the device. // The packet is stored in gpuPacket_ and submitted during graph launch. command->submit(*(command->queue())->vdev()); diff --git a/rocclr/device/rocm/rocvirtual.cpp b/rocclr/device/rocm/rocvirtual.cpp index ea3716b4fb..c173d60dd0 100644 --- a/rocclr/device/rocm/rocvirtual.cpp +++ b/rocclr/device/rocm/rocvirtual.cpp @@ -1545,7 +1545,9 @@ address VirtualGPU::allocKernelArguments(size_t size, size_t alignment) { * and then calls start() to get the current host timestamp. */ void VirtualGPU::profilingBegin(amd::Command& command, bool sdmaProfiling) { - if (command.profilingInfo().enabled_) { + // Disable profiling when command is being captured to prevent memory leak from created timestamp_ + // which won't get freed, since the command is not being executed until graph launch + if (!command.getPktCapturingState() && command.profilingInfo().enabled_) { if (timestamp_ != nullptr) { LogWarning("Trying to create a second timestamp in VirtualGPU. \ This could have unintended consequences."); @@ -1581,7 +1583,7 @@ void VirtualGPU::profilingBegin(amd::Command& command, bool sdmaProfiling) { } } } - if (command.getCapturingState()) { + if (command.getPktCapturingState()) { currCmd_ = &command; } } @@ -1592,7 +1594,7 @@ void VirtualGPU::profilingBegin(amd::Command& command, bool sdmaProfiling) { * current host timestamp if no signal is available. */ void VirtualGPU::profilingEnd(amd::Command& command) { - if (command.profilingInfo().enabled_) { + if (!command.getPktCapturingState() && command.profilingInfo().enabled_) { if (timestamp_->HwProfiling() == false) { timestamp_->end(); } @@ -3111,7 +3113,7 @@ bool VirtualGPU::submitKernelInternal(const amd::NDRangeContainer& sizes, amd::Memory* const* memories = reinterpret_cast(parameters + kernelParams.memoryObjOffset()); - bool isGraphCapture = currCmd_ != nullptr && currCmd_->getCapturingState(); + bool isGraphCapture = currCmd_ != nullptr && currCmd_->getPktCapturingState(); for (int j = 0; j < iteration; j++) { // Reset global size for dimension dim if split is needed if (dim != -1) { @@ -3424,9 +3426,9 @@ bool VirtualGPU::submitKernelInternal(const amd::NDRangeContainer& sizes, if (isGraphCapture) { // Dispatch the packet if (!dispatchAqlPacket(&dispatchPacket, aqlHeaderWithOrder, - (sizes.dimensions() << HSA_KERNEL_DISPATCH_PACKET_SETUP_DIMENSIONS), - GPU_FLUSH_ON_EXECUTION, currCmd_->getCapturingState(), - currCmd_->getAqlPacket())) { + (sizes.dimensions() << HSA_KERNEL_DISPATCH_PACKET_SETUP_DIMENSIONS), + GPU_FLUSH_ON_EXECUTION, currCmd_->getPktCapturingState(), + currCmd_->getAqlPacket())) { return false; } } else { diff --git a/rocclr/platform/command.hpp b/rocclr/platform/command.hpp index 1a98349172..ef6e4b2d93 100644 --- a/rocclr/platform/command.hpp +++ b/rocclr/platform/command.hpp @@ -268,7 +268,7 @@ class Command : public Event { std::vector data_; const Event* waitingEvent_; //!< Waiting event associated with the marker - bool capturing_ = false; //!< Flag to enable/disable graph gpu packet capture + bool packetCapturing_ = false; //!< Flag to enable/disable graph gpu packet capture std::vector* gpuPackets_; //!< GPU packets captured when graph capturing is enabled GraphKernelArgManager* graphKernArgMgr_ = nullptr; //!< KernelMgr for graph address kernArgOffset_ = nullptr; //!< KernelArg buffer to used when graph capturing is enabled @@ -316,13 +316,13 @@ class Command : public Event { command_pool_ = nullptr; } } - bool getCapturingState() const { return capturing_; } + bool getPktCapturingState() const { return packetCapturing_; } //! Sets AQL capture state, aql packet to capture and where to copy kernArgs - void setCapturingState(bool state, std::vector* packet, + void setPktCapturingState(bool state, std::vector* packet, amd::GraphKernelArgManager* graphKernArgMgr, std::string* capturedKernelName) { - capturing_ = state; + packetCapturing_ = state; gpuPackets_ = packet; graphKernArgMgr_ = graphKernArgMgr; capturedKernelName_ = capturedKernelName;