Skip to content

test(amdsmi): stop partition tests staging NPS modes - #12472

Open
koushikbillakanti-amd wants to merge 6 commits into
developfrom
users/kbillaka/ROCM-29523
Open

koushikbillakanti-amd wants to merge 6 commits into
developfrom
users/kbillaka/ROCM-29523

Conversation

@koushikbillakanti-amd

@koushikbillakanti-amd koushikbillakanti-amd commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Motivation

On develop, the memory partition tests request NPS modes other than the active one: the C++ test asks for each supported mode from NPS1 to NPS8, and so do the Python setter and benchmark tests and the CLI set sweep. Each request stays staged, and asking for the active mode again doesn't cancel it. The tests pass, and the next amdgpu reload by anyone brings the GPUs up in a different mode. #8575 also removed the reload from the C++ test, so it lost its NPS coverage.

Technical Details

  • Default runs only request the active NPS mode. The C++ test also fails if the memory or compute partition changes during the run.
  • AMDSMI_ALLOW_DESTRUCTIVE_TESTS=1 brings the NPS coverage back to amdsmitst: it applies each supported mode with a driver reload, then restores the starting mode and accelerator profiles. test_memory_partition_cycle.py uses the same flag, and both follow the rules in test-design.md. Before staging anything they check for modprobe, CAP_SYS_MODULE and an idle amdgpu, and any failure that leaves a mode staged names it.
  • The set benchmarks report as ..._same_mode, and the CLI sweep no longer accepts INVAL for --memory-partition.
  • Unit tests run both cycles against a fake driver. The READMEs cover what the flag needs.

Test-only. amd_smi_drm_ex still requests NPS1 to NPS8 in the same CI step; that's a follow-up.

JIRA ID

JIRA ID: ROCM-29523

Test Plan

From projects/amdsmi, built with BUILD_TESTS=ON:

  • amdsmitst --gtest_filter='GpuUnit.MemoryPartition*' and --gtest_filter='*Unit*'
  • python3 -m unittest unit.gpu.test_memory_partition_staging, from tests/python
  • On an idle MI3xx: TestMemoryPartitionReadWrite with and without the flag, comparing amd-smi partition --current before and after

Test Result

  • All 24 C++ memory partition unit tests and the full C++ unit suite pass (308 passed, 25 skipped). So do the 20 Python staging tests and the full Python unit suite (513). Reverting any of the reload checks makes a test fail. Pre-commit hooks pass.
  • Hardware, with the earlier version of the cycle (77d1f98): on MI350 starting from NPS1/SPX and from NPS2/QPX, and on 8x MI300X starting from NPS4/CPX, every supported mode applied and the starting state came back. A single MI300X rejected NPS4 and NPS8 in firmware, and the test failed with "reload applied the wrong mode". A re-run on this head is pending.

Submission Checklist

@therock-pr-bot

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?

@charis-poag-amd charis-poag-amd 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.

Thanks for digging into this. Looking at the history here, I think the underlying defect (ROCM-29523) is already resolved independent of this PR:

amdsmi_gpu_driver_reload() was removed entirely in #8575 (706209f4f54, 2026-07-28), along with this test's calls to it via ReloadDriverWithMessages. Since that commit, TestMemoryPartitionReadWrite can no longer trigger a real driver reload — it only stages a mode and prints an instructional message. The exact failure mode ROCM-29523 describes (~72 in-test reloads scrambling QPX→DPX topology, masked by a PASS) has been structurally impossible on develop since before the ticket was even filed.

Given that, what's left in this PR is test hygiene (dropping the now-dead staging loop, hard-failing on drift instead of skipping, the 19 new DI-backed unit tests) rather than an active defect fix. I'd suggest:

  • Close this PR and ROCM-29523 with a note that the root cause (the reload API itself) was already removed in #8575, so the reported random-partition-drift can no longer occur via this path.
  • If there's still appetite for keeping the cleanup/hardening parts (drift assertions, unit tests), those can land separately as a hygiene PR, decoupled from the "fixes a defect" framing.

If instead we want to keep real cross-reload NPS coverage for QA/validation rather than dropping it, we already have an established pattern for this: test_memory_partition_cycle.py stages every NPS mode, does the actual modprobe -r amdgpu && modprobe amdgpu reload, re-enumerates, verifies, and restores — gated behind AMDSMI_ALLOW_DESTRUCTIVE_TESTS=1 so it's opt-in and never runs by accident in CI. Mirroring that same env-var gate in the C++ functional test (rather than removing the NPS-cycling coverage outright) would give QA a way to still exercise real reload behavior on demand, without the always-on risk that caused ROCM-29523.

Open to it either way — just flagging that as the one path I see for not losing coverage, if there's a reason to keep this open beyond the hygiene changes.

@koushikbillakanti-amd koushikbillakanti-amd changed the title fix(amdsmi): guard partition cleanup fix(amdsmi): stop partition tests staging NPS modes Oct 12, 2026
@github-actions github-actions Bot added documentation Improvements or additions to documentation MINOR ABI BREAKAGE and removed MINOR ABI BREAKAGE labels Oct 12, 2026
@marifamd
marifamd force-pushed the users/kbillaka/ROCM-29523 branch from 4f7c402 to 5d61f99 Compare October 19, 2026 20:36
Signed-off-by: Koushik Billakanti <koushik.billakanti@amd.com>
Signed-off-by: Koushik Billakanti <koushik.billakanti@amd.com>
Signed-off-by: Koushik Billakanti <koushik.billakanti@amd.com>
TestMemoryPartitionReadWrite lost its NPS coverage when the driver reload
API was removed, and only requests the active mode by default. With
AMDSMI_ALLOW_DESTRUCTIVE_TESTS=1 it now sets each other supported mode on
every GPU, reloads amdgpu once per mode, and verifies the mode on each GPU.
It then restores the starting mode and each GPU's accelerator profile,
retrying BUSY and failing when the profile does not come back.

The reload runs modprobe from a fixed path with fixed arguments, without a
shell. The test shuts down the library first so its own descriptors do not
hold the driver, refuses to start when another process holds amdgpu, and
applies a mode left staged by a failed reload before restoring.

Without the flag the test is unchanged: it never reloads and requests only
the active mode.

Signed-off-by: Koushik Billakanti <koushik.billakanti@amd.com>
Signed-off-by: Koushik Billakanti <koushik.billakanti@amd.com>
@koushikbillakanti-amd koushikbillakanti-amd changed the title fix(amdsmi): stop partition tests staging NPS modes test(amdsmi): stop partition tests staging NPS modes Oct 29, 2026
Signed-off-by: Koushik Billakanti <koushik.billakanti@amd.com>

This branch has not been deployed

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

Labels

documentation Improvements or additions to documentation organization: ROCm project: amdsmi

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants