Re: [RFC PATCH v1 0/2] kselftest/resctrl: CAT functional tests

[Ilpo Järvinen]

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.

> 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.

-- 
 i.