Skip to content

(bugfix) mori bug on RoCM714#414

Merged
QizhouZhang97 merged 6 commits into
mainfrom
bugfix/sglang_714
Jun 23, 2026
Merged

(bugfix) mori bug on RoCM714#414
QizhouZhang97 merged 6 commits into
mainfrom
bugfix/sglang_714

Conversation

@QizhouZhang97

@QizhouZhang97 QizhouZhang97 commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Fix MORI build + multi-process topology crash on ROCm 7.14

Summary

Two fixes that let MORI build and run on ROCm 7.14 (no regression on 7.2).

1. CMake: missing numa::numa target

ROCm 7.14's hsakmt-config.cmake declares that hsakmt::hsakmt links numa::numa, but ships no find_package/finder to define it, so configuration aborts:

The link interface of target "hsakmt::hsakmt" contains: numa::numa but the target was not found.

Fix: define a numa::numa IMPORTED target from the system libnuma before find_package(hsakmt).

2. Runtime: rsmi_init() crash under multi-process (rsmi symbol split-brain)

On the ROCm 7.14 docker, the rsmi_* symbols resolve to two different shared objects in the same process: some calls (e.g. rsmi_init, rsmi_is_P2P_accessible) bind to libamd_smi.so (amdsmi), while the rest (rsmi_num_monitor_devices, rsmi_dev_pci_id_get, …) bind to librocm_smi64. amdsmi sits in the global symbol scope and interposes the subset of the rsmi ABI it re-exports.

The two libraries keep separate global singletons, so rsmi_init() initializes one while rsmi_num_monitor_devices() queries the other (uninitialized) → RSMI_STATUS_INIT_ERROR, and the check's exit(-1) kills every rank. This does not happen on ROCm 7.2, where all symbols resolve to a single library.

Fix: pin the library ourselves. TopoSystemGpu::Load() now dlopens librocm_smi64 with RTLD_NOW | RTLD_LOCAL and resolves every rsmi_* symbol from that one handle, so all calls hit the same instance and amdsmi can no longer interpose. The link-time dependency on librocm_smi64 is removed from CMake, which also means the library is loaded lazily, after torch has initialized HIP — its NEEDED libamdhip64 then resolves to the already-loaded copy by SONAME instead of pulling in a second HIP runtime.

An optional MORI_ROCM_SMI_PATH env var overrides the library path.

Test plan

  • pip install . --no-build-isolation builds on ROCm 7.14.
  • 8-rank test_dispatch_combine_internode.py --cmd stress completes 5000/5000 dispatch + combine with no RSMI_STATUS_INIT_ERROR (no LD_PRELOAD needed).
  • No regression on ROCm 7.2.

@QizhouZhang97 QizhouZhang97 requested a review from jhchouuu June 22, 2026 07:50

@jhchouuu jhchouuu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Overall: Right direction, should land — but please don't remove the P2P topology capability in this PR.

✅ CMake numa::numa fix — LGTM. Defining the imported target from system libnuma to work around ROCm 7.14's missing target is correct.

✅ rsmi → HIP for bus id — correct. This avoids rocm-smi's cross-process mutex contention under /dev/shm/rocm_smi_* (concurrent rsmi_init() races return RSMI_STATUS_INIT_ERROR, and ROCM_SMI_CHECK's exit(-1) kills every rank). HIP's query APIs are read-only and multi-process safe.

⚠️ Don't drop the GPU↔GPU P2P topology. The crash root cause is rsmi's process safety, not P2P collection itself. The rack solution will need this P2P graph (link type/hops/weight) for scale-up routing and cost modeling. Suggest either:

  1. (Preferred) keep it, swap the source — rebuild via hipDeviceCanAccessPeer + hipDeviceGetP2PAttribute; or
  2. (Minimum) keep the TopoNodeGpuP2pLink struct and p2ps fields with a TODO, drop only the rsmi collection.

Removing the capability is a rack-roadmap decision and shouldn't ride along in a bugfix.

@jhchouuu jhchouuu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM need minor modification

Comment thread src/application/CMakeLists.txt Outdated
Comment thread src/application/topology/gpu.cpp Outdated
@QizhouZhang97 QizhouZhang97 merged commit 79e465f into main Jun 23, 2026
11 of 13 checks passed
@QizhouZhang97 QizhouZhang97 deleted the bugfix/sglang_714 branch June 23, 2026 06:16
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.

2 participants