Repository navigation
Conversation
RdcTelemetryModule copied every GPU-field pair due in one update into a stack array of MAX_NUM_FIELDS (8,192) per telemetry library. Two watches of 128 fields on 64 GPUs, due together, wrote 16,384 pairs into it, and rdcd crashed in about half of the runs with the fake amd-smi. It now hands each library at most MAX_NUM_FIELDS pairs per call, from the vector it already built. One watch holds at most 64 x 128 = 8,192 pairs, so watching and unwatching never exceed it. It also called back for unsupported fields with &unsupport_fields[0] of an empty vector on every update (UBSan: reference binding to null pointer); it now calls back only when there are some. It looked fields up with fields_id_module_[id], which adds an entry for every unknown field to a map only the constructor should fill; it now uses find() in a const member. RdcEmbeddedHandler::rdc_device_get_all() wrote one index per GPU and GPU partition into the caller's array of RDC_MAX_NUM_DEVICES (128). With 128 fake GPUs of two partitions each, rdci discovery crashed rdcd every time (ASan: stack-buffer-overflow in GetAllDevices); starting a job lists the devices the same way. It now lists the first 128, logs once how many there are, and rdc.h says so. rdci already rejects a longer list from rdcd. rdc_telemetry_tests, a gtest built from RdcTelemetryModule.cc with a stub library (without librdc, so it needs no GPU), fails before this change: a library call gets more than MAX_NUM_FIELDS pairs, and the callback gets a null pointer. test_fake_gpu_telemetry.py runs it, discovers 256 fake GPU partitions and starts a job, and runs the two watches. Before this change the first two fail every time and the watches in two of four runs. Signed-off-by: Adam360x <Adam.pryor@amd.com>
1 task done
This was referenced Oct 22, 2026
fix(rdc): rdci options, output and exit statuses, and the names and policy values RDC accepts
#14058
Draft
test_more_gpus_and_partitions_than_rdc_lists looked for the line the embedded handler logs when amd-smi lists more GPUs and partitions than RDC_MAX_NUM_DEVICES. A GPU table that keeps only the first RDC_MAX_NUM_DEVICES GPUs logs its own line first, and the handler then never sees more. Accept either line, so that the test holds with or without such a table. Signed-off-by: Adam360x <Adam.pryor@amd.com>
adam360x
commented
Oct 23, 2026
| rdc_gpu_field_t f[MAX_NUM_FIELDS]; | ||
| std::copy(ite->second.begin(), ite->second.end(), f); | ||
| ite->first->rdc_telemetry_fields_value_get(f, ite->second.size(), callback, user_data); | ||
| for (auto& [module, due] : fields_to_fetch) { |
Contributor
Author
There was a problem hiding this comment.
Can we make this better than o(n^2)?
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
joshnguy-amd
added a commit
that referenced
this pull request
Nov 6, 2026
> Stacked on #14041 as a draft: it shows only its own commit. When #14041 merges, this PR moves to develop, where RDC CI runs, and becomes ready for review. ## Motivation RDC's tests run without sanitizers, so a read past a buffer or an invalid enum value that does not crash goes unseen. ## Technical Details - A new job, `rdc_sanitize`, has two legs: GCC `-fsanitize=address` and `-fsanitize=undefined`, one build each. With both runtimes in one process, UBSan's reports go to stderr instead of `log_path`. - Each leg runs the installed gtests that need no GPU (all but `rdc_diag_kernel_tests`, which has no install RPATH), the fake-GPU suite (#14041) and `tests/ci` against its build. Every process writes its reports to one directory. - A later step prints them, lists the distinct errors in the job summary, and fails the leg if there are any, if a report lands in output the tests keep (such as `rdcd`'s logs) instead, or if no test ran. - Before the tests, a step checks that the binaries load the leg's runtime, that librdc calls its checks, and that a program with a known bug leaves a report at `log_path`. - ASan's leak detection is off (Python under the runtime does not free everything at exit, and `rdcd` and `rdci` inherit its options), and so is `verify_asan_link_order` (the tests preload the fake amd-smi, which links the runtime, into `rdcd`). - The legs build gRPC 1.67.1 as `rdc_build` does and cache it. They do not gate until the repo variable `RDC_SANITIZE_GATING` is `true`. CI-only; no CHANGELOG entry. ## Issue Tracking ISSUE ID : #14107 Part of a series of RDC fixes, which #14107 lists. No JIRA ticket. ## Test Plan Ran each leg's build and test steps on this branch in a GPU-less Ubuntu 22.04 container. ## Test Result Both legs pass every test: the 8 gtest programs the job runs (43 cases), 106 fake-GPU tests (24 skipped), and `tests/ci` (92 passed, 6 skipped; `test_prometheus_help` needs `prometheus_client`, which the job installs). The ASan leg reports nothing. The UBSan leg reports, in 89 processes, findings that are on develop today: - a reference bound to a null pointer when `RdcTelemetryModule` passes `&v[0]` of an empty vector (the telemetry fix in this series, #14045, removes it), - an invalid `rdc_field_type_t` in `fetch_smi_field()` and the cache, because `RDC_HEALTH_EEPROM_CONFIG_VALID` reports success without setting a type or value, - the link type and P2P flag that `rdc_device_topology_get()` reads after a failed amd-smi call, - an invalid `rdc_field_t` where `rdci dmon`, the client library and `rdcd` load the field ID 99999 that a `tests/ci` test passes on purpose. ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md.
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.
Motivation
RdcTelemetryModulecopied every GPU-field pair due in one update into a stack array of 8,192 pairs per telemetry library. Two watches of 128 fields on 64 GPUs, due together, wrote 16,384 pairs into it, andrdcdcrashed in about half of the runs with the fake amd-smi.rdc_device_get_all()wrote one index per GPU and GPU partition into the caller's 128-entry array. With 128 fake GPUs of two partitions each,rdci discoverycrashedrdcdevery time.&v[0]of an empty vector (UBSan: reference binding to null pointer), and its field lookup added an entry for every unknown field to a map only the constructor should fill.Technical Details
find()in a const member.rdc.hsays so.rdcialready rejects a longer list fromrdcd.Issue Tracking
ISSUE ID : #14107
Part of a series of RDC fixes, which #14107 lists. No JIRA ticket.
Test Plan
rdc_telemetry_tests, a gtest built fromRdcTelemetryModule.ccwith a stub library (no GPU needed), andtest_fake_gpu_telemetry.py, which discovers 256 fake GPU partitions, starts a job and runs the two watches. Before this change the first two fail every time and the watches in two of four runs. The 256-partition test accepts either log line for the GPUs past the first 128: this handler's, or the one that #14071 logs when its GPU table keeps only the first 128, so it passes whichever of the two lands first (rerun on this branch: passed).Test Result
Fake-GPU suite: 109 passed, 24 skipped.
tests/ci: 92 passed, 6 skipped.Submission Checklist