Message-ID: <8dff86d9-18b1-487f-9f75-84735a6815f5@intel.com>
Date: Thu, 3 Jul 2025 08:42:47 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
CC: <linux-kselftest@...r.kernel.org>, Shuah Khan <shuah@...nel.org>, "Tony
 Luck" <tony.luck@...el.com>, Dave Martin <Dave.Martin@....com>, James Morse
	<james.morse@....com>, Shaopeng Tan <tan.shaopeng@...fujitsu.com>, LKML
	<linux-kernel@...r.kernel.org>, Fenghua Yu <fenghuay@...dia.com>
Subject: Re: [RFC PATCH v1 0/2] kselftest/resctrl: CAT functional tests

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

Powered by blists - more mailing lists