Message-ID: <0e9137e0-ec21-42b9-98be-af90becc2318@intel.com>
Date: Fri, 27 Jun 2025 11:04:28 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
	<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>
CC: <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 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

Powered by blists - more mailing lists