Hi Ilpo,
On 6/16/25 1:24 AM, Ilpo Järvinen wrote:
Hi all,
In the last Fall Reinette mentioned functional tests of resctrl would be preferred over selftests that are based on performance measurement. This series tries to address that shortcoming by adding some functional tests for resctrl FS interface and another that checks MSRs match to what is written through resctrl FS. The MSR test is only available for Intel CPUs at the moment.
Thank you very much for keeping this in mind and taking this on!
Why RFC?
The new functional selftest itself works, AFAIK. However, calling ksft_test_result_skip() in cat.c if MSR reading is found to be unavailable is problematic because of how kselftest harness is architected. The kselftest.h header itself defines some variables, so including it into different .c files results in duplicating the test framework related variables (duplication of ksft_count matters in this case).
The duplication problem could be worked around by creating a resctrl selftest specific wrapper for ksft_test_result_skip() into resctrl_tests.c so the accounting would occur in the "correct" .c file, but perhaps that is considered hacky and the selftest framework/build systems should be reworked to avoid duplicating variables?
I do not think resctrl selftest's design can demand such a change from kselftest. The way I understand this there is opportunity to improve (fix?) resctrl's side.
Just for benefit of anybody following (as I am sure you are very familiar with this), on a high level the resctrl selftests are run via a wrapper that calls a test specific function: run_single_test() { ... ret = test->run_test(test, uparams); ksft_test_result(!ret, "%s: test\n", test->name); ... }
I believe that you have stumbled onto a problem with this since the wrapper can only handle "pass" and "fail" (i.e. not "skip").
This is highlighted by patch #2 that sets cat_ctrlgrp_msr_test() as the "test->run_test" and it does this:
cat_ctrlgrp_msr_test() { ... if (!msr_access_supported(uparams->cpu)) { ksft_test_result_skip("Cannot access MSRs\n"); return 0; } }
The problem with above is that run_single_test() will then set "ret" to 0, and run_single_test()->ksft_test_result() will consider the test a "pass".
To address this I do not think the tests should call any of the ksft_test_result_*() wrappers but instead should return the actual kselftest exit code. For example, cat_ctrl_grp_msr_test() can be:
cat_ctrlgrp_msr_test() { ... if (!msr_access_supported(uparams->cpu)) return KSFT_SKIP; ... }
To support that run_single_test() can be: run_single_test() { ... ret = test->run_test(test, uparams); ksft_test_result_report(ret, "%s: test\n", test->name); ... }
I think making this explicit will make the tests also easier to read. For example, cat_ctrlgrp_tasks_test() in patch #1 contains many instances of the below pattern: ksft_print_msg("some error message"); ret = 1;
A positive return can be interpreted many ways. Something like below seems much clearer to me:
ksft_print_msg("some error message"); ret = KSFT_FAIL;
What do you think?
On a different topic, the part of this series that *does* raise a question in my mind is the introduction of the read_msr() utility local to resctrl. Duplicating code always concerns me and I see that there are already a few places where user space tools and tests read MSRs by opening/closing the file while there is also one utility (tools/power/cpupower/utils/helpers/msr.c) that looks quite similar to what is created here.
It is not obvious to me how to address this though. Looking around I see tools/lib may be a possible candidate and the changelog of commit 553873e1df63 ("tools/: Convert to new topic libraries") gave me impression that the goal of this area is indeed to host code shared by things living in tools/ (that includes kselftest). While digging I could not find a clear pattern of how this is done in the kselftests though. This could perhaps be an opportunity to pave the way for more code sharing among selftests by creating such a pattern with this already duplicated code?
Thanks again.
Reinette