Track cuMemAllocFromPoolAsync allocations and free untracked async pointers#217
Track cuMemAllocFromPoolAsync allocations and free untracked async pointers#217imlach wants to merge 2 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: imlach The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @imlach! It looks like this is your first PR to Project-HAMi/HAMi-core 🎉 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR updates async CUDA allocation and free handling for default-pool and explicit-pool allocations. Untracked async frees now fall through to ChangesPool-aware async allocation and free tracking
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant allocator as allocate_from_pool_async_raw
participant pool_add as add_chunk_from_pool_async
participant accounting as account_async_chunk
Caller->>allocator: allocate_from_pool_async_raw(dptr, size, pool, hStream)
allocator->>pool_add: cuMemAllocFromPoolAsync(...)
pool_add->>accounting: account_async_chunk(pool, entry)
accounting-->>pool_add: adjusted tracked length
pool_add-->>allocator: allocation result
allocator-->>Caller: CUresult
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…inters cuMemAllocFromPoolAsync was a bare pass-through, so its pointer was never registered; the matching cuMemFreeAsync then returned -1 without freeing (issue Project-HAMi#93, e.g. RAPIDS RMM) - an invalid CUresult ("unrecognized error code") plus a leak. It now registers its allocation like cuMemAllocAsync, accounting against the caller's pool, and remove_chunk_async frees any pointer it doesn't recognise for real instead of returning -1. Both async alloc paths also return CUDA_ERROR_OUT_OF_MEMORY rather than -1 on the limit. Adds test/test_alloc_pool_async.c. Fixes Project-HAMi#93 Relates to Project-HAMi#67 Signed-off-by: Ross Imlach <ross@imla.ch>
imlach
force-pushed
the
imlach/fix-cumemallocfrompoolasync-tracking
branch
from
4f92197 to
9e1b0c3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/allocator/allocator.c (1)
290-334: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winSame allocate-then-leak-on-failure pattern in the new pool-aware paths.
Both
add_chunk_async(newcuDeviceGetMemPoolfailure branch, lines 306-312) andadd_chunk_from_pool_async(cuMemAllocFromPoolAsyncfailure branch, lines 327-330) write*addressand/or leakebefore all failure paths are handled:
- In
add_chunk_async,*address = e->entry->address;(line 305) runs beforecuDeviceGetMemPool. If that call fails, the function returns an error but the real device allocation from the precedingcuMemAllocAsynccall is already live and now untracked/unfreeable, andeis leaked.- In
add_chunk_from_pool_async, ifcuMemAllocFromPoolAsyncfails,eis leaked (matches cppcheck's memory leak flag at line 325) — no device leak here since the allocation itself failed, but the host-sideallocated_list_entryis never freed.🐛 Proposed fix
res = CUDA_OVERRIDE_CALL(cuda_library_entry,cuMemAllocFromPoolAsync,&e->entry->address,size,pool,hStream); if (res != CUDA_SUCCESS) { LOG_ERROR("cuMemAllocFromPoolAsync failed res=%d",res); + free(e); return res; }res = CUDA_OVERRIDE_CALL(cuda_library_entry,cuDeviceGetMemPool,&pool,dev); if (res != CUDA_SUCCESS) { LOG_ERROR("cuDeviceGetMemPool failed res=%d",res); + CUDA_OVERRIDE_CALL(cuda_library_entry,cuMemFreeAsync,e->entry->address,hStream); + free(e); return res; }Same root cause as the
account_async_chunkleak flagged above.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/allocator/allocator.c` around lines 290 - 334, The new async allocation paths still leak on failure: `add_chunk_async` assigns `*address` before `cuDeviceGetMemPool`, so if that call fails the `cuMemAllocAsync` allocation and `allocated_list_entry` are left untracked; `add_chunk_from_pool_async` also leaks its `allocated_list_entry` when `cuMemAllocFromPoolAsync` fails. Fix both functions by deferring `*address` until all setup succeeds, and by cleaning up the allocated entry and any successful device allocation on every error path in `add_chunk_async` and `add_chunk_from_pool_async`.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/allocator/allocator.c`:
- Around line 265-288: The early return in account_async_chunk leaves both the
tracking entry e and the already-created async device allocation unhandled when
cuMemPoolGetAttribute fails. Update account_async_chunk and its callers
(add_chunk_async and add_chunk_from_pool_async) so the failure path frees or
rolls back the allocated device memory, releases the host tracking entry, and
does not skip cleanup before returning the CUDA error.
- Around line 265-296: The async accounting in account_async_chunk() is using
the shared device_allocasync->limit for CU_MEMPOOL_ATTR_RESERVED_MEM_HIGH, which
mixes different pools together. Update the accounting to track high-water usage
per CUmemoryPool (using the pool passed into account_async_chunk and the chunk’s
pool association) instead of a global counter, and make the corresponding
free/unaccount path subtract against the same per-pool state so allocations from
one pool do not suppress or erase accounting for another.
---
Outside diff comments:
In `@src/allocator/allocator.c`:
- Around line 290-334: The new async allocation paths still leak on failure:
`add_chunk_async` assigns `*address` before `cuDeviceGetMemPool`, so if that
call fails the `cuMemAllocAsync` allocation and `allocated_list_entry` are left
untracked; `add_chunk_from_pool_async` also leaks its `allocated_list_entry`
when `cuMemAllocFromPoolAsync` fails. Fix both functions by deferring `*address`
until all setup succeeds, and by cleaning up the allocated entry and any
successful device allocation on every error path in `add_chunk_async` and
`add_chunk_from_pool_async`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0c263669-e3a4-4aa2-83a2-7af139c2a743
📒 Files selected for processing (4)
src/allocator/allocator.csrc/allocator/allocator.hsrc/cuda/memory.ctest/test_alloc_pool_async.c
| /* Track an allocated async chunk and account it against its pool's high-water | ||
| * limit. Call with mutex held. */ | ||
| static int account_async_chunk(allocated_list_entry *e, size_t size, CUmemoryPool pool) { | ||
| size_t poollimit; | ||
| CUresult res = CUDA_OVERRIDE_CALL(cuda_library_entry,cuMemPoolGetAttribute,pool,CU_MEMPOOL_ATTR_RESERVED_MEM_HIGH,&poollimit); | ||
| if (res != CUDA_SUCCESS) { | ||
| LOG_ERROR("cuMemPoolGetAttribute failed res=%d",res); | ||
| return res; | ||
| } | ||
| if (poollimit != 0) { | ||
| if (poollimit> device_allocasync->limit) { | ||
| size_t allocsize = (poollimit-device_allocasync->limit < size)? poollimit-device_allocasync->limit : size; | ||
| add_gpu_device_memory_usage(getpid(), e->entry->dev, allocsize, 2); | ||
| device_allocasync->limit=device_allocasync->limit+allocsize; | ||
| e->entry->length=allocsize; | ||
| }else{ | ||
| e->entry->length=0; | ||
| } | ||
| }else{ | ||
| e->entry->length=0; | ||
| } | ||
| LIST_ADD(device_allocasync,e); | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Leaks the driver allocation and the tracking entry when cuMemPoolGetAttribute fails.
By the time account_async_chunk is called, the real device allocation (cuMemAllocAsync/cuMemAllocFromPoolAsync) has already succeeded. If cuMemPoolGetAttribute fails, the function returns early without LIST_ADD-ing e into device_allocasync and without freeing e or the underlying device pointer. The result: e (host memory) leaks, and the already-allocated device memory becomes permanently untracked and unfreeable — the exact class of leak this PR set out to fix, just relocated to a rarer failure path. This matches cppcheck's nullPointerOutOfResources/leak signals around this region.
🐛 Proposed fix — release what was already allocated on this failure path
-static int account_async_chunk(allocated_list_entry *e, size_t size, CUmemoryPool pool) {
+static int account_async_chunk(allocated_list_entry *e, size_t size, CUmemoryPool pool, CUstream hStream) {
size_t poollimit;
CUresult res = CUDA_OVERRIDE_CALL(cuda_library_entry,cuMemPoolGetAttribute,pool,CU_MEMPOOL_ATTR_RESERVED_MEM_HIGH,&poollimit);
if (res != CUDA_SUCCESS) {
LOG_ERROR("cuMemPoolGetAttribute failed res=%d",res);
+ /* The real allocation already succeeded; don't strand it. */
+ CUDA_OVERRIDE_CALL(cuda_library_entry,cuMemFreeAsync,e->entry->address,hStream);
+ free(e);
return res;
}Callers need the extra hStream argument:
- return account_async_chunk(e, size, pool);
+ return account_async_chunk(e, size, pool, hStream);(applies to both call sites in add_chunk_async and add_chunk_from_pool_async)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /* Track an allocated async chunk and account it against its pool's high-water | |
| * limit. Call with mutex held. */ | |
| static int account_async_chunk(allocated_list_entry *e, size_t size, CUmemoryPool pool) { | |
| size_t poollimit; | |
| CUresult res = CUDA_OVERRIDE_CALL(cuda_library_entry,cuMemPoolGetAttribute,pool,CU_MEMPOOL_ATTR_RESERVED_MEM_HIGH,&poollimit); | |
| if (res != CUDA_SUCCESS) { | |
| LOG_ERROR("cuMemPoolGetAttribute failed res=%d",res); | |
| return res; | |
| } | |
| if (poollimit != 0) { | |
| if (poollimit> device_allocasync->limit) { | |
| size_t allocsize = (poollimit-device_allocasync->limit < size)? poollimit-device_allocasync->limit : size; | |
| add_gpu_device_memory_usage(getpid(), e->entry->dev, allocsize, 2); | |
| device_allocasync->limit=device_allocasync->limit+allocsize; | |
| e->entry->length=allocsize; | |
| }else{ | |
| e->entry->length=0; | |
| } | |
| }else{ | |
| e->entry->length=0; | |
| } | |
| LIST_ADD(device_allocasync,e); | |
| return 0; | |
| } | |
| /* Track an allocated async chunk and account it against its pool's high-water | |
| * limit. Call with mutex held. */ | |
| static int account_async_chunk(allocated_list_entry *e, size_t size, CUmemoryPool pool, CUstream hStream) { | |
| size_t poollimit; | |
| CUresult res = CUDA_OVERRIDE_CALL(cuda_library_entry,cuMemPoolGetAttribute,pool,CU_MEMPOOL_ATTR_RESERVED_MEM_HIGH,&poollimit); | |
| if (res != CUDA_SUCCESS) { | |
| LOG_ERROR("cuMemPoolGetAttribute failed res=%d",res); | |
| /* The real allocation already succeeded; don't strand it. */ | |
| CUDA_OVERRIDE_CALL(cuda_library_entry,cuMemFreeAsync,e->entry->address,hStream); | |
| free(e); | |
| return res; | |
| } | |
| if (poollimit != 0) { | |
| if (poollimit> device_allocasync->limit) { | |
| size_t allocsize = (poollimit-device_allocasync->limit < size)? poollimit-device_allocasync->limit : size; | |
| add_gpu_device_memory_usage(getpid(), e->entry->dev, allocsize, 2); | |
| device_allocasync->limit=device_allocasync->limit+allocsize; | |
| e->entry->length=allocsize; | |
| }else{ | |
| e->entry->length=0; | |
| } | |
| }else{ | |
| e->entry->length=0; | |
| } | |
| LIST_ADD(device_allocasync,e); | |
| return 0; | |
| } |
🧰 Tools
🪛 Cppcheck (2.21.0)
[warning] 269-269: If resource allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfResources)
[warning] 271-271: If resource allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfResources)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/allocator/allocator.c` around lines 265 - 288, The early return in
account_async_chunk leaves both the tracking entry e and the already-created
async device allocation unhandled when cuMemPoolGetAttribute fails. Update
account_async_chunk and its callers (add_chunk_async and
add_chunk_from_pool_async) so the failure path frees or rolls back the allocated
device memory, releases the host tracking entry, and does not skip cleanup
before returning the CUDA error.
Source: Linters/SAST tools
| /* Track an allocated async chunk and account it against its pool's high-water | ||
| * limit. Call with mutex held. */ | ||
| static int account_async_chunk(allocated_list_entry *e, size_t size, CUmemoryPool pool) { | ||
| size_t poollimit; | ||
| CUresult res = CUDA_OVERRIDE_CALL(cuda_library_entry,cuMemPoolGetAttribute,pool,CU_MEMPOOL_ATTR_RESERVED_MEM_HIGH,&poollimit); | ||
| if (res != CUDA_SUCCESS) { | ||
| LOG_ERROR("cuMemPoolGetAttribute failed res=%d",res); | ||
| return res; | ||
| } | ||
| if (poollimit != 0) { | ||
| if (poollimit> device_allocasync->limit) { | ||
| size_t allocsize = (poollimit-device_allocasync->limit < size)? poollimit-device_allocasync->limit : size; | ||
| add_gpu_device_memory_usage(getpid(), e->entry->dev, allocsize, 2); | ||
| device_allocasync->limit=device_allocasync->limit+allocsize; | ||
| e->entry->length=allocsize; | ||
| }else{ | ||
| e->entry->length=0; | ||
| } | ||
| }else{ | ||
| e->entry->length=0; | ||
| } | ||
| LIST_ADD(device_allocasync,e); | ||
| return 0; | ||
| } | ||
|
|
||
| int add_chunk_async(CUdeviceptr *address, size_t size, CUstream hStream) { | ||
| size_t addr=0; | ||
| size_t allocsize; | ||
| CUresult res = CUDA_SUCCESS; | ||
| CUdevice dev; | ||
| cuCtxGetDevice(&dev); | ||
| if (oom_check(dev,size)) | ||
| return -1; | ||
| return CUDA_ERROR_OUT_OF_MEMORY; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect other consumers of device_allocasync->limit / oom_check to confirm
# whether cross-pool undercounting can actually bypass the configured memory limit.
rg -n 'device_allocasync->limit|oom_check\(' src/allocator/allocator.c src/allocator/*.hRepository: Project-HAMi/HAMi-core
Length of output: 1010
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== allocator.c outline =="
ast-grep outline src/allocator/allocator.c --view expanded || true
echo
echo "== relevant sections =="
sed -n '1,220p' src/allocator/allocator.c
echo
sed -n '220,420p' src/allocator/allocator.c
echo
echo "== allocator.h relevant definitions =="
sed -n '1,260p' src/allocator/allocator.hRepository: Project-HAMi/HAMi-core
Length of output: 19457
Track async memory per pool, not through a shared counter. CU_MEMPOOL_ATTR_RESERVED_MEM_HIGH is per-pool, but account_async_chunk() compares it against the global device_allocasync->limit, so one pool can suppress accounting in another and later frees subtract 0. Multi-pool async allocations then disappear from the process GPU-memory limit.
🧰 Tools
🪛 Cppcheck (2.21.0)
[warning] 269-269: If resource allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfResources)
[warning] 271-271: If resource allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfResources)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/allocator/allocator.c` around lines 265 - 296, The async accounting in
account_async_chunk() is using the shared device_allocasync->limit for
CU_MEMPOOL_ATTR_RESERVED_MEM_HIGH, which mixes different pools together. Update
the accounting to track high-water usage per CUmemoryPool (using the pool passed
into account_async_chunk and the chunk’s pool association) instead of a global
counter, and make the corresponding free/unaccount path subtract against the
same per-pool state so allocations from one pool do not suppress or erase
accounting for another.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/allocator/allocator.c`:
- Around line 324-329: The cuMemAllocFromPoolAsync failure path in allocator
logic leaks the temporary allocated_list_entry created by
INIT_ALLOCATED_LIST_ENTRY, along with its nested entry and allocHandle. Update
the failure branch in the allocation routine that calls CUDA_OVERRIDE_CALL on
cuMemAllocFromPoolAsync to use the same cleanup pattern as LIST_REMOVE before
returning, so e, e->entry, and e->entry->allocHandle are released on error.
- Around line 306-311: The default-pool lookup failure path in allocator.c leaks
the already-created driver allocation and tracking entry. In the cuMemAllocAsync
flow around cuDeviceGetMemPool, add cleanup before returning when res !=
CUDA_SUCCESS: release the allocation created earlier and undo the initialized
bookkeeping so the allocator state stays consistent. Use the existing
cuMemAllocAsync path and the surrounding allocation/tracking symbols in
allocator.c to locate the rollback point.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ab946327-95e5-41ac-b4d1-a403e592584a
📒 Files selected for processing (4)
src/allocator/allocator.csrc/allocator/allocator.hsrc/cuda/memory.ctest/test_alloc_pool_async.c
🚧 Files skipped from review as they are similar to previous changes (3)
- test/test_alloc_pool_async.c
- src/cuda/memory.c
- src/allocator/allocator.h
| /* cuMemAllocAsync draws from the device's default pool. */ | ||
| CUmemoryPool pool; | ||
| res = CUDA_OVERRIDE_CALL(cuda_library_entry,cuDeviceGetMemPool,&pool,dev); | ||
| if (res != CUDA_SUCCESS) { | ||
| LOG_ERROR("cuDeviceGetMemPool failed res=%d",res); | ||
| return res; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Roll back the allocation on default-pool lookup failure.
Line 300 already created the driver allocation, so returning here leaks the device memory and the initialized tracking entry.
🧰 Tools
🪛 Cppcheck (2.21.0)
[warning] 308-308: If resource allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfResources)
[warning] 310-310: If resource allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfResources)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/allocator/allocator.c` around lines 306 - 311, The default-pool lookup
failure path in allocator.c leaks the already-created driver allocation and
tracking entry. In the cuMemAllocAsync flow around cuDeviceGetMemPool, add
cleanup before returning when res != CUDA_SUCCESS: release the allocation
created earlier and undo the initialized bookkeeping so the allocator state
stays consistent. Use the existing cuMemAllocAsync path and the surrounding
allocation/tracking symbols in allocator.c to locate the rollback point.
| allocated_list_entry *e; | ||
| INIT_ALLOCATED_LIST_ENTRY(e, addr, size, dev); | ||
| res = CUDA_OVERRIDE_CALL(cuda_library_entry,cuMemAllocFromPoolAsync,&e->entry->address,size,pool,hStream); | ||
| if (res != CUDA_SUCCESS) { | ||
| LOG_ERROR("cuMemPoolGetAttribute failed res=%d",res); | ||
| LOG_ERROR("cuMemAllocFromPoolAsync failed res=%d",res); | ||
| return res; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- src/allocator/allocator.c around lines 280-360 ---'
sed -n '280,360p' src/allocator/allocator.c | cat -n
echo '--- search for INIT_ALLOCATED_LIST_ENTRY ---'
rg -n "INIT_ALLOCATED_LIST_ENTRY|allocated_list_entry|allocated_list" src
echo '--- search for destructor/free/cleanup related to allocated_list_entry ---'
rg -n "free.*allocated_list_entry|destroy.*allocated_list_entry|cleanup.*allocated_list_entry|INIT_ALLOCATED_LIST_ENTRY|TRACKING_ENTRY|allocated_list_entry.*free" srcRepository: Project-HAMi/HAMi-core
Length of output: 6294
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- src/allocator/allocator.h around INIT_ALLOCATED_LIST_ENTRY ---'
sed -n '80,150p' src/allocator/allocator.h | cat -n
echo '--- src/allocator/allocator.c remove/cleanup paths ---'
sed -n '150,280p' src/allocator/allocator.c | cat -nRepository: Project-HAMi/HAMi-core
Length of output: 10365
Release the temporary entry on cuMemAllocFromPoolAsync failure. INIT_ALLOCATED_LIST_ENTRY allocates e before this call, so the early return leaks e, e->entry, and e->entry->allocHandle; free them with the same cleanup pattern used by LIST_REMOVE.
🧰 Tools
🪛 Cppcheck (2.21.0)
[error] 325-325: Memory leak
(memleak)
[warning] 325-325: If resource allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfResources)
[warning] 326-326: If resource allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfResources)
[warning] 328-328: If resource allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfResources)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/allocator/allocator.c` around lines 324 - 329, The
cuMemAllocFromPoolAsync failure path in allocator logic leaks the temporary
allocated_list_entry created by INIT_ALLOCATED_LIST_ENTRY, along with its nested
entry and allocHandle. Update the failure branch in the allocation routine that
calls CUDA_OVERRIDE_CALL on cuMemAllocFromPoolAsync to use the same cleanup
pattern as LIST_REMOVE before returning, so e, e->entry, and
e->entry->allocHandle are released on error.
Source: Linters/SAST tools
|
Per-pool accounting. This is already called out under "Known Leak-on-failure. Real, but a pre-existing, file-wide pattern rather than Two notes on the suggested patch:
I'd rather fix these error paths consistently across the async allocator (the |
Signed-off-by: Ross Imlach <ross@imla.ch>
1 participant
Fixes #93. Relates to #67.
The bug
libvgpu tracks stream-ordered allocations so it can enforce the memory limit and
free them correctly. It tracks the ones from
cuMemAllocAsync, butcuMemAllocFromPoolAsyncwas left as a bare pass-through, so a pool allocationwas never registered.
When that pointer is freed,
remove_chunk_asyncwalks the tracking list, failsto find it, and returns -1 without calling the real
cuMemFreeAsync. The callergets -1, which isn't a valid
CUresultand shows up as "unrecognized errorcode" (issue #93, e.g. a RAPIDS RMM coredump), and the memory leaks.
We ran into this getting NVIDIA Dynamo's snapshot feature working on a
HAMi-managed GPU, and reduced it to the minimal reproducer in the test.
The fix
cuMemAllocFromPoolAsyncnow registers its allocation the same waycuMemAllocAsyncdoes, so the matching free finds and releases it. It isaccounted against the caller's pool (the one it allocates from) rather than the
device default pool; a per-pool limit would be the alternative if you'd prefer
it. The shared registration and accounting logic is factored into one helper so
the two paths can't drift.
remove_chunk_asyncalso stops returning -1 for pointers it doesn't recognise:it now frees them for real and returns the driver's own result. Tracked pointers
are unaffected, since they return earlier in the match loop, so there is no
double-free.
Two smaller changes fell out of this: both async alloc paths now return
CUDA_ERROR_OUT_OF_MEMORYinstead of -1 when over the limit (RMM keys itscache-release-and-retry on that code), and the shared limit counter can no
longer underflow when a pool reports no reserved high-water.
Known limitations
limit counter can under-account a pool allocation (it may register as zero), so
enforcement is approximate for multi-pool workloads. Still better than before,
when pool allocations were not tracked at all; per-pool accounting is the
follow-up.
cuMemAlloc) but freed withcuMemFreeAsyncis now freed correctly, but its synchronous tracking entry is not reclaimed, so
it stays accounted. This is unusual, and a proper fix needs the synchronous
free path, which this PR leaves alone.
device is charged to the wrong limit. Benign in single-GPU containers and when
the caller selects the device first (as RMM does).
Testing
Adds
test/test_alloc_pool_async.c, picked up automatically by the test CMakeglob. It exercises two paths on one stream: a default-pool control
(
cuMemAllocAsync) and the pool-explicit case. On stock CUDA both free cleanly;under libvgpu the pool-explicit free fails before this change and succeeds after.
Built this branch's
libvgpu.soand ran it on an Ampere GPU under a HAMi v2.9.0deployment: the pool-explicit free goes from -1 ("unrecognized error code") to
CUDA_SUCCESS, with the control path unchanged.Summary by CodeRabbit
cuMemFreeAsyncbehavior for both default-pool and explicit pool async allocations.