Hello,
The aim of this patch series is to improve the resctrl selftest.
Without these fixes, some unnecessary processing will be executed
and test results will be confusing.
There is no behavior change in test themselves.
[patch 1] Make write_schemata() run to set up shemata with 100% allocation
on first run in MBM test.
[patch 2] The MBA test result message is always output as "ok",
make output message to be "not ok" if MBA check result is failed.
[patch 3] When a child process is created by fork(), the buffer of the
parent process is also copied. Flush the buffer before
executing fork().
[patch 4] Add a signal handler to cleanup properly before exiting the
parent process if there is an error occurs after creating
a child process with fork() in the CAT test, and unregister
signal handler when each test finished.
[patch 5] Before exiting each test CMT/CAT/MBM/MBA, clear test result
files function cat/cmt/mbm/mba_test_cleanup() are called
twice. Delete once.
This patch series is based on Linux v6.2-rc3.
Difference from v4:
[patch 4]
- Reuse signal handler of other tests(MBM/MBA/CAT).
- Unregister signal handler when tests finished.
- Fix change log.
Pervious versions of this series:
[v1] https://lore.kernel.org/lkml/[email protected]/
[v2] https://lore.kernel.org/lkml/[email protected]/
[v3] https://lore.kernel.org/lkml/[email protected]/
[v4] https://lore.kernel.org/lkml/[email protected]/
Shaopeng Tan (5):
selftests/resctrl: Fix set up schemata with 100% allocation on first
run in MBM test
selftests/resctrl: Return MBA check result and make it to output
message
selftests/resctrl: Flush stdout file buffer before executing fork()
selftests/resctrl: Cleanup properly when an error occurs in CAT test
selftests/resctrl: Remove duplicate codes that clear each test result
file
tools/testing/selftests/resctrl/cat_test.c | 27 +++++----
tools/testing/selftests/resctrl/cmt_test.c | 7 +--
tools/testing/selftests/resctrl/fill_buf.c | 14 -----
tools/testing/selftests/resctrl/mba_test.c | 23 ++++----
tools/testing/selftests/resctrl/mbm_test.c | 20 +++----
tools/testing/selftests/resctrl/resctrl.h | 2 +
.../testing/selftests/resctrl/resctrl_tests.c | 4 --
tools/testing/selftests/resctrl/resctrl_val.c | 57 ++++++++++++++-----
tools/testing/selftests/resctrl/resctrlfs.c | 5 +-
9 files changed, 89 insertions(+), 70 deletions(-)
--
2.27.0
After creating a child process with fork() in CAT test, if there is an
error occurs or a signal such as SIGINT is received, the parent process
will be terminated immediately, and therefor the child process will not
be killed and also resctrlfs is not unmounted.
There is a signal handler registered in CMT/MBM/MBA tests, which kills
child process, unmount resctrlfs, cleanups result files, etc., if a
signal such as SIGINT is received.
Commonize the signal handler registered for CMT/MBM/MBA tests and reuse
it in CAT too.
To reuse the signal handler, make the child process in CAT wait to be
killed by parent process in any case (an error occurred or a signal was
received), and when killing child process use global bm_pid instead of
local bm_pid.
Also, since the MBA/MBA/CMT/CAT are run in order, unregister the signal
handler at the end of each test so that the signal handler cannot be
inherited by other tests.
Signed-off-by: Shaopeng Tan <[email protected]>
---
tools/testing/selftests/resctrl/cat_test.c | 26 +++++----
tools/testing/selftests/resctrl/fill_buf.c | 14 -----
tools/testing/selftests/resctrl/resctrl.h | 2 +
tools/testing/selftests/resctrl/resctrl_val.c | 56 ++++++++++++++-----
4 files changed, 59 insertions(+), 39 deletions(-)
diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
index 6a8306b0a109..87302b882929 100644
--- a/tools/testing/selftests/resctrl/cat_test.c
+++ b/tools/testing/selftests/resctrl/cat_test.c
@@ -103,7 +103,6 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
unsigned long l_mask, l_mask_1;
int ret, pipefd[2], sibling_cpu_no;
char pipe_message;
- pid_t bm_pid;
cache_size = 0;
@@ -181,28 +180,29 @@ 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)
+ goto out;
}
remove(param.filename);
ret = cat_val(¶m);
- if (ret)
- return ret;
-
- ret = check_results(¶m);
- if (ret)
- return ret;
+ if (ret == 0)
+ ret = check_results(¶m);
if (bm_pid == 0) {
/* Tell parent that child is ready */
close(pipefd[0]);
pipe_message = 1;
if (write(pipefd[1], &pipe_message, sizeof(pipe_message)) <
- sizeof(pipe_message)) {
- close(pipefd[1]);
+ sizeof(pipe_message))
+ /*
+ * Just print the error message.
+ * Let while(1) run and wait for itself to be killed.
+ */
perror("# failed signaling parent process");
- return errno;
- }
close(pipefd[1]);
while (1)
@@ -226,5 +226,7 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
if (bm_pid)
umount_resctrlfs();
- return 0;
+out:
+ ret = signal_handler_unregister();
+ return ret;
}
diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c
index 56ccbeae0638..322c6812e15c 100644
--- a/tools/testing/selftests/resctrl/fill_buf.c
+++ b/tools/testing/selftests/resctrl/fill_buf.c
@@ -33,14 +33,6 @@ static void sb(void)
#endif
}
-static void ctrl_handler(int signo)
-{
- free(startptr);
- printf("\nEnding\n");
- sb();
- exit(EXIT_SUCCESS);
-}
-
static void cl_flush(void *p)
{
#if defined(__i386) || defined(__x86_64)
@@ -198,12 +190,6 @@ int run_fill_buf(unsigned long span, int malloc_and_init_memory,
unsigned long long cache_size = span;
int ret;
- /* set up ctrl-c handler */
- if (signal(SIGINT, ctrl_handler) == SIG_ERR)
- printf("Failed to catch SIGINT!\n");
- if (signal(SIGHUP, ctrl_handler) == SIG_ERR)
- printf("Failed to catch SIGHUP!\n");
-
ret = fill_cache(cache_size, malloc_and_init_memory, memflush, op,
resctrl_val);
if (ret) {
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
index f0ded31fb3c7..14a5e21497e1 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -107,6 +107,8 @@ void mba_test_cleanup(void);
int get_cbm_mask(char *cache_type, char *cbm_mask);
int get_cache_size(int cpu_no, char *cache_type, unsigned long *cache_size);
void ctrlc_handler(int signum, siginfo_t *info, void *ptr);
+int signal_handler_register(void);
+int signal_handler_unregister(void);
int cat_val(struct resctrl_val_param *param);
void cat_test_cleanup(void);
int cat_perf_miss_val(int cpu_no, int no_of_bits, char *cache_type);
diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
index 6948843bf995..91a3cf8b308b 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -476,6 +476,46 @@ void ctrlc_handler(int signum, siginfo_t *info, void *ptr)
exit(EXIT_SUCCESS);
}
+struct sigaction sigact;
+
+/*
+ * Register CTRL-C handler for parent, as it has to kill
+ * child process before exiting
+ */
+int signal_handler_register(void)
+{
+ int ret = 0;
+ struct sigaction sigact;
+
+ sigact.sa_sigaction = ctrlc_handler;
+ sigemptyset(&sigact.sa_mask);
+ sigact.sa_flags = SA_SIGINFO;
+ if (sigaction(SIGINT, &sigact, NULL) ||
+ sigaction(SIGTERM, &sigact, NULL) ||
+ sigaction(SIGHUP, &sigact, NULL)) {
+ perror("# sigaction");
+ ret = errno;
+ }
+ return ret;
+}
+
+/* reset signal handler to SIG_DFL. */
+int signal_handler_unregister(void)
+{
+ int ret = 0;
+ struct sigaction sigact;
+
+ sigact.sa_handler = SIG_DFL;
+ sigemptyset(&sigact.sa_mask);
+ if (sigaction(SIGINT, &sigact, NULL) ||
+ sigaction(SIGTERM, &sigact, NULL) ||
+ sigaction(SIGHUP, &sigact, NULL)) {
+ perror("# sigaction");
+ ret = errno;
+ }
+ return ret;
+}
+
/*
* print_results_bw: the memory bandwidth results are stored in a file
* @filename: file that stores the results
@@ -671,20 +711,9 @@ int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param)
ksft_print_msg("Benchmark PID: %d\n", bm_pid);
- /*
- * Register CTRL-C handler for parent, as it has to kill benchmark
- * before exiting
- */
- sigact.sa_sigaction = ctrlc_handler;
- sigemptyset(&sigact.sa_mask);
- sigact.sa_flags = SA_SIGINFO;
- if (sigaction(SIGINT, &sigact, NULL) ||
- sigaction(SIGTERM, &sigact, NULL) ||
- sigaction(SIGHUP, &sigact, NULL)) {
- perror("# sigaction");
- ret = errno;
+ ret = signal_handler_register();
+ if (ret)
goto out;
- }
value.sival_ptr = benchmark_cmd;
@@ -764,6 +793,7 @@ int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param)
out:
kill(bm_pid, SIGKILL);
umount_resctrlfs();
+ ret = signal_handler_unregister();
return ret;
}
--
2.27.0
When a process has buffered output, a child process created by fork()
will also copy buffered output. When using kselftest framework,
the output (resctrl test result message) will be printed multiple times.
Add fflush() to flush out the buffered output before executing fork().
Reviewed-by: Shuah Khan <[email protected]>
Reviewed-by: Reinette Chatre <[email protected]>
Signed-off-by: Shaopeng Tan <[email protected]>
---
tools/testing/selftests/resctrl/cat_test.c | 1 +
tools/testing/selftests/resctrl/resctrl_val.c | 1 +
tools/testing/selftests/resctrl/resctrlfs.c | 1 +
3 files changed, 3 insertions(+)
diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
index 1c5e90c63254..6a8306b0a109 100644
--- a/tools/testing/selftests/resctrl/cat_test.c
+++ b/tools/testing/selftests/resctrl/cat_test.c
@@ -167,6 +167,7 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
return errno;
}
+ fflush(stdout);
bm_pid = fork();
/* Set param values for child thread which will be allocated bitmask
diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
index b32b96356ec7..6948843bf995 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -629,6 +629,7 @@ int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param)
* Fork to start benchmark, save child's pid so that it can be killed
* when needed
*/
+ fflush(stdout);
bm_pid = fork();
if (bm_pid == -1) {
perror("# Unable to fork");
diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
index 8546bc9f1786..d95688298469 100644
--- a/tools/testing/selftests/resctrl/resctrlfs.c
+++ b/tools/testing/selftests/resctrl/resctrlfs.c
@@ -678,6 +678,7 @@ int filter_dmesg(void)
perror("pipe");
return ret;
}
+ fflush(stdout);
pid = fork();
if (pid == 0) {
close(pipefds[0]);
--
2.27.0
Before exiting each test function(run_cmt/cat/mbm/mba_test()),
test results("ok","not ok") are printed by ksft_test_result() and then
temporary result files are cleaned by function
cmt/cat/mbm/mba_test_cleanup().
However, before running ksft_test_result(),
function cmt/cat/mbm/mba_test_cleanup()
has been run in each test function as follows:
cmt_resctrl_val()
cat_perf_miss_val()
mba_schemata_change()
mbm_bw_change()
Remove duplicate codes that clear each test result file,
while ensuring cleanup properly even when errors occur in each test.
Reviewed-by: Reinette Chatre <[email protected]>
Signed-off-by: Shaopeng Tan <[email protected]>
---
tools/testing/selftests/resctrl/cmt_test.c | 7 +++----
tools/testing/selftests/resctrl/mba_test.c | 7 +++----
tools/testing/selftests/resctrl/mbm_test.c | 7 +++----
tools/testing/selftests/resctrl/resctrl_tests.c | 4 ----
4 files changed, 9 insertions(+), 16 deletions(-)
diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
index 8968e36db99d..617ee95d272d 100644
--- a/tools/testing/selftests/resctrl/cmt_test.c
+++ b/tools/testing/selftests/resctrl/cmt_test.c
@@ -133,13 +133,12 @@ int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd)
ret = resctrl_val(benchmark_cmd, ¶m);
if (ret)
- return ret;
+ goto out;
ret = check_results(¶m, n);
- if (ret)
- return ret;
+out:
cmt_test_cleanup();
- return 0;
+ return ret;
}
diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c
index e3a473976d74..b948938a3b4d 100644
--- a/tools/testing/selftests/resctrl/mba_test.c
+++ b/tools/testing/selftests/resctrl/mba_test.c
@@ -160,13 +160,12 @@ int mba_schemata_change(int cpu_no, char *bw_report, char **benchmark_cmd)
ret = resctrl_val(benchmark_cmd, ¶m);
if (ret)
- return ret;
+ goto out;
ret = check_results();
- if (ret)
- return ret;
+out:
mba_test_cleanup();
- return 0;
+ return ret;
}
diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
index 6d550f012829..040ca1f9c173 100644
--- a/tools/testing/selftests/resctrl/mbm_test.c
+++ b/tools/testing/selftests/resctrl/mbm_test.c
@@ -134,13 +134,12 @@ int mbm_bw_change(int span, int cpu_no, char *bw_report, char **benchmark_cmd)
ret = resctrl_val(benchmark_cmd, ¶m);
if (ret)
- return ret;
+ goto out;
ret = check_results(span);
- if (ret)
- return ret;
+out:
mbm_test_cleanup();
- return 0;
+ return ret;
}
diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
index df0d8d8526fc..8732cf736528 100644
--- a/tools/testing/selftests/resctrl/resctrl_tests.c
+++ b/tools/testing/selftests/resctrl/resctrl_tests.c
@@ -88,7 +88,6 @@ static void run_mbm_test(bool has_ben, char **benchmark_cmd, int span,
ksft_test_result(!res, "MBM: bw change\n");
if ((get_vendor() == ARCH_INTEL) && res)
ksft_print_msg("Intel MBM may be inaccurate when Sub-NUMA Clustering is enabled. Check BIOS configuration.\n");
- mbm_test_cleanup();
}
static void run_mba_test(bool has_ben, char **benchmark_cmd, int span,
@@ -107,7 +106,6 @@ static void run_mba_test(bool has_ben, char **benchmark_cmd, int span,
sprintf(benchmark_cmd[1], "%d", span);
res = mba_schemata_change(cpu_no, bw_report, benchmark_cmd);
ksft_test_result(!res, "MBA: schemata change\n");
- mba_test_cleanup();
}
static void run_cmt_test(bool has_ben, char **benchmark_cmd, int cpu_no)
@@ -126,7 +124,6 @@ static void run_cmt_test(bool has_ben, char **benchmark_cmd, int cpu_no)
ksft_test_result(!res, "CMT: test\n");
if ((get_vendor() == ARCH_INTEL) && res)
ksft_print_msg("Intel CMT may be inaccurate when Sub-NUMA Clustering is enabled. Check BIOS configuration.\n");
- cmt_test_cleanup();
}
static void run_cat_test(int cpu_no, int no_of_bits)
@@ -142,7 +139,6 @@ static void run_cat_test(int cpu_no, int no_of_bits)
res = cat_perf_miss_val(cpu_no, no_of_bits, "L3");
ksft_test_result(!res, "CAT: test\n");
- cat_test_cleanup();
}
int main(int argc, char **argv)
--
2.27.0
Hi Shaopeng,
On 1/10/2023 11:58 PM, Shaopeng Tan wrote:
> After creating a child process with fork() in CAT test, if there is an
Please delete the "there is" so that it reads "if an error occurs"
> error occurs or a signal such as SIGINT is received, the parent process
> will be terminated immediately, and therefor the child process will not
> be killed and also resctrlfs is not unmounted.
>
> There is a signal handler registered in CMT/MBM/MBA tests, which kills
> child process, unmount resctrlfs, cleanups result files, etc., if a
> signal such as SIGINT is received.
>
> Commonize the signal handler registered for CMT/MBM/MBA tests and reuse
> it in CAT too.
>
> To reuse the signal handler, make the child process in CAT wait to be
> killed by parent process in any case (an error occurred or a signal was
> received), and when killing child process use global bm_pid instead of
> local bm_pid.
>
> Also, since the MBA/MBA/CMT/CAT are run in order, unregister the signal
> handler at the end of each test so that the signal handler cannot be
> inherited by other tests.
>
> Signed-off-by: Shaopeng Tan <[email protected]>
> ---
> tools/testing/selftests/resctrl/cat_test.c | 26 +++++----
> tools/testing/selftests/resctrl/fill_buf.c | 14 -----
> tools/testing/selftests/resctrl/resctrl.h | 2 +
> tools/testing/selftests/resctrl/resctrl_val.c | 56 ++++++++++++++-----
> 4 files changed, 59 insertions(+), 39 deletions(-)
>
> diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
> index 6a8306b0a109..87302b882929 100644
> --- a/tools/testing/selftests/resctrl/cat_test.c
> +++ b/tools/testing/selftests/resctrl/cat_test.c
> @@ -103,7 +103,6 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
> unsigned long l_mask, l_mask_1;
> int ret, pipefd[2], sibling_cpu_no;
> char pipe_message;
> - pid_t bm_pid;
>
> cache_size = 0;
>
> @@ -181,28 +180,29 @@ 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)
> + goto out;
The "goto" will unregister the signal handler. Is that necessary if
the registration failed?
Also, if signal_handler_register() fails then the child will
keep running and run its test ... would child not then run forever?
> }
>
> remove(param.filename);
>
> ret = cat_val(¶m);
> - if (ret)
> - return ret;
> -
> - ret = check_results(¶m);
> - if (ret)
> - return ret;
> + if (ret == 0)
> + ret = check_results(¶m);
>
> if (bm_pid == 0) {
> /* Tell parent that child is ready */
> close(pipefd[0]);
> pipe_message = 1;
> if (write(pipefd[1], &pipe_message, sizeof(pipe_message)) <
> - sizeof(pipe_message)) {
> - close(pipefd[1]);
> + sizeof(pipe_message))
> + /*
> + * Just print the error message.
> + * Let while(1) run and wait for itself to be killed.
> + */
> perror("# failed signaling parent process");
> - return errno;
> - }
>
> close(pipefd[1]);
> while (1)
> @@ -226,5 +226,7 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
> if (bm_pid)
> umount_resctrlfs();
>
> - return 0;
> +out:
> + ret = signal_handler_unregister();
> + return ret;
Be careful here ... any earlier value of "ret" will be overwritten with
the result of signal_handler_unregister(). I think the return of
signal_handler_unregister() can be ignored. There will be an
error message if it failed.
> }
> diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c
> index 56ccbeae0638..322c6812e15c 100644
> --- a/tools/testing/selftests/resctrl/fill_buf.c
> +++ b/tools/testing/selftests/resctrl/fill_buf.c
> @@ -33,14 +33,6 @@ static void sb(void)
> #endif
> }
>
> -static void ctrl_handler(int signo)
> -{
> - free(startptr);
> - printf("\nEnding\n");
> - sb();
> - exit(EXIT_SUCCESS);
> -}
> -
> static void cl_flush(void *p)
> {
> #if defined(__i386) || defined(__x86_64)
> @@ -198,12 +190,6 @@ int run_fill_buf(unsigned long span, int malloc_and_init_memory,
> unsigned long long cache_size = span;
> int ret;
>
> - /* set up ctrl-c handler */
> - if (signal(SIGINT, ctrl_handler) == SIG_ERR)
> - printf("Failed to catch SIGINT!\n");
> - if (signal(SIGHUP, ctrl_handler) == SIG_ERR)
> - printf("Failed to catch SIGHUP!\n");
> -
> ret = fill_cache(cache_size, malloc_and_init_memory, memflush, op,
> resctrl_val);
> if (ret) {
> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> index f0ded31fb3c7..14a5e21497e1 100644
> --- a/tools/testing/selftests/resctrl/resctrl.h
> +++ b/tools/testing/selftests/resctrl/resctrl.h
> @@ -107,6 +107,8 @@ void mba_test_cleanup(void);
> int get_cbm_mask(char *cache_type, char *cbm_mask);
> int get_cache_size(int cpu_no, char *cache_type, unsigned long *cache_size);
> void ctrlc_handler(int signum, siginfo_t *info, void *ptr);
> +int signal_handler_register(void);
> +int signal_handler_unregister(void);
> int cat_val(struct resctrl_val_param *param);
> void cat_test_cleanup(void);
> int cat_perf_miss_val(int cpu_no, int no_of_bits, char *cache_type);
> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
> index 6948843bf995..91a3cf8b308b 100644
> --- a/tools/testing/selftests/resctrl/resctrl_val.c
> +++ b/tools/testing/selftests/resctrl/resctrl_val.c
> @@ -476,6 +476,46 @@ void ctrlc_handler(int signum, siginfo_t *info, void *ptr)
> exit(EXIT_SUCCESS);
> }
>
> +struct sigaction sigact;
> +
> +/*
> + * Register CTRL-C handler for parent, as it has to kill
> + * child process before exiting
> + */
> +int signal_handler_register(void)
> +{
> + int ret = 0;
> + struct sigaction sigact;
Why is there a global sigact as well as a local sigact?
Also, please do keep using reverse fir-tree format.
> +
> + sigact.sa_sigaction = ctrlc_handler;
> + sigemptyset(&sigact.sa_mask);
> + sigact.sa_flags = SA_SIGINFO;
> + if (sigaction(SIGINT, &sigact, NULL) ||
> + sigaction(SIGTERM, &sigact, NULL) ||
> + sigaction(SIGHUP, &sigact, NULL)) {
> + perror("# sigaction");
> + ret = errno;
I understand that you copied from the original code here but I
do think there is an issue here in that errno is undefined after
a library call. perror() will print errno message so keeping
it (errno) around may not be useful. Please do keep the custom
of returning negative value as error though. I think just
returning -1 would work.
> + }
> + return ret;
> +}
> +
> +/* reset signal handler to SIG_DFL. */
> +int signal_handler_unregister(void)
> +{
> + int ret = 0;
> + struct sigaction sigact;
> +
> + sigact.sa_handler = SIG_DFL;
> + sigemptyset(&sigact.sa_mask);
> + if (sigaction(SIGINT, &sigact, NULL) ||
> + sigaction(SIGTERM, &sigact, NULL) ||
> + sigaction(SIGHUP, &sigact, NULL)) {
> + perror("# sigaction");
> + ret = errno;
Same comment about errno and returning -1 on failure.
> + }
> + return ret;
> +}
> +
> /*
> * print_results_bw: the memory bandwidth results are stored in a file
> * @filename: file that stores the results
> @@ -671,20 +711,9 @@ int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param)
>
> ksft_print_msg("Benchmark PID: %d\n", bm_pid);
>
> - /*
> - * Register CTRL-C handler for parent, as it has to kill benchmark
> - * before exiting
> - */
> - sigact.sa_sigaction = ctrlc_handler;
> - sigemptyset(&sigact.sa_mask);
> - sigact.sa_flags = SA_SIGINFO;
> - if (sigaction(SIGINT, &sigact, NULL) ||
> - sigaction(SIGTERM, &sigact, NULL) ||
> - sigaction(SIGHUP, &sigact, NULL)) {
> - perror("# sigaction");
> - ret = errno;
> + ret = signal_handler_register();
> + if (ret)
> goto out;
> - }
>
> value.sival_ptr = benchmark_cmd;
>
> @@ -764,6 +793,7 @@ int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param)
> out:
> kill(bm_pid, SIGKILL);
> umount_resctrlfs();
> + ret = signal_handler_unregister();
>
> return ret;
Same here ... any earlier value of ret will be overwritten with result
of signal_handler_unregister().
Reinette
> }
Hi Reinette,
> On 1/10/2023 11:58 PM, Shaopeng Tan wrote:
> > After creating a child process with fork() in CAT test, if there is an
>
> Please delete the "there is" so that it reads "if an error occurs"
Thanks.
> > error occurs or a signal such as SIGINT is received, the parent
> > process will be terminated immediately, and therefor the child process
> > will not be killed and also resctrlfs is not unmounted.
> >
> > There is a signal handler registered in CMT/MBM/MBA tests, which kills
> > child process, unmount resctrlfs, cleanups result files, etc., if a
> > signal such as SIGINT is received.
> >
> > Commonize the signal handler registered for CMT/MBM/MBA tests and
> > reuse it in CAT too.
> >
> > To reuse the signal handler, make the child process in CAT wait to be
> > killed by parent process in any case (an error occurred or a signal
> > was received), and when killing child process use global bm_pid
> > instead of local bm_pid.
> >
> > Also, since the MBA/MBA/CMT/CAT are run in order, unregister the
> > signal handler at the end of each test so that the signal handler
> > cannot be inherited by other tests.
> >
> > Signed-off-by: Shaopeng Tan <[email protected]>
> > ---
> > tools/testing/selftests/resctrl/cat_test.c | 26 +++++----
> > tools/testing/selftests/resctrl/fill_buf.c | 14 -----
> > tools/testing/selftests/resctrl/resctrl.h | 2 +
> > tools/testing/selftests/resctrl/resctrl_val.c | 56
> > ++++++++++++++-----
> > 4 files changed, 59 insertions(+), 39 deletions(-)
> >
> > diff --git a/tools/testing/selftests/resctrl/cat_test.c
> > b/tools/testing/selftests/resctrl/cat_test.c
> > index 6a8306b0a109..87302b882929 100644
> > --- a/tools/testing/selftests/resctrl/cat_test.c
> > +++ b/tools/testing/selftests/resctrl/cat_test.c
> > @@ -103,7 +103,6 @@ int cat_perf_miss_val(int cpu_no, int n, char
> *cache_type)
> > unsigned long l_mask, l_mask_1;
> > int ret, pipefd[2], sibling_cpu_no;
> > char pipe_message;
> > - pid_t bm_pid;
> >
> > cache_size = 0;
> >
> > @@ -181,28 +180,29 @@ 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)
> > + goto out;
>
> The "goto" will unregister the signal handler. Is that necessary if the registration
> failed?
>
> Also, if signal_handler_register() fails then the child will keep running and run
> its test ... would child not then run forever?
A signal handler is needed here, but it is rarely used.
Also, the registration rarely fails.
Therefore, if registration failed,
just print a warning/info message as follow.
how about this idea?
ksft_print_msg("Failed to register signal handler, signals SIGINT/SIGTERM/SIGHUP will not be handled as expected");
> > }
> >
> > remove(param.filename);
> >
> > ret = cat_val(¶m);
> > - if (ret)
> > - return ret;
> > -
> > - ret = check_results(¶m);
> > - if (ret)
> > - return ret;
> > + if (ret == 0)
> > + ret = check_results(¶m);
> >
> > if (bm_pid == 0) {
> > /* Tell parent that child is ready */
> > close(pipefd[0]);
> > pipe_message = 1;
> > if (write(pipefd[1], &pipe_message, sizeof(pipe_message)) <
> > - sizeof(pipe_message)) {
> > - close(pipefd[1]);
> > + sizeof(pipe_message))
> > + /*
> > + * Just print the error message.
> > + * Let while(1) run and wait for itself to be killed.
> > + */
> > perror("# failed signaling parent process");
> > - return errno;
> > - }
> >
> > close(pipefd[1]);
> > while (1)
> > @@ -226,5 +226,7 @@ int cat_perf_miss_val(int cpu_no, int n, char
> *cache_type)
> > if (bm_pid)
> > umount_resctrlfs();
> >
> > - return 0;
> > +out:
> > + ret = signal_handler_unregister();
> > + return ret;
>
> Be careful here ... any earlier value of "ret" will be overwritten with the result of
> signal_handler_unregister(). I think the return of
> signal_handler_unregister() can be ignored. There will be an error message if it
> failed.
Thanks for your advice, I will ignore the return of signal_handler_unregister().
> > }
> > diff --git a/tools/testing/selftests/resctrl/fill_buf.c
> > b/tools/testing/selftests/resctrl/fill_buf.c
> > index 56ccbeae0638..322c6812e15c 100644
> > --- a/tools/testing/selftests/resctrl/fill_buf.c
> > +++ b/tools/testing/selftests/resctrl/fill_buf.c
> > @@ -33,14 +33,6 @@ static void sb(void) #endif }
> >
> > -static void ctrl_handler(int signo)
> > -{
> > - free(startptr);
> > - printf("\nEnding\n");
> > - sb();
> > - exit(EXIT_SUCCESS);
> > -}
> > -
> > static void cl_flush(void *p)
> > {
> > #if defined(__i386) || defined(__x86_64) @@ -198,12 +190,6 @@ int
> > run_fill_buf(unsigned long span, int malloc_and_init_memory,
> > unsigned long long cache_size = span;
> > int ret;
> >
> > - /* set up ctrl-c handler */
> > - if (signal(SIGINT, ctrl_handler) == SIG_ERR)
> > - printf("Failed to catch SIGINT!\n");
> > - if (signal(SIGHUP, ctrl_handler) == SIG_ERR)
> > - printf("Failed to catch SIGHUP!\n");
> > -
> > ret = fill_cache(cache_size, malloc_and_init_memory, memflush, op,
> > resctrl_val);
> > if (ret) {
> > diff --git a/tools/testing/selftests/resctrl/resctrl.h
> > b/tools/testing/selftests/resctrl/resctrl.h
> > index f0ded31fb3c7..14a5e21497e1 100644
> > --- a/tools/testing/selftests/resctrl/resctrl.h
> > +++ b/tools/testing/selftests/resctrl/resctrl.h
> > @@ -107,6 +107,8 @@ void mba_test_cleanup(void); int
> > get_cbm_mask(char *cache_type, char *cbm_mask); int
> > get_cache_size(int cpu_no, char *cache_type, unsigned long
> > *cache_size); void ctrlc_handler(int signum, siginfo_t *info, void
> > *ptr);
> > +int signal_handler_register(void);
> > +int signal_handler_unregister(void);
> > int cat_val(struct resctrl_val_param *param); void
> > cat_test_cleanup(void); int cat_perf_miss_val(int cpu_no, int
> > no_of_bits, char *cache_type); diff --git
> > a/tools/testing/selftests/resctrl/resctrl_val.c
> > b/tools/testing/selftests/resctrl/resctrl_val.c
> > index 6948843bf995..91a3cf8b308b 100644
> > --- a/tools/testing/selftests/resctrl/resctrl_val.c
> > +++ b/tools/testing/selftests/resctrl/resctrl_val.c
> > @@ -476,6 +476,46 @@ void ctrlc_handler(int signum, siginfo_t *info, void
> *ptr)
> > exit(EXIT_SUCCESS);
> > }
> >
> > +struct sigaction sigact;
> > +
> > +/*
> > + * Register CTRL-C handler for parent, as it has to kill
> > + * child process before exiting
> > + */
> > +int signal_handler_register(void)
> > +{
> > + int ret = 0;
> > + struct sigaction sigact;
>
> Why is there a global sigact as well as a local sigact?
>
> Also, please do keep using reverse fir-tree format.
I was going to use local sigact.
I will delete global sigact and keep using reverse fir-tree format.
+ struct sigaction sigact;
+ int ret = 0;
> > +
> > + sigact.sa_sigaction = ctrlc_handler;
> > + sigemptyset(&sigact.sa_mask);
> > + sigact.sa_flags = SA_SIGINFO;
> > + if (sigaction(SIGINT, &sigact, NULL) ||
> > + sigaction(SIGTERM, &sigact, NULL) ||
> > + sigaction(SIGHUP, &sigact, NULL)) {
> > + perror("# sigaction");
> > + ret = errno;
>
> I understand that you copied from the original code here but I do think there is
> an issue here in that errno is undefined after a library call. perror() will print
> errno message so keeping it (errno) around may not be useful. Please do keep
> the custom of returning negative value as error though. I think just returning -1
> would work.
Thanks for your advice. I will return -1 here.
> > + }
> > + return ret;
> > +}
> > +
> > +/* reset signal handler to SIG_DFL. */ int
> > +signal_handler_unregister(void) {
> > + int ret = 0;
> > + struct sigaction sigact;
> > +
> > + sigact.sa_handler = SIG_DFL;
> > + sigemptyset(&sigact.sa_mask);
> > + if (sigaction(SIGINT, &sigact, NULL) ||
> > + sigaction(SIGTERM, &sigact, NULL) ||
> > + sigaction(SIGHUP, &sigact, NULL)) {
> > + perror("# sigaction");
> > + ret = errno;
>
> Same comment about errno and returning -1 on failure.
I will return -1 here.
> > + }
> > + return ret;
> > +}
> > +
> > /*
> > * print_results_bw: the memory bandwidth results are stored in a file
> > * @filename: file that stores the results
> > @@ -671,20 +711,9 @@ int resctrl_val(char **benchmark_cmd, struct
> > resctrl_val_param *param)
> >
> > ksft_print_msg("Benchmark PID: %d\n", bm_pid);
> >
> > - /*
> > - * Register CTRL-C handler for parent, as it has to kill benchmark
> > - * before exiting
> > - */
> > - sigact.sa_sigaction = ctrlc_handler;
> > - sigemptyset(&sigact.sa_mask);
> > - sigact.sa_flags = SA_SIGINFO;
> > - if (sigaction(SIGINT, &sigact, NULL) ||
> > - sigaction(SIGTERM, &sigact, NULL) ||
> > - sigaction(SIGHUP, &sigact, NULL)) {
> > - perror("# sigaction");
> > - ret = errno;
> > + ret = signal_handler_register();
> > + if (ret)
> > goto out;
> > - }
> >
> > value.sival_ptr = benchmark_cmd;
> >
> > @@ -764,6 +793,7 @@ int resctrl_val(char **benchmark_cmd, struct
> > resctrl_val_param *param)
> > out:
> > kill(bm_pid, SIGKILL);
> > umount_resctrlfs();
> > + ret = signal_handler_unregister();
> >
> > return ret;
>
> Same here ... any earlier value of ret will be overwritten with result of
> signal_handler_unregister().
I will ignore the return of signal_handler_unregister().
Best regards,
Shaopeng TAN
Hi Shaopeng,
On 1/22/2023 8:22 PM, Shaopeng Tan (Fujitsu) wrote:
>> On 1/10/2023 11:58 PM, Shaopeng Tan wrote:
...
>>> diff --git a/tools/testing/selftests/resctrl/cat_test.c
>>> b/tools/testing/selftests/resctrl/cat_test.c
>>> index 6a8306b0a109..87302b882929 100644
>>> --- a/tools/testing/selftests/resctrl/cat_test.c
>>> +++ b/tools/testing/selftests/resctrl/cat_test.c
>>> @@ -103,7 +103,6 @@ int cat_perf_miss_val(int cpu_no, int n, char
>> *cache_type)
>>> unsigned long l_mask, l_mask_1;
>>> int ret, pipefd[2], sibling_cpu_no;
>>> char pipe_message;
>>> - pid_t bm_pid;
>>>
>>> cache_size = 0;
>>>
>>> @@ -181,28 +180,29 @@ 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)
>>> + goto out;
>>
>> The "goto" will unregister the signal handler. Is that necessary if the registration
>> failed?
>>
>> Also, if signal_handler_register() fails then the child will keep running and run
>> its test ... would child not then run forever?
>
> A signal handler is needed here, but it is rarely used.
> Also, the registration rarely fails.
> Therefore, if registration failed,
> just print a warning/info message as follow.
> how about this idea?
>
> ksft_print_msg("Failed to register signal handler, signals SIGINT/SIGTERM/SIGHUP will not be handled as expected");
>
I do not think this is necessary considering that signal_handler_register()
already prints an error on failure.
Adding an error message does not address the two issues I raised.
Reinette
Hi Reinette,
> On 1/22/2023 8:22 PM, Shaopeng Tan (Fujitsu) wrote:
> >> On 1/10/2023 11:58 PM, Shaopeng Tan wrote:
>
> ...
>
> >>> diff --git a/tools/testing/selftests/resctrl/cat_test.c
> >>> b/tools/testing/selftests/resctrl/cat_test.c
> >>> index 6a8306b0a109..87302b882929 100644
> >>> --- a/tools/testing/selftests/resctrl/cat_test.c
> >>> +++ b/tools/testing/selftests/resctrl/cat_test.c
> >>> @@ -103,7 +103,6 @@ int cat_perf_miss_val(int cpu_no, int n, char
> >> *cache_type)
> >>> unsigned long l_mask, l_mask_1;
> >>> int ret, pipefd[2], sibling_cpu_no;
> >>> char pipe_message;
> >>> - pid_t bm_pid;
> >>>
> >>> cache_size = 0;
> >>>
> >>> @@ -181,28 +180,29 @@ 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)
> >>> + goto out;
> >>
> >> The "goto" will unregister the signal handler. Is that necessary if
> >> the registration failed?
> >>
> >> Also, if signal_handler_register() fails then the child will keep
> >> running and run its test ... would child not then run forever?
> >
> > A signal handler is needed here, but it is rarely used.
> > Also, the registration rarely fails.
> > Therefore, if registration failed,
> > just print a warning/info message as follow.
> > how about this idea?
> >
> > ksft_print_msg("Failed to register signal handler, signals
> > SIGINT/SIGTERM/SIGHUP will not be handled as expected");
> >
>
> I do not think this is necessary considering that signal_handler_register()
> already prints an error on failure.
>
> Adding an error message does not address the two issues I raised.
The previous idea was just to print a message instead of "goto".
How about the idea to keep the parent&child process running forever even if the signal handler registration fails.
+ } else {
+ signal_handler_register();
+ }
I don't think the test need stop when the signal handler registration fails,
since this signal handler is rarely used and registration of signal handlers rarely fails.
Best regards,
Shaopeng TAN
Hi Shaopeng,
On 1/23/2023 6:16 PM, Shaopeng Tan (Fujitsu) wrote:
>> On 1/22/2023 8:22 PM, Shaopeng Tan (Fujitsu) wrote:
>>>> On 1/10/2023 11:58 PM, Shaopeng Tan wrote:
>>>>> diff --git a/tools/testing/selftests/resctrl/cat_test.c
>>>>> b/tools/testing/selftests/resctrl/cat_test.c
>>>>> index 6a8306b0a109..87302b882929 100644
>>>>> --- a/tools/testing/selftests/resctrl/cat_test.c
>>>>> +++ b/tools/testing/selftests/resctrl/cat_test.c
>>>>> @@ -103,7 +103,6 @@ int cat_perf_miss_val(int cpu_no, int n, char
>>>> *cache_type)
>>>>> unsigned long l_mask, l_mask_1;
>>>>> int ret, pipefd[2], sibling_cpu_no;
>>>>> char pipe_message;
>>>>> - pid_t bm_pid;
>>>>>
>>>>> cache_size = 0;
>>>>>
>>>>> @@ -181,28 +180,29 @@ 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)
>>>>> + goto out;
>>>>
>>>> The "goto" will unregister the signal handler. Is that necessary if
>>>> the registration failed?
>>>>
>>>> Also, if signal_handler_register() fails then the child will keep
>>>> running and run its test ... would child not then run forever?
>>>
>>> A signal handler is needed here, but it is rarely used.
>>> Also, the registration rarely fails.
>>> Therefore, if registration failed,
>>> just print a warning/info message as follow.
>>> how about this idea?
>>>
>>> ksft_print_msg("Failed to register signal handler, signals
>>> SIGINT/SIGTERM/SIGHUP will not be handled as expected");
>>>
>>
>> I do not think this is necessary considering that signal_handler_register()
>> already prints an error on failure.
>>
>> Adding an error message does not address the two issues I raised.
>
> The previous idea was just to print a message instead of "goto".
> How about the idea to keep the parent&child process running forever even if the signal handler registration fails.
>
> + } else {
> + signal_handler_register();
> + }
>
> I don't think the test need stop when the signal handler registration fails,
> since this signal handler is rarely used and registration of signal handlers rarely fails.
Please always handle errors properly. If the registration fails the test should be
stopped and proper cleanup should be done.
Reinette