From e4d29e228d89a61c3315ed19377204dfc781e4f4 Mon Sep 17 00:00:00 2001 From: Saleel Kudchadker Date: Tue, 12 Nov 2024 17:25:09 +0000 Subject: [PATCH] SWDEV-495987 - Use shared mutex for MemObj maps - Remove binning logic, although useful it doesnt work in current scenario as there is no upper limit on the size of allocation. If an app or framework uses entire VRAM and then creates suballocs, binning would result in failure. Change-Id: Icc27c13e433bb4a1f03e82028d8718488b43bfa5 --- rocclr/device/device.cpp | 79 ++++++++++++++++++---------------------- rocclr/device/device.hpp | 19 +++------- 2 files changed, 40 insertions(+), 58 deletions(-) diff --git a/rocclr/device/device.cpp b/rocclr/device/device.cpp index d179d69ca0..d090516013 100644 --- a/rocclr/device/device.cpp +++ b/rocclr/device/device.cpp @@ -305,13 +305,13 @@ Context* Device::glb_ctx_ = nullptr; Monitor Device::p2p_stage_ops_(true); Memory* Device::p2p_stage_ = nullptr; -std::array MemObjMap::MemObjBin_ ROCCLR_INIT_PRIORITY(101) = {} ; -std::array MemObjMap::VirtualMemObjBin_ ROCCLR_INIT_PRIORITY(101) = {} ; +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); void MemObjMap::AddMemObj(const void* k, amd::Memory* v) { - auto& bin = getMapBin(k, MemObjBin_); - std::unique_lock lock(bin.AllocatedLock_); - auto rval = bin.map_.insert({ reinterpret_cast(k), v }); + std::unique_lock lock(AllocatedLock_); + auto rval = MemObjMap_.insert({ reinterpret_cast(k), v }); if (!rval.second) { DevLogPrintfError("Memobj map already has an entry for ptr: 0x%x", reinterpret_cast(k)); @@ -319,19 +319,17 @@ void MemObjMap::AddMemObj(const void* k, amd::Memory* v) { } void MemObjMap::RemoveMemObj(const void* k) { - auto& bin = getMapBin(k, MemObjBin_); - std::unique_lock lock(bin.AllocatedLock_); - auto rval = bin.map_.erase(reinterpret_cast(k)); + std::unique_lock lock(AllocatedLock_); + auto rval = MemObjMap_.erase(reinterpret_cast(k)); guarantee(rval == 1, "Memobj map does not have ptr: 0x%x", reinterpret_cast(k)); } amd::Memory* MemObjMap::FindMemObj(const void* k, size_t* offset) { - auto& bin = getMapBin(k, MemObjBin_); - std::shared_lock lock(bin.AllocatedLock_); + std::shared_lock lock(AllocatedLock_); uintptr_t key = reinterpret_cast(k); - auto it = bin.map_.upper_bound(key); - if (it == bin.map_.begin()) { + auto it = MemObjMap_.upper_bound(key); + if (it == MemObjMap_.begin()) { return nullptr; } @@ -354,16 +352,14 @@ void MemObjMap::UpdateAccess(amd::Device *peerDev) { } // Provides access to all memory allocated on peerDev but // hsa_amd_agents_allow_access was not called because there was no peer - for (auto& bin : MemObjBin_) { - std::shared_lock lock(bin.AllocatedLock_); - for (auto it : bin.map_) { - const std::vector& devices = it.second->getContext().devices(); - if (devices.size() == 1 && devices[0] == peerDev) { - device::Memory* devMem = it.second->getDeviceMemory(*devices[0]); - if (!devMem->getAllowedPeerAccess()) { - peerDev->deviceAllowAccess(reinterpret_cast(it.first)); - devMem->setAllowedPeerAccess(true); - } + std::shared_lock lock(AllocatedLock_); + for (auto it : MemObjMap_) { + const std::vector& devices = it.second->getContext().devices(); + if (devices.size() == 1 && devices[0] == peerDev) { + device::Memory* devMem = it.second->getDeviceMemory(*devices[0]); + if (!devMem->getAllowedPeerAccess()) { + peerDev->deviceAllowAccess(reinterpret_cast(it.first)); + devMem->setAllowedPeerAccess(true); } } } @@ -371,26 +367,23 @@ void MemObjMap::UpdateAccess(amd::Device *peerDev) { void MemObjMap::Purge(amd::Device* dev) { assert(dev != nullptr); - for (auto& bin : MemObjBin_) { - std::unique_lock lock(bin.AllocatedLock_); - for (auto it = bin.map_.cbegin(); it != bin.map_.cend(); ) { - amd::Memory* memObj = it->second; - unsigned int flags = memObj->getMemFlags(); - const std::vector& devices = memObj->getContext().devices(); - if (devices.size() == 1 && devices[0] == dev && !(flags & ROCCLR_MEM_INTERNAL_MEMORY)) { - memObj->release(); - it = bin.map_.erase(it); - } else { - ++it; - } + std::unique_lock lock(AllocatedLock_); + for (auto it = MemObjMap_.cbegin(); it != MemObjMap_.cend(); ) { + amd::Memory* memObj = it->second; + unsigned int flags = memObj->getMemFlags(); + const std::vector& devices = memObj->getContext().devices(); + if (devices.size() == 1 && devices[0] == dev && !(flags & ROCCLR_MEM_INTERNAL_MEMORY)) { + memObj->release(); + it = MemObjMap_.erase(it); + } else { + ++it; } } } void MemObjMap::AddVirtualMemObj(const void* k, amd::Memory* v) { - auto& bin = getMapBin(k, VirtualMemObjBin_); - std::unique_lock lock(bin.AllocatedLock_); - auto rval = bin.map_.insert({ reinterpret_cast(k), v }); + std::unique_lock lock(AllocatedLock_); + auto rval = VirtualMemObjMap_.insert({ reinterpret_cast(k), v }); if (!rval.second) { DevLogPrintfError("Virtual Memobj map already has an entry for ptr: 0x%x", reinterpret_cast(k)); @@ -398,19 +391,17 @@ void MemObjMap::AddVirtualMemObj(const void* k, amd::Memory* v) { } void MemObjMap::RemoveVirtualMemObj(const void* k) { - auto& bin = getMapBin(k, VirtualMemObjBin_); - std::unique_lock lock(bin.AllocatedLock_); - auto rval = bin.map_.erase(reinterpret_cast(k)); + std::unique_lock lock(AllocatedLock_); + auto rval = VirtualMemObjMap_.erase(reinterpret_cast(k)); guarantee(rval == 1, "Virtual Memobj map does not have ptr: 0x%x", reinterpret_cast(k)); } amd::Memory* MemObjMap::FindVirtualMemObj(const void* k) { - auto& bin = getMapBin(k, VirtualMemObjBin_); - std::shared_lock lock(bin.AllocatedLock_); + std::shared_lock lock(AllocatedLock_); uintptr_t key = reinterpret_cast(k); - auto it = bin.map_.upper_bound(key); - if (it == bin.map_.begin()) { + auto it = VirtualMemObjMap_.upper_bound(key); + if (it == VirtualMemObjMap_.begin()) { return nullptr; } diff --git a/rocclr/device/device.hpp b/rocclr/device/device.hpp index 224fb3ea17..45cdd1befd 100644 --- a/rocclr/device/device.hpp +++ b/rocclr/device/device.hpp @@ -58,7 +58,6 @@ #include #include #include -#include namespace amd { class Command; @@ -1380,22 +1379,14 @@ class MemObjMap : public AllStatic { static void RemoveVirtualMemObj(const void* k); //!< Same as FindMemObj but for virtual addressing static amd::Memory* FindVirtualMemObj(const void* k); - struct Bin { - std::shared_mutex AllocatedLock_; - std::map map_; - }; - static constexpr size_t kNumBins = 16; + private: - //!< the mem object<->hostptr information container - static std::array MemObjBin_; + static std::map MemObjMap_; //!< the virtual mem object<->hostptr information container - static std::array VirtualMemObjBin_; - - static Bin& getMapBin(const void* k, std::array& bin) { - size_t index = std::hash{}(k) % kNumBins; - return bin.at(index); - } + static std::map VirtualMemObjMap_; + //!< Shared read/write lock + static std::shared_mutex AllocatedLock_; }; /// @brief Instruction Set Architecture properties.