Hi Ilpo,
On 7/3/25 2:27 AM, Ilpo Järvinen wrote:
> On Fri, 27 Jun 2025, Reinette Chatre wrote:
>> On 6/16/25 1:24 AM, Ilpo Järvinen wrote:
>>>
>>> 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.
>
> Perhaps resctrl can be improved as well but I think it's also a bad
> practice to create variables in any header like that. I just don't know
> what would be the preferred way to address that in the context of
> kselftest because AFAIK, there's no .c file currently injected into all
> selftests by the build system.
>
>> 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?
>
> I hadn't notice there are already these defines for the status value
> in kselftest.h. Yes, it definitely makes sense to use them in resctrl
> selftests instead of literal return values.
>
> That, however, addresses only half of the problem as
> ksft_test_result_skip() takes string which would naturally come from
> the test case because it knows better what went wrong.
>
> IMO, most optimal solution would be to call ksft_test_result_skip() right
> at the test case ifself and then return KSFT_SKIP from the test to
> run_single_test(). run_single_test() would then skip doing
> ksft_test_result() call. But that messes up the test result counts due to
> the duplicated ksft_cnt in different .c files.
Your response makes me wonder if you noticed the switch to calling
ksft_test_result_report() from run_single_test(). Now looking back it may
have been too subtle in my response ...
I agree that the test self will know best what went wrong. Tests can still
use ksft_print_msg() for informational text.
Doing something like:
cat_ctrlgrp_msr_test() {
...
if (!msr_access_supported(uparams->cpu)) {
ksft_print_msg("MSR access not supported\n");
return KSFT_SKIP;
...
}
run_single_test() {
...
ret = test->run_test(test, uparams);
ksft_test_result_report(ret, "%s: test\n", test->name);
...
}
Can result in output like:
# MSR access not supported
ok X SKIP CAT_GROUP_MASK: test
As I understand this will keep accurate test counts and the user output
seems intuitive enough to understand why a test may have been skipped.
>
>> 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?
>
> The duplication of MSR reading code was a bit annoying to me as well,
> although I only thought it within inside kselftests. But I can look at
> this considering tools/ too now that you pointed to that direction.
>
Thank you very much for considering this.
Reinette
