From f7482ef0a6c5c6c2840a6d74016ede51aefa2f83 Mon Sep 17 00:00:00 2001 From: "Assiouras, Ioannis" Date: Tue, 13 May 2025 19:18:22 +0100 Subject: [PATCH] SWDEV-529449 - Bug fix when retrieving a memobj from the IPC mem handle --- hipamd/src/hip_internal.hpp | 10 ---------- hipamd/src/hip_memory.cpp | 31 +++++++++++-------------------- hipamd/src/hip_mempool_impl.hpp | 2 +- rocclr/device/device.cpp | 26 ++++++++++++-------------- rocclr/device/device.hpp | 29 ++++++++++++++++++++++------- rocclr/device/pal/paldevice.cpp | 2 +- rocclr/platform/memory.cpp | 8 ++++++++ 7 files changed, 55 insertions(+), 53 deletions(-) diff --git a/hipamd/src/hip_internal.hpp b/hipamd/src/hip_internal.hpp index 45d7dbe225..86cad9597c 100644 --- a/hipamd/src/hip_internal.hpp +++ b/hipamd/src/hip_internal.hpp @@ -48,9 +48,6 @@ #define KCYN "\x1B[36m" #define KWHT "\x1B[37m" -/*! IHIP IPC MEMORY Structure */ -#define IHIP_IPC_MEM_HANDLE_SIZE 32 -#define IHIP_IPC_MEM_RESERVED_SIZE LP64_SWITCH(20,12) namespace hip{ extern std::once_flag g_ihipInitialized; } @@ -89,13 +86,6 @@ struct GraphNode; struct GraphExec; struct UserObject; class Stream; -typedef struct ihipIpcMemHandle_st { - char ipc_handle[IHIP_IPC_MEM_HANDLE_SIZE]; ///< ipc memory handle on ROCr - size_t psize; - size_t poffset; - int owners_process_id; - char reserved[IHIP_IPC_MEM_RESERVED_SIZE]; -} ihipIpcMemHandle_t; #define IHIP_IPC_EVENT_HANDLE_SIZE 32 #define IHIP_IPC_EVENT_RESERVED_SIZE LP64_SWITCH(28,24) diff --git a/hipamd/src/hip_memory.cpp b/hipamd/src/hip_memory.cpp index caf5fa79f6..4b99b27de0 100644 --- a/hipamd/src/hip_memory.cpp +++ b/hipamd/src/hip_memory.cpp @@ -3236,16 +3236,16 @@ hipError_t hipIpcGetMemHandle(hipIpcMemHandle_t* handle, void* dev_ptr) { HIP_INIT_API(hipIpcGetMemHandle, handle, dev_ptr); amd::Device* device = nullptr; - ihipIpcMemHandle_t* ihandle = nullptr; + amd::MemObjMap::IpcMemHandle* ihandle = nullptr; if ((handle == nullptr) || (dev_ptr == nullptr)) { HIP_RETURN(hipErrorInvalidValue); } device = hip::getCurrentDevice()->devices()[0]; - ihandle = reinterpret_cast(handle); + ihandle = reinterpret_cast(handle); - if(!device->IpcCreate(dev_ptr, &(ihandle->psize), &(ihandle->ipc_handle), &(ihandle->poffset))) { + if (!device->IpcCreate(dev_ptr, &(ihandle->psize), ihandle->ipc_handle, &(ihandle->poffset))) { LogPrintfError("IPC memory creation failed for memory: 0x%x", dev_ptr); HIP_RETURN(hipErrorInvalidValue); } @@ -3256,10 +3256,9 @@ hipError_t hipIpcGetMemHandle(hipIpcMemHandle_t* handle, void* dev_ptr) { hipError_t hipIpcOpenMemHandle(void** dev_ptr, hipIpcMemHandle_t handle, unsigned int flags) { HIP_INIT_API(hipIpcOpenMemHandle, dev_ptr, &handle, flags); - amd::Memory* amd_mem_obj = nullptr; amd::Device* device = nullptr; - ihipIpcMemHandle_t* ihandle = nullptr; + amd::MemObjMap::IpcMemHandle* ihandle = nullptr; size_t offset = 0; if (dev_ptr == nullptr || flags != hipIpcMemLazyEnablePeerAccess) { @@ -3268,7 +3267,7 @@ hipError_t hipIpcOpenMemHandle(void** dev_ptr, hipIpcMemHandle_t handle, unsigne /* Call the IPC Attach from Device class */ device = hip::getCurrentDevice()->devices()[0]; - ihandle = reinterpret_cast(&handle); + ihandle = reinterpret_cast(&handle); if (ihandle->psize == 0) { HIP_RETURN(hipErrorInvalidValue); @@ -3278,9 +3277,9 @@ hipError_t hipIpcOpenMemHandle(void** dev_ptr, hipIpcMemHandle_t handle, unsigne HIP_RETURN(hipErrorInvalidContext); } - amd_mem_obj = amd::MemObjMap::FindIpcHandleMemObj(ihandle); + amd_mem_obj = amd::MemObjMap::FindIpcHandleMemObj(*ihandle); if (amd_mem_obj == nullptr) { - if (!device->IpcAttach(&(ihandle->ipc_handle), ihandle->psize, ihandle->poffset, flags, + if (!device->IpcAttach(ihandle->ipc_handle, ihandle->psize, ihandle->poffset, flags, dev_ptr)) { LogPrintfError( "Cannot attach ipc_handle: with ipc_size: %u" @@ -3289,7 +3288,7 @@ hipError_t hipIpcOpenMemHandle(void** dev_ptr, hipIpcMemHandle_t handle, unsigne HIP_RETURN(hipErrorInvalidDevicePointer); } amd_mem_obj = getMemoryObject(*dev_ptr, offset); - amd::MemObjMap::AddIpcHandleMemObj(ihandle, amd_mem_obj); + amd::MemObjMap::AddIpcHandleMemObj(*ihandle, amd_mem_obj); } else { // Handle was already opened by the same process *dev_ptr = amd_mem_obj->getSvmPtr(); @@ -3330,18 +3329,10 @@ hipError_t hipIpcCloseMemHandle(void* dev_ptr) { HIP_RETURN(hipErrorInvalidValue); } - // If handle is opened more than once, do detach on last release call - if (amd_mem_obj->referenceCount() == 1) { - auto device_id = amd_mem_obj->getUserData().deviceId; - g_devices[device_id]->SyncAllStreams(); + auto device_id = amd_mem_obj->getUserData().deviceId; + g_devices[device_id]->SyncAllStreams(); - // Remove entry from the map - amd::MemObjMap::RemoveIpcHandleMemObj(amd_mem_obj); - /* detach the memory */ - device->IpcDetach(dev_ptr); - } else { - amd_mem_obj->release(); - } + amd_mem_obj->release(); HIP_RETURN(hipSuccess); } diff --git a/hipamd/src/hip_mempool_impl.hpp b/hipamd/src/hip_mempool_impl.hpp index 095105d3cb..52905613ed 100644 --- a/hipamd/src/hip_mempool_impl.hpp +++ b/hipamd/src/hip_mempool_impl.hpp @@ -35,7 +35,7 @@ class Stream; struct SharedMemPointer { size_t offset_; size_t size_; - char handle_[IHIP_IPC_MEM_HANDLE_SIZE]; + char handle_[AMD_IPC_MEM_HANDLE_SIZE]; }; struct MemoryTimestamp { diff --git a/rocclr/device/device.cpp b/rocclr/device/device.cpp index d3201126d3..a791aa46f3 100644 --- a/rocclr/device/device.cpp +++ b/rocclr/device/device.cpp @@ -341,7 +341,8 @@ bool Device::device_not_usable_ = false; std::shared_mutex MemObjMap::AllocatedLock_ ROCCLR_INIT_PRIORITY(101); std::map MemObjMap::MemObjMap_ ROCCLR_INIT_PRIORITY(101); std::map MemObjMap::VirtualMemObjMap_ ROCCLR_INIT_PRIORITY(101); -std::unordered_map MemObjMap::IpcHandleMemObjMap_ ROCCLR_INIT_PRIORITY(101); +std::map MemObjMap::IpcHandleMemObjMap_ ROCCLR_INIT_PRIORITY(101); + void MemObjMap::AddMemObj(const void* k, amd::Memory* v) { std::unique_lock lock(AllocatedLock_); @@ -449,12 +450,12 @@ amd::Memory* MemObjMap::FindVirtualMemObj(const void* k) { } } -void MemObjMap::AddIpcHandleMemObj(const void* k, amd::Memory* v) { +void MemObjMap::AddIpcHandleMemObj(const IpcMemHandle& k, amd::Memory* v) { std::unique_lock lock(AllocatedLock_); - auto rval = IpcHandleMemObjMap_.insert({reinterpret_cast(k), v}); + auto rval = IpcHandleMemObjMap_.insert({k, v}); if (!rval.second) { - DevLogPrintfError("IpcHandle Memobj map already has an entry for ptr: 0x%x", - reinterpret_cast(k)); + DevLogPrintfError( + "Error adding entry for Memobj 0x%x in IpcHandle map. The handle already exists.", v); } } @@ -469,11 +470,10 @@ void MemObjMap::RemoveIpcHandleMemObj(amd::Memory* v) { } } -amd::Memory* MemObjMap::FindIpcHandleMemObj(const void* k) { +amd::Memory* MemObjMap::FindIpcHandleMemObj(const IpcMemHandle& k) { std::shared_lock lock(AllocatedLock_); - uintptr_t key = reinterpret_cast(k); - auto it = IpcHandleMemObjMap_.find(key); + auto it = IpcHandleMemObjMap_.find(k); if (it == IpcHandleMemObjMap_.cend()) { return nullptr; } @@ -1020,7 +1020,7 @@ char* Device::getExtensionString() { } // ================================================================================================ -bool Device::IpcCreate(void* dev_ptr, size_t* mem_size, void* handle, size_t* mem_offset) const { +bool Device::IpcCreate(void* dev_ptr, size_t* mem_size, char* handle, size_t* mem_offset) const { amd::Memory* amd_mem_obj = amd::MemObjMap::FindMemObj(dev_ptr); if (amd_mem_obj == nullptr) { DevLogPrintfError("Cannot retrieve amd_mem_obj for dev_ptr: 0x%x", dev_ptr); @@ -1059,7 +1059,7 @@ bool Device::IpcCreate(void* dev_ptr, size_t* mem_size, void* handle, size_t* me } // ================================================================================================ -bool Device::IpcAttach(const void* handle, size_t mem_size, size_t mem_offset, unsigned int flags, +bool Device::IpcAttach(const char* handle, size_t mem_size, size_t mem_offset, unsigned int flags, void** dev_ptr) const { amd::Memory* amd_mem_obj = nullptr; @@ -1093,8 +1093,7 @@ bool Device::IpcAttach(const void* handle, size_t mem_size, size_t mem_offset, u } // ================================================================================================ -void Device::IpcDetach(void* dev_ptr) const { - amd::Memory* amd_mem_obj = amd::MemObjMap::FindMemObj(dev_ptr); +void Device::IpcDetach(amd::Memory* amd_mem_obj) const { // Get the original pointer from the amd::Memory object void* orig_dev_ptr = nullptr; @@ -1105,8 +1104,7 @@ void Device::IpcDetach(void* dev_ptr) const { } else { ShouldNotReachHere(); } - - if (amd_mem_obj->release() == 0) { + if (amd::MemObjMap::FindMemObj(orig_dev_ptr)) { amd::MemObjMap::RemoveMemObj(orig_dev_ptr); } } diff --git a/rocclr/device/device.hpp b/rocclr/device/device.hpp index 603638b68d..7906faffa7 100644 --- a/rocclr/device/device.hpp +++ b/rocclr/device/device.hpp @@ -1374,10 +1374,25 @@ class VirtualDevice : public amd::HeapObject { } // namespace amd::device namespace amd { - +/*! IHIP IPC MEMORY Structure */ +#define AMD_IPC_MEM_HANDLE_SIZE 32 //! MemoryObject map lookup class class MemObjMap : public AllStatic { public: + struct IpcMemHandle { + char ipc_handle[AMD_IPC_MEM_HANDLE_SIZE]; ///< ipc memory handle on ROCr + size_t psize; ///< Total size of the device memory allocation + size_t poffset; ///< Offset within the allocation + int owners_process_id; ///< ID of the process that owns the allocation + char reserved[LP64_SWITCH(20, 12)]; ///< Reserved for future extensions + + bool operator<(const IpcMemHandle& h) const { + int cmp = std::memcmp(ipc_handle, h.ipc_handle, AMD_IPC_MEM_HANDLE_SIZE); + if (cmp != 0) return cmp < 0; + + return poffset < h.poffset; + } + }; //!< add the host mem pointer and buffer in the container static void AddMemObj(const void* k, amd::Memory* v); @@ -1398,11 +1413,11 @@ class MemObjMap : public AllStatic { static amd::Memory* FindVirtualMemObj(const void* k); //!< Same as AddMemObj but for virtual ipc handle to MemObj mapping - static void AddIpcHandleMemObj(const void* k, amd::Memory* v); + static void AddIpcHandleMemObj(const IpcMemHandle& k, amd::Memory* v); //!< Remove entry from the map by searching values static void RemoveIpcHandleMemObj(amd::Memory* v); //!< Same as FindMemObj but for ipc handle to MemObj mapping - static amd::Memory* FindIpcHandleMemObj(const void* k); + static amd::Memory* FindIpcHandleMemObj(const IpcMemHandle& k); private: //!< the mem object<->hostptr information container @@ -1412,7 +1427,7 @@ class MemObjMap : public AllStatic { //!< Shared read/write lock static std::shared_mutex AllocatedLock_; //!< the ipc handle<->mem object information container - static std::unordered_map IpcHandleMemObjMap_; + static std::map IpcHandleMemObjMap_; }; /// @brief Instruction Set Architecture properties. @@ -2080,12 +2095,12 @@ class Device : public RuntimeObject { //! Checks if OCL runtime can use hsail for compilation bool ValidateHsail(); - bool IpcCreate(void* dev_ptr, size_t* mem_size, void* handle, size_t* mem_offset) const; + bool IpcCreate(void* dev_ptr, size_t* mem_size, char* handle, size_t* mem_offset) const; - bool IpcAttach(const void* handle, size_t mem_size, size_t mem_offset, unsigned int flags, + bool IpcAttach(const char* handle, size_t mem_size, size_t mem_offset, unsigned int flags, void** dev_ptr) const; - void IpcDetach(void* dev_ptr) const; + void IpcDetach(amd::Memory* amd_mem_obj) const; //! Return context amd::Context& context() const { return *context_; } diff --git a/rocclr/device/pal/paldevice.cpp b/rocclr/device/pal/paldevice.cpp index 1541972439..e10f8e2114 100644 --- a/rocclr/device/pal/paldevice.cpp +++ b/rocclr/device/pal/paldevice.cpp @@ -2505,8 +2505,8 @@ void Device::svmFree(void* ptr) const { } else { amd::Memory* svmMem = amd::MemObjMap::FindMemObj(ptr); if (nullptr != svmMem) { - svmMem->release(); amd::MemObjMap::RemoveMemObj(ptr); + svmMem->release(); } } } diff --git a/rocclr/platform/memory.cpp b/rocclr/platform/memory.cpp index ab4e683ce8..8d515df9bc 100644 --- a/rocclr/platform/memory.cpp +++ b/rocclr/platform/memory.cpp @@ -416,6 +416,14 @@ device::Memory* Memory::getDeviceMemory(const Device& dev, bool alloc) { // ================================================================================================ Memory::~Memory() { + + if (ipcShared()) { + amd::MemObjMap::RemoveIpcHandleMemObj(this); + auto device = context_().devices()[0]; + if (device != nullptr) { + device->IpcDetach(this); + } + } // For_each destructor callback: DestructorCallBackEntry* entry; for (entry = destructorCallbacks_; entry != nullptr; entry = entry->next_) {