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
