Fix four issues with resctrl selftests.
The signal handling fix became necessary after the mount/umount fixes
and the uninitialized member bug was discovered during the review.
The other two came up when I ran resctrl selftests across the server
fleet in our lab to validate the upcoming CAT test rewrite (the rewrite
is not part of this series).
These are developed and should apply cleanly at least on top the
benchmark cleanup series (might apply cleanly also w/o the benchmark
series, I didn't test).
v4:
- Use func(void) for functions taking no arguments
- Correct Fixes tag formatting
v3:
- Add fix to uninitialized sa_flags
- Handle ksft_exit_fail_msg() in per test functions
- Make signal handler register fails to also exit
- Improve changelogs
v2:
- Include patch to move _GNU_SOURCE to Makefile to allow normal #include
placement
- Rework the signal register/unregister into patch to use helpers
- Fixed incorrect function parameter description
- Use return !!res to avoid confusing implicit boolean conversion
- Improve MBA/MBM success bound patch's changelog
- Tweak Cc: stable dependencies (make it a chain).
Ilpo Järvinen (7):
selftests/resctrl: Fix uninitialized .sa_flags
selftests/resctrl: Extend signal handler coverage to unmount on
receiving signal
selftests/resctrl: Remove duplicate feature check from CMT test
selftests/resctrl: Move _GNU_SOURCE define into Makefile
selftests/resctrl: Refactor feature check to use resource and feature
name
selftests/resctrl: Fix feature checks
selftests/resctrl: Reduce failures due to outliers in MBA/MBM tests
tools/testing/selftests/resctrl/Makefile | 2 +-
tools/testing/selftests/resctrl/cat_test.c | 8 --
tools/testing/selftests/resctrl/cmt_test.c | 3 -
tools/testing/selftests/resctrl/mba_test.c | 2 +-
tools/testing/selftests/resctrl/mbm_test.c | 2 +-
tools/testing/selftests/resctrl/resctrl.h | 7 +-
.../testing/selftests/resctrl/resctrl_tests.c | 82 ++++++++++++-------
tools/testing/selftests/resctrl/resctrl_val.c | 26 +++---
tools/testing/selftests/resctrl/resctrlfs.c | 69 ++++++----------
9 files changed, 97 insertions(+), 104 deletions(-)
--
2.30.2
The test runner run_cmt_test() in resctrl_tests.c checks for CMT
feature and does not run cmt_resctrl_val() if CMT is not supported.
Then cmt_resctrl_val() also check is CMT is supported.
Remove the duplicated feature check for CMT from cmt_resctrl_val().
Signed-off-by: Ilpo Järvinen <[email protected]>
Tested-by: Shaopeng Tan <[email protected]>
Reviewed-by: Reinette Chatre <[email protected]>
Reviewed-by: Shaopeng Tan <[email protected]>
Cc: <[email protected]>
---
tools/testing/selftests/resctrl/cmt_test.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
index cf2f5e92dea6..50bdbce9fba9 100644
--- a/tools/testing/selftests/resctrl/cmt_test.c
+++ b/tools/testing/selftests/resctrl/cmt_test.c
@@ -80,9 +80,6 @@ int cmt_resctrl_val(int cpu_no, int n, const char * const *benchmark_cmd)
size_t span;
int ret, i;
- if (!validate_resctrl_feature_request(CMT_STR))
- return -1;
-
ret = get_cbm_mask("L3", cbm_mask);
if (ret)
return ret;
--
2.30.2
signal_handler_unregister() calls sigaction() with uninitializing
sa_flags in the struct sigaction.
Make sure sa_flags is always initialized in signal_handler_unregister()
by initializing the struct sigaction when declaring it. Also add the
initialization to signal_handler_register() even if there are no know
bugs in there because correctness is then obvious from the code itself.
Fixes: 73c55fa5ab55 ("selftests/resctrl: Commonize the signal handler register/unregister for all tests")
Suggested-by: Reinette Chatre <[email protected]>
Signed-off-by: Ilpo Järvinen <[email protected]>
Cc: <[email protected]>
---
tools/testing/selftests/resctrl/resctrl_val.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
index 51963a6f2186..01bbe11a8983 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -482,7 +482,7 @@ void ctrlc_handler(int signum, siginfo_t *info, void *ptr)
*/
int signal_handler_register(void)
{
- struct sigaction sigact;
+ struct sigaction sigact = {};
int ret = 0;
sigact.sa_sigaction = ctrlc_handler;
@@ -504,7 +504,7 @@ int signal_handler_register(void)
*/
void signal_handler_unregister(void)
{
- struct sigaction sigact;
+ struct sigaction sigact = {};
sigact.sa_handler = SIG_DFL;
sigemptyset(&sigact.sa_mask);
--
2.30.2
Unmounting resctrl FS has been moved into the per test functions in
resctrl_tests.c by commit caddc0fbe495 ("selftests/resctrl: Move
resctrl FS mount/umount to higher level"). In case a signal (SIGINT,
SIGTERM, or SIGHUP) is received, the running selftest is aborted by
ctrlc_handler() which then unmounts resctrl fs before exiting. The
current section between signal_handler_register() and
signal_handler_unregister(), however, does not cover the entire
duration when resctrl FS is mounted.
Move signal_handler_register() and signal_handler_unregister() calls
from per test files into resctrl_tests.c to properly unmount resctrl
fs. In order to not add signal_handler_register()/unregister() n times,
create helpers test_prepare() and test_cleanup().
Do not call ksft_exit_fail_msg() in test_prepare() but only in the per
test function to keep the control flow cleaner without adding calls to
exit() deep into the call chain.
Adjust child process kill() call in ctrlc_handler() to only be invoked
if the child was already forked.
Fixes: caddc0fbe495 ("selftests/resctrl: Move resctrl FS mount/umount to higher level")
Signed-off-by: Ilpo Järvinen <[email protected]>
Tested-by: Shaopeng Tan <[email protected]>
Reviewed-by: Shaopeng Tan <[email protected]>
Reviewed-by: Reinette Chatre <[email protected]>
Cc: <[email protected]>
---
tools/testing/selftests/resctrl/cat_test.c | 8 ---
.../testing/selftests/resctrl/resctrl_tests.c | 69 ++++++++++++-------
tools/testing/selftests/resctrl/resctrl_val.c | 22 +++---
3 files changed, 55 insertions(+), 44 deletions(-)
diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
index 97b87285ab2a..224ba8544d8a 100644
--- a/tools/testing/selftests/resctrl/cat_test.c
+++ b/tools/testing/selftests/resctrl/cat_test.c
@@ -167,12 +167,6 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
strcpy(param.filename, RESULT_FILE_NAME1);
param.num_of_runs = 0;
param.cpu_no = sibling_cpu_no;
- } else {
- ret = signal_handler_register();
- if (ret) {
- kill(bm_pid, SIGKILL);
- goto out;
- }
}
remove(param.filename);
@@ -209,10 +203,8 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
}
close(pipefd[0]);
kill(bm_pid, SIGKILL);
- signal_handler_unregister();
}
-out:
cat_test_cleanup();
return ret;
diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
index 823672a20a43..495aeee5b734 100644
--- a/tools/testing/selftests/resctrl/resctrl_tests.c
+++ b/tools/testing/selftests/resctrl/resctrl_tests.c
@@ -67,21 +67,45 @@ void tests_cleanup(void)
cat_test_cleanup();
}
-static void run_mbm_test(const char * const *benchmark_cmd, int cpu_no)
+static int test_prepare(void)
{
int res;
- ksft_print_msg("Starting MBM BW change ...\n");
+ res = signal_handler_register();
+ if (res) {
+ ksft_print_msg("Failed to register signal handler\n");
+ return res;
+ }
res = mount_resctrlfs();
if (res) {
- ksft_exit_fail_msg("Failed to mount resctrl FS\n");
+ signal_handler_unregister();
+ ksft_print_msg("Failed to mount resctrl FS\n");
+ return res;
+ }
+ return 0;
+}
+
+static void test_cleanup(void)
+{
+ umount_resctrlfs();
+ signal_handler_unregister();
+}
+
+static void run_mbm_test(const char * const *benchmark_cmd, int cpu_no)
+{
+ int res;
+
+ ksft_print_msg("Starting MBM BW change ...\n");
+
+ if (test_prepare()) {
+ ksft_exit_fail_msg("Abnormal failure when preparing for the test\n");
return;
}
if (!validate_resctrl_feature_request(MBM_STR) || (get_vendor() != ARCH_INTEL)) {
ksft_test_result_skip("Hardware does not support MBM or MBM is disabled\n");
- goto umount;
+ goto cleanup;
}
res = mbm_bw_change(cpu_no, benchmark_cmd);
@@ -89,8 +113,8 @@ static void run_mbm_test(const char * const *benchmark_cmd, int cpu_no)
if ((get_vendor() == ARCH_INTEL) && res)
ksft_print_msg("Intel MBM may be inaccurate when Sub-NUMA Clustering is enabled. Check BIOS configuration.\n");
-umount:
- umount_resctrlfs();
+cleanup:
+ test_cleanup();
}
static void run_mba_test(const char * const *benchmark_cmd, int cpu_no)
@@ -99,22 +123,21 @@ static void run_mba_test(const char * const *benchmark_cmd, int cpu_no)
ksft_print_msg("Starting MBA Schemata change ...\n");
- res = mount_resctrlfs();
- if (res) {
- ksft_exit_fail_msg("Failed to mount resctrl FS\n");
+ if (test_prepare()) {
+ ksft_exit_fail_msg("Abnormal failure when preparing for the test\n");
return;
}
if (!validate_resctrl_feature_request(MBA_STR) || (get_vendor() != ARCH_INTEL)) {
ksft_test_result_skip("Hardware does not support MBA or MBA is disabled\n");
- goto umount;
+ goto cleanup;
}
res = mba_schemata_change(cpu_no, benchmark_cmd);
ksft_test_result(!res, "MBA: schemata change\n");
-umount:
- umount_resctrlfs();
+cleanup:
+ test_cleanup();
}
static void run_cmt_test(const char * const *benchmark_cmd, int cpu_no)
@@ -123,15 +146,14 @@ static void run_cmt_test(const char * const *benchmark_cmd, int cpu_no)
ksft_print_msg("Starting CMT test ...\n");
- res = mount_resctrlfs();
- if (res) {
- ksft_exit_fail_msg("Failed to mount resctrl FS\n");
+ if (test_prepare()) {
+ ksft_exit_fail_msg("Abnormal failure when preparing for the test\n");
return;
}
if (!validate_resctrl_feature_request(CMT_STR)) {
ksft_test_result_skip("Hardware does not support CMT or CMT is disabled\n");
- goto umount;
+ goto cleanup;
}
res = cmt_resctrl_val(cpu_no, 5, benchmark_cmd);
@@ -139,8 +161,8 @@ static void run_cmt_test(const char * const *benchmark_cmd, int cpu_no)
if ((get_vendor() == ARCH_INTEL) && res)
ksft_print_msg("Intel CMT may be inaccurate when Sub-NUMA Clustering is enabled. Check BIOS configuration.\n");
-umount:
- umount_resctrlfs();
+cleanup:
+ test_cleanup();
}
static void run_cat_test(int cpu_no, int no_of_bits)
@@ -149,22 +171,21 @@ static void run_cat_test(int cpu_no, int no_of_bits)
ksft_print_msg("Starting CAT test ...\n");
- res = mount_resctrlfs();
- if (res) {
- ksft_exit_fail_msg("Failed to mount resctrl FS\n");
+ if (test_prepare()) {
+ ksft_exit_fail_msg("Abnormal failure when preparing for the test\n");
return;
}
if (!validate_resctrl_feature_request(CAT_STR)) {
ksft_test_result_skip("Hardware does not support CAT or CAT is disabled\n");
- goto umount;
+ goto cleanup;
}
res = cat_perf_miss_val(cpu_no, no_of_bits, "L3");
ksft_test_result(!res, "CAT: test\n");
-umount:
- umount_resctrlfs();
+cleanup:
+ test_cleanup();
}
int main(int argc, char **argv)
diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
index 01bbe11a8983..b8ca6fa40b3b 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -468,7 +468,9 @@ pid_t bm_pid, ppid;
void ctrlc_handler(int signum, siginfo_t *info, void *ptr)
{
- kill(bm_pid, SIGKILL);
+ /* Only kill child after bm_pid is set after fork() */
+ if (bm_pid)
+ kill(bm_pid, SIGKILL);
umount_resctrlfs();
tests_cleanup();
ksft_print_msg("Ending\n\n");
@@ -485,6 +487,8 @@ int signal_handler_register(void)
struct sigaction sigact = {};
int ret = 0;
+ bm_pid = 0;
+
sigact.sa_sigaction = ctrlc_handler;
sigemptyset(&sigact.sa_mask);
sigact.sa_flags = SA_SIGINFO;
@@ -706,10 +710,6 @@ int resctrl_val(const char * const *benchmark_cmd, struct resctrl_val_param *par
ksft_print_msg("Benchmark PID: %d\n", bm_pid);
- ret = signal_handler_register();
- if (ret)
- goto out;
-
/*
* The cast removes constness but nothing mutates benchmark_cmd within
* the context of this process. At the receiving process, it becomes
@@ -721,19 +721,19 @@ int resctrl_val(const char * const *benchmark_cmd, struct resctrl_val_param *par
/* Taskset benchmark to specified cpu */
ret = taskset_benchmark(bm_pid, param->cpu_no);
if (ret)
- goto unregister;
+ goto out;
/* Write benchmark to specified control&monitoring grp in resctrl FS */
ret = write_bm_pid_to_resctrl(bm_pid, param->ctrlgrp, param->mongrp,
resctrl_val);
if (ret)
- goto unregister;
+ goto out;
if (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) ||
!strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR))) {
ret = initialize_mem_bw_imc();
if (ret)
- goto unregister;
+ goto out;
initialize_mem_bw_resctrl(param->ctrlgrp, param->mongrp,
param->cpu_no, resctrl_val);
@@ -748,7 +748,7 @@ int resctrl_val(const char * const *benchmark_cmd, struct resctrl_val_param *par
sizeof(pipe_message)) {
perror("# failed reading message from child process");
close(pipefd[0]);
- goto unregister;
+ goto out;
}
}
close(pipefd[0]);
@@ -757,7 +757,7 @@ int resctrl_val(const char * const *benchmark_cmd, struct resctrl_val_param *par
if (sigqueue(bm_pid, SIGUSR1, value) == -1) {
perror("# sigqueue SIGUSR1 to child");
ret = errno;
- goto unregister;
+ goto out;
}
/* Give benchmark enough time to fully run */
@@ -786,8 +786,6 @@ int resctrl_val(const char * const *benchmark_cmd, struct resctrl_val_param *par
}
}
-unregister:
- signal_handler_unregister();
out:
kill(bm_pid, SIGKILL);
--
2.30.2
On Mon, 2 Oct 2023, Ilpo Järvinen wrote:
> Fix four issues with resctrl selftests.
>
> The signal handling fix became necessary after the mount/umount fixes
> and the uninitialized member bug was discovered during the review.
>
> The other two came up when I ran resctrl selftests across the server
> fleet in our lab to validate the upcoming CAT test rewrite (the rewrite
> is not part of this series).
>
> These are developed and should apply cleanly at least on top the
> benchmark cleanup series (might apply cleanly also w/o the benchmark
> series, I didn't test).
LKP seems to no longer happy to apply this cleanly without the benchmark
rework series as the signal handling fix got now a bigger footprint
(maybe LKP didn't build v3 at all as the changes from v3->v4 were really
small and I did not get error out of v3).
Shuah, would you want me to reorganize this such that it goes in before
the benchmark series? In any case, I'll wait until Reinette has had time
to look at the first patch if I'm to send the series reordered.
--
i.
On 10/2/2023 4:31 AM, Ilpo Järvinen wrote:
> On Mon, 2 Oct 2023, Ilpo Järvinen wrote:
>
>> Fix four issues with resctrl selftests.
>>
>> The signal handling fix became necessary after the mount/umount fixes
>> and the uninitialized member bug was discovered during the review.
>>
>> The other two came up when I ran resctrl selftests across the server
>> fleet in our lab to validate the upcoming CAT test rewrite (the rewrite
>> is not part of this series).
>>
>> These are developed and should apply cleanly at least on top the
>> benchmark cleanup series (might apply cleanly also w/o the benchmark
>> series, I didn't test).
>
> LKP seems to no longer happy to apply this cleanly without the benchmark
> rework series as the signal handling fix got now a bigger footprint
> (maybe LKP didn't build v3 at all as the changes from v3->v4 were really
> small and I did not get error out of v3).
>
> Shuah, would you want me to reorganize this such that it goes in before
> the benchmark series? In any case, I'll wait until Reinette has had time
> to look at the first patch if I'm to send the series reordered.
That sounds unnecessary to me because I assume that doing such reorganization
would require a new version of the benchmark series [1] that has been ready for
a while now.
Both series look good to me. I just added my "Reviewed-by" to the first patch of
this series and it (this series) applies cleanly on top of the benchmark series.
Reinette
[1] https://lore.kernel.org/lkml/[email protected]/
The MBA and CMT tests expect support of other features to be able to
run.
When platform only supports MBA but not MBM, MBA test will fail with:
Failed to open total bw file: No such file or directory
When platform only supports CMT but not CAT, CMT test will fail with:
Failed to open bit mask file '/sys/fs/resctrl/info/L3/cbm_mask': No such file or directory
It leads to the test reporting test fail (even if no test was run at
all).
Extend feature checks to cover these two conditions to show these tests
were skipped rather than failed.
Fixes: ee0415681eb6 ("selftests/resctrl: Use resctrl/info for feature detection")
Signed-off-by: Ilpo Järvinen <[email protected]>
Tested-by: Shaopeng Tan <[email protected]>
Reviewed-by: Reinette Chatre <[email protected]>
Reviewed-by: Shaopeng Tan <[email protected]>
Cc: <[email protected]> # selftests/resctrl: Refactor feature check to use resource and feature name
---
tools/testing/selftests/resctrl/resctrl_tests.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
index a19dcc3f8fb0..2bbe3045a018 100644
--- a/tools/testing/selftests/resctrl/resctrl_tests.c
+++ b/tools/testing/selftests/resctrl/resctrl_tests.c
@@ -130,7 +130,9 @@ static void run_mba_test(const char * const *benchmark_cmd, int cpu_no)
return;
}
- if (!validate_resctrl_feature_request("MB", NULL) || (get_vendor() != ARCH_INTEL)) {
+ if (!validate_resctrl_feature_request("MB", NULL) ||
+ !validate_resctrl_feature_request("L3_MON", "mbm_local_bytes") ||
+ (get_vendor() != ARCH_INTEL)) {
ksft_test_result_skip("Hardware does not support MBA or MBA is disabled\n");
goto cleanup;
}
@@ -153,7 +155,8 @@ static void run_cmt_test(const char * const *benchmark_cmd, int cpu_no)
return;
}
- if (!validate_resctrl_feature_request("L3_MON", "llc_occupancy")) {
+ if (!validate_resctrl_feature_request("L3_MON", "llc_occupancy") ||
+ !validate_resctrl_feature_request("L3", NULL)) {
ksft_test_result_skip("Hardware does not support CMT or CMT is disabled\n");
goto cleanup;
}
--
2.30.2
_GNU_SOURCE is defined in resctrl.h. Defining _GNU_SOURCE has a large
impact on what gets defined when including headers either before or
after it. This can result in compile failures if .c file decides to
include a standard header file before resctrl.h.
It is safer to define _GNU_SOURCE in Makefile so it is always defined
regardless of in which order includes are done.
Signed-off-by: Ilpo Järvinen <[email protected]>
Tested-by: Shaopeng Tan <[email protected]>
Reviewed-by: Reinette Chatre <[email protected]>
Reviewed-by: Shaopeng Tan <[email protected]>
Cc: <[email protected]>
---
tools/testing/selftests/resctrl/Makefile | 2 +-
tools/testing/selftests/resctrl/resctrl.h | 1 -
2 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/tools/testing/selftests/resctrl/Makefile b/tools/testing/selftests/resctrl/Makefile
index 5073dbc96125..2deac2031de9 100644
--- a/tools/testing/selftests/resctrl/Makefile
+++ b/tools/testing/selftests/resctrl/Makefile
@@ -1,6 +1,6 @@
# SPDX-License-Identifier: GPL-2.0
-CFLAGS = -g -Wall -O2 -D_FORTIFY_SOURCE=2
+CFLAGS = -g -Wall -O2 -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE
CFLAGS += $(KHDR_INCLUDES)
TEST_GEN_PROGS := resctrl_tests
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
index dd07463cdf48..d9b5df95849d 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -1,5 +1,4 @@
/* SPDX-License-Identifier: GPL-2.0 */
-#define _GNU_SOURCE
#ifndef RESCTRL_H
#define RESCTRL_H
#include <stdio.h>
--
2.30.2
Hi Ilpo,
On 10/2/2023 2:48 AM, Ilpo Järvinen wrote:
> signal_handler_unregister() calls sigaction() with uninitializing
> sa_flags in the struct sigaction.
>
> Make sure sa_flags is always initialized in signal_handler_unregister()
> by initializing the struct sigaction when declaring it. Also add the
> initialization to signal_handler_register() even if there are no know
> bugs in there because correctness is then obvious from the code itself.
>
> Fixes: 73c55fa5ab55 ("selftests/resctrl: Commonize the signal handler register/unregister for all tests")
> Suggested-by: Reinette Chatre <[email protected]>
> Signed-off-by: Ilpo Järvinen <[email protected]>
> Cc: <[email protected]>
Thank you.
Reviewed-by: Reinette Chatre <[email protected]>
Reinette
Hi Ilpo,
I ran the test set on Intel(R) Xeon(R) Gold 6254 CPU.
There is no problem.
Reviewed-by: Shaopeng Tan <[email protected]>
Tested-by: Shaopeng Tan <[email protected]>
Best regards,
Shaopeng TAN