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

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

[Ilpo Järvinen]

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.

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?


Ilpo Järvinen (2):
  kselftest/resctrl: CAT L3 resctrl FS function tests
  kselftest/resctrl: Add CAT L3 CBM functional test for Intel RDT

 tools/testing/selftests/resctrl/cat_test.c    | 210 ++++++++++++++++++
 tools/testing/selftests/resctrl/msr.c         |  55 +++++
 tools/testing/selftests/resctrl/resctrl.h     |   6 +
 .../testing/selftests/resctrl/resctrl_tests.c |   2 +
 tools/testing/selftests/resctrl/resctrlfs.c   |  48 ++++
 5 files changed, 321 insertions(+)
 create mode 100644 tools/testing/selftests/resctrl/msr.c


base-commit: c1d7e19c70cbb8a19f63c190cf53e71b5f970514
-- 
2.39.5

[RFC PATCH v1 1/2] kselftest/resctrl: L3 CAT resctrl FS functional tests

[Ilpo Järvinen]

Resctrl CAT selftests have been limited to mainly testing performance.
In order to validate the kernel side behavior better, add a functional
test that validates .../tasks file content while performing moves of
the task to different control groups.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 tools/testing/selftests/resctrl/cat_test.c    | 84 +++++++++++++++++++
 tools/testing/selftests/resctrl/resctrl.h     |  2 +
 .../testing/selftests/resctrl/resctrl_tests.c |  1 +
 tools/testing/selftests/resctrl/resctrlfs.c   | 48 +++++++++++
 4 files changed, 135 insertions(+)

diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
index 94cfdba5308d..78cb9ac90bb1 100644
--- a/tools/testing/selftests/resctrl/cat_test.c
+++ b/tools/testing/selftests/resctrl/cat_test.c
@@ -376,6 +376,82 @@ static bool noncont_cat_feature_check(const struct resctrl_test *test)
 	return resource_info_file_exists(test->resource, "sparse_masks");
 }
 
+static int cat_ctrlgrp_tasks_test(const struct resctrl_test *test,
+				  const struct user_params *uparams)
+{
+	cpu_set_t old_affinity;
+	pid_t bm_pid;
+	int ret;
+
+	bm_pid = getpid();
+
+	ret = resctrl_grp_has_task(NULL, bm_pid);
+	if (ret < 0)
+		return ret;
+	if (!ret) {
+		ksft_print_msg("PID not found in the root group\n");
+		return 1;
+	}
+
+	/* Taskset benchmark to specified CPU */
+	ret = taskset_benchmark(bm_pid, uparams->cpu, &old_affinity);
+	if (ret)
+		return ret;
+	ret = resctrl_grp_has_task(NULL, bm_pid);
+	if (ret < 0)
+		goto reset_affinity;
+	if (!ret) {
+		ksft_print_msg("PID not found in the root group\n");
+		ret = 1;
+		goto reset_affinity;
+	}
+
+	ret = write_bm_pid_to_resctrl(bm_pid, "c1", NULL);
+	if (ret)
+		goto reset_affinity;
+	ret = resctrl_grp_has_task("c1", bm_pid);
+	if (ret < 0)
+		goto reset_affinity;
+	if (!ret) {
+		ksft_print_msg("PID not found in the control group\n");
+		ret = 1;
+		goto reset_affinity;
+	}
+	ret = resctrl_grp_has_task(NULL, bm_pid);
+	if (ret < 0)
+		goto reset_affinity;
+	if (ret) {
+		ksft_print_msg("PID duplicate remains in the root group\n");
+		ret = 1;
+		goto reset_affinity;
+	}
+
+	ret = write_bm_pid_to_resctrl(bm_pid, "c2", NULL);
+	if (ret)
+		goto reset_affinity;
+	ret = resctrl_grp_has_task("c2", bm_pid);
+	if (ret < 0)
+		goto reset_affinity;
+	if (!ret) {
+		ksft_print_msg("PID not found in the new control group\n");
+		ret = 1;
+		goto reset_affinity;
+	}
+	ret = resctrl_grp_has_task("c1", bm_pid);
+	if (ret < 0)
+		goto reset_affinity;
+	if (ret) {
+		ksft_print_msg("PID duplicate remains in the old control group\n");
+		ret = 1;
+		goto reset_affinity;
+	}
+
+reset_affinity:
+	taskset_restore(bm_pid, &old_affinity);
+
+	return ret;
+}
+
 struct resctrl_test l3_cat_test = {
 	.name = "L3_CAT",
 	.group = "CAT",
@@ -400,3 +476,11 @@ struct resctrl_test l2_noncont_cat_test = {
 	.feature_check = noncont_cat_feature_check,
 	.run_test = noncont_cat_run_test,
 };
+
+struct resctrl_test cat_grp_tasks_test = {
+	.name = "CAT_GROUP_TASKS",
+	.group = "CAT",
+	.resource = "L3",
+	.feature_check = test_resource_feature_check,
+	.run_test = cat_ctrlgrp_tasks_test,
+};
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
index cd3adfc14969..d25f83d0a54d 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -174,6 +174,7 @@ bool resctrl_mon_feature_exists(const char *resource, const char *feature);
 bool resource_info_file_exists(const char *resource, const char *file);
 bool test_resource_feature_check(const struct resctrl_test *test);
 char *fgrep(FILE *inf, const char *str);
+int resctrl_grp_has_task(const char *grp, pid_t bm_pid);
 int taskset_benchmark(pid_t bm_pid, int cpu_no, cpu_set_t *old_affinity);
 int taskset_restore(pid_t bm_pid, cpu_set_t *old_affinity);
 int write_schemata(const char *ctrlgrp, char *schemata, int cpu_no,
@@ -241,6 +242,7 @@ static inline unsigned long cache_portion_size(unsigned long cache_size,
 extern struct resctrl_test mbm_test;
 extern struct resctrl_test mba_test;
 extern struct resctrl_test cmt_test;
+extern struct resctrl_test cat_grp_tasks_test;
 extern struct resctrl_test l3_cat_test;
 extern struct resctrl_test l3_noncont_cat_test;
 extern struct resctrl_test l2_noncont_cat_test;
diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
index 5154ffd821c4..71b7cd846dc1 100644
--- a/tools/testing/selftests/resctrl/resctrl_tests.c
+++ b/tools/testing/selftests/resctrl/resctrl_tests.c
@@ -18,6 +18,7 @@ static struct resctrl_test *resctrl_tests[] = {
 	&mbm_test,
 	&mba_test,
 	&cmt_test,
+	&cat_grp_tasks_test,
 	&l3_cat_test,
 	&l3_noncont_cat_test,
 	&l2_noncont_cat_test,
diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
index 195f04c4d158..0bc92eee0080 100644
--- a/tools/testing/selftests/resctrl/resctrlfs.c
+++ b/tools/testing/selftests/resctrl/resctrlfs.c
@@ -601,6 +601,54 @@ static int create_grp(const char *grp_name, char *grp, const char *parent_grp)
 	return 0;
 }
 
+/*
+ * resctrl_grp_has_task - Check if group tasks include a PID
+ * @grp:	Name of the group (use NULL for root group)
+ * @bm_id:	PID that should be checked
+ *
+ * Return: 1 if PID is a task in @grp, 0 if not, and <0 on error.
+ */
+int resctrl_grp_has_task(const char *grp, pid_t bm_pid)
+{
+	unsigned long tasks_pid;
+	char tasks[PATH_MAX];
+	int ret = 0;
+	FILE *fp;
+
+	if (grp)
+		snprintf(tasks, sizeof(tasks), "%s/%s/tasks", RESCTRL_PATH, grp);
+	else
+		snprintf(tasks, sizeof(tasks), "%s/tasks", RESCTRL_PATH);
+
+	fp = fopen(tasks, "r");
+	if (!fp) {
+		ksft_print_msg("Error opening %s: %m\n", tasks);
+		return -EIO;
+	}
+	while (1) {
+		ret = fscanf(fp, "%lu", &tasks_pid);
+		if (ret == EOF) {
+			if (errno) {
+				ksft_print_msg("Error reading %s: %m\n", tasks);
+				ret = -EIO;
+			} else {
+				ret = 0;
+			}
+			break;
+		}
+		if (!ret)
+			break;
+
+		if (tasks_pid == bm_pid) {
+			ret = 1;
+			break;
+		}
+	}
+
+	fclose(fp);
+	return ret;
+}
+
 static int write_pid_to_tasks(char *tasks, pid_t pid)
 {
 	FILE *fp;
-- 
2.39.5

[RFC PATCH v1 2/2] kselftest/resctrl: Add L3 CAT CBM functional test for Intel RDT

[Ilpo Järvinen]

Resctrl CAT selftests have been limited to mainly testing performance.
In order to validate the kernel side behavior better, add a functional
test that checks the MSR contents (if MSRs are accessible). As the
low-level mapping is architecture specific, this test is currently
limited to testing resctrl only on Intel CPUs.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 tools/testing/selftests/resctrl/cat_test.c    | 126 ++++++++++++++++++
 tools/testing/selftests/resctrl/msr.c         |  55 ++++++++
 tools/testing/selftests/resctrl/resctrl.h     |   4 +
 .../testing/selftests/resctrl/resctrl_tests.c |   1 +
 4 files changed, 186 insertions(+)
 create mode 100644 tools/testing/selftests/resctrl/msr.c

diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
index 78cb9ac90bb1..fbe1d2f7657f 100644
--- a/tools/testing/selftests/resctrl/cat_test.c
+++ b/tools/testing/selftests/resctrl/cat_test.c
@@ -9,8 +9,15 @@
  *    Fenghua Yu <fenghua.yu@intel.com>
  */
 #include "resctrl.h"
+
+#include <string.h>
 #include <unistd.h>
 
+#define MSR_IA32_PQR_ASSOC	0xc8f
+#define  MSR_IA32_PQR_ASSOC_COS	0xffffffff00000000ULL
+
+#define MSR_IA32_L3_CBM_BASE	0xc90
+
 #define RESULT_FILE_NAME	"result_cat"
 #define NUM_OF_RUNS		5
 
@@ -452,6 +459,116 @@ static int cat_ctrlgrp_tasks_test(const struct resctrl_test *test,
 	return ret;
 }
 
+static int cat_ctrlgrp_check_msr(const struct user_params *uparams,
+				 unsigned long mask)
+{
+	__u64 msr_val;
+	__u32 clos;
+	int ret;
+
+	ret = read_msr(uparams->cpu, MSR_IA32_PQR_ASSOC, &msr_val);
+	if (ret)
+		return -1;
+
+	clos = (msr_val & MSR_IA32_PQR_ASSOC_COS) >> (ffsl(MSR_IA32_PQR_ASSOC_COS) - 1);
+
+	ret = read_msr(uparams->cpu, MSR_IA32_L3_CBM_BASE + clos, &msr_val);
+	if (ret)
+		return -1;
+
+	if (msr_val != mask) {
+		ksft_print_msg("Incorrect CBM mask %llx, should be %lx\n",
+			       msr_val, mask);
+		return -1;
+	}
+
+	return 0;
+}
+
+static int cat_ctrlgrp_msr_test(const struct resctrl_test *test,
+				const struct user_params *uparams)
+{
+	unsigned long mask1 = 0x3, mask2 = 0x6, mask3 = 0x0c;
+	cpu_set_t old_affinity;
+	char schemata[64];
+	pid_t bm_pid;
+	int ret;
+
+	bm_pid = getpid();
+
+	if (!msr_access_supported(uparams->cpu)) {
+		ksft_test_result_skip("Cannot access MSRs\n");
+		return 0;
+	}
+
+	ret = resctrl_grp_has_task(NULL, bm_pid);
+	if (ret < 0)
+		return ret;
+	if (!ret) {
+		ksft_print_msg("PID not found in the root group\n");
+		return 1;
+	}
+
+	/* Taskset benchmark to specified CPU */
+	ksft_print_msg("Placing task to ctrlgrp 'c1'\n");
+	ret = taskset_benchmark(bm_pid, uparams->cpu, &old_affinity);
+	if (ret)
+		return ret;
+	ret = write_bm_pid_to_resctrl(bm_pid, "c1", NULL);
+	if (ret)
+		goto reset_affinity;
+	snprintf(schemata, sizeof(schemata), "%lx", mask1);
+	ret = write_schemata("c1", schemata, uparams->cpu, test->resource);
+	if (ret)
+		goto reset_affinity;
+	ret = cat_ctrlgrp_check_msr(uparams, mask1);
+	if (ret)
+		goto reset_affinity;
+
+	ksft_print_msg("Placing task to ctrlgrp 'c2'\n");
+	ret = write_bm_pid_to_resctrl(bm_pid, "c2", NULL);
+	if (ret)
+		goto reset_affinity;
+	snprintf(schemata, sizeof(schemata), "%lx", mask2);
+	ret = write_schemata("c2", schemata, uparams->cpu, test->resource);
+	if (ret)
+		goto reset_affinity;
+	ret = cat_ctrlgrp_check_msr(uparams, mask2);
+	if (ret)
+		goto reset_affinity;
+
+	ksft_print_msg("Adjusting CBM for unrelated ctrlgrp 'c1'\n");
+	snprintf(schemata, sizeof(schemata), "%lx", mask3);
+	ret = write_schemata("c1", schemata, uparams->cpu, test->resource);
+	if (ret)
+		goto reset_affinity;
+	ret = cat_ctrlgrp_check_msr(uparams, mask2);
+	if (ret)
+		goto reset_affinity;
+
+	ksft_print_msg("Adjusting CBM for ctrlgrp 'c2'\n");
+	snprintf(schemata, sizeof(schemata), "%lx", mask1);
+	ret = write_schemata("c2", schemata, uparams->cpu, test->resource);
+	if (ret)
+		goto reset_affinity;
+	ret = cat_ctrlgrp_check_msr(uparams, mask1);
+	if (ret)
+		goto reset_affinity;
+
+	ksft_print_msg("Placing task to ctrlgrp 'c1'\n");
+	ret = write_bm_pid_to_resctrl(bm_pid, "c1", NULL);
+	if (ret)
+		goto reset_affinity;
+	ret = cat_ctrlgrp_check_msr(uparams, mask3);
+	if (ret)
+		goto reset_affinity;
+
+reset_affinity:
+	taskset_restore(bm_pid, &old_affinity);
+
+	return ret;
+}
+
 struct resctrl_test l3_cat_test = {
 	.name = "L3_CAT",
 	.group = "CAT",
@@ -484,3 +601,12 @@ struct resctrl_test cat_grp_tasks_test = {
 	.feature_check = test_resource_feature_check,
 	.run_test = cat_ctrlgrp_tasks_test,
 };
+
+struct resctrl_test cat_grp_mask_test = {
+	.name = "CAT_GROUP_MASK",
+	.group = "CAT",
+	.resource = "L3",
+	.vendor_specific = ARCH_INTEL,
+	.feature_check = test_resource_feature_check,
+	.run_test = cat_ctrlgrp_msr_test,
+};
diff --git a/tools/testing/selftests/resctrl/msr.c b/tools/testing/selftests/resctrl/msr.c
new file mode 100644
index 000000000000..1e8674036c7d
--- /dev/null
+++ b/tools/testing/selftests/resctrl/msr.c
@@ -0,0 +1,55 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <fcntl.h>
+#include <limits.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <unistd.h>
+
+#include <linux/types.h>
+
+#include "resctrl.h"
+
+#define MSR_PATH	"/dev/cpu/%d/msr"
+
+static int open_msr_file(int cpu)
+{
+	char msr_path[PATH_MAX];
+
+	snprintf(msr_path, sizeof(msr_path), MSR_PATH, cpu);
+
+	return open(msr_path, O_RDONLY);
+}
+
+int read_msr(int cpu, __u32 msr, __u64 *val)
+{
+	ssize_t res;
+	int fd;
+
+	fd = open_msr_file(cpu);
+	if (fd < 0) {
+		ksft_print_msg("Failed to open msr file for CPU %d\n", cpu);
+		return -1;
+	}
+
+	res = pread(fd, val, sizeof(*val), msr);
+	close(fd);
+	if (res < sizeof(*val)) {
+		ksft_print_msg("Reading MSR failed\n");
+		return -1;
+	}
+
+	return 0;
+}
+
+bool msr_access_supported(int cpu)
+{
+	int fd;
+
+	fd = open_msr_file(cpu);
+	if (fd < 0)
+		return false;
+
+	close(fd);
+	return true;
+}
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
index d25f83d0a54d..909a101f0e4c 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -243,8 +243,12 @@ extern struct resctrl_test mbm_test;
 extern struct resctrl_test mba_test;
 extern struct resctrl_test cmt_test;
 extern struct resctrl_test cat_grp_tasks_test;
+extern struct resctrl_test cat_grp_mask_test;
 extern struct resctrl_test l3_cat_test;
 extern struct resctrl_test l3_noncont_cat_test;
 extern struct resctrl_test l2_noncont_cat_test;
 
+bool msr_access_supported(int cpu);
+int read_msr(int cpu, __u32 msr, __u64 *val);
+
 #endif /* RESCTRL_H */
diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
index 71b7cd846dc1..08ae48165c7a 100644
--- a/tools/testing/selftests/resctrl/resctrl_tests.c
+++ b/tools/testing/selftests/resctrl/resctrl_tests.c
@@ -19,6 +19,7 @@ static struct resctrl_test *resctrl_tests[] = {
 	&mba_test,
 	&cmt_test,
 	&cat_grp_tasks_test,
+	&cat_grp_mask_test,
 	&l3_cat_test,
 	&l3_noncont_cat_test,
 	&l2_noncont_cat_test,
-- 
2.39.5

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

[Reinette Chatre]

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

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

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 5786 bytes --]

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.

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

[Reinette Chatre]

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