Skip to content

fix(amdsmi): stop leaking processors when exiting without shutdown - #13007

Merged
adam360x merged 2 commits into
developfrom
users/adam360x/amdsmi-init-exit-leak
Oct 8, 2026
Merged

adam360x merged 2 commits into
developfrom
users/adam360x/amdsmi-init-exit-leak

Conversation

@adam360x

@adam360x adam360x commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Motivation

A process that calls amdsmi_init() and exits without amdsmi_shut_down() leaks every socket and processor object the library created, so under LeakSanitizer its exit fails with code 1 (for example, a tool that only prints --version). Same ownership fix as #12362, one level up.

Technical Details

Library (src/amd_smi/amd_smi_system.cc):

  • AMDSmiSystem::getInstance() allocates the system once and never destroys it, so the GPU, CPU and NIC objects it owns stay reachable until the process ends. The function-local static it replaces was destroyed at exit, which freed only its containers and left those objects unreachable
  • Never destroying it is deliberate, as in fix(amdsmi): stop leaking the per-GPU process-list cache #12362: freeing the objects at exit would pull them out from under any thread still inside the library while static destructors run
  • amdsmi_shut_down() still frees everything; init and shut-down cycles are unchanged
  • Trade-off: a host that unloads the library with dlclose() leaves the system object (about 0.5 KB) unreachable, as it already does fix(amdsmi): stop leaking the per-GPU process-list cache #12362's map

Headers (include/amd_smi/impl/amd_smi_system.h, include/amd_smi/impl/amd_smi_drm.h):

  • Give the NIC context pointer and the DRM vendor ID default member initializers (nullptr, 0). The static object was zero-initialized before its constructor ran; the heap object is not, so both would otherwise start indeterminate

Tests (tests/amd_smi_test/functional/system/exit_without_shutdown_test.cc):

  • Add SystemFunctionalReadOnly.ExitWithoutShutDownIsClean: a threadsafe death test re-runs itself in a fresh process that exits right after amdsmi_init() without amdsmi_shut_down(), and expects exit code 0. In the AMDSMI ASAN GPU job, LeakSanitizer turns a leak into a failure
  • Skips when the amdgpu driver is not loaded

No public header, wrapper, CLI or docs change. No CHANGELOG entry: the leak only shows up at exit.

Issue Tracking

JIRA ID: TBD (ticket to be created)

Test Plan

  • Debug ASAN build (-DADDRESS_SANITIZER=ON -DBUILD_TESTS=ON) with the CI's ASAN_OPTIONS and LSAN_OPTIONS, A/B against the same build with only the library change reverted:
    • 2 Navi GPUs (gfx1030 + gfx1100), non-root, AMDSMI_NON_PRIVILEGED=1: full amdsmitst
    • 8x MI308X, root: *Unit*:GpuFunctionalReadOnly.*:SystemFunctionalReadOnly.*
    • MI455X host with an AMD CPU (HSMP) and an AI NIC, root: the same filter, plus a small program that calls amdsmi_init() with the GPU, CPU, NIC and combined flags and exits without amdsmi_shut_down()
  • gdb on the Navi box (ASAN build): the constructed NIC context pointer and vendor ID, and a retry of amdsmi_init() with GPU and NIC flags after forcing the first call to fail once rocm_smi is initialized
  • Non-ASAN RelWithDebInfo build: SystemFunctionalReadOnly.*, 3 runs
  • pre-commit with projects/amdsmi/.pre-commit-config.yaml

Test Result

  • Without the fix, the new test fails with the LeakSanitizer report (exit status 1): 3,712 bytes in 24 allocations on 8x MI308X, 928 bytes on the 2 Navi GPUs, 464 bytes on MI455X. With the fix it passes on all three
  • Init-and-exit program on MI455X, before → after: GPU 464 B → 0, CPU 7,408 B in 132 allocations → 0, AI NIC 649,320 B → 0, all three 657,192 B → 0
  • Suites: the new test is the only difference; with the fix, 0 sanitizer reports (Navi 243 passed / 26 skipped, MI308X 244 / 3, MI455X 244 / 3)
  • Constructed values with the initializers: null context and vendor ID 0, as on develop (without them, ASAN's fill pattern 0xbebe...). The forced retry crashed in smi_nic_destroy_context() without the initializers and succeeds with them
  • Non-ASAN: 6/6 system tests pass in all 3 runs

Submission Checklist

- Allocate the AMDSmiSystem singleton once and never destroy it. When
  a process exited without amdsmi_shut_down(), destroying the static
  freed only its containers, so every GPU, CPU and NIC object it owned
  was reported as a leak and the exit failed under LeakSanitizer
- Keep it alive rather than freeing those objects at exit, since a
  thread can still be inside the library while static destructors run
- Add a death test that exits a fresh process right after amdsmi_init()
  without amdsmi_shut_down(); under LeakSanitizer a leak fails it

Signed-off-by: Adam360x <Adam.pryor@amd.com>
@adam360x
adam360x requested review from a team and a balanced review from Copilot October 7, 2026 10:01
@therock-pr-bot

therock-pr-bot Bot commented Oct 7, 2026

Copy link
Copy Markdown

✅ All Policy Checks Passed

Check Status Details
📝 PR Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ✅ Pass —
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —

🎉 All policy checks passed!

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

Copilot AI left a comment

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.

Copilot review overview

🟡 Changes recommended

Heap allocation removes implicit zero-initialization, leaving members indeterminate and allowing combined GPU/NIC initialization to crash.

Review effort: Balanced
Findings: 1 High severity

Open (1)
What changed in this PR

Extends #12362’s lifetime approach to AMD-SMI’s system singleton, preventing exit-time leak reports when shutdown is omitted.

Changes:

  • Keeps the singleton alive until process termination while preserving explicit shutdown.
  • Adds a subprocess regression test for initialization followed by exit without shutdown.
File Description
projects/​amdsmi/​tests/​amd_smi_test/​functional/​system/​exit_without_shutdown_test.cc Tests exit without shutdown under LeakSanitizer.
projects/​amdsmi/​src/​amd_smi/​amd_smi_system.cc Replaces the destructed static singleton with permanent heap storage.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

- Give ainic_ctx_ and AMDSmiDrm::vendor_id default member initializers.
  The static system object was zero-initialized before construction;
  the heap-allocated one is not, so both started indeterminate, and
  retrying amdsmi_init() with NIC flags after an init that failed
  partway could pass a garbage context to smi_nic_destroy_context()

Signed-off-by: Adam360x <Adam.pryor@amd.com>
@adam360x
adam360x merged commit f565e90 into develop Oct 8, 2026
129 of 133 checks passed
@adam360x
adam360x deleted the users/adam360x/amdsmi-init-exit-leak branch October 8, 2026 18:50
@systems-assistant

Copy link
Copy Markdown
Contributor

TheRock Submodule Bump Activity

Newest first

anujshuk-amd pushed a commit that referenced this pull request Oct 12, 2026
…13007)

## Motivation

A process that calls `amdsmi_init()` and exits without
`amdsmi_shut_down()` leaks every socket and processor object the library
created, so under LeakSanitizer its exit fails with code 1 (for example,
a tool that only prints `--version`). Same ownership fix as #12362, one
level up.


Signed-off-by: Adam360x <Adam.pryor@amd.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants