Cleanup NNAPI runtime Memory objects
Prior to this CL, runtime Memory* objects were default constructed and
set to a value later. Rebinding the value led to multiple bugs in the
past and made the Memory* objects prone to data races. This CL addresses
these issues by determining all values in the Memory* objects' factory
methods, and making the Memory* objects immutable after construction.
This CL also untangles some of the inheritance hierarchy. Prior to this
CL, all MemoryFd and MemoryAHWB inherited from Memory, which was
effectively ashmem-based memory. This CL separates MemoryAshmem into its
own, unique class type, and has a common base Memory class. This
reorganization also uncovered that getPointer was only used in the
runtime on ashmem-based memory, and removes the unused getPointer
methods.
Finally, this CL improves documentation of NeuralNetworks.h in two ways:
1) Fixes the typo "ANeuralNetworksMemory_createFromAHardwarBuffer"
2) Documents the missing lifetime constraints of
ANeuralNetworksMemory_createFromAHardwareBuffer
Fixes: 138852228
Fixes: 69632863
Fixes: 69633035
Fixes: 129572123
Fixes: 132323765
Fixes: 139213289
Test: mma
Test: atest NeuralNetworksTest_static
Test: atest CtsNNAPITestCases
Change-Id: I49a2356d6b8fc38e501d8e37ba8a8893f3e91395
Merged-In: I49a2356d6b8fc38e501d8e37ba8a8893f3e91395
(cherry picked from commit 008caeab69787b8978e874f8fb3811e2400f5d55)
diff --git a/runtime/Memory.cpp b/runtime/Memory.cpp
index 20fabfa..6e972c6 100644
--- a/runtime/Memory.cpp
+++ b/runtime/Memory.cpp
@@ -18,6 +18,8 @@
#include "Memory.h"
+#include <memory>
+#include <utility>
#include "ExecutionBurstController.h"
#include "MemoryUtils.h"
#include "Utils.h"
@@ -27,6 +29,8 @@
using namespace hal;
+Memory::Memory(hidl_memory memory) : kHidlMemory(std::move(memory)) {}
+
Memory::~Memory() {
for (const auto [ptr, weakBurst] : mUsedBy) {
if (const std::shared_ptr<ExecutionBurstController> burst = weakBurst.lock()) {
@@ -35,23 +39,12 @@
}
}
-int Memory::create(uint32_t size) {
- mHidlMemory = allocateSharedMemory(size);
- mMemory = mapMemory(mHidlMemory);
- if (mMemory == nullptr) {
- LOG(ERROR) << "Memory::create failed";
- return ANEURALNETWORKS_OUT_OF_MEMORY;
- }
- return ANEURALNETWORKS_NO_ERROR;
-}
-
bool Memory::validateSize(uint32_t offset, uint32_t length) const {
- if (offset + length > mHidlMemory.size()) {
+ if (offset + length > kHidlMemory.size()) {
LOG(ERROR) << "Request size larger than the memory size.";
return false;
- } else {
- return true;
}
+ return true;
}
intptr_t Memory::getKey() const {
@@ -63,83 +56,98 @@
mUsedBy.emplace(burst.get(), burst);
}
-MemoryFd::~MemoryFd() {
- // Unmap the memory.
- if (mMapping) {
- munmap(mMapping, mHidlMemory.size());
+std::pair<int, std::unique_ptr<MemoryAshmem>> MemoryAshmem::create(uint32_t size) {
+ hidl_memory hidlMemory = allocateSharedMemory(size);
+ sp<IMemory> mapped = mapMemory(hidlMemory);
+ if (mapped == nullptr || mapped->getPointer() == nullptr) {
+ LOG(ERROR) << "Memory::create failed";
+ return {ANEURALNETWORKS_OUT_OF_MEMORY, nullptr};
}
- // Delete the native_handle.
- if (mHandle) {
- int fd = mHandle->data[0];
- if (fd != -1) {
- close(fd);
- }
- native_handle_delete(mHandle);
- }
+ return {ANEURALNETWORKS_NO_ERROR,
+ std::make_unique<MemoryAshmem>(std::move(mapped), std::move(hidlMemory))};
}
-int MemoryFd::set(size_t size, int prot, int fd, size_t offset) {
+uint8_t* MemoryAshmem::getPointer() const {
+ return static_cast<uint8_t*>(static_cast<void*>(kMappedMemory->getPointer()));
+}
+
+MemoryAshmem::MemoryAshmem(sp<IMemory> mapped, hidl_memory memory)
+ : Memory(std::move(memory)), kMappedMemory(std::move(mapped)) {}
+
+std::pair<int, std::unique_ptr<MemoryFd>> MemoryFd::create(size_t size, int prot, int fd,
+ size_t offset) {
if (size == 0 || fd < 0) {
LOG(ERROR) << "Invalid size or fd";
- return ANEURALNETWORKS_BAD_DATA;
+ return {ANEURALNETWORKS_BAD_DATA, nullptr};
}
+
+ // Duplicate the file descriptor so MemoryFd owns its own version.
int dupfd = dup(fd);
if (dupfd == -1) {
LOG(ERROR) << "Failed to dup the fd";
- return ANEURALNETWORKS_UNEXPECTED_NULL;
+ // TODO(b/120417090): is ANEURALNETWORKS_UNEXPECTED_NULL the correct
+ // error to return here?
+ return {ANEURALNETWORKS_UNEXPECTED_NULL, nullptr};
}
- if (mMapping) {
- if (munmap(mMapping, mHidlMemory.size()) != 0) {
- LOG(ERROR) << "Failed to remove the existing mapping";
- // This is not actually fatal.
- }
- mMapping = nullptr;
- }
- if (mHandle) {
- native_handle_delete(mHandle);
- }
- mHandle = native_handle_create(1, 3);
- if (mHandle == nullptr) {
+ // Create a temporary native handle to own the dupfd.
+ native_handle_t* nativeHandle = native_handle_create(1, 3);
+ if (nativeHandle == nullptr) {
LOG(ERROR) << "Failed to create native_handle";
- return ANEURALNETWORKS_UNEXPECTED_NULL;
+ // TODO(b/120417090): is ANEURALNETWORKS_UNEXPECTED_NULL the correct
+ // error to return here?
+ return {ANEURALNETWORKS_UNEXPECTED_NULL, nullptr};
}
- mHandle->data[0] = dupfd;
- mHandle->data[1] = prot;
- mHandle->data[2] = (int32_t)(uint32_t)(offset & 0xffffffff);
-#if defined(__LP64__)
- mHandle->data[3] = (int32_t)(uint32_t)(offset >> 32);
-#else
- mHandle->data[3] = 0;
-#endif
- mHidlMemory = hidl_memory("mmap_fd", mHandle, size);
- return ANEURALNETWORKS_NO_ERROR;
+ nativeHandle->data[0] = dupfd;
+ nativeHandle->data[1] = prot;
+ const uint64_t bits = static_cast<uint64_t>(offset);
+ nativeHandle->data[2] = (int32_t)(uint32_t)(bits & 0xffffffff);
+ nativeHandle->data[3] = (int32_t)(uint32_t)(bits >> 32);
+
+ // Create a hidl_handle which owns the native handle and fd so that we don't
+ // have to manually clean either the native handle or the fd.
+ hardware::hidl_handle hidlHandle;
+ hidlHandle.setTo(nativeHandle, /*shouldOwn=*/true);
+
+ // Push the hidl_handle into a hidl_memory object. The hidl_memory object is
+ // responsible for cleaning the hidl_handle, the native handle, and the fd.
+ hidl_memory hidlMemory = hidl_memory("mmap_fd", std::move(hidlHandle), size);
+
+ return {ANEURALNETWORKS_NO_ERROR, std::make_unique<MemoryFd>(std::move(hidlMemory))};
}
-int MemoryFd::getPointer(uint8_t** buffer) const {
- if (mMapping) {
- *buffer = mMapping;
- return ANEURALNETWORKS_NO_ERROR;
- }
+MemoryFd::MemoryFd(hidl_memory memory) : Memory(std::move(memory)) {}
- if (mHandle == nullptr) {
- LOG(ERROR) << "Memory not initialized";
- return ANEURALNETWORKS_UNEXPECTED_NULL;
- }
-
- int fd = mHandle->data[0];
- int prot = mHandle->data[1];
- size_t offset = getSizeFromInts(mHandle->data[2], mHandle->data[3]);
- void* data = mmap(nullptr, mHidlMemory.size(), prot, MAP_SHARED, fd, offset);
- if (data == MAP_FAILED) {
- LOG(ERROR) << "MemoryFd::getPointer(): Can't mmap the file descriptor.";
- return ANEURALNETWORKS_UNMAPPABLE;
+std::pair<int, std::unique_ptr<MemoryAHWB>> MemoryAHWB::create(const AHardwareBuffer& ahwb) {
+ AHardwareBuffer_Desc bufferDesc;
+ AHardwareBuffer_describe(&ahwb, &bufferDesc);
+ const native_handle_t* handle = AHardwareBuffer_getNativeHandle(&ahwb);
+ hidl_memory hidlMemory;
+ if (bufferDesc.format == AHARDWAREBUFFER_FORMAT_BLOB) {
+ hidlMemory = hidl_memory("hardware_buffer_blob", handle, bufferDesc.width);
} else {
- mMapping = *buffer = static_cast<uint8_t*>(data);
- return ANEURALNETWORKS_NO_ERROR;
+ // memory size is not used.
+ hidlMemory = hidl_memory("hardware_buffer", handle, 0);
}
+
+ std::unique_ptr<MemoryAHWB> memory =
+ std::make_unique<MemoryAHWB>(bufferDesc, std::move(hidlMemory));
+ return {ANEURALNETWORKS_NO_ERROR, std::move(memory)};
+};
+
+bool MemoryAHWB::validateSize(uint32_t offset, uint32_t length) const {
+ // validateSize should only be called on BLOB mode buffer.
+ if (!kBlobMode) {
+ LOG(ERROR) << "Invalid AHARDWAREBUFFER_FORMAT, must be AHARDWAREBUFFER_FORMAT_BLOB.";
+ return false;
+ }
+ // Use normal validation.
+ return Memory::validateSize(offset, length);
}
+MemoryAHWB::MemoryAHWB(const AHardwareBuffer_Desc& desc, hidl_memory memory)
+ : Memory(std::move(memory)), kBlobMode(desc.format == AHARDWAREBUFFER_FORMAT_BLOB) {}
+
uint32_t MemoryTracker::add(const Memory* memory) {
VLOG(MODEL) << __func__ << "(" << SHOW_IF_DEBUG(memory) << ")";
// See if we already have this memory. If so,
