fix: forward untracked pointers to the driver on the synchronous free path - #258
Conversation
… path remove_chunk returned -1 without freeing when a pointer was not found in device_overallocated, and on the empty-list early return. free_raw and the cuMemFree_v2 hook pass that value straight back, so freeing a pointer libvgpu did not track (a stream-ordered allocation reaching cuMemFree, or one made before tracking started) leaves the memory allocated on the device and hands the caller -1, which is not a valid CUresult. remove_chunk_async already forwards such frees to the real driver; do the same on the synchronous path. Signed-off-by: Shridhar Panigrahi <198173519+veyron-kairo@users.noreply.github.com>
📝 WalkthroughWalkthrough
ChangesSynchronous free handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 184-187: Update remove_chunk so the device_overallocated
empty-list check and subsequent lookup/removal are performed while holding
mutex. Ensure concurrent inserts cannot cause cuMemoryFree(dptr) to bypass
removal of a matching chunk, and preserve the existing not-found behavior after
the synchronized check.
🪄 Autofix
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: 6bba5273-9445-429d-97b5-1c85cec4049a
📒 Files selected for processing (1)
src/allocator/allocator.c
|
/assign |
|
Looks good overall. Please move the empty-list check under the mutex and remove the verbose comments. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: archlitchi, veyron-kairo The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3 participants
What this does
remove_chunkis the synchronous free path behindcuMemFree_v2->free_raw. It returned-1without freeing when a pointer wasn't found indevice_overallocated, both on the empty-list early return and on the loop not-found return. That-1propagates back throughfree_rawand thecuMemFree_v2hook, so freeing a pointer libvgpu didn't track leaves the memory live on the device and hands the caller a value that isn't a validCUresult.remove_chunk_asyncalready forwards such frees to the real driver; its comment notes the old behavior "leaked and surfaced as an unrecognized error code". This applies the same fix to the synchronous path, using the existingcuMemoryFreehelper that the found path already calls.Fixes #257
How a pointer reaches this path untracked
cuMemAllocAsync) freed withcuMemFreeinstead ofcuMemFreeAsyncVerification
The change reuses
cuMemoryFree(dptr), whichremove_chunkalready calls on its found path, so it adds no new dependency and is build-identical to existing code. I couldn't run a full build locally (it needs the CUDA toolkit); the Build libvgpu CI will confirm compilation.I used AI assistance while reviewing the allocator . I read the code and the async twin's fix myself and take responsibility for the change.
Summary by CodeRabbit