From b60289375df1bfc59ea3a8a0fc0ce0e075344cd4 Mon Sep 17 00:00:00 2001 From: Janet Schneider Date: Wed, 28 Sep 2022 12:00:05 -0700 Subject: [PATCH 1/5] TEST code to investigate allocs during session creation --- src/runtime/hexagon/hexagon_device_api.cc | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/runtime/hexagon/hexagon_device_api.cc b/src/runtime/hexagon/hexagon_device_api.cc index 84232a614428..4bdf82548faa 100644 --- a/src/runtime/hexagon/hexagon_device_api.cc +++ b/src/runtime/hexagon/hexagon_device_api.cc @@ -91,6 +91,8 @@ void* HexagonDeviceAPI::AllocDataSpace(Device dev, int ndim, const int64_t* shap const size_t typesize = (dtype.bits / 8) * dtype.lanes; + CHECK(runtime_hexbuffs) << "AllocateHexagonBuffer - no runtime_hexbuffs"; // jlsfix + if (ndim == 0) { // Allocate storage for a single scalar value. return mgr->AllocateHexagonBuffer(typesize, kHexagonAllocAlignment, mem_scope); @@ -116,12 +118,29 @@ void* HexagonDeviceAPI::AllocDataSpace(Device dev, size_t nbytes, size_t alignme if (alignment < kHexagonAllocAlignment) { alignment = kHexagonAllocAlignment; } + if (runtime_hexbuffs == nullptr) // jlsfix + { + static int count = 0; + if (count > 0) + { + void* ptr = mgr->AllocateHexagonBuffer(nbytes, alignment, String("global")); + LOG(INFO) << "jlsfix AllocateHexagonBuffer nbytes: " << nbytes << " ptr: " << ptr; + (void) runtime_hexbuffs->empty(); + CHECK(runtime_hexbuffs) << "jlsfix STOP SECOND"; + return ptr; + } + count++; + } return mgr->AllocateHexagonBuffer(nbytes, alignment, String("global")); } void HexagonDeviceAPI::FreeDataSpace(Device dev, void* ptr) { CHECK(ptr) << "buffer pointer is null"; CHECK(IsValidDevice(dev)) << "dev.device_type: " << dev.device_type; + if (runtime_hexbuffs == nullptr) // jlsfix + { + LOG(INFO) << "jlsfix FreeHexagonBuffer ptr: " << ptr; + } mgr->FreeHexagonBuffer(ptr); } From 4b9932984d362c6be5337f1d800600117de72e5b Mon Sep 17 00:00:00 2001 From: Janet Schneider Date: Mon, 10 Oct 2022 16:59:24 -0700 Subject: [PATCH 2/5] Proper fix for pre-Acquire allocaitons --- src/runtime/hexagon/hexagon_device_api.cc | 60 +++++++++++-------- src/runtime/hexagon/hexagon_device_api.h | 21 ++++--- src/runtime/hexagon/rpc/hexagon/rpc_server.cc | 36 ++++++++++- .../hexagon/hexagon_device_api_tests.cc | 26 +++++--- 4 files changed, 100 insertions(+), 43 deletions(-) diff --git a/src/runtime/hexagon/hexagon_device_api.cc b/src/runtime/hexagon/hexagon_device_api.cc index 0320a73e1e74..9db0388ac431 100644 --- a/src/runtime/hexagon/hexagon_device_api.cc +++ b/src/runtime/hexagon/hexagon_device_api.cc @@ -90,20 +90,21 @@ void* HexagonDeviceAPI::AllocDataSpace(Device dev, int ndim, const int64_t* shap const size_t typesize = (dtype.bits / 8) * dtype.lanes; - CHECK(runtime_hexbuffs) << "AllocateHexagonBuffer - no runtime_hexbuffs"; // jlsfix + CHECK(runtime_hexbuffs) << "runtime_hexbuffs is not initalized"; if (ndim == 0) { // Allocate storage for a single scalar value. - return mgr->AllocateHexagonBuffer(typesize, kHexagonAllocAlignment, mem_scope); + return runtime_hexbuffs->AllocateHexagonBuffer(typesize, kHexagonAllocAlignment, mem_scope); } else if (ndim == 1) { // Allocate a single, contiguous memory region. size_t nbytes = shape[0] * typesize; - return mgr->AllocateHexagonBuffer(nbytes, kHexagonAllocAlignment, mem_scope); + return runtime_hexbuffs->AllocateHexagonBuffer(nbytes, kHexagonAllocAlignment, mem_scope); } else if (ndim == 2) { // Allocate the region(s) needed for Hexagon's indirect-tensor format. size_t nallocs = shape[0]; size_t nbytes = shape[1] * typesize; - return mgr->AllocateHexagonBuffer(nallocs, nbytes, kHexagonAllocAlignment, mem_scope); + return runtime_hexbuffs->AllocateHexagonBuffer(nallocs, nbytes, kHexagonAllocAlignment, + mem_scope); } else { return nullptr; // unreachable } @@ -117,30 +118,37 @@ void* HexagonDeviceAPI::AllocDataSpace(Device dev, size_t nbytes, size_t alignme if (alignment < kHexagonAllocAlignment) { alignment = kHexagonAllocAlignment; } - if (runtime_hexbuffs == nullptr) // jlsfix - { - static int count = 0; - if (count > 0) - { - void* ptr = mgr->AllocateHexagonBuffer(nbytes, alignment, String("global")); - LOG(INFO) << "jlsfix AllocateHexagonBuffer nbytes: " << nbytes << " ptr: " << ptr; - (void) runtime_hexbuffs->empty(); - CHECK(runtime_hexbuffs) << "jlsfix STOP SECOND"; - return ptr; - } - count++; - } - return mgr->AllocateHexagonBuffer(nbytes, alignment, String("global")); + CHECK(runtime_hexbuffs) << "runtime_hexbuffs is not initalized"; + void* ptr = runtime_hexbuffs->AllocateHexagonBuffer(nbytes, alignment, String("global")); + return ptr; } void HexagonDeviceAPI::FreeDataSpace(Device dev, void* ptr) { CHECK(ptr) << "buffer pointer is null"; CHECK(IsValidDevice(dev)) << "dev.device_type: " << dev.device_type; - if (runtime_hexbuffs == nullptr) // jlsfix - { - LOG(INFO) << "jlsfix FreeHexagonBuffer ptr: " << ptr; + CHECK(runtime_hexbuffs) << "runtime_hexbuffs is not initalized"; + runtime_hexbuffs->FreeHexagonBuffer(ptr); +} + +void* HexagonDeviceAPI::AllocRpcDataSpace(Device dev, size_t nbytes, size_t alignment, + DLDataType type_hint) { + CHECK(nbytes) << "number of bytes is zero"; + CHECK(alignment) << "alignment is zero"; + CHECK(IsValidDevice(dev)) << "dev.device_type: " << dev.device_type; + if (alignment < kHexagonAllocAlignment) { + alignment = kHexagonAllocAlignment; } - mgr->FreeHexagonBuffer(ptr); + CHECK(rpc_hexbuffs) << "rpc_hexbuffs is not initalized"; + void* ptr = rpc_hexbuffs->AllocateHexagonBuffer(nbytes, alignment, String("global")); + DLOG(INFO) << "AllocRpcDataSpace nbytes: " << nbytes << " ptr: " << ptr; + return ptr; +} + +void HexagonDeviceAPI::FreeRpcDataSpace(Device dev, void* ptr) { + CHECK(ptr) << "buffer pointer is null"; + CHECK(IsValidDevice(dev)) << "dev.device_type: " << dev.device_type; + CHECK(rpc_hexbuffs) << "rpc_hexbuffs is not initalized"; + rpc_hexbuffs->FreeHexagonBuffer(ptr); } // WorkSpace: runtime allocations for Hexagon @@ -156,7 +164,8 @@ void* HexagonDeviceAPI::AllocWorkspace(Device dev, size_t size, DLDataType type_ void HexagonDeviceAPI::FreeWorkspace(Device dev, void* data) { CHECK(IsValidDevice(dev)) << "dev.device_type: " << dev.device_type; - CHECK(mgr->count(data) != 0) + CHECK(runtime_hexbuffs) << "runtime_hexbuffs is not initalized"; + CHECK(runtime_hexbuffs->count(data) != 0) << "Attempt made to free unknown or already freed workspace allocation"; dmlc::ThreadLocalStore::Get()->FreeWorkspace(dev, data); } @@ -179,8 +188,11 @@ void HexagonDeviceAPI::CopyDataFromTo(DLTensor* from, DLTensor* to, TVMStreamHan CHECK_EQ(from->byte_offset, 0); CHECK_EQ(to->byte_offset, 0); CHECK_EQ(GetDataSize(*from), GetDataSize(*to)); + CHECK(runtime_hexbuffs) << "runtime_hexbuffs is not initalized"; - auto lookup_hexagon_buffer = [this](void* ptr) -> HexagonBuffer* { return mgr->find(ptr); }; + auto lookup_hexagon_buffer = [this](void* ptr) -> HexagonBuffer* { + return runtime_hexbuffs->find(ptr); + }; HexagonBuffer* hex_from_buf = lookup_hexagon_buffer(from->data); HexagonBuffer* hex_to_buf = lookup_hexagon_buffer(to->data); diff --git a/src/runtime/hexagon/hexagon_device_api.h b/src/runtime/hexagon/hexagon_device_api.h index 1c802f353062..04788788a892 100644 --- a/src/runtime/hexagon/hexagon_device_api.h +++ b/src/runtime/hexagon/hexagon_device_api.h @@ -48,7 +48,7 @@ class HexagonDeviceAPI final : public DeviceAPI { static HexagonDeviceAPI* Global(); //! \brief Constructor - HexagonDeviceAPI() { mgr = &hexbuffs; } + HexagonDeviceAPI() { rpc_hexbuffs = std::make_unique(); } //! \brief Destructor ~HexagonDeviceAPI() {} @@ -60,7 +60,6 @@ class HexagonDeviceAPI final : public DeviceAPI { CHECK_EQ(runtime_hexbuffs, nullptr); runtime_hexbuffs = std::make_unique(); - mgr = runtime_hexbuffs.get(); CHECK_EQ(runtime_threads, nullptr); runtime_threads = std::make_unique(threads, stack_size, pipe_size); @@ -81,7 +80,6 @@ class HexagonDeviceAPI final : public DeviceAPI { if (runtime_hexbuffs && !runtime_hexbuffs->empty()) { LOG(INFO) << "runtime_hexbuffs was not empty in ReleaseResources"; } - mgr = &hexbuffs; runtime_hexbuffs.reset(); CHECK(runtime_vtcm) << "runtime_vtcm was not created in AcquireResources"; @@ -106,6 +104,12 @@ class HexagonDeviceAPI final : public DeviceAPI { //! \brief Free the allocated HexagonBuffer. void FreeDataSpace(Device dev, void* ptr) final; + //! \brief Hexagon-only interface to allocate buffers used for the RPC server + void* AllocRpcDataSpace(Device dev, size_t nbytes, size_t alignment, DLDataType type_hint); + + //! \brief Hexagon-only interface to free buffers used for the RPC server + void FreeRpcDataSpace(Device dev, void* ptr); + /*! \brief Request a dynamically allocated HexagonBuffer from a workspace pool. * \returns The underlying allocation pointer. */ @@ -190,16 +194,15 @@ class HexagonDeviceAPI final : public DeviceAPI { (DLDeviceType(dev.device_type) == kDLCPU); } - //! \brief Manages underlying HexagonBuffer allocations + //! \brief Manages RPC HexagonBuffer allocations + // rpc_hexbuffs is used only in Alloc/FreeRpcDataSpace + std::unique_ptr rpc_hexbuffs; + + //! \brief Manages runtime HexagonBuffer allocations // runtime_hexbuffs is used for runtime allocations. It is created // with a call to AcquireResources, and destroyed on ReleaseResources. - // hexbuffs is used for all allocations outside of the session lifetime. - HexagonBufferManager hexbuffs; std::unique_ptr runtime_hexbuffs; - //! \brief Current buffer manager - HexagonBufferManager* mgr; - //! \brief Thread manager std::unique_ptr runtime_threads; const unsigned threads{6}; diff --git a/src/runtime/hexagon/rpc/hexagon/rpc_server.cc b/src/runtime/hexagon/rpc/hexagon/rpc_server.cc index 22a54043cd9f..8bee4ca19323 100644 --- a/src/runtime/hexagon/rpc/hexagon/rpc_server.cc +++ b/src/runtime/hexagon/rpc/hexagon/rpc_server.cc @@ -38,6 +38,7 @@ extern "C" { #include "../../../library_module.h" #include "../../../minrpc/minrpc_server.h" #include "../../hexagon/hexagon_common.h" +#include "../../hexagon/hexagon_device_api.h" #include "hexagon_rpc.h" namespace tvm { @@ -145,6 +146,39 @@ class HexagonIOHandler { uint32_t write_buffer_available_length_; }; +// Internal allocator that redirects alloc to TVM's C API. +template +class HexagonPageAllocator { + public: + using ArenaPageHeader = tvm::support::ArenaPageHeader; + + explicit HexagonPageAllocator(TIOHandler* io) : io_(io), dev_(DLDevice{kDLCPU, 0}) {} + + ArenaPageHeader* allocate(size_t min_size) { + size_t npages = ((min_size + kPageSize - 1) / kPageSize); + void* data; + + data = HexagonDeviceAPI::Global()->AllocRpcDataSpace(dev_, npages * kPageSize, kPageAlign, + DLDataType{kDLInt, 1, 1}); + + ArenaPageHeader* header = static_cast(data); + header->size = npages * kPageSize; + header->offset = sizeof(ArenaPageHeader); + return header; + } + + void deallocate(ArenaPageHeader* page) { + HexagonDeviceAPI::Global()->FreeRpcDataSpace(dev_, page); + } + + static const constexpr int kPageSize = 2 << 10; + static const constexpr int kPageAlign = 8; + + private: + TIOHandler* io_; + Device dev_; +}; + class HexagonRPCServer { public: explicit HexagonRPCServer(uint8_t* receive_buffer, size_t receive_buffer_size_bytes) @@ -185,7 +219,7 @@ class HexagonRPCServer { private: HexagonIOHandler io_; - MinRPCServer rpc_server_; + MinRPCServer rpc_server_; }; } // namespace hexagon diff --git a/tests/cpp-runtime/hexagon/hexagon_device_api_tests.cc b/tests/cpp-runtime/hexagon/hexagon_device_api_tests.cc index 2139aa78f7ae..d96d1847a88c 100644 --- a/tests/cpp-runtime/hexagon/hexagon_device_api_tests.cc +++ b/tests/cpp-runtime/hexagon/hexagon_device_api_tests.cc @@ -147,24 +147,32 @@ TEST_F(HexagonDeviceAPITest, DISABLED_alloc_free_diff_dev) { EXPECT_THROW(hexapi->FreeDataSpace(cpu_dev, buf), InternalError); } -// Alloc a non-runtime buffer -// Alloc a runtime buffer -// "Release" resources for runtime -// Verify the runtime buffer cannot be freed, but the non-runtime buffer can -// This test should be run last -TEST_F(HexagonDeviceAPITest, leak_resources) { +// Ensure runtime buffer manager is properly configured and destroyed +// in Acquire/Release +TEST_F(HexagonDeviceAPITest, runtime_buffer_manager) { hexapi->ReleaseResources(); - void* pre_runtime_buf = hexapi->AllocDataSpace(hex_dev, nbytes, alignment, int8); - CHECK(pre_runtime_buf != nullptr); + EXPECT_THROW(hexapi->AllocDataSpace(hex_dev, nbytes, alignment, int8), InternalError); hexapi->AcquireResources(); void* runtime_buf = hexapi->AllocDataSpace(hex_dev, nbytes, alignment, int8); CHECK(runtime_buf != nullptr); hexapi->ReleaseResources(); EXPECT_THROW(hexapi->FreeDataSpace(hex_dev, runtime_buf), InternalError); - hexapi->FreeDataSpace(hex_dev, pre_runtime_buf); hexapi->AcquireResources(); } +// Ensure RPC buffer manager is always available +TEST_F(HexagonDeviceAPITest, rpc_buffer_manager) { + void* rpc_buf; + rpc_buf = hexapi->AllocRpcDataSpace(hex_dev, nbytes, alignment, int8); + CHECK(rpc_buf != nullptr); + hexapi->ReleaseResources(); + hexapi->FreeRpcDataSpace(hex_dev, rpc_buf); + rpc_buf = hexapi->AllocRpcDataSpace(hex_dev, nbytes, alignment, int8); + CHECK(rpc_buf != nullptr); + hexapi->AcquireResources(); + hexapi->FreeRpcDataSpace(hex_dev, rpc_buf); +} + // Ensure thread manager is properly configured and destroyed // in Acquire/Release TEST_F(HexagonDeviceAPITest, thread_manager) { From cc950f62e643a2845c5b8928c7961b94dc21d815 Mon Sep 17 00:00:00 2001 From: Janet Schneider Date: Mon, 10 Oct 2022 17:56:07 -0700 Subject: [PATCH 3/5] Fix FreeDataSpace --- src/runtime/hexagon/hexagon_device_api.cc | 14 +++++++------- .../hexagon/hexagon_device_api_tests.cc | 3 ++- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/runtime/hexagon/hexagon_device_api.cc b/src/runtime/hexagon/hexagon_device_api.cc index 9db0388ac431..5a4bf9b04d04 100644 --- a/src/runtime/hexagon/hexagon_device_api.cc +++ b/src/runtime/hexagon/hexagon_device_api.cc @@ -119,15 +119,17 @@ void* HexagonDeviceAPI::AllocDataSpace(Device dev, size_t nbytes, size_t alignme alignment = kHexagonAllocAlignment; } CHECK(runtime_hexbuffs) << "runtime_hexbuffs is not initalized"; - void* ptr = runtime_hexbuffs->AllocateHexagonBuffer(nbytes, alignment, String("global")); - return ptr; + return runtime_hexbuffs->AllocateHexagonBuffer(nbytes, alignment, String("global")); } void HexagonDeviceAPI::FreeDataSpace(Device dev, void* ptr) { CHECK(ptr) << "buffer pointer is null"; CHECK(IsValidDevice(dev)) << "dev.device_type: " << dev.device_type; - CHECK(runtime_hexbuffs) << "runtime_hexbuffs is not initalized"; - runtime_hexbuffs->FreeHexagonBuffer(ptr); + if (runtime_hexbuffs) { + runtime_hexbuffs->FreeHexagonBuffer(ptr); + } else { + LOG(INFO) << "FreeDataSpace called when runtime_hexbuffs is not initialized"; + } } void* HexagonDeviceAPI::AllocRpcDataSpace(Device dev, size_t nbytes, size_t alignment, @@ -139,9 +141,7 @@ void* HexagonDeviceAPI::AllocRpcDataSpace(Device dev, size_t nbytes, size_t alig alignment = kHexagonAllocAlignment; } CHECK(rpc_hexbuffs) << "rpc_hexbuffs is not initalized"; - void* ptr = rpc_hexbuffs->AllocateHexagonBuffer(nbytes, alignment, String("global")); - DLOG(INFO) << "AllocRpcDataSpace nbytes: " << nbytes << " ptr: " << ptr; - return ptr; + return rpc_hexbuffs->AllocateHexagonBuffer(nbytes, alignment, String("global")); } void HexagonDeviceAPI::FreeRpcDataSpace(Device dev, void* ptr) { diff --git a/tests/cpp-runtime/hexagon/hexagon_device_api_tests.cc b/tests/cpp-runtime/hexagon/hexagon_device_api_tests.cc index d96d1847a88c..d2d79e626b37 100644 --- a/tests/cpp-runtime/hexagon/hexagon_device_api_tests.cc +++ b/tests/cpp-runtime/hexagon/hexagon_device_api_tests.cc @@ -156,8 +156,9 @@ TEST_F(HexagonDeviceAPITest, runtime_buffer_manager) { void* runtime_buf = hexapi->AllocDataSpace(hex_dev, nbytes, alignment, int8); CHECK(runtime_buf != nullptr); hexapi->ReleaseResources(); - EXPECT_THROW(hexapi->FreeDataSpace(hex_dev, runtime_buf), InternalError); + hexapi->FreeDataSpace(hex_dev, runtime_buf); hexapi->AcquireResources(); + EXPECT_THROW(hexapi->FreeDataSpace(hex_dev, runtime_buf), InternalError); } // Ensure RPC buffer manager is always available From dee71d3c66bc30bdcf4d981a041e37dab4ae2bb6 Mon Sep 17 00:00:00 2001 From: Janet Schneider Date: Tue, 11 Oct 2022 15:18:35 -0700 Subject: [PATCH 4/5] PR feedback --- src/runtime/hexagon/hexagon_buffer_manager.h | 13 ++++++++ src/runtime/hexagon/hexagon_device_api.cc | 33 ++++++++++--------- src/runtime/hexagon/hexagon_device_api.h | 27 ++++++++++----- src/runtime/hexagon/rpc/hexagon/rpc_server.cc | 10 ++---- .../hexagon/hexagon_device_api_tests.cc | 8 ++--- 5 files changed, 56 insertions(+), 35 deletions(-) diff --git a/src/runtime/hexagon/hexagon_buffer_manager.h b/src/runtime/hexagon/hexagon_buffer_manager.h index a698b0ecb163..4381f6fa442f 100644 --- a/src/runtime/hexagon/hexagon_buffer_manager.h +++ b/src/runtime/hexagon/hexagon_buffer_manager.h @@ -25,6 +25,7 @@ #include #include #include +#include #include "hexagon_buffer.h" @@ -85,6 +86,18 @@ class HexagonBufferManager { return hexagon_buffer_map_.empty(); } + //! \brief Returns a list of allocated pointers. + // Note - this should only be used by the device API to keep track of what + // was in the manager when HexagonDeviceAPI::ReleaseResources is called. + std::vector vector() { + std::vector allocated; + std::lock_guard lock(map_mutex_); + for (auto it = hexagon_buffer_map_.begin(); it != hexagon_buffer_map_.end(); it++) { + allocated.push_back(it->first); + } + return allocated; + } + private: //! \brief Contains the HexagonBuffer objects managed by this class. std::unordered_map> hexagon_buffer_map_; diff --git a/src/runtime/hexagon/hexagon_device_api.cc b/src/runtime/hexagon/hexagon_device_api.cc index 5a4bf9b04d04..5c5bdfff7e8f 100644 --- a/src/runtime/hexagon/hexagon_device_api.cc +++ b/src/runtime/hexagon/hexagon_device_api.cc @@ -90,7 +90,9 @@ void* HexagonDeviceAPI::AllocDataSpace(Device dev, int ndim, const int64_t* shap const size_t typesize = (dtype.bits / 8) * dtype.lanes; - CHECK(runtime_hexbuffs) << "runtime_hexbuffs is not initalized"; + CHECK(runtime_hexbuffs) << "Attempted to allocate Hexagon data with " + << "HexagonDeviceAPI::AllocDataSpace before initializing resources. " + << "Please call HexagonDeviceAPI::AcquireResources"; if (ndim == 0) { // Allocate storage for a single scalar value. @@ -118,7 +120,9 @@ void* HexagonDeviceAPI::AllocDataSpace(Device dev, size_t nbytes, size_t alignme if (alignment < kHexagonAllocAlignment) { alignment = kHexagonAllocAlignment; } - CHECK(runtime_hexbuffs) << "runtime_hexbuffs is not initalized"; + CHECK(runtime_hexbuffs) << "Attempted to allocate Hexagon data with " + << "HexagonDeviceAPI::AllocDataSpace before initializing resources. " + << "Please call HexagonDeviceAPI::AcquireResources"; return runtime_hexbuffs->AllocateHexagonBuffer(nbytes, alignment, String("global")); } @@ -128,27 +132,25 @@ void HexagonDeviceAPI::FreeDataSpace(Device dev, void* ptr) { if (runtime_hexbuffs) { runtime_hexbuffs->FreeHexagonBuffer(ptr); } else { - LOG(INFO) << "FreeDataSpace called when runtime_hexbuffs is not initialized"; + auto it = std::find(released_runtime_buffers.begin(), released_runtime_buffers.end(), ptr); + CHECK(it != released_runtime_buffers.end()) << "Attempted to free Hexagon data with " + << "HexagonDeviceAPI::FreeDataSpace that was not " + << "allocated during the session."; } } -void* HexagonDeviceAPI::AllocRpcDataSpace(Device dev, size_t nbytes, size_t alignment, - DLDataType type_hint) { +void* HexagonDeviceAPI::AllocRpcBuffer(size_t nbytes, size_t alignment) { CHECK(nbytes) << "number of bytes is zero"; CHECK(alignment) << "alignment is zero"; - CHECK(IsValidDevice(dev)) << "dev.device_type: " << dev.device_type; if (alignment < kHexagonAllocAlignment) { alignment = kHexagonAllocAlignment; } - CHECK(rpc_hexbuffs) << "rpc_hexbuffs is not initalized"; - return rpc_hexbuffs->AllocateHexagonBuffer(nbytes, alignment, String("global")); + return rpc_hexbuffs.AllocateHexagonBuffer(nbytes, alignment, String("global")); } -void HexagonDeviceAPI::FreeRpcDataSpace(Device dev, void* ptr) { +void HexagonDeviceAPI::FreeRpcBuffer(void* ptr) { CHECK(ptr) << "buffer pointer is null"; - CHECK(IsValidDevice(dev)) << "dev.device_type: " << dev.device_type; - CHECK(rpc_hexbuffs) << "rpc_hexbuffs is not initalized"; - rpc_hexbuffs->FreeHexagonBuffer(ptr); + rpc_hexbuffs.FreeHexagonBuffer(ptr); } // WorkSpace: runtime allocations for Hexagon @@ -164,8 +166,7 @@ void* HexagonDeviceAPI::AllocWorkspace(Device dev, size_t size, DLDataType type_ void HexagonDeviceAPI::FreeWorkspace(Device dev, void* data) { CHECK(IsValidDevice(dev)) << "dev.device_type: " << dev.device_type; - CHECK(runtime_hexbuffs) << "runtime_hexbuffs is not initalized"; - CHECK(runtime_hexbuffs->count(data) != 0) + CHECK(runtime_hexbuffs == nullptr || runtime_hexbuffs->count(data) != 0) << "Attempt made to free unknown or already freed workspace allocation"; dmlc::ThreadLocalStore::Get()->FreeWorkspace(dev, data); } @@ -188,7 +189,9 @@ void HexagonDeviceAPI::CopyDataFromTo(DLTensor* from, DLTensor* to, TVMStreamHan CHECK_EQ(from->byte_offset, 0); CHECK_EQ(to->byte_offset, 0); CHECK_EQ(GetDataSize(*from), GetDataSize(*to)); - CHECK(runtime_hexbuffs) << "runtime_hexbuffs is not initalized"; + CHECK(runtime_hexbuffs) << "Attempted to copy Hexagon data with " + << "HexagonDeviceAPI::CopyDataFromTo before initializing resources. " + << "Please call HexagonDeviceAPI::AcquireResources"; auto lookup_hexagon_buffer = [this](void* ptr) -> HexagonBuffer* { return runtime_hexbuffs->find(ptr); diff --git a/src/runtime/hexagon/hexagon_device_api.h b/src/runtime/hexagon/hexagon_device_api.h index 04788788a892..d5909ef99b4e 100644 --- a/src/runtime/hexagon/hexagon_device_api.h +++ b/src/runtime/hexagon/hexagon_device_api.h @@ -48,7 +48,7 @@ class HexagonDeviceAPI final : public DeviceAPI { static HexagonDeviceAPI* Global(); //! \brief Constructor - HexagonDeviceAPI() { rpc_hexbuffs = std::make_unique(); } + HexagonDeviceAPI() {} //! \brief Destructor ~HexagonDeviceAPI() {} @@ -60,6 +60,7 @@ class HexagonDeviceAPI final : public DeviceAPI { CHECK_EQ(runtime_hexbuffs, nullptr); runtime_hexbuffs = std::make_unique(); + released_runtime_buffers.clear(); CHECK_EQ(runtime_threads, nullptr); runtime_threads = std::make_unique(threads, stack_size, pipe_size); @@ -77,8 +78,9 @@ class HexagonDeviceAPI final : public DeviceAPI { runtime_threads.reset(); CHECK(runtime_hexbuffs) << "runtime_hexbuffs was not created in AcquireResources"; - if (runtime_hexbuffs && !runtime_hexbuffs->empty()) { - LOG(INFO) << "runtime_hexbuffs was not empty in ReleaseResources"; + if (!runtime_hexbuffs->empty()) { + DLOG(INFO) << "runtime_hexbuffs was not empty in ReleaseResources"; + released_runtime_buffers = runtime_hexbuffs->vector(); } runtime_hexbuffs.reset(); @@ -105,10 +107,10 @@ class HexagonDeviceAPI final : public DeviceAPI { void FreeDataSpace(Device dev, void* ptr) final; //! \brief Hexagon-only interface to allocate buffers used for the RPC server - void* AllocRpcDataSpace(Device dev, size_t nbytes, size_t alignment, DLDataType type_hint); + void* AllocRpcBuffer(size_t nbytes, size_t alignment); //! \brief Hexagon-only interface to free buffers used for the RPC server - void FreeRpcDataSpace(Device dev, void* ptr); + void FreeRpcBuffer(void* ptr); /*! \brief Request a dynamically allocated HexagonBuffer from a workspace pool. * \returns The underlying allocation pointer. @@ -195,14 +197,21 @@ class HexagonDeviceAPI final : public DeviceAPI { } //! \brief Manages RPC HexagonBuffer allocations - // rpc_hexbuffs is used only in Alloc/FreeRpcDataSpace - std::unique_ptr rpc_hexbuffs; + // rpc_hexbuffs is used only in Alloc/FreeRpcBuffer. It is static because it lives for the + // lifetime of the static Device API. + HexagonBufferManager rpc_hexbuffs; //! \brief Manages runtime HexagonBuffer allocations - // runtime_hexbuffs is used for runtime allocations. It is created - // with a call to AcquireResources, and destroyed on ReleaseResources. + // runtime_hexbuffs is used for runtime allocations, separate from rpc_hexbuffs. It is created + // with a call to AcquireResources, and destroyed on ReleaseResources. The buffers in this + // manager are scoped to the lifetime of a user application session. std::unique_ptr runtime_hexbuffs; + //! \brief Keeps a list of released runtime HexagonBuffer allocations + // ReleaseResources can be called when there are still buffers in runtime_hexbuffs. This list + // stores the buffers that were released. + std::vector released_runtime_buffers; + //! \brief Thread manager std::unique_ptr runtime_threads; const unsigned threads{6}; diff --git a/src/runtime/hexagon/rpc/hexagon/rpc_server.cc b/src/runtime/hexagon/rpc/hexagon/rpc_server.cc index 8bee4ca19323..29c3a1bdfe6d 100644 --- a/src/runtime/hexagon/rpc/hexagon/rpc_server.cc +++ b/src/runtime/hexagon/rpc/hexagon/rpc_server.cc @@ -152,14 +152,13 @@ class HexagonPageAllocator { public: using ArenaPageHeader = tvm::support::ArenaPageHeader; - explicit HexagonPageAllocator(TIOHandler* io) : io_(io), dev_(DLDevice{kDLCPU, 0}) {} + explicit HexagonPageAllocator(TIOHandler* io) : io_(io) {} ArenaPageHeader* allocate(size_t min_size) { size_t npages = ((min_size + kPageSize - 1) / kPageSize); void* data; - data = HexagonDeviceAPI::Global()->AllocRpcDataSpace(dev_, npages * kPageSize, kPageAlign, - DLDataType{kDLInt, 1, 1}); + data = HexagonDeviceAPI::Global()->AllocRpcBuffer(npages * kPageSize, kPageAlign); ArenaPageHeader* header = static_cast(data); header->size = npages * kPageSize; @@ -167,16 +166,13 @@ class HexagonPageAllocator { return header; } - void deallocate(ArenaPageHeader* page) { - HexagonDeviceAPI::Global()->FreeRpcDataSpace(dev_, page); - } + void deallocate(ArenaPageHeader* page) { HexagonDeviceAPI::Global()->FreeRpcBuffer(page); } static const constexpr int kPageSize = 2 << 10; static const constexpr int kPageAlign = 8; private: TIOHandler* io_; - Device dev_; }; class HexagonRPCServer { diff --git a/tests/cpp-runtime/hexagon/hexagon_device_api_tests.cc b/tests/cpp-runtime/hexagon/hexagon_device_api_tests.cc index d2d79e626b37..e262a16ada5c 100644 --- a/tests/cpp-runtime/hexagon/hexagon_device_api_tests.cc +++ b/tests/cpp-runtime/hexagon/hexagon_device_api_tests.cc @@ -164,14 +164,14 @@ TEST_F(HexagonDeviceAPITest, runtime_buffer_manager) { // Ensure RPC buffer manager is always available TEST_F(HexagonDeviceAPITest, rpc_buffer_manager) { void* rpc_buf; - rpc_buf = hexapi->AllocRpcDataSpace(hex_dev, nbytes, alignment, int8); + rpc_buf = hexapi->AllocRpcBuffer(nbytes, alignment); CHECK(rpc_buf != nullptr); hexapi->ReleaseResources(); - hexapi->FreeRpcDataSpace(hex_dev, rpc_buf); - rpc_buf = hexapi->AllocRpcDataSpace(hex_dev, nbytes, alignment, int8); + hexapi->FreeRpcBuffer(rpc_buf); + rpc_buf = hexapi->AllocRpcBuffer(nbytes, alignment); CHECK(rpc_buf != nullptr); hexapi->AcquireResources(); - hexapi->FreeRpcDataSpace(hex_dev, rpc_buf); + hexapi->FreeRpcBuffer(rpc_buf); } // Ensure thread manager is properly configured and destroyed From 9d3fddf8e82a02d56c927e6cadbd45837b551fff Mon Sep 17 00:00:00 2001 From: Janet Schneider Date: Wed, 12 Oct 2022 09:09:46 -0700 Subject: [PATCH 5/5] PR feedback --- src/runtime/hexagon/hexagon_buffer_manager.h | 8 ++++---- src/runtime/hexagon/hexagon_device_api.cc | 8 ++++---- src/runtime/hexagon/hexagon_device_api.h | 2 +- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/runtime/hexagon/hexagon_buffer_manager.h b/src/runtime/hexagon/hexagon_buffer_manager.h index 4381f6fa442f..eecf96a6db07 100644 --- a/src/runtime/hexagon/hexagon_buffer_manager.h +++ b/src/runtime/hexagon/hexagon_buffer_manager.h @@ -86,14 +86,14 @@ class HexagonBufferManager { return hexagon_buffer_map_.empty(); } - //! \brief Returns a list of allocated pointers. + //! \brief Returns a vector of currently allocated pointers, owned by the manager. // Note - this should only be used by the device API to keep track of what // was in the manager when HexagonDeviceAPI::ReleaseResources is called. - std::vector vector() { + std::vector current_allocations() { std::vector allocated; std::lock_guard lock(map_mutex_); - for (auto it = hexagon_buffer_map_.begin(); it != hexagon_buffer_map_.end(); it++) { - allocated.push_back(it->first); + for (const auto& [data_ptr, buffer] : hexagon_buffer_map_) { + allocated.push_back(data_ptr); } return allocated; } diff --git a/src/runtime/hexagon/hexagon_device_api.cc b/src/runtime/hexagon/hexagon_device_api.cc index 5c5bdfff7e8f..3574ab50182c 100644 --- a/src/runtime/hexagon/hexagon_device_api.cc +++ b/src/runtime/hexagon/hexagon_device_api.cc @@ -142,9 +142,6 @@ void HexagonDeviceAPI::FreeDataSpace(Device dev, void* ptr) { void* HexagonDeviceAPI::AllocRpcBuffer(size_t nbytes, size_t alignment) { CHECK(nbytes) << "number of bytes is zero"; CHECK(alignment) << "alignment is zero"; - if (alignment < kHexagonAllocAlignment) { - alignment = kHexagonAllocAlignment; - } return rpc_hexbuffs.AllocateHexagonBuffer(nbytes, alignment, String("global")); } @@ -166,7 +163,10 @@ void* HexagonDeviceAPI::AllocWorkspace(Device dev, size_t size, DLDataType type_ void HexagonDeviceAPI::FreeWorkspace(Device dev, void* data) { CHECK(IsValidDevice(dev)) << "dev.device_type: " << dev.device_type; - CHECK(runtime_hexbuffs == nullptr || runtime_hexbuffs->count(data) != 0) + CHECK(runtime_hexbuffs) << "Attempted to free Hexagon workspace with " + << "HexagonDeviceAPI::FreeWorkspace outside of a session. " + << "Please call HexagonDeviceAPI::AcquireResources"; + CHECK(runtime_hexbuffs->count(data) != 0) << "Attempt made to free unknown or already freed workspace allocation"; dmlc::ThreadLocalStore::Get()->FreeWorkspace(dev, data); } diff --git a/src/runtime/hexagon/hexagon_device_api.h b/src/runtime/hexagon/hexagon_device_api.h index d5909ef99b4e..8d2795e7a04e 100644 --- a/src/runtime/hexagon/hexagon_device_api.h +++ b/src/runtime/hexagon/hexagon_device_api.h @@ -80,7 +80,7 @@ class HexagonDeviceAPI final : public DeviceAPI { CHECK(runtime_hexbuffs) << "runtime_hexbuffs was not created in AcquireResources"; if (!runtime_hexbuffs->empty()) { DLOG(INFO) << "runtime_hexbuffs was not empty in ReleaseResources"; - released_runtime_buffers = runtime_hexbuffs->vector(); + released_runtime_buffers = runtime_hexbuffs->current_allocations(); } runtime_hexbuffs.reset();