Repository navigation
Commit f1eb18b
Validate and lock SharedBuffer allocations (#24463)
Summary:
## TL;DR
Serialize the QNN shared-buffer manager's bookkeeping, preserve zero-byte allocations, and return `nullptr` for invalid requests or recoverable bookkeeping failures. A duplicate pointer returned by the RPC allocator must leave the existing live allocation intact. Host regressions cover these contracts, and the existing OSS QNN Buck job now executes the suite instead of only building it.
## Context
`QnnExecuTorchAllocCustomMem` allocates RPC-backed memory through the process-wide `SharedBuffer` singleton. Clients can associate tensor addresses with an allocation using `QnnExecuTorchAddCustomMemTensorAddr`; `QnnManager::RegisterMem` then selects direct ION registration or custom-memory registration during execution. `QnnExecuTorchFreeCustomMem` frees the original RPC pointer and removes the allocation and its tensor aliases.
The manager maintains four maps: aligned pointer to original RPC pointer, aligned pointer to allocated size, tensor address to custom-memory base, and custom-memory base to tensor addresses. These maps describe ownership and registration metadata; they are not owners of the client's execution lifetime. The client must retain an allocation and its aliases until every registration and execution using them has finished.
```text
client allocation client execution
QnnExecuTorchAllocCustomMem QnnManager::RegisterMem
-> SharedBuffer::AllocMem -> alias lookup
-> RPC allocation [bytes + alignment] -> RegisterCustomMem / RegisterIonMem
-> aligned address + bookkeeping -> QNN execution
client teardown, after registration and execution have finished
QnnExecuTorchFreeCustomMem -> FreeMem -> RPC free + removal of all aliases
```
This is allocator hardening for every QNN custom-memory client. It does not enable shared I/O, change exported model contents, or establish safe concurrent execution and freeing of the same allocation.
## Problem
Map reads and writes previously had no common mutex. Calls on independent threads could mutate the same containers while another thread looked up registration metadata. Even locking each lookup separately does not make a series of size and file-descriptor reads one consistent observation.
Allocation bookkeeping also had failure cases. The RPC allocator takes a signed 32-bit byte count, but the requested size and alignment are `size_t`. For example, `bytes = INT32_MAX - 63` and `alignment = 64` require `2,147,483,648` bytes, which exceeds its limit by one byte. Alignment zero and non-power-of-two alignment are invalid. Zero **bytes**, however, were historically valid: the alignment padding still makes the RPC request positive, and rejecting that request breaks callers with empty I/O tensors.
The duplicate-address failure is distinct from allocation failure. If an allocator returns the same still-live pointer twice, failure to insert the second mapping must not free that pointer or overwrite the first allocation's size. A successful RPC allocation can also be followed by `std::bad_alloc` from either map insertion; that must release the new allocation and return `nullptr` when the standard library supports throwing allocation failures.
```text
duplicate RPC address P
before: existing P -> first size
second allocation returns P
insertion fails -> free(P) -> first allocation now refers to freed memory
after: existing P -> first size
second allocation returns P
reject with nullptr -> no free, no map changes
first allocation's eventual FreeMem(P) frees it once
```
## Solution
One `memory_mutex_` protects allocation, freeing, alias bookkeeping and lookup operations. `GetAllocationInfo` obtains the allocated size and RPC file descriptor under one lock and returns `{0, -1}` for an allocation that is no longer recorded. `RegisterCustomMem` consumes that snapshot instead of taking independent size and descriptor snapshots. This makes the snapshot consistent; it does not retain memory after the lock is released. The public free API documents the client's lifetime obligation explicitly. Remove the obsolete `GetAllocatedSize` accessor now that runtime registration uses `GetAllocationInfo`. Tests read `allocated_size` from that same aggregate API; the live-allocation test checks it directly against requested bytes plus alignment, preserving an independent expected value.
Validate alignment and require `bytes <= INT32_MAX - alignment` before calling RPC. This avoids overflowing the sum and accepts the exact limit `bytes + alignment == INT32_MAX`. `AllocMem(0, alignment)` allocates only the padding. Reject a duplicate aligned address before taking ownership of it, because freeing it would destroy the existing live allocation.
For a new address, insert the original-pointer and size mappings while holding the mutex. If an insertion throws `std::bad_alloc`, erase the partial pointer mapping, free the new RPC allocation, log the failure and return `nullptr`. There is no redundant assignment after insertion. Toolchains without throwing allocation support retain their allocation-failure behavior; the regression that injects throwing `operator new` is enabled only where those exceptions are supported.
```text
AllocMem(bytes, alignment), size units: bytes
validate -> lock -> RPC allocation -> align address
|
address already live? -> nullptr; preserve live owner
|
insert original-pointer mapping
|
insert allocated-size mapping
|
return aligned pointer
either insertion throws bad_alloc
-> erase partial bookkeeping -> RPC free -> nullptr
```
The host tests exercise offset alignment and freeing the original pointer, zero-byte compatibility, invalid requests without an RPC call, the exact signed-32-bit limit, duplicate live addresses, allocation snapshots before and after free, and concurrent bookkeeping on distinct allocations. Where supported, an injected allocation failure exercises each insertion separately and checks cleanup and successful retry. Test teardown uses the configured RPC free callback, so an offset allocator is cleaned up correctly even after a failed assertion.
The OSS `test-qnn-buck-build-linux` job already installs the QNN SDK and builds the backend. It now runs `buck2 test //backends/qualcomm/runtime/tests:shared_buffer_test`. The suite uses fake RPC callbacks and needs no device or loaded RPC library; an actual green OSS run remains a separate verification gate.
## Compatibility and evidence limits
No exported C function signature changes. Empty tensors keep their previous allocation behavior. Invalid alignment and requests exceeding the signed RPC allocation limit return `nullptr`; duplicate allocator addresses preserve the original allocation. The aggregate lookup is a C++ backend helper, not a change to model serialization.
The mutex protects bookkeeping and metadata snapshots, not a registration-to-execution lease. Clients must coordinate `FreeMem` with all users of that allocation. Host tests verify allocator contracts; they do not establish device registration behavior or performance. Concrete results and earlier device evidence are recorded separately in the Test Plan.
Authored with AI assistance from Codex and Claude Code.
Differential Revision: D128567553
Signed-off-by: Ali Monfared <alimonfared@meta.com>1 parent b21fa8b commit f1eb18b
8 files changed
Lines changed: 402 additions & 18 deletions
File tree
- .github/workflows
- backends/qualcomm/runtime
- tests
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
986 | 986 | | |
987 | 987 | | |
988 | 988 | | |
| 989 | + | |
| 990 | + | |
| 991 | + | |
989 | 992 | | |
990 | 993 | | |
991 | 994 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
89 | 89 | | |
90 | 90 | | |
91 | 91 | | |
92 | | - | |
| 92 | + | |
| 93 | + | |
93 | 94 | | |
94 | 95 | | |
95 | 96 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
214 | 214 | | |
215 | 215 | | |
216 | 216 | | |
217 | | - | |
218 | | - | |
219 | 217 | | |
220 | | - | |
221 | | - | |
222 | | - | |
223 | | - | |
| 218 | + | |
| 219 | + | |
224 | 220 | | |
225 | 221 | | |
226 | 222 | | |
227 | | - | |
| 223 | + | |
228 | 224 | | |
229 | 225 | | |
230 | 226 | | |
| |||
234 | 230 | | |
235 | 231 | | |
236 | 232 | | |
237 | | - | |
| 233 | + | |
238 | 234 | | |
239 | | - | |
| 235 | + | |
240 | 236 | | |
241 | 237 | | |
242 | 238 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
7 | 7 | | |
8 | 8 | | |
9 | 9 | | |
| 10 | + | |
10 | 11 | | |
11 | 12 | | |
12 | 13 | | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
13 | 17 | | |
14 | 18 | | |
15 | 19 | | |
| |||
63 | 67 | | |
64 | 68 | | |
65 | 69 | | |
| 70 | + | |
66 | 71 | | |
67 | 72 | | |
68 | 73 | | |
69 | 74 | | |
70 | 75 | | |
71 | 76 | | |
72 | 77 | | |
73 | | - | |
| 78 | + | |
| 79 | + | |
74 | 80 | | |
75 | 81 | | |
76 | | - | |
| 82 | + | |
77 | 83 | | |
78 | | - | |
| 84 | + | |
79 | 85 | | |
80 | 86 | | |
81 | 87 | | |
| |||
109 | 115 | | |
110 | 116 | | |
111 | 117 | | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
112 | 125 | | |
113 | 126 | | |
114 | 127 | | |
| |||
117 | 130 | | |
118 | 131 | | |
119 | 132 | | |
| 133 | + | |
120 | 134 | | |
121 | 135 | | |
122 | | - | |
123 | | - | |
124 | | - | |
125 | | - | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
| 149 | + | |
| 150 | + | |
126 | 151 | | |
| 152 | + | |
| 153 | + | |
127 | 154 | | |
| 155 | + | |
128 | 156 | | |
129 | 157 | | |
130 | 158 | | |
131 | 159 | | |
| 160 | + | |
132 | 161 | | |
133 | 162 | | |
134 | 163 | | |
| |||
139 | 168 | | |
140 | 169 | | |
141 | 170 | | |
| 171 | + | |
142 | 172 | | |
143 | 173 | | |
144 | 174 | | |
| |||
159 | 189 | | |
160 | 190 | | |
161 | 191 | | |
| 192 | + | |
162 | 193 | | |
163 | 194 | | |
164 | 195 | | |
| |||
197 | 228 | | |
198 | 229 | | |
199 | 230 | | |
| 231 | + | |
200 | 232 | | |
201 | 233 | | |
202 | 234 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
59 | 59 | | |
60 | 60 | | |
61 | 61 | | |
62 | | - | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
63 | 70 | | |
64 | 71 | | |
65 | 72 | | |
66 | 73 | | |
| 74 | + | |
67 | 75 | | |
68 | 76 | | |
69 | 77 | | |
| |||
88 | 96 | | |
89 | 97 | | |
90 | 98 | | |
| 99 | + | |
91 | 100 | | |
92 | 101 | | |
93 | 102 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
0 commit comments