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
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;
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,
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
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.
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
linux-kselftest-mirror@lists.linaro.org
-
Ilpo Järvinen -
Reinette Chatre