Repository navigation
fix(amdsmi): [ROCM-1553] Bump GPU metrics accumulator counters - #8210
Conversation
0ab2a18 to
1c95396
Compare
❌ 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 |
d4ab083 to
95f0b9c
Compare
|
Ticket for CI failure - ROCm/TheRock#6501 |
95f0b9c to
f177eff
Compare
f177eff to
347f79c
Compare
amd-josnarlo
left a comment
There was a problem hiding this comment.
Do any of the m_gfx_activity_acc members located in rocm_smi_gpu_metrics.cc have to be bumped from uint32_t to uint64_t?
347f79c to
7699ac4
Compare
31354a4 to
49f7f06
Compare
49f7f06 to
e235232
Compare
e235232 to
8bf64e2
Compare
These changes are only for dynamic metrics. |
8bf64e2 to
55b9056
Compare
Changed below five GPU metrics accumulator counter fields from uint32_t to uint64_t to match amdgpu pmfw header: - gfx_activity_acc - mem_activity_acc - pcie_nak_sent_count_acc - pcie_nak_rcvd_count_acc - pcie_lc_perf_other_end_recovery Signed-off-by: Bindhiya Kanangot Balakrishnan <Bindhiya.KanangotBalakrishnan@amd.com>
Signed-off-by: Maisam Arif <Maisam.Arif@amd.com>
Signed-off-by: Maisam Arif <Maisam.Arif@amd.com>
55b9056 to
0c02484
Compare
#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>
…mulators #8210 widened five `amdsmi_gpu_metrics_t` accumulators from 32-bit to 64-bit. The amdgpu metrics table they mirror still reports 32 bits, and the internal `m_*_acc` members correctly stayed `uint32_t`, so copying a table value into the public struct now zero-extends it. The table's "unset" sentinel `0xFFFFFFFF` becomes `0x00000000FFFFFFFF`, which no longer equals the 64-bit maximum that every consumer treats as "not reported": `_validate_if_max_uint(..., MaxUIntegerTypes.UINT64_T)` in `amdsmi_interface.py`, and the `numeric_limits<decltype(...)>::max()` comparisons in `rsmi_utilization_count_get()`. On a GPU that does not report these counters (verified on a W6800): before #8210 after #8210 gfx_activity_acc = N/A gfx_activity_acc = 4294967295 mem_activity_acc = N/A mem_activity_acc = 4294967295 Consumers cannot distinguish that from a real reading, and 4294967295 is not a plausible activity percentage accumulator. Add `widen_keep_sentinel<D>(value)`, which maps an all-ones source to an all-ones destination when the destination is wider, and is a plain cast otherwise. Apply it at the 27 static-table copy sites for the five widened fields and in the dynamic path's `assign_by_type`, whose variant carries the source at its real width and so hits the same defect. Same-width and narrowing conversions are unaffected, so no other field changes behavior. Verified on hardware: `gfx_activity_acc` and `mem_activity_acc` report `N/A` again, and building the same tree without the change reproduces `4294967295`. Signed-off-by: Galantsev, Dmitrii <dmitrii.galantsev@amd.com>
…mulators #8210 widened five `amdsmi_gpu_metrics_t` accumulators from 32-bit to 64-bit. The amdgpu metrics table they mirror still reports 32 bits, and the internal `m_*_acc` members correctly stayed `uint32_t`, so copying a table value into the public struct now zero-extends it. The table's "unset" sentinel `0xFFFFFFFF` becomes `0x00000000FFFFFFFF`, which no longer equals the 64-bit maximum that every consumer treats as "not reported": `_validate_if_max_uint(..., MaxUIntegerTypes.UINT64_T)` in `amdsmi_interface.py`, and the `numeric_limits<decltype(...)>::max()` comparisons in `rsmi_utilization_count_get()`. On a GPU that does not report these counters (verified on a W6800): before #8210 after #8210 gfx_activity_acc = N/A gfx_activity_acc = 4294967295 mem_activity_acc = N/A mem_activity_acc = 4294967295 Consumers cannot distinguish that from a real reading, and 4294967295 is not a plausible activity percentage accumulator. Add `widen_keep_sentinel<D>(value)`, which maps an all-ones source to an all-ones destination when the destination is wider, and is a plain cast otherwise. Apply it at the 27 static-table copy sites for the five widened fields and in the dynamic path's `assign_by_type`, whose variant carries the source at its real width and so hits the same defect. Same-width and narrowing conversions are unaffected, so no other field changes behavior. Verified on hardware: `gfx_activity_acc` and `mem_activity_acc` report `N/A` again, and building the same tree without the change reproduces `4294967295`. Tests: `GpuUnit.MetricsSentinel*` cover the helper. Two more feed an unset 32-bit accumulator through the static v1.4 reader and the dynamic v1.9 reader and check the public field; both fail without the call-site change. Signed-off-by: Galantsev, Dmitrii <dmitrii.galantsev@amd.com>
Motivation
Align GPU metrics accumulator counter types with the driver's 64-bit MetricsTable_t definition in the latest pmfw header to prevent overflow and maintain compatibility.
Technical Details
Changed below five GPU metrics accumulator counter fields from uint32_t to uint64_t to match amdgpu pmfw header:
JIRA ID
JIRA ID : ROCM-1553
Test Plan
amd-smi metric
Test Result
Submission Checklist