Repository navigation
fix(rccl): correct UALoE fabric detection and diagnostics on the amdsmi path - #9464
Conversation
❌ PR Check — Action Required
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🚫 Please fix the failed policies before requesting reviews. The following policy checks failed:
The |
There was a problem hiding this comment.
Pull request overview
This PR fixes incorrect UALoE/UALLink fabric detection and confusing diagnostics when RCCL is configured to use the amd_smi (AMDSMI) discovery path, aligning behavior with the sysfs/ARSMI path and preventing silent loss of fabric support on supported hardware.
Changes:
- Treat the AMDSMI “unreported” sentinel fabric-info version (
0xFFFFFFFF) as valid (assume v1) instead of rejecting otherwise-correct fabric data. - Replace a noisy/misleading warning about
RCCL_USE_AMD_SMI_LIB/HIP_FABRIC_APIwith anINFOmessage that accurately describes which discovery path is active. - Fix AMDSMI fabric struct ABI mismatch by increasing
local_acceleratorscapacity to 16 and pinning key layout properties withstatic_assert, and improve AMDSMI version logging formatting.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| projects/rccl/src/misc/amdsmi_wrap.cc | Adjusts AMDSMI fabric-info version handling and updates initialization diagnostics/version logging. |
| projects/rccl/src/include/amdsmi_wrap.h | Corrects compat struct sizing to match AMDSMI ABI and adds compile-time layout guards. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
12edb6b to
4e2428f
Compare
The warning claimed fabric support required RCCL_USE_AMD_SMI_LIB=1, which stopped being true once ARSMI_get_fabric_info() added sysfs-based UALoE/UALLink discovery. Since both the env var and HIP_FABRIC_API default to on, it printed on every rank of every job and sent readers chasing a non-existent config problem. Replace it with an INFO noting which discovery path is in use. Co-authored-by: Cursor <cursoragent@cursor.com>
amdsmi_get_gpu_fabric_info() returns SUCCESS with every fabric field correctly populated, but never writes fabric_info.version: fields it cannot source from sysfs keep an all-ones sentinel. The equality check against AMDSMI_FABRIC_INFO_CURRENT_VERSION therefore never passed against libamd_smi.so.26, so RCCL_USE_AMD_SMI_LIB=1 discarded valid UALoE data and marked every device unsupported while the sysfs path worked fine. Treat the sentinel as "unreported" and assume v1, still rejecting a version the library actually claims. Also stop printing amdsmi's full version string as a fourth dotted component, which rendered as "version 26.5.0.26.5.0". Co-authored-by: Cursor <cursoragent@cursor.com>
The compat declaration of amdsmi_fabric_info_v1_t still sized local_accelerators for 8 GPUs while the shipped library uses 16, making our struct 32 bytes short. Since amdsmi_get_gpu_fabric_info() is reached through dlopen, the library writes its own layout: that overflows the caller's object and shifts addr_mode/accel_state out from under us, with no compiler diagnostic either way. Bump the bound to 16 (matching the change in #7844 so the hunks merge) and add static_asserts on the struct sizes and payload offset so future ABI drift fails the build instead of corrupting memory. The pinned values are measured against amdsmi 26.5.0 and were chosen to be stable across the #7844 flatten, which leaves both sizes and the offset of reserved unchanged. Co-authored-by: Cursor <cursoragent@cursor.com>
The version-sentinel fix and the struct layout guard had no regression coverage, and the logic they corrected sat inline in amd_smi_ensureFabricInitialized(), which needs real amd_smi and GPUs to reach. Extract the two decisions into header-inline predicates so they can be exercised directly. Adds rccl-UnitTestsAmdSmi. AmdSmiFabricTests covers version acceptance (including the all-ones sentinel that was disabling fabric), the type/state eligibility matrix, and the field offsets a short local_accelerators[] would shift. Keeping the predicates inline means it needs no librccl symbols, so it runs in Release as well as Debug. test/amdsmi_wrap_test/ was a Makefile project outside the build that reached amdsmi_wrap through hand-written core.h/utils.h stubs, reported results by printing, and was never run by CTest. Its coverage moves into AmdSmiWrapTests as gtest cases that link librccl rather than stub it, so they exercise the code the library ships; each skips instead of failing when the devices, amd_smi library, or fabric it needs is absent. That linkage is also what confines the file to Debug, the only configuration where the internal amd_smi_* symbols are exported. AmdSmiWrapFabricPaths is the case that would have caught the original bug: it asserts amdsmi_get_gpu_fabric_info() and the ualink sysfs nodes agree about device 0, which they did not while the unwritten version field was grounds for rejecting the payload. It runs process-isolated because RCCL_PARAM caches RCCL_USE_AMD_SMI_LIB on first read. Co-authored-by: Cursor <cursoragent@cursor.com>
The CTest registration for rccl-UnitTestsAmdSmi went in next to the FixturesDebug and AltRsmi ones, which sit behind CMAKE_BUILD_TYPE MATCHES "Debug". Those targets are Debug-only, but this one is not: its fabric predicate and layout cases are header-inline and need no librccl symbols, so a Release build compiled and installed them and then never ran them. Move the registration up into the ROCM_VERSION-only block and drop the "debug" label the tiers inherited. Only AmdSmiWrapTests.cpp is Debug-gated, for the exported amd_smi_* symbols it calls, so the yaml's AmdSmiWrap* patterns simply match nothing in a Release binary. Co-authored-by: Cursor <cursoragent@cursor.com>
amd_smi 27.0 flattened amdsmi_fabric_info_t and changed how amdsmi_fabric_telem_id_to_string returns a name. RCCL followed suit, which left it unable to build against, or safely call into, the older amd_smi that current ROCm installs still ship. Read the struct through accessors that select on its shape rather than on a version macro, so a header carrying the change under any version resolves correctly. The rename moved no fields, so the existing ABI asserts cover both layouts and no runtime branch is needed. Keep the deprecated AMDSMI_FABRIC_TYPE_UALLINK spelling, the only one that exists in every supported header. The telemetry call is not so lucky: the two signatures are not call-compatible and we reach the symbol through dlopen, so retain the major version the loaded runtime reports and call it accordingly. An unknown version has to assume the newer convention, since calling a 27+ implementation the old way leaves it writing through an uninitialized pointer. Mock both struct shapes in the tests, which is the only way to compile and exercise the branch the installed header does not select. Co-authored-by: Cursor <cursoragent@cursor.com>
Folding the hardware checks into rccl-UnitTestsAmdSmi does not have to cost the standalone harness, which builds amdsmi_wrap.cc on its own against stub headers and so stays useful outside a full RCCL build. Restore it unchanged and keep both. Note it does not currently build against develop: device.h now reaches for nccl_tuner.h and rccl_ptr.h, which are outside the harness include paths, and its minimal nccl.h stub no longer satisfies nccl_common.h or device.h. That bit-rot predates this branch and reproduces with develop's own amdsmi_wrap sources. Co-authored-by: Cursor <cursoragent@cursor.com>
amd_smi_fabricTelemIdToString dispatches on the loaded runtime's major version between the pre-27 return-value signature and the 27+ out-param signature. Add a Debug case that drives the real dispatch against whichever runtime is loaded and pins the contract its callers rely on: for any id it returns a printable string, never nullptr. A regression in either call-convention path would otherwise only surface as a crash or garbage log line on hardware. Skips when fabric or amd_smi is unavailable, like the rest of the suite. Signed-off-by: Maisam Arif <Maisam.Arif@amd.com>
4f977ac to
00140ae
Compare
topo_expl bypasses RCCL's CMake configuration but imports headers whose compat declarations depend on HIP_FABRIC_API and AMDSMI_FABRIC_DIRECT. Probe the same APIs before compiling so current ROCm headers select their native types instead of redefining them. Co-authored-by: Cursor <cursoragent@cursor.com>
9d127f8 to
e8037ca
Compare
amd_smi_init published its once-only flag before initialization completed, allowing concurrent callers to observe the default success result. Macro returns also bypassed the cached result assignment when initialization failed. Serialize initialization and shutdown, and cache every result before marking the attempt complete. The mutex is process-local and only contended by concurrent communicator initialization, so it adds no cross-rank scaling cost. Track backend initialization separately so shutdown cleans up when the later version query fails and resets the runtime ABI selector. Add isolated tests with a version-matched amd_smi stub to cover concurrent failure caching, partial initialization cleanup, and retry after shutdown. Co-authored-by: Cursor <cursoragent@cursor.com>
37897b0 to
13febdb
Compare
|
rccl/precheckin CI timed-out on one test on gfx908 rccl/extra CI passed. |
Some pre-release ROCm 7.14 snapshots used an 8-GPU fabric payload while final ROCm 7.14 and ROCm 7.15 use 16 GPUs under the same amd_smi major. The absolute 16-GPU ABI guard therefore rejected a supported GIN build configuration. Accept only the coherent size and offset tuple for either layout, preserving a compile-time failure for any other ABI drift. Update the layout tests to cover the selected tuple and its trailing field offsets. Co-authored-by: Cursor <cursoragent@cursor.com>
Probe the loaded fabric payload layout before typed access so mixed header/runtime installations safely use sysfs instead of corrupting memory. Co-authored-by: Cursor <cursoragent@cursor.com>
mustafabar
left a comment
There was a problem hiding this comment.
LGTM - thanks for this, especially for adding the concurrent callers
…mi path (#9464) ## Motivation `RCCL_USE_AMD_SMI_LIB=1` silently disabled UALoE fabric on hardware where it is present and `ready`, while the default sysfs path worked. At the same time, the default path printed a warning telling users to set that very variable to "enable fabric support" — advice that would have made things worse. These two together sent readers (and agents) chasing a configuration problem that did not exist. ## Technical Details **Version check (`src/misc/amdsmi_wrap.cc`)** `amdsmi_get_gpu_fabric_info()` returns `SUCCESS` with a correct payload but never writes `fabric_info.version`, which stays at the sentinel `amdsmi.h` documents for fields it cannot source from sysfs. Strict equality against `AMDSMI_FABRIC_INFO_CURRENT_VERSION` could therefore never pass against `libamd_smi.so.26`, so every device was dropped with a warning. A probe showed `version = 0xFFFFFFFF` alongside `fabric_type = UALOE`, `accel_state = READY` and an `accel_id`/`vpod` matching sysfs; pre-setting version is overwritten, so it is not an input field. The sentinel now means "unreported", and v1 is assumed, while a version the library actually claims is still rejected. **False warning (`src/misc/amdsmi_wrap.cc`)** The "RCCL_USE_AMD_SMI_LIB not set, but HIP_FABRIC_API is defined" warning predates sysfs fabric discovery via `ARSMI_get_fabric_info()`, which populates `amdsmiFabricDevices` in the same way. With the env var defaulting to 0 and `HIP_FABRIC_API` auto-detected on modern ROCm, it printed on every rank of every job, and it conflated `HIP_FABRIC_API` (a `hipMemFabricHandle_t` build flag) with which library does discovery. Replaced by an `INFO` naming the active path. **Struct layout compatibility (`src/include/amdsmi_wrap.h`)** amd-smi 27.0 flattened `amdsmi_fabric_info_t`: the version moved from `fabric_info.version` to a top-level `fabric_version`, and the payload from `fabric_info.fabric_version.v1` to `fabric_info.v1`. Field offsets did not move (version at byte 8, v1 at 12), so this is a source-level rename only. A template-based shape detector (`amdSmiFabricInfoIsFlat`) with `if constexpr` accessor functions handles either layout at compile time. `static_assert` guards pin the struct sizes (244/320 bytes) and `reserved` offset (256) to catch future ABI drift. **Telemetry ABI (`src/misc/amdsmi_wrap.cc`)** `amdsmi_fabric_telem_id_to_string` changed from returning the name to writing it through an out-parameter in 27.0. The two are not call-compatible and we reach it through `dlopen`, so the choice follows the version the loaded runtime reports. An unknown version assumes the out-parameter form: calling a 27+ implementation the old way would leave it writing through an uninitialized pointer, whereas calling an older one the new way merely leaves the name unwritten. **Minor** amd-smi's version string was printed as a fourth dotted component, rendering as `version 26.5.0.26.5.0`. Now it will be printed as `version 26.5.0 (build 26.5.0)`. ## JIRA ID AICOMRCCL-1656 Signed-off-by: Maisam Arif <Maisam.Arif@amd.com> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Maisam Arif <Maisam.Arif@amd.com> Signed-off-by: Maisam Arif <Maisam.Arif@amd.com>
#9864) ## Motivation Bring the unified ABI changes onto the `release/therock-10.0` release branch. These already-merged `develop` commits change the amd_smi ABI (64-bit metrics counters, driver-reload removal, unified struct sync) and must land together so the 10.0 header, wrapper and interface stay consistent with each other — and so 10.0 does not ship an ABI that differs from `develop`. The two earlier attempts are closed: #9852 stacked these changes on the Python packaging series, and #9734 was that packaging series. With #9734 closed, the packaging rework is not coming to 10.0, so this PR applies the ABI changes to the raw release branch. ## Technical Details **Five commits** on the `release/therock-10.0` tip (`6ccde5dbc27`). Four are cherry-picks of merged `develop` commits (`-x` provenance preserved); one is a changelog retarget. | # | Commit | Cherry-picked from | Notes | |---|--------|--------------------|-------| | 1 | `15432e923f` fix(amdsmi): Bump GPU metrics accumulator counters (#8210) | `aeb9da985b` | byte-identical | | 2 | `7ff1686439` refactor(amdsmi): remove driver reload API and related functions (#8575) | `706209f4f5` | byte-identical | | 3 | `23ec5edc37` refactor(amdsmi): Sync with unified with known abi changes (#7844) | `923767a953` | 3 conflicts, resolved below | | 4 | `9030ac095c` docs(amdsmi): set changelog version to 10.0 | — | retargets the changelog heading from `7.15.0` to `10.0` | | 5 | `94cec8fda6` fix(rccl): correct UALoE fabric detection and diagnostics on the amdsmi path (#9464) | `0f674c16e8` | byte-identical — **required**, see below | ### Why the RCCL commit is part of this PR Commit 5 is not incidental: **#7844 does not build without it.** `#7844` flattens `amdsmi_fabric_info_t`, deleting `amdsmi_fabric_info_ver_t` and moving the version to a top-level `fabric_version`. RCCL on the release branch hardcodes the old nested spelling: ```cpp // projects/rccl/src/misc/amdsmi_wrap.cc @ 6ccde5d fabricInfo.fabric_info.version // deleted by #7844 fabricInfo.fabric_info.fabric_version.v1 // deleted by #7844 ``` Both identifiers cease to exist after commit 3, so RCCL fails to compile against the new header. `#9464` replaces those accesses with a `amdSmiFabricInfoIsFlat` SFINAE trait that compiles against **both** the flat and nested layouts, and fixes the UALoE detection bugs described in that PR. It is a large diff (8 files, +1140/−29, including new RCCL tests), so it is called out explicitly here. **These five commits must land as a unit.** Splitting the PR, or reverting the RCCL commit on its own, breaks the RCCL build. ### Conflicts resolved in #7844 Three conflicts, all because the release branch lacks changes `#7844` was written on top of: - **`py-interface/CMakeLists.txt`** — kept the release version. `#7844`'s only hunk here reworks the packaging site-packages install block, which does not exist on this branch (it came from the packaging series in the now-closed #9734). - **`py-interface/amdsmi_interface.py`** — keep-both: added the `AMDSMI_GPU_CUID_SIZE` alias (context on `develop`, which has #9273) alongside the release branch's `_AMDSMI_STRING_LENGTH` constants. - **`CHANGELOG.md`** — dropped the packaging-rework entry, which describes work not on this branch. `#7844`'s ABI entries merged cleanly. ## Issue Tracking JIRA ID: AILITOOLS-270, ROCM-1553, ROCM-26658 JIRA ID: AIDREL-13 JIRA ID: AIDREL-19 ## Test Plan - Cherry-picked #8210 → #8575 → #7844 → #9464 onto the `release/therock-10.0` tip (`6ccde5dbc27`) - Full amd_smi build (library + CLI + `amdsmitst`) - Regenerated `amdsmi_wrapper.py` and diffed against the committed file - Diffed every cherry-pick against its `develop` original - Verified the `amdsmi_fabric_info_t` flattening is ABI-preserving, by compiling both headers and comparing `sizeof`/`offsetof` - Checked every in-repo consumer of the changed/removed types for breakage ## Test Result - **Build:** clean (library + CLI + tests, 0 errors) - **Wrapper:** byte-identical after regeneration (0-line diff) - **Cherry-pick fidelity:** commits 1, 2 and 5 diff byte-identically against their `develop` originals (modulo hunk offsets); commit 3 differs only in the three conflicts documented above - **Fabric ABI is preserved:** `sizeof(amdsmi_fabric_info_t) = 320` before and after, with every offset unchanged — `bdf=0`, version `=8`, `v1=12`, `v1.accel_state=252`, `reserved=256` - **Removed symbols:** `amdsmi_gpu_driver_reload` has no remaining references outside the CHANGELOG; none of the 16 removed/renamed symbols has a stale consumer anywhere under `projects/` - **In-repo consumers:** - `projects/rdc` reads `amdsmi_gpu_metrics_t` and the widened accumulators and is *not* modified here — a `-fsyntax-only` A/B of `RdcMetricFetcherImpl.cc` against the old and new headers produces the identical error set (pre-existing, ESMI-gated), so nothing regresses. The fields only widen, and they feed a `uint64_t`-valued map. - `projects/rocshmem` keeps a private dlopen mirror of `amdsmi_fabric_info_t`; it does not include `amd_smi/amdsmi.h`, and the layout is unchanged, so it is unaffected. - `projects/rocprofiler-systems` uses `amdsmi_gpu_metrics_t` and builds green in this PR's TheRock CI. - `projects/rocm-smi-lib` has its own independent `rocm_smi.h`; unaffected. ## Known issues inherited from `develop` Faithful cherry-picks, so these come along unchanged; they are present on `develop` today and are not introduced by this backport. Listed so the release branch owners know what is shipping. Fixes belong on `develop` first. - **`gpu_metrics` accumulators lose their "unset" sentinel** (from #8210). The counters widened to 64-bit but the amdgpu table still reports 32, so the `0xFFFFFFFF` sentinel zero-extends and stops matching the 64-bit sentinel consumers check. `amdsmi_get_gpu_metrics_info()` reports `gfx_activity_acc`/`mem_activity_acc` as a literal `4294967295` instead of `N/A` on hardware that does not report them (reproduced on gfx1030). Unchanged on parts that do report real values. - **`tests/python/cli/test_set.py` fails instead of skipping** (from #7844). `cmds_need_permission` is assigned in `setUpClass` after the `_initialized` early-return, so `TestSet` raises `AttributeError`; and the filter that replaced the old blanket skip covers only 3 of the privileged `set` subcommands, so an unprivileged run also fails on `--perf-level`, `--profile`, `--perf-determinism`, `--clk-level` and others. - **`amd-smi set --json` and `amd-smi static --partition --json` disagree on one key** (from #7844). `set` now emits `accelerator_partition_mem_alloc_mode` while `static` and the `--compute-partition-mem-alloc-mode` flag keep `compute_partition_mem_alloc_mode`. - **Dead constants**: the keep-both resolution leaves `_AMDSMI_STRING_LENGTH` and `_AMDSMI_MAX_STRING_LENGTH` with zero references (every call site moved to the public `AMDSMI_MAX_STRING_LENGTH`). ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md. --------- Signed-off-by: Bindhiya Kanangot Balakrishnan <Bindhiya.KanangotBalakrishnan@amd.com> Signed-off-by: Maisam Arif <Maisam.Arif@amd.com> Signed-off-by: gabrpham <Gabriel.Pham@amd.com> Signed-off-by: josnarlo <joseph.narlo@amd.com> Signed-off-by: Galantsev, Dmitrii <dmitrii.galantsev@amd.com> Co-authored-by: gabrpham <Gabriel.Pham@amd.com> Co-authored-by: Joseph Narlo <joseph.narlo@amd.com> Co-authored-by: Galantsev, Dmitrii <dmitrii.galantsev@amd.com> Co-authored-by: Mika Laitio <mika.laitio@amd.com> Co-authored-by: Nilesh M Negi <Nilesh.Negi@amd.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Motivation
RCCL_USE_AMD_SMI_LIB=1silently disabled UALoE fabric on hardware where it is present andready, while the default sysfs path worked. At the same time, the default path printed a warning telling users to set that very variable to "enable fabric support" — advice that would have made things worse.These two together sent readers (and agents) chasing a configuration problem that did not exist.
Technical Details
Version check (
src/misc/amdsmi_wrap.cc)amdsmi_get_gpu_fabric_info()returnsSUCCESSwith a correct payload but never writesfabric_info.version, which stays at the sentinelamdsmi.hdocuments for fields it cannot source from sysfs. Strict equality againstAMDSMI_FABRIC_INFO_CURRENT_VERSIONcould therefore never pass againstlibamd_smi.so.26, so every device was dropped with a warning. A probe showedversion = 0xFFFFFFFFalongsidefabric_type = UALOE,accel_state = READYand anaccel_id/vpodmatching sysfs; pre-setting version is overwritten, so it is not an input field. The sentinel now means "unreported", and v1 is assumed, while a version the library actually claims is still rejected.False warning (
src/misc/amdsmi_wrap.cc)The "RCCL_USE_AMD_SMI_LIB not set, but HIP_FABRIC_API is defined" warning predates sysfs fabric discovery via
ARSMI_get_fabric_info(), which populatesamdsmiFabricDevicesin the same way. With the env var defaulting to 0 andHIP_FABRIC_APIauto-detected on modern ROCm, it printed on every rank of every job, and it conflatedHIP_FABRIC_API(ahipMemFabricHandle_tbuild flag) with which library does discovery. Replaced by anINFOnaming the active path.Struct layout compatibility (
src/include/amdsmi_wrap.h)amd-smi 27.0 flattened
amdsmi_fabric_info_t: the version moved fromfabric_info.versionto a top-levelfabric_version, and the payload fromfabric_info.fabric_version.v1tofabric_info.v1. Field offsets did not move (version at byte 8, v1 at 12), so this is a source-level rename only. A template-based shape detector (amdSmiFabricInfoIsFlat) withif constexpraccessor functions handles either layout at compile time.static_assertguards pin the struct sizes (244/320 bytes) andreservedoffset (256) to catch future ABI drift.Telemetry ABI (
src/misc/amdsmi_wrap.cc)amdsmi_fabric_telem_id_to_stringchanged from returning the name to writing it through an out-parameter in 27.0. The two are not call-compatible and we reach it throughdlopen, so the choice follows the version the loaded runtime reports. An unknown version assumes the out-parameter form: calling a 27+ implementation the old way would leave it writing through an uninitialized pointer, whereas calling an older one the new way merely leaves the name unwritten.Minor
amd-smi's version string was printed as a fourth dotted component, rendering as
version 26.5.0.26.5.0. Now it will be printed asversion 26.5.0 (build 26.5.0).JIRA ID
AICOMRCCL-1656
Test Plan
alltoall_perfwithNCCL_DEBUG=INFOon a UALoE node, withRCCL_USE_AMD_SMI_LIB=1and=0: expectUALoE fabric detectedfor all ranks on both paths, agreeing onaccelId/vpod, and no fabric warnings.amdsmi_wrap.hcompiles withAMDSMI_FABRIC_DIRECTboth set and unset.Test Result
libamd_smi.so.26.5.0establishing the sentinel behavior and the correct payload.AMDSMI_FABRIC_DIRECTmodes; the new guard confirmed to fire whenAMDSMI_FABRIC_MAX_LOCAL_GPUSis reverted to 8.RCCL_USE_AMD_SMI_LIB=0and1.Submission Checklist