Skip to content

fix(cuda): handle null meminfo outputs and free untracked pointers - #322

Open
liuxu623 wants to merge 1 commit into
Project-HAMi:mainfrom
liuxu623:codex/fix-cuda-memory-compat
Open

liuxu623 wants to merge 1 commit into
Project-HAMi:mainfrom
liuxu623:codex/fix-cuda-memory-compat

Conversation

@liuxu623

@liuxu623 liuxu623 commented Sep 11, 2026

Copy link
Copy Markdown

OptiX can call cuMemGetInfo_v2 with an omitted output, which currently causes the hook to dereference NULL. Separately, CUDA pointers returned by external-memory mapping are not in the allocator's tracked list, so a valid cuMemFree_v2 call returns -1 instead of reaching the driver.

This change queries memory information into local variables, propagates driver errors, and writes only requested outputs while preserving virtual memory reporting and clamping subtraction at zero. Synchronous frees of untracked pointers now reach the real driver and return its result without changing allocation accounting. The existing tracked-pointer path is unchanged.

Fixes #321.

Validation

  • Added a GPU-free regression test linked to the production memory wrappers and allocator, registered with CTest. Direct GCC builds passed with debug logging enabled; the new test also passes the repository's cpplint configuration.
  • Replacing the fixed memory-query implementation with the original produces SIGSEGV. Replacing the fixed allocator implementation with the original produces eight failed assertions.
  • Tested on real NVIDIA GPU hardware with driver 580.126.09 (CUDA Driver API 13000), using the same application image and GPU for all three paths. The device model is intentionally omitted.
Path Valid outputs Either/both outputs NULL Minimal OptiX/Vulkan-CUDA rendering
Previously deployed library Pass SIGSEGV SIGSEGV
Library built from this change Pass Pass Pass, exit 0
Native NVIDIA driver Pass Pass Pass, exit 0

The patched render completed all seven logged synchronous frees successfully. Library provenance was checked with dladdr; no diagnostic workaround shim was loaded. The full production library was compiled directly with GCC inside the target container using the hook/multiprocess options from build.sh; a complete CMake build was not run.

Scope

The hardware comparison used a dedicated GPU with per-process library loading and a configured virtual memory quota to avoid loading both library versions together. Virtual memory reporting was verified, but shared scheduling, concurrent isolation, and full application end-to-end behavior were not validated.

This fixes compatibility and release behavior; it does not add quota enforcement or allocation tracking for Vulkan-owned external memory. Reporting a virtual quota does not demonstrate that external allocations are constrained by it. Existing error/accounting behavior for tracked frees is outside this change.

AI assistance was used for implementation, tests, and this description.

Summary by CodeRabbit

  • Bug Fixes

    • Improved GPU memory reporting when requesting only selected values.
    • Corrected memory totals when usage reaches or exceeds configured limits.
    • Ensured externally allocated or untracked memory is properly released.
    • Improved handling of invalid contexts and driver-reported errors during memory operations.
  • Tests

    • Added regression coverage for GPU memory queries and release behavior, including partial outputs, error handling, accounting updates, and lock safety.

@hami-robot

hami-robot Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: liuxu623
Once this PR has been reviewed and has the lgtm label, please assign archlitchi for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@coderabbitai

coderabbitai Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 47e55366-f3d6-4309-986f-95550f94dd1f

📥 Commits

Reviewing files that changed from the base of the PR and between aa322f4 and 545efc8.

📒 Files selected for processing (1)
  • test/CMakeLists.txt

Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The CUDA memory wrappers now handle NULL output pointers and configured memory limits. Untracked pointers now reach the real driver free path. A GPU-free compatibility test covers query, free, accounting, locking, and error behavior.

Changes

CUDA compatibility fixes

Layer / File(s) Summary
Untracked pointer release
src/allocator/allocator.c
remove_chunk no longer exits early for an empty list. Missing pointers now use cuMemoryFree(dptr) instead of returning -1.
NULL-safe memory information
src/cuda/memory.c
cuMemGetInfo_v2 queries into local variables, applies configured limits and tracked usage, clamps free memory, and writes only to non-NULL outputs.
Compatibility regression coverage
test/test_cuda_memory_compat.c, test/CMakeLists.txt
The new GPU-free test stubs driver and accounting dependencies, checks query and free behavior, and registers a CTest target with the required build and link settings.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 545ef

The added compatibility test is built, linked, and registered consistently with its intended CUDA regression coverage.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The implementation addresses the two runtime defects in #321. cuMemGetInfo_v2 uses local driver outputs, supports NULL output masks, preserves the virtual-memory limit calculation, clamps free memor… Add and register regression coverage for the cuImportExternalMemorycuExternalMemoryGetMappedBuffercuMemFree_v2 → external-memory destruction lifecycle. Verify that a valid untracked mapped pointer reaches the driver, that the dr…
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 3 files. (1 skipped: 1… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes both primary changes: handling NULL memory-information outputs and freeing untracked pointers.
Out of Scope Changes check ✅ Passed The allocator change, cuMemGetInfo_v2 change, regression test, and CTest registration support the defects and coding requirements in #321. The reviewed changes do not add unrelated quota enforcement…
Full details: Linked Issues check

Explanation

The implementation addresses the two runtime defects in #321. cuMemGetInfo_v2 uses local driver outputs, supports NULL output masks, preserves the virtual-memory limit calculation, clamps free memory, and propagates driver errors. remove_chunk forwards untracked pointers to cuMemoryFree without accounting changes. The GPU-free test covers NULL queries, driver errors, tracked frees, untracked frees, and driver-call locking. The test does not exercise the required external-memory import, cuExternalMemoryGetMappedBuffer, mapped-buffer free, and external-memory destruction lifecycle from #321.

Resolution

Add and register regression coverage for the cuImportExternalMemorycuExternalMemoryGetMappedBuffercuMemFree_v2 → external-memory destruction lifecycle. Verify that a valid untracked mapped pointer reaches the driver, that the driver result propagates, and that tracked allocation accounting remains unchanged.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 3 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

A rabbit frees a buffer bright
NULL queries now return right
Locks release before the call
Tracked bytes shrink, leaks fall
Stubs test each path with care
CUDA winds move through the air

Comment @coderabbitai help to get the list of available commands.

@liuxu623
liuxu623 force-pushed the codex/fix-cuda-memory-compat branch from ed86a59 to aa322f4 Compare September 11, 2026 06:24
@liuxu623
liuxu623 force-pushed the codex/fix-cuda-memory-compat branch from aa322f4 to 545efc8 Compare September 11, 2026 06:31
Comment thread src/allocator/allocator.c
return -1;
/* External-memory mappings and other untracked pointers still belong
* to the driver. Do not adjust accounting for memory we did not track. */
return cuMemoryFree(dptr);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#258 has been forwarding untracked pointers here since august. does this part need to land twice?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#258 is still open and main still returns -1 here, so nothing has landed twice. This hunk is required for #321: OptiX external-memory mappings never go through add_chunk, so cuMemFree_v2 on them fails without it. Happy to rebase and drop it if #258 merges first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] OptiX/Vulkan-CUDA interop: NULL cuMemGetInfo output crashes and external mapped buffers fail cuMemFree

2 participants