Repository navigation
fix(rdc): evaluate health checks on MI300-series+ and stop the fetch error flood - #13570
Open
dmitrii-galantsev wants to merge 22 commits into
Open
dmitrii-galantsev wants to merge 22 commits into
dmitrii-galantsev wants to merge 22 commits into
Conversation
rdc_health_set() and rdc_health_check() both broke out of their fetch-and-cache loops on the first field that failed, so every field after it was never primed or refreshed. memory_check() returned as soon as the pending-page fetch failed, so ECC/retired-page evaluation never ran, and the other component checks propagated fetch errors upward. Continue past a failing field in both loops. In memory_check, evaluate each sub-check independently: a field that cannot be read on a GPU skips only its own incident. Component checks return RDC_ST_OK (skip) when their field is unavailable, matching what rdc_health_check already did with non-MAX_LIMIT returns. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
RDC_HEALTH_XGMI_ERROR reads the legacy xgmi_error sysfs node. On gfx950 (MI355) the node exists but reading it returns EINVAL, so the XGMI health component could never be evaluated there. The same GPUs report XGMI faults as RAS errors on the XGMI_WAFL block, which RDC already fetches successfully as RDC_FI_ECC_XGMI_WAFL_UE. Watch RDC_FI_ECC_XGMI_WAFL_UE alongside RDC_HEALTH_XGMI_ERROR for the XGMI component. xgmi_check tries the legacy status first so existing platforms are unchanged; when it is unavailable, an uncorrectable WAFL count of 1 maps to RDC_FR_XGMI_SINGLE_ERROR and >1 to RDC_FR_XGMI_MULTIPLE_ERROR. If neither source is readable the component is skipped for that GPU. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
rdc_health_set() watched every field for the requested components and let the 1 s updater discover at runtime that some of them cannot be read on this platform, logging an ERROR per field per GPU every second for the life of the daemon. The telemetry module still advertises all health fields, so the existing skip in rdc_field_watch() does not catch this. Probe each (gpu, field) once whenever the health watch is set. A field whose status is a capability miss (not supported, invalid, not found, or no permission) is reported at default verbosity and left out of both the health watch table and the 1 s field group. Where a fallback source exists (kHealthFieldFallbacks, currently XGMI_ERROR -> ECC_XGMI_WAFL_UE) and probes OK, the line says so: Health field XGMI_ERROR is not supported on GPU 3 (status 5); using ECC_XGMI_WAFL_UE for this health component instead. Other failures are treated as transient and stay watched. The health check functions consult the surviving (gpu, field) set through get_start_end_values() and skip silently rather than fetching an unsupported field again at -c time. rdc_health_set is only reached from an explicit client call, so the report appears on each `rdci health -s` and never on -c/-f or the 1 s updater. fetch_smi_field() collapses every SMI failure to RDC_ST_SMI_ERROR and leaves the specific status in value.status; the probe reads that. Note that status is an rdc_status_t: BAD_PARAMETER here is AMDSMI_STATUS_INVAL, which is what reading the xgmi_error sysfs node returns on gfx950 (EINVAL -> RSMI_STATUS_INVALID_ARGS -> AMDSMI_STATUS_INVAL). Field groups are per group, so a field remains in the 1 s watch if any GPU in the group supports it; -c still skips it per GPU. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
eeprom_check() reads RDC_FI_ECC_UNCORRECT_TOTAL and looks for RDC_ST_CORRUPTED_EEPROM; nothing reads RDC_HEALTH_EEPROM_CONFIG_VALID. Yet the EEPROM component watched (and polled every second) only the latter, and with -e alone the field the check does read was not in the watch set at all. Watch RDC_FI_ECC_UNCORRECT_TOTAL for the EEPROM component (skipped when MEM already adds it) and stop polling the unconsumed field. No change to what eeprom_check reports. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Once rdc_health_set drops fields the platform cannot serve, nothing should reach fetch_smi_field for them at 1 s. If one does (a field that is supported on some GPUs in a group but not others, or a future SKU quirk), an ERROR every second is a flood rather than a signal: the watch-set report already told the operator, at default verbosity, that the field is unsupported. For RDC_HEALTH_* fields whose status is a capability miss, log the fetch result at INFO and skip the "Fetch status is not ok" line. The latter prints fetch_gpu_field_'s return, which re-translates an rdc_status_t through Smi2RdcError and so shows meaningless values (RDC_ST_BAD_PARAMETER becomes 4294967295); fixing that return is left for a status-type cleanup. Non-health fields and real failures (I/O, permission, unknown) are unchanged and stay at ERROR. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Every fetch path stores Smi2RdcError(ret) in value->status, and callers (including the health probe in rdc_health_set) read it as an rdc_status_t. get_ecc() stored the raw amdsmi_status_t instead, so a per-block ECC field that is not supported on a device was reported as AMDSMI_STATUS_NOT_SUPPORTED (2), which reads as RDC_ST_SMI_ERROR and is not recognised as a capability miss. That matters for the XGMI health fallback, RDC_FI_ECC_XGMI_WAFL_UE, which goes through get_ecc. Store Smi2RdcError(err) like the other paths. Successful fetches are unchanged (both enums are 0). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
health_component_fields() appends the fallback for any candidate that has one, so RDC_FI_ECC_XGMI_WAFL_UE was polled every second on every platform, including those where the legacy xgmi_error status works and xgmi_check never reads the fallback. Drop a fallback from the probe result on GPUs where its primary probed OK; it is then neither cached nor watched, and a platform that lacks the fallback's RAS block is not affected unless it also lacks the primary. Also keep the pre-probe behaviour for an empty GPU group: there is nothing to probe, so watch every candidate rather than failing with "No supported health fields". Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The legacy xgmi_error sysfs read fails with EINVAL on every DF 4.x part (MI300X/MI325X gfx942 as well as MI355 gfx950), because amdgpu's amdgpu_xgmi_show_error needs DF FICA hooks that DF 4.x does not provide. Earlier comments and the CHANGELOG entry said "gfx950"; the XGMI fallback observed firing on MI325X shows the wider scope. Comments only. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
rdc_health_set() runs its probe without watch_mutex_ (it makes SMI calls, ~1.6 s on an 8-GPU node), so two concurrent SetHealth requests for the same group could both pass rdc_health_clear, and the second to reach health_watch_table_.insert() would have its entry dropped while its field group and 1 s watch stayed registered. Repeated collisions leak field groups. The race predates the probe but was microseconds wide before it. Add health_api_mutex_, held across the whole of rdc_health_set and rdc_health_clear (rdc_health_set now calls an unlocked clear so it does not re-take it). Use insert_or_assign so a stale entry can never mask a new one. If rdc_field_watch fails, remove the table entry, destroy the field group and drop the primed cache, mirroring the existing rollback on rdc_group_field_create failure. Also probe a fallback field only where its primary is unavailable, so platforms with a working primary neither fetch nor cache it; look up probe results with find() rather than operator[]; and never push a fallback into the candidate list twice. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Three loose ends in how a field's status is recorded and read: - fetch_gpu_field_/fetch_smi_field initialised value->status to AMDSMI_STATUS_NOT_SUPPORTED (2) and several cases assigned it explicitly. Callers read that slot as an rdc_status_t, where 2 is RDC_ST_SMI_ERROR, so a field whose handler never set a status was classified as a transient error rather than unsupported -- the same class of bug as the PENDING_PAGE_NUM fallthrough. Store RDC_ST_NOT_SUPPORTED, which is what was meant. Every comparison is against 0, so success paths are unchanged. - get_ecc() set an error for an invalid block and then carried on into the SMI calls, which could overwrite it with success. Return. - is_health_field() only covered the RDC_HEALTH_* id range, so the XGMI fallback RDC_FI_ECC_XGMI_WAFL_UE was not treated as a health field by fetch_smi_field's log-level rule: in a group where some GPUs dropped it and others kept it, the 1 s fetch on the former would still log at ERROR. Move the fallback table to SmiUtils (health_field_fallbacks()) so both the watch table and the fetcher use one definition, and make is_health_field() include fallback fields. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
get_start_end_values() consulted the health watch table through is_health_field_watched(), which took watch_mutex_ on every read -- roughly 64 acquisitions per `rdci health -c` on an 8-GPU node. The 1 s updater holds that mutex across its whole SMI batch, so each read could stall behind it. rdc_health_check() already copies the group's watched (gpu, field) pairs under the lock before running the checks; build a std::set from that copy and pass it down instead. This also removes a method whose name was one word away from the pre-existing lock-free is_health_watch_field(), which handle_fields() calls while holding watch_mutex_. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…mantics eeprom_check() compared get_start_end_values()'s return against RDC_ST_CORRUPTED_EEPROM, but that function only ever returns OK, NOT_FOUND, or fetch_smi_field's collapsed RDC_ST_SMI_ERROR; the specific status is in end.status. The incident branch was therefore unreachable. Read end.status. AMDSMI's EEPROM validator is a hardcoded NOT_SUPPORTED today, so the branch still cannot fire in practice; the comment says so. The EEPROM component's watched field is load-bearing for the API contract of `-s e` alone, not for this check. Widen retired_page_threshold to uint64_t to match the int64 it is read from. Note in xgmi_check that the WAFL fallback counter is cumulative and does not clear on read. Document in rdc.h that rdc_health_set probes and skips unsupported fields and may return RDC_ST_NOT_SUPPORTED. Tighten the CHANGELOG wording on mixed groups, `-s e` cost and the XGMI fallback semantics. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
rdctst had no coverage of rdc_health_set/get/check/clear. Add a test in the existing functional pattern (runs in embedded and standalone mode) that: - sets every health component on a one-GPU group, reads the setting back, runs a check and clears it; - asserts RDC_HEALTH_PENDING_PAGE_NUM is readable exactly where RDC_HEALTH_RETIRED_PAGE_NUM is (both come from one SMI call), and that a GPU with no bad pages reports zero pending pages -- the inequality the zero-bad-pages fix removed; - asserts that watching the EEPROM component alone makes RDC_FI_ECC_UNCORRECT_TOTAL, the field eeprom_check reads, available; - asserts rdc_health_set succeeds on a group with no GPUs. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
get_ecc_deferred() arrived on develop with the pattern get_ecc() had before: it stored the raw amdsmi_status_t in value->status, which every consumer reads as an rdc_status_t. Store Smi2RdcError(err) and RDC_ST_OK so RDC_FI_ECC_*_DE fields classify like the other ECC fields. Success values are unchanged (both enums are 0). The CPER/AFID fetch added upstream has the same pattern; it is not a health field and is left for the wider status-type cleanup. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Rename tests/rdc_tests/functional/rdci_health.{h,cc} to test_health.{h,cc},
matching the test_bad_page.cc precedent and the test_<name> pattern the
Systems PR bot looks for. Class and registration are unchanged; rdctst
still runs it in embedded and standalone mode.
Add tests/unit/test_health_fields.cc to rdc_unit_tests: hardware-free
checks of is_capability_miss(), is_health_field() and
health_field_fallbacks(), the classification rules rdc_health_set's probe
and fetch_smi_field's log-level rule depend on.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The all-components watch caches RDC_FI_ECC_UNCORRECT_TOTAL, so the EEPROM-only assertion passed on the merge base too. Running it first on the fresh group makes it fail without the EEPROM field change. Signed-off-by: Galantsev, Dmitrii <dmitrii.galantsev@amd.com>
…fetcher The probe, the fallback and the independent memory sub-checks had no test; on the merge base four of these six cases fail. A scripted RdcMetricFetcher drives RdcWatchTableImpl without MI300 hardware. Signed-off-by: Galantsev, Dmitrii <dmitrii.galantsev@amd.com>
… own When neither XGMI source exists, the probe logged a separate 'skipping' line for ECC_XGMI_WAFL_UE, which the user never asked for, and get_ecc logged its NOT_SUPPORTED count read at ERROR. Signed-off-by: Galantsev, Dmitrii <dmitrii.galantsev@amd.com>
The clang-format pre-commit hook (v22.1.0) rewrites these hunks; only lines this PR touches are changed. Signed-off-by: Galantsev, Dmitrii <dmitrii.galantsev@amd.com>
rdc_status_tests (#13413) compiles RdcWatchTableImpl.cc without librdc. The health watch now calls is_capability_miss() and health_field_fallbacks() from SmiUtils.cc, so after rebasing onto develop the target failed to link with undefined references to both. Compile SmiUtils.cc into the test and link amd_smi for it; the test still never initializes AMD SMI. Built on an MI350 host: all 4 rdc_status_tests pass.
This was referenced Oct 15, 2026
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Missing cache recovery and discarded EEPROM corruption statuses can leave health faults undetected.
2 open findings
What changed in this PR
Updates RDC health monitoring for MI300-series and later GPUs, addressing unsupported-field polling and skipped health checks.
Changes:
- Adds capability probing, XGMI fallback, and independent memory sub-checks.
- Adjusts status handling, logging, and watch lifecycle synchronization.
- Adds health regression tests and documents the behavior.
| File | Description |
|---|---|
| projects/rdc/tests/unit/test_health_watch.cc | Tests probing, fallback, and memory checks. |
| projects/rdc/tests/unit/test_health_fields.cc | Tests health-field classification. |
| projects/rdc/tests/unit/CMakeLists.txt | Registers tests and required dependencies. |
| projects/rdc/tests/rdc_tests/main.cc | Registers functional health testing. |
| projects/rdc/tests/rdc_tests/functional/test_health.h | Declares the health test fixture. |
| projects/rdc/tests/rdc_tests/functional/test_health.cc | Exercises health lifecycle and field availability. |
| projects/rdc/tests/rdc_tests/CMakeLists.txt | Builds the functional health test. |
| projects/rdc/rdc_libs/rdc/src/SmiUtils.cc | Implements capability and fallback helpers. |
| projects/rdc/rdc_libs/rdc/src/RdcWatchTableImpl.cc | Updates probing, checks, and watch lifecycle. |
| projects/rdc/rdc_libs/rdc/src/RdcMetricFetcherImpl.cc | Normalizes statuses and adjusts logging. |
| projects/rdc/include/rdc/rdc.h | Documents health probing and return status. |
| projects/rdc/include/rdc_lib/impl/SmiUtils.h | Declares classification helpers. |
| projects/rdc/include/rdc_lib/impl/RdcWatchTableImpl.h | Adds watch snapshots and synchronization. |
| projects/rdc/CHANGELOG.md | Records health-monitoring changes. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
On MI300-series and later the legacy xgmi_error node is unreadable, so every health set with the XGMI component announced the switch to ECC_XGMI_WAFL_UE at ERROR, once per GPU: eight ERROR lines on a healthy 8-GPU MI350 for each `rdci health -s x` or `-s a`. The fallback working is the expected path there, and fetch_smi_field already logs capability misses at INFO. A field with no working source still logs at ERROR. The fake-fetcher tests now capture the log: the fallback case fails with the old ERROR level and passes with INFO; the no-source case still expects an ERROR line. rdc_unit_tests 18/18, rdc_status_tests 4/4. Signed-off-by: Galantsev, Dmitrii <dmitrii.galantsev@amd.com>
Contributor
Author
|
🤖 claude: Pushed 9fef2f3. A fallback that works (
|
rdc_health_set keeps a field watched when its probe fails with a non-capability error, but it only seeds the health cache for fields that read OK. Later refreshes go through rdc_update_health_stats, which returns RDC_ST_NOT_FOUND for a (gpu, field) with no history, so the field never got any. The PCIe, thermal, power and weekly retired-page checks need a start value from that history and stayed silent until the watch was set again. Both refresh paths (the foreground refresh in rdc_health_check and the 1 s handle_fields updater) now start the history from the first successful read when the update reports no entry, after checking under watch_mutex_ that the pair is still in the group's health watch. The failed probe is never cached. Two fake-fetcher tests set PCIe with a failing probe, let it recover through one path, then raise the replay count past the limit. Each fails without the change to its path and passes with it.
This was referenced Oct 16, 2026
adam360x
added a commit
that referenced
this pull request
Oct 16, 2026
## Motivation `rdci dmon -e 9` (`RDC_FI_GPU_PARTITION_COUNT`) reports 65535 on an AMD Instinct MI325X in SPX mode, where it should be 1. RDC copied the partition count from the GPU metrics, which have that field only in versions 1.6 to 1.8. Drivers that publish GPU metrics v1.9 (current MI300 and MI350 drivers, MI455X) never fill it, and v1.3 (MI200, Radeon) has no such field. AMD SMI then reports 65535, its "not available" value. An earlier partition fix did not change this field. ## Technical Details - Field 9 now comes from `get_num_partition()`, the call `rdci discovery` already uses. It reads AMD SMI's current partition profile (SPX 1, DPX 2, TPX 3, QPX 4, CPX = number of XCDs) and works without root. - **Behavior change:** a GPU that cannot be partitioned reports 1 (was 65535). A failed profile query makes the field N/A. - The profile query takes ~0.28 ms per sample, against ~0.14 ms for the old GPU metrics read (MI455X). - `SmiUtils`: removed `get_metrics_info()`, which had no other caller. - Tests: new GPU-less `rdc_partition_count_tests` (6 tests) builds the metric fetcher against fake AMD SMI replies; `tests/ci/test_gtest_runner.py` runs it in the no-GPU CI job. - The tests share one metric fetcher and never destroy it (it keeps no state for the field they fetch): `~RdcMetricFetcherImpl()` can miss waking its worker thread and then wait for it forever, which hung this PR's first CI run. That race predates this PR and can also hang rdcd when it stops; it is not fixed here. - CHANGELOG entry under Resolved Issues. **Not changed here** - Partition instances (`gN.P`) still report N/A for this field, as before; each partition's own GPU index reports the count. - Sibling fix for `RDC_FI_POWER_USAGE`: #13637 (it also updates the CI runner's docstring). ## Issue Tracking JIRA ID: ROCM-33007 ## Test Plan - `rdc_partition_count_tests` against develop's fetcher and the fix; CI pytest wrapper against a private install. - `rdcd -u` + `rdci dmon -u -e 9` in a private network/mount namespace: 1x MI455X (ROCm 10.2.0 nightly; stock, develop, fix; non-root), 4x MI300A (gfx942) in SPX/TPX/CPX, 2x Radeon (RX 6800 XT, RX 7900 XT). - Trial merges with #13386, #13570, #13637 and current develop. ## Test Result - Tests: all 6 fail when built against develop's fetcher and `SmiUtils`; all 6 pass with the fix. - Hang check: with 16 copies of the test binary running 5000 repeats each at once, all 16 hung with a fetcher per test and none with the shared fetcher. - MI455X in SPX: 65535 -> 1, as root and non-root. - MI300A: SPX 65535 -> 1 on all 4 GPUs; TPX -> 3 on all 12 logical GPUs (develop: 65535 on the 4 primaries, N/A on the other 8); CPX -> 6 on all 24. - Radeon (cannot be partitioned): 65535 -> 1. - Trial merges clean in every order and with current develop; the combined tree builds and all GPU-less tests pass. ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md. --------- Signed-off-by: Adam360x <Adam.pryor@amd.com>
adam360x
added a commit
that referenced
this pull request
Oct 16, 2026
) ## Motivation `rdci dmon -e 300` (`RDC_FI_POWER_USAGE`) reports 4294967295000000 µW on AMD Instinct MI300 and newer GPUs, while `amd-smi metric --power` shows the real socket power (140 W on the MI325X in the ticket). RDC treats a socket power of 65535 as "not available", but AMD SMI has reported an unreadable power as UINT32_MAX since ROCm 7.11 (ROCm 7.2 and earlier: 65535). These GPUs have no average socket power, so RDC scaled UINT32_MAX to microwatts, and power policies triggered on every check. ## Technical Details - Per-field fetch: UINT32_MAX (current AMD SMI) and UINT16_MAX (older AMD SMI) both mean "not available" for the average and current socket power. GPUs without an average socket power report the current one; partitions without any socket power report N/A (was the bogus value). - Bulk fetch (`RDC_BULK_FETCH_ENABLED`, off by default): a GPU whose 16-bit GPU metrics report neither an average nor a current socket power now falls through to the per-field fetch instead of reporting 65535 W. - Tests: new GPU-less `rdc_power_usage_tests` (9 tests) builds the metric fetcher against fake AMD SMI replies; `tests/ci/test_gtest_runner.py` runs it in the no-GPU CI job. - The tests share one metric fetcher and never destroy it (it keeps no state for the field they fetch): `~RdcMetricFetcherImpl()` can miss waking its worker thread and then wait for it forever. That race predates this PR and can also hang rdcd when it stops; it is not fixed here. - CHANGELOG entry under Resolved Issues. **Not changed here** - Logging is not improved here: partitions without socket power now log, on every fetch, the errors any unavailable field logs, plus "AMDSMI: cannot get POWER_USAGE". #13570 reworks that logging and edits the adjacent line. - Sibling fix for `RDC_FI_GPU_PARTITION_COUNT`: #13638. ## Issue Tracking JIRA ID: ROCM-33008 ## Test Plan - `rdc_power_usage_tests` against develop's fetcher and the fix; CI pytest wrapper against a private install. - `rdcd -u` + `rdci dmon -u -e 300` in a private network/mount namespace: 1x MI455X (ROCm 10.2.0 nightly; stock, develop, fix; non-root; bulk fetch on), 4x MI300A (gfx942) in SPX/TPX/CPX, 2x Radeon (RX 6800 XT, RX 7900 XT). - MI455X: 2000 W power policy, print-only action, `rdci policy --reg`. - Trial merges with #13386, #13570, #13638 and current develop. ## Test Result - Tests: 3 of 9 fail when built against develop's fetcher; all 9 pass with the fix. - Hang check: with 16 copies of the test binary running 5000 repeats each at once, all 16 hung with a fetcher per test and none with the shared fetcher. - MI455X: 4294967295000000 -> 1125000000-1129000000 µW (amd-smi: 1126-1127 W), as root and non-root. With the bulk fetch on, develop already reported the current power. - MI300A: 4294967295000000 -> 59-133 W per GPU; TPX/CPX partitions without socket power -> N/A. - Radeon (has an average socket power): unchanged. - Power policy: stock RDC reported "A Power Limit exceeds the threshold 2000 with the value 4294967295" every 5 s at ~1126 W; the fix reported nothing in 12 s. - Trial merges clean in every order and with current develop; the combined tree builds and all GPU-less tests pass. ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md. --------- Signed-off-by: Adam360x <Adam.pryor@amd.com>
This was referenced Oct 16, 2026
adam360x
added a commit
that referenced
this pull request
Oct 19, 2026
## Motivation Stopping `rdcd`, or embedded RDC with `rdc_stop_embedded()`, could hang: the metric fetcher's destructor could miss waking its worker thread and then wait for it forever. The race timed out the first CI run of #13638 in `rdc_partition_count_tests`; that PR's tests and #13637's then worked around it by sharing one fetcher that was never destroyed. ## Technical Details - `~RdcMetricFetcherImpl()` now stores the stop flag under the worker's mutex, so a worker between its check and its wait cannot miss the wake-up, and waits for the worker in its body, before the members the worker uses are destroyed. Before, the future's destructor joined the worker only after `cv_` and `task_started_` were destroyed. - Tests: new `rdc_metric_fetcher_tests` builds and destroys 1000 fetchers in each of 16 threads; it compiles the fetcher into the binary, so it runs without a GPU. `tests/ci/test_gtest_runner.py` runs it in the no-GPU CI job, and its ctest timeout is 120 s. - The power usage and partition count tests give each test its own fetcher again, as before the workaround. - CHANGELOG entry under Resolved Issues. ## Issue Tracking JIRA ID: ROCM-33007 Follow-up to the merged #13638 (ROCM-33007) and #13637 (ROCM-33008): the CI of #13638 found this race, and this PR removes the workaround in their tests. There is no separate ticket for the race, so it reuses ROCM-33007. ## Test Plan - `rdc_metric_fetcher_tests` against develop's destructor and the fix, also 16 copies at once. - Power usage and partition count tests with a fetcher per test: 16 copies at once, 3000 repeats each. - ThreadSanitizer build of the three fetcher test binaries; clang and GCC with `-Wall -Wextra`. - CI pytest wrappers against a private install. - Trial merges with #13386 and #13570. ## Test Result - `rdc_metric_fetcher_tests`: develop's destructor hung in 5 of 5 runs (killed after 30 s); with the fix it passes in ~0.12 s, also as 16 copies at once with 20 repeats each. - Power usage and partition count tests: all 16 copies pass (with develop's destructor and a fetcher per test, all 16 hung). - ThreadSanitizer: no warnings in the three fetcher test binaries. clang and GCC with `-Wall -Wextra`: no warnings on the changed sources. - Trial merges clean in all four orders tried; the combined tree builds and all GPU-less unit test binaries pass. ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md. Signed-off-by: Adam360x <Adam.pryor@amd.com>
adam360x
added a commit
that referenced
this pull request
Oct 19, 2026
## Motivation `rdci topo` without `-i` should stop with "Need to specify the group index to notify". The constructor of `RdciTopologyLinkSubSystem` left the group index and its "was set" flag uninitialized, so on a dirty heap the check read garbage as "index given". rdci then skipped it and asked rdcd for the topology of an undefined GPU index. Depending on the garbage, that printed the topology of an arbitrary GPU or an unrelated error. ## Technical Details - Initialize `group_index_` and `is_group_index_set` in the constructor, as the group subcommand already does. - New CI test `TestCLIUsage::test_topo_without_gpu_index` in `tests/ci/test_rdc_core.py`. It runs `rdci topo` without `-i` with `MALLOC_PERTURB_` set, so new heap memory is nonzero. It needs no GPU or daemon because rdci checks the options before it connects. - CHANGELOG entry under Resolved Issues. No overlap with #13386, which changes `RdciTopologyLinkSubSystem::process()` in the same file. Trial merges with the open RDC PRs that touch the same files (#13386, #13570, #13658) are clean. ## Issue Tracking No ticket. Found during a code review; valgrind and `MALLOC_PERTURB_` confirmed it. ## Test Plan - Run the new test against an rdci built with and without the fix (ASan Debug build, whose allocator also fills new memory with nonzero bytes). - Run the whole `TestCLIUsage` class with the fix. - Run RDC pre-commit on the changed files. ## Test Result - Without the fix: the test fails, rdci exits 8 instead of 5 (`RDC_ST_BAD_PARAMETER`). - With the fix: the test passes, and `TestCLIUsage` passes 16/16. - Pre-commit: all hooks pass. ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md. Signed-off-by: Adam360x <Adam.pryor@amd.com>
1 task done
adam360x
added a commit
that referenced
this pull request
Nov 3, 2026
## Motivation QA reported that `rdcd` aborts during `rdci diag -u -r 2 -g 1` on AMD Instinct MI455X (gfx1250) with ROCm build #265: ``` F0921 ... queue_controller.cpp:147] Could not find agent - 129205111608944 ``` The checks of the ROCr diagnostic module (`librdc_rocr.so`: Compute Queue ready, System memory check) build test objects that call `hsa_init()` and `hsa_shut_down()` themselves. Nothing else held a reference, so ROCr was unloaded and loaded again between them. `rdcd` always loads rocprofiler-sdk (the profiler module registers a tool at the first `hsa_init()`, even with `RDC_DISABLE_ROCP=1`), and rocprofiler-sdk does not support a ROCr reload. Depending on its version, the next queue creation aborts `rdcd` (`Could not find agent`) or fails, or the next profiler field read aborts it (`HSA Queue is not initialized`). ROCm 10.2.0 and develop are affected too, and not only on MI455X: with a 10.2.0 nightly on MI350X the checks pass, but the next profiler field read aborts `rdcd`. Keeping ROCr loaded exposed a second rocprofiler-sdk problem: it never destroys a queue it intercepted. With a queue per check, each `rdci diag` run left two queues and about 70 MB of VRAM in `rdcd`, until queue creation failed after 126 runs (MI455X allows 254 queues per process). So the checks now share one queue per GPU. ## Technical Details - **ROCr lifetime** (`RdcDiagnosticLib.cc`): the module takes one `hsa_init()` reference at its first check and releases it in `rdc_diag_destroy()`; the test objects' calls only move the count. If that `hsa_init()` fails, the run fails (`ROCr initialization failed`) before any check is built, and the next run retries. - **Shared queue**: `AcquireDiagQueue()` creates one 128-packet queue per GPU, destroyed by `rdc_diag_destroy()` (the System memory check used to create a 131072-packet queue per run). A mutex runs one check at a time, since they share the queues (and `MemoryAccess.cc` a global kernel-argument pointer). The unused `CreateQueue()` helper is removed. - **Cleanup** (`ComputeQueueTest.cc`, `MemoryAccess.cc`, `base_rocr_utils.cc`, `RdcRocrBase.*`): both checks release their signals, memory, file descriptors and loaded code objects on every path, including Skip and failure (`ScopeExit` guard in `common.h`). A copy that fails after it started no longer leaks its signal. - **gfx1250 kernels**: without them both checks reported Skip on MI455X. They are code object v4 (ROCm 10.1 compiler: `clang -x cl -Xclang -finclude-default-header -target amdgcn-amd-amdhsa -mcpu=gfx1250 -mcode-object-version=4 -O3`), because the checks fill in the v4 kernel arguments; the default v6 fails the System memory check. - **Tests**: new GPU-less `rdc_rocr_lifetime_tests` runs the module against fake HSA calls that count loads, references and objects: ROCr is loaded once, one queue per GPU, a failed `hsa_init()` loads nothing and is retried, and no check leaves objects behind. It is built with the module, installed with the RDC test package, and run in RDC CI by `tests/ci/test_gtest_runner.py`. `rdc_diag_kernel_tests` requires v4 gfx1250 kernels. - `CHANGELOG.md` entries. No public API, proto or struct changes (the changed headers are not installed). Trade-off: after the first `rdci diag`, `rdcd` keeps ROCr and one queue per GPU until it stops (MI455X: 3 KFD queues and about 149 MB of VRAM, flat across runs after the first), as it already does after the first profiler read. Changing the GPU partition mode then needs `rdcd` stopped, and a second `rdci diag` waits for the first. #13570 and #13874 also edit `CHANGELOG.md`; trial merges show the three PRs merge cleanly in any order. Not in this PR: - rocprofiler-sdk: support a ROCr reload and destroy the queues it intercepts (ROCM-33402). A process that creates and destroys a queue in a loop aborts after about 250 iterations under `rocprofv3 --pmc`. - Kernel arguments: the shipped gfx950 kernels (code object v6) and the gfx940-gfx942 binary search kernel (v5) declare 280-288 bytes of kernel arguments, while the checks fill in the 80-byte v4 layout. Release builds pass both checks on MI350X with them, but an assertion-enabled build aborts `rdcd` on the Compute Queue check's existing kernel-argument size assertion. v4 rebuilds match and pass on MI350X. ## Issue Tracking JIRA ID: ROCM-31379 ## Test Plan - 1x MI455X (gfx1250), Ubuntu 26.04: QA's build #265, ROCm 10.2.0, and a 10.2.0 BKC build of 2026-10-13. Each experiment runs its own `rdcd -u` in a private network namespace: N `rdci diag -u -r 2 -g 1` runs, a profiler `rdci dmon` (fields 805/806), one more diag; also profiler first. KFD queues and VRAM from `/sys/class/kfd/kfd/proc/<pid>/`. The module is built from this branch against each ROCm. - 8x MI350X (gfx950), Ubuntu 24.04, 10.2.0 nightly of 2026-10-22: on GPU 0, the stock module and this PR's module (release and assertion-enabled builds, shipped gfx950 kernels and v4 rebuilds); on a group of all 8 GPUs, this PR's release build with the shipped kernels, in both orders. - `rdc_rocr_lifetime_tests` and `rdc_diag_kernel_tests` (GCC 11) on this branch, against develop's module, and with each part of the fix removed; the CI wrapper; `pre-commit`. ## Test Result | MI455X | `rdci diag` runs | KFD queues / VRAM, run 1 → last | profiler after | `rdcd` exit | |---|---|---|---|---| | #265 stock | aborts in run 1 (`Could not find agent`) | - | - | 134 | | 10.2.0 stock | run 1 Skip (no gfx1250 kernels), run 2 fails queue creation | - | aborts | 134 | | BKC 10-13 stock | aborts in run 1 | - | - | 134 | | 10.2.0, ROCr kept loaded, a queue per check | 126 Pass, then Fail | 4 → 254 / 0.39 → 9.3 GB | aborts | 134 | | 10.2.0, this PR | 141/141 Pass | 3 → 3 / 149 → 149 MB | works | 0 | | #265, this PR | 21/21 Pass | 3 → 3 / 149 → 149 MB | works | 0 | | BKC 10-13, this PR | 21/21 Pass | 3 → 3 / 287 → 149 MB | works | 0 | - Pass = both checks Pass. Profiler read first: 3/3 on #265 and on the BKC build. Stock builds run clean with `HSA_TOOLS_DISABLE_REGISTER=1` (rocprofiler-sdk not registered with ROCr). On the BKC build `rdci` itself exits with SIGSEGV after printing (the client exit crash fixed on develop by #13296), which does not affect `rdcd`. - 8x MI350X: stock passes 3/3 diags on GPU 0, then the profiler read aborts `rdcd` (`HSA Queue is not initialized`, exit 134). This PR (release build, shipped kernels): 4/4 on GPU 0 and 11/11 on all 8 GPUs, every GPU passing both checks, including a run after the profiler read; 3/3 on all 8 GPUs with the profiler read first. KFD queues stay flat across runs (24 with all 8 GPUs) and `rdcd` exits 0. - gfx1250 kernels: v4 passes both checks; v6 fails the System memory check in a release build. - `rdc_rocr_lifetime_tests` 5/5 pass and 4 of 5 fail against develop's module; each of 14 mutants of the fix, and reverting either review fix, makes a test fail. The mutex is not unit-tested. `rdc_diag_kernel_tests`, the CI wrapper and pre-commit pass. ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md. --------- Signed-off-by: Adam360x <Adam.pryor@amd.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 claude: Opened by Claude on behalf of @dmitrii-galantsev.
Motivation
Supersedes #11415 by @Bellk17, which the stale bot closed and which could not run RDC CI from its fork. This branch retains Kyle Bell's authorship on 15 rebased commits and adds seven review commits. The rebase dropped his now-empty PENDING_PAGE_NUM fix, already covered by #8488.
On the tested MI350 system, health monitoring repeatedly logs errors for unreadable XGMI_ERROR and unsupported EEPROM_CONFIG_VALID. Failed field reads also prevent some health checks from being evaluated, even though the report says Pass.
Technical Details
The seven review commits move the EEPROM-only test before other watches, add six fake-fetcher regression tests, suppress the redundant missing-fallback log, format the changes, compile SmiUtils.cc/link amd_smi in rdc_status_tests after the rebase, lower a successful XGMI fallback announcement from ERROR to INFO with log-capturing regression assertions, and start health history for a field whose first probe failed transiently once it recovers (from Copilot review; two regression tests).
Issue Tracking
Fixes #11467
Fixes #11468
Supersedes #11415
Test Plan
Built develop 9a24973 and this branch on 8x MI350 (gfx950, SPX, ROCm 7.13 AMD SMI, amdgpu 6.19.14), with rdcd running as root and all eight GPUs in the group. No faults were injected.
For each component set, counted ERROR lines in a 30-second window beginning one second after health setup, then in the interval running
-c,-c --json, and-f. The checks interval includes concurrent background polling; startup, setup, and teardown errors are excluded.Ran unit and health functional tests, built the six fake-fetcher tests against develop as a negative control, and ran RDC pre-commit hooks on changed files.
Test Result
-sxeamIn the MI350 run above, setup with
xoraalso emitted eight one-time ERROR lines announcing the XGMI fallback;eandmemitted none. Those setup lines are excluded from the table. Commit9fef2f3e6c4changes a successful fallback announcement to INFO, visible withRDC_LOG=INFOorDEBUG; a field with no working source still logs ERROR. This follow-up was verified locally with fake-fetcher tests, not re-run on MI350. The table remains the measured result from before that commit.On develop with
a, the two check calls together log unavailable XGMI current data for all eight GPUs, missing PCIe history for GPUs 1–7, missing power/thermal history for all eight, and missing retired-page history for GPU 7. With this PR, DEBUG logs show the watch/check path reading ECC_XGMI_WAFL_UE on all eight GPUs, all zero.9fef2f3e6c4): the fake-fetcher fallback test fails with the old ERROR level and passes with INFO; the no-source test still requires ERROR. Independently repeated locally: rdc_unit_tests 18/18 and rdc_status_tests 4/4. RDC pre-commit hooks passed on the two changed files.Not covered on hardware: actual XGMI/memory faults, MI2xx, mixed-capability GPU groups, or partition modes other than SPX. Fake-fetcher tests cover XGMI failure reporting and the retired-page-limit incident.
Submission Checklist