Skip to content

fix(rdc): handle sample limits above INT32_MAX in the field cache - #14052

Draft
adam360x wants to merge 1 commit into
developfrom
users/adam360x/rdc-cache-bounds
Draft

adam360x wants to merge 1 commit into
developfrom
users/adam360x/rdc-cache-bounds

Conversation

@adam360x

@adam360x adam360x commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

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_field_watch() takes any uint32_t max_keep_samples, but the cache's eviction computed the number of samples to drop in an int. For limits above INT32_MAX, UINT32_MAX among them, that number could wrap around to more samples than the field held, and the eviction erased past the end of the field's samples: rdcd or the process that embeds RDC crashed or corrupted its memory.

Technical Details

  • The eviction drops samples only when a field holds more than its limit, and keeps the newest ones.
  • rdc.h said that a max_keep_samples of 0 means no limit, but RDC keeps no samples for 0 and relies on it (the job statistics watch passes 0). rdc.h now documents what 0 does, for max_keep_age too, and how watches of one field combine.
  • rdc_field_get_value_since() no longer copies a field's whole sample vector, under the cache lock, to return one sample.

Issue Tracking

ISSUE ID : #14107

Part of a series of RDC fixes, which #14107 lists. No JIRA ticket.

Test Plan

rdc_cache_bounds_tests compiles RdcCacheManagerImpl.cc, and test_fake_gpu_cache_bounds.py watches fields in embedded RDC against the fake amd-smi. Without the fix, the large limits crash (AddressSanitizer: negative-size-param in evict_cache) or read outside the cache; the cases for the documented limits pass with and without it.

Test Result

Fake-GPU suite: 139 passed, 24 skipped. tests/ci: 93 passed, 6 skipped.

Submission Checklist

rdc_field_watch() takes any uint32_t max_keep_samples, but the cache's
eviction computed the number of samples to drop in an int. For limits
above INT32_MAX, UINT32_MAX among them, that number could wrap around to
more samples than the field held, and the eviction erased past the end of
the field's samples: rdcd or the process that embeds RDC crashed or
corrupted its memory. The eviction now drops samples only when a field
holds more than its limit, and keeps the newest ones.

rdc.h said that a max_keep_samples of 0 means no limit, but RDC keeps no
samples for 0, and relies on it: the job statistics watch passes 0, and
watches of one field merge to the larger limits. rdc.h now documents what
0 does, for max_keep_age too, and how watches of one field combine.

rdc_field_get_value_since() no longer copies a field's whole sample
vector, under the cache lock, to return one sample.

Tests: rdc_cache_bounds_tests compiles RdcCacheManagerImpl.cc, and
test_fake_gpu_cache_bounds.py watches fields in embedded RDC against the
fake amd-smi. Without the fix, the large limits crash (AddressSanitizer:
negative-size-param in evict_cache) or read outside the cache; the cases
for the documented limits pass with and without it.

Signed-off-by: Adam360x <Adam.pryor@amd.com>
Base automatically changed from users/adam360x/rdc-fake-amdsmi-ext to users/adam360x/rdc-fake-gpu-tests October 23, 2026 15:39
Base automatically changed from users/adam360x/rdc-fake-gpu-tests to develop November 5, 2026 16:51
@therock-pr-bot

therock-pr-bot Bot commented Nov 5, 2026

Copy link
Copy Markdown

✅ All Policy Checks Passed

Check Status Details
📝 PR Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ✅ Pass —
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —

🎉 All policy checks passed!

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant