Repository navigation
Conversation
rdc_config_set() read its status uninitialized for a GPU group with no GPU, so the setting was cached and reported as set, or failed, by chance. A setting type other than the three in rdc_config_type_t reached no GPU but was cached and reported as set, and a client could cache as many as it liked. A power limit whose microwatts did not fit in 64 bits wrapped around to another limit, which the GPUs took. All three are now RDC_ST_BAD_PARAMETER, before any GPU is touched; the type is compared as an integer, since loading a value outside the enum through it is undefined. When one GPU of a group rejected a setting, the GPUs before it kept the new value, which RDC neither tracked nor cleared. They now get the group's previous setting of that type back, or their default. rdc_config_clear() stopped at the first GPU or setting it could not reset and dropped the group's settings anyway; it now resets every GPU and setting it can, then reports the failure. A clear that removed the last setting joined the monitoring thread outside the lock while a concurrent set could start a new thread in the same std::thread: a data race, and std::terminate while the old thread was still joinable. The monitoring thread now runs from the first setting until the object is destroyed, waits without waking up while no setting is cached, and set, clear and each monitoring pass hold the lock throughout. The monitoring thread read the groups' GPUs into one structure for all groups and passes, so a group lookup that succeeded without filling it reused another group's GPUs. It compared the power cap in whole watts converted through an int, which re-applied caps above 2147 W every minute and missed changes of less than a watt, and it read every setting of a GPU and skipped the rest after any failed read. It now reads each group afresh, compares microwatts, and checks only the cached settings, each on its own. A group that reports more GPUs than rdc_group_info_t holds is an error instead of a read past the array, and an empty frequency table no longer makes rdc_config_clear() read far outside it. Tests: rdc_config_tests compiles RdcConfigSettingsImpl.cc with fakes for the group table and amd-smi, and a second constructor sets the monitoring period to 10 ms; the uninitialized cases fill the stack with 0xA5 first, behind a check that the fill shows. rdc_race_tests sets, reads and clears several groups from several threads under ThreadSanitizer, and test_fake_gpu_config.py drives embedded RDC against the fake amd-smi. Without the fix, 17 of the 20 gtests fail (the round trip passes either way, and the two oversized-table cases fail only under AddressSanitizer), the race test ends in std::terminate in every run, and the 6 fake-GPU tests fail. Signed-off-by: Adam360x <Adam.pryor@amd.com>
1 task done
The config race test asked any other race test that defines get_processor_handle_from_id() to drop its copy. The policy race test keeps a weak one, which links with this test's definition, so say that a weak copy works too. 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.
Motivation
rdc_config_set() read its status uninitialized for a GPU group with no GPU, so the setting was cached and reported as set, or failed, by chance. A setting type other than the three in rdc_config_type_t reached no GPU but was cached and reported as set, and a client could cache as many as it liked. A power limit whose microwatts did not fit in 64 bits wrapped around to another limit, which the GPUs took. All three are now RDC_ST_BAD_PARAMETER, before any GPU is touched; the type is compared as an integer, since loading a value outside the enum through it is undefined.
Technical Details
When one GPU of a group rejected a setting, the GPUs before it kept the new value, which RDC neither tracked nor cleared. They now get the group's previous setting of that type back, or their default. rdc_config_clear() stopped at the first GPU or setting it could not reset and dropped the group's settings anyway; it now resets every GPU and setting it can, then reports the failure.
A clear that removed the last setting joined the monitoring thread outside the lock while a concurrent set could start a new thread in the same std::thread: a data race, and std::terminate while the old thread was still joinable. The monitoring thread now runs from the first setting until the object is destroyed, waits without waking up while no setting is cached, and set, clear and each monitoring pass hold the lock throughout.
The monitoring thread read the groups' GPUs into one structure for all groups and passes, so a group lookup that succeeded without filling it reused another group's GPUs. It compared the power cap in whole watts converted through an int, which re-applied caps above 2147 W every minute and missed changes of less than a watt, and it read every setting of a GPU and skipped the rest after any failed read. It now reads each group afresh, compares microwatts, and checks only the cached settings, each on its own. A group that reports more GPUs than rdc_group_info_t holds is an error instead of a read past the array, and an empty frequency table no longer makes rdc_config_clear() read far outside it.
Issue Tracking
ISSUE ID : #14107
Part of a series of RDC fixes, which #14107 lists. No JIRA ticket.
Test Plan
Tests: rdc_config_tests compiles RdcConfigSettingsImpl.cc with fakes for the group table and amd-smi, and a second constructor sets the monitoring period to 10 ms; the uninitialized cases fill the stack with 0xA5 first, behind a check that the fill shows. rdc_race_tests sets, reads and clears several groups from several threads under ThreadSanitizer, and test_fake_gpu_config.py drives embedded RDC against the fake amd-smi. Without the fix, 17 of the 20 gtests fail (the round trip passes either way, and the two oversized-table cases fail only under AddressSanitizer), the race test ends in std::terminate in every run, and the 6 fake-GPU tests fail.
Test Result
GPU-less container, as CI runs the tests: Fake-GPU suite: 136 passed, 25 skipped.
tests/ci: 93 passed, 6 skipped.Submission Checklist