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. [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.1-rc5
Difference from v3: [patch 2] Rename "failed" to "ret" to avoid confusion. [patch 4] - Use sigaction(2) instead of signal(). - Add a description of using global bm_pid in commit message. - Add comments to clarify why let the child continue to its infinite loop after the write() failed. [patch 5] Ensure to run cat/cmt/mbm/mba_test_cleanup() to clear test result file before return if an error occurs.
Pervious versions of this series: [v1] https://lore.kernel.org/lkml/20220914015147.3071025-1-tan.shaopeng@jp.fujits... [v2] https://lore.kernel.org/lkml/20221005013933.1486054-1-tan.shaopeng@jp.fujits... [v3] https://lore.kernel.org/lkml/20221101094341.3383073-1-tan.shaopeng@jp.fujits...
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 | 31 +++++++++++++------ tools/testing/selftests/resctrl/cmt_test.c | 7 ++--- tools/testing/selftests/resctrl/mba_test.c | 23 +++++++------- tools/testing/selftests/resctrl/mbm_test.c | 20 ++++++------ .../testing/selftests/resctrl/resctrl_tests.c | 4 --- tools/testing/selftests/resctrl/resctrl_val.c | 1 + tools/testing/selftests/resctrl/resctrlfs.c | 5 ++- 7 files changed, 50 insertions(+), 41 deletions(-)
There is a comment "Set up shemata with 100% allocation on the first run" in function mbm_setup(), but there is an increment bug and the condition "num_of_runs == 0" will never be met and write_schemata() will never be called to set schemata to 100%. Even if write_schemata() is called in MBM test, since it is not supported for MBM test it does not set the schemata. This is currently fine because resctrl_val_parm->mum_resctrlfs is always 1 and umount/mount will be run in each test to set the schemata to 100%.
To support the usage when MBM test does not unmount/remount resctrl filesystem before the test starts, fix to call write_schemata() and set schemata properly when the function is called for the first time.
Also, remove static local variable 'num_of_runs' because this is not needed as there is resctrl_val_param->num_of_runs which should be used instead like in cat_setup().
Reviewed-by: Reinette Chatre reinette.chatre@intel.com Signed-off-by: Shaopeng Tan tan.shaopeng@jp.fujitsu.com --- tools/testing/selftests/resctrl/mbm_test.c | 13 +++++++------ tools/testing/selftests/resctrl/resctrlfs.c | 4 +++- 2 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c index 8392e5c55ed0..6d550f012829 100644 --- a/tools/testing/selftests/resctrl/mbm_test.c +++ b/tools/testing/selftests/resctrl/mbm_test.c @@ -89,23 +89,24 @@ static int check_results(int span) static int mbm_setup(int num, ...) { struct resctrl_val_param *p; - static int num_of_runs; va_list param; int ret = 0;
- /* Run NUM_OF_RUNS times */ - if (num_of_runs++ >= NUM_OF_RUNS) - return -1; - va_start(param, num); p = va_arg(param, struct resctrl_val_param *); va_end(param);
+ /* Run NUM_OF_RUNS times */ + if (p->num_of_runs >= NUM_OF_RUNS) + return -1; + /* Set up shemata with 100% allocation on the first run. */ - if (num_of_runs == 0) + if (p->num_of_runs == 0) ret = write_schemata(p->ctrlgrp, "100", p->cpu_no, p->resctrl_val);
+ p->num_of_runs++; + return ret; }
diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c index 6f543e470ad4..8546bc9f1786 100644 --- a/tools/testing/selftests/resctrl/resctrlfs.c +++ b/tools/testing/selftests/resctrl/resctrlfs.c @@ -498,6 +498,7 @@ int write_schemata(char *ctrlgrp, char *schemata, int cpu_no, char *resctrl_val) FILE *fp;
if (strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR)) && + strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) && strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR)) && strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR))) return -ENOENT; @@ -523,7 +524,8 @@ int write_schemata(char *ctrlgrp, char *schemata, int cpu_no, char *resctrl_val) if (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR)) || !strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR))) sprintf(schema, "%s%d%c%s", "L3:", resource_id, '=', schemata); - if (!strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR))) + if (!strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR)) || + !strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR))) sprintf(schema, "%s%d%c%s", "MB:", resource_id, '=', schemata);
fp = fopen(controlgroup, "w");
Since MBA check result is not returned, the MBA test result message is always output as "ok" regardless of whether the MBA check result is true or false.
Make output message to be "not ok" if MBA check result is failed.
Reviewed-by: Shuah Khan skhan@linuxfoundation.org Reviewed-by: Reinette Chatre reinette.chatre@intel.com Signed-off-by: Shaopeng Tan tan.shaopeng@jp.fujitsu.com --- tools/testing/selftests/resctrl/mba_test.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c index 1a1bdb6180cf..e3a473976d74 100644 --- a/tools/testing/selftests/resctrl/mba_test.c +++ b/tools/testing/selftests/resctrl/mba_test.c @@ -51,10 +51,10 @@ static int mba_setup(int num, ...) return 0; }
-static void show_mba_info(unsigned long *bw_imc, unsigned long *bw_resc) +static bool show_mba_info(unsigned long *bw_imc, unsigned long *bw_resc) { int allocation, runs; - bool failed = false; + bool ret = false;
ksft_print_msg("Results are displayed in (MB)\n"); /* Memory bandwidth from 100% down to 10% */ @@ -90,13 +90,15 @@ static void show_mba_info(unsigned long *bw_imc, unsigned long *bw_resc) ksft_print_msg("avg_bw_imc: %lu\n", avg_bw_imc); ksft_print_msg("avg_bw_resc: %lu\n", avg_bw_resc); if (avg_diff_per > MAX_DIFF_PERCENT) - failed = true; + ret = true; }
ksft_print_msg("%s Check schemata change using MBA\n", - failed ? "Fail:" : "Pass:"); - if (failed) + ret ? "Fail:" : "Pass:"); + if (ret) ksft_print_msg("At least one test failed\n"); + + return ret; }
static int check_results(void) @@ -132,9 +134,7 @@ static int check_results(void)
fclose(fp);
- show_mba_info(bw_imc, bw_resc); - - return 0; + return show_mba_info(bw_imc, bw_resc); }
void mba_test_cleanup(void)
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 skhan@linuxfoundation.org Reviewed-by: Reinette Chatre reinette.chatre@intel.com Signed-off-by: Shaopeng Tan tan.shaopeng@jp.fujitsu.com --- 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]);
After creating a child process with fork() in CAT test, if there is an error occurs or such as a SIGINT signal is received, the parent process will be terminated immediately, but the child process will not be killed and also umount_resctrlfs() will not be called.
Add a signal handler like other tests to kill child process, umount resctrlfs, cleanup result files, etc. if an error occurs in parent process. To use ctrlc_handler() of other tests to kill child process(bm_pid), using global bm_pid instead of local bm_pid.
Wait for child process to be killed if an error occurs in child process.
Reviewed-by: Shuah Khan skhan@linuxfoundation.org Signed-off-by: Shaopeng Tan tan.shaopeng@jp.fujitsu.com --- tools/testing/selftests/resctrl/cat_test.c | 30 ++++++++++++++-------- 1 file changed, 20 insertions(+), 10 deletions(-)
diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c index 6a8306b0a109..1f8f5cf94e95 100644 --- a/tools/testing/selftests/resctrl/cat_test.c +++ b/tools/testing/selftests/resctrl/cat_test.c @@ -100,10 +100,10 @@ void cat_test_cleanup(void)
int cat_perf_miss_val(int cpu_no, int n, char *cache_type) { + struct sigaction sigact; 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,17 +181,25 @@ 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 { + /* + * Register CTRL-C handler for parent, as it has to kill + * child process 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"); }
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 */ @@ -199,9 +207,11 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type) pipe_message = 1; if (write(pipefd[1], &pipe_message, sizeof(pipe_message)) < sizeof(pipe_message)) { - close(pipefd[1]); + /* + * 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]); @@ -226,5 +236,5 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type) if (bm_pid) umount_resctrlfs();
- return 0; + return ret; }
Hi Shaopeng,
On 11/16/2022 5:05 PM, Shaopeng Tan wrote:
After creating a child process with fork() in CAT test, if there is an error occurs or such as a SIGINT signal is received, the parent process will be terminated immediately, but the child process will not be killed and also umount_resctrlfs() will not be called.
Add a signal handler like other tests to kill child process, umount resctrlfs, cleanup result files, etc. if an error occurs in parent process. To use ctrlc_handler() of other tests to kill child process(bm_pid), using global bm_pid instead of local bm_pid.
Wait for child process to be killed if an error occurs in child process.
Reviewed-by: Shuah Khan skhan@linuxfoundation.org Signed-off-by: Shaopeng Tan tan.shaopeng@jp.fujitsu.com
tools/testing/selftests/resctrl/cat_test.c | 30 ++++++++++++++-------- 1 file changed, 20 insertions(+), 10 deletions(-)
diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c index 6a8306b0a109..1f8f5cf94e95 100644 --- a/tools/testing/selftests/resctrl/cat_test.c +++ b/tools/testing/selftests/resctrl/cat_test.c @@ -100,10 +100,10 @@ void cat_test_cleanup(void) int cat_perf_miss_val(int cpu_no, int n, char *cache_type) {
- struct sigaction sigact; 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,17 +181,25 @@ 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 {
/** Register CTRL-C handler for parent, as it has to kill* child process 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");
Why keep going at this point?
I tried this change but I was not able to trigger ctrlc_handler(). It seems that the handler is changed soon after (see cat_val()->run_fill_buf()) and ctrl_handler() (note the subtle name difference) is run instead when a SIGINT is received. The value of ctrl_handler() is not clear to me though, and it could even be argued that it is broken because it starts with free(startptr) and startptr would likely already be free'd at this point without resetting its value to NULL.
From what I understand ctrl_handler() and its installation from run_fill_buf() could be removed so that it does not override what is being done with this change. Otherwise, from what I can tell, this new handler will never run.
Reinette
Hi Reinette,
On 11/16/2022 5:05 PM, Shaopeng Tan wrote:
After creating a child process with fork() in CAT test, if there is an error occurs or such as a SIGINT signal is received, the parent process will be terminated immediately, but the child process will not be killed and also umount_resctrlfs() will not be called.
Add a signal handler like other tests to kill child process, umount resctrlfs, cleanup result files, etc. if an error occurs in parent process. To use ctrlc_handler() of other tests to kill child process(bm_pid), using global bm_pid instead of local bm_pid.
Wait for child process to be killed if an error occurs in child process.
Reviewed-by: Shuah Khan skhan@linuxfoundation.org Signed-off-by: Shaopeng Tan tan.shaopeng@jp.fujitsu.com
tools/testing/selftests/resctrl/cat_test.c | 30
++++++++++++++--------
1 file changed, 20 insertions(+), 10 deletions(-)
diff --git a/tools/testing/selftests/resctrl/cat_test.c
b/tools/testing/selftests/resctrl/cat_test.c
index 6a8306b0a109..1f8f5cf94e95 100644 --- a/tools/testing/selftests/resctrl/cat_test.c +++ b/tools/testing/selftests/resctrl/cat_test.c @@ -100,10 +100,10 @@ void cat_test_cleanup(void)
int cat_perf_miss_val(int cpu_no, int n, char *cache_type) {
- struct sigaction sigact; 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,17 +181,25 @@ 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 {
/** Register CTRL-C handler for parent, as it has to kill* child process 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");Why keep going at this point?
I tried this change but I was not able to trigger ctrlc_handler(). It
I tested this change using kselftest framework, In this case, the timeout command sent a SIGTERM signal, and then ctrlc_handler() was triggered. Since the handling of SIGINT and SIGHUP signals is overridden in run_fill_buf(), ctrl_handler() may be called if ctrl-c is received.
seems that the handler is changed soon after (see cat_val()->run_fill_buf()) and ctrl_handler() (note the subtle name difference) is run instead when a SIGINT is received. The value of ctrl_handler() is not clear to me though, and it could even be argued that it is broken because it starts with free(startptr) and startptr would likely already be free'd at this point without resetting its value to NULL.
From what I understand ctrl_handler() and its installation from run_fill_buf() could be removed so that it does not override what is being done with this change. Otherwise, from what I can tell, this new handler will never run.
I think removing ctrl_handler() is fine. In CAT test, it overrides ctrlc_handler(). In other tests(CMT,MBA,MBM), the child process used ctrl_handler() to cleanup itself, but the parent process cleanup the child process with ctrlc_handler() properly. Also, avoid using signal(). fill_buf.c：run_fill_buf() 201 /* set up ctrl-c handler */ 202 if (signal(SIGINT, ctrl_handler) == SIG_ERR) 203 printf("Failed to catch SIGINT!\n");
I removed ctrl_handler() and ran resctrl_tests with and without the kselftest framework. There is no problem.
Best regards, Shaopeng Tan
Hi Shaopeng,
On 11/24/2022 12:17 AM, Shaopeng Tan (Fujitsu) wrote:
Hi Reinette,
On 11/16/2022 5:05 PM, Shaopeng Tan wrote:
After creating a child process with fork() in CAT test, if there is an error occurs or such as a SIGINT signal is received, the parent process will be terminated immediately, but the child process will not be killed and also umount_resctrlfs() will not be called.
Add a signal handler like other tests to kill child process, umount resctrlfs, cleanup result files, etc. if an error occurs in parent process. To use ctrlc_handler() of other tests to kill child process(bm_pid), using global bm_pid instead of local bm_pid.
Wait for child process to be killed if an error occurs in child process.
Reviewed-by: Shuah Khan skhan@linuxfoundation.org Signed-off-by: Shaopeng Tan tan.shaopeng@jp.fujitsu.com
tools/testing/selftests/resctrl/cat_test.c | 30
++++++++++++++--------
1 file changed, 20 insertions(+), 10 deletions(-)
diff --git a/tools/testing/selftests/resctrl/cat_test.c
b/tools/testing/selftests/resctrl/cat_test.c
index 6a8306b0a109..1f8f5cf94e95 100644 --- a/tools/testing/selftests/resctrl/cat_test.c +++ b/tools/testing/selftests/resctrl/cat_test.c @@ -100,10 +100,10 @@ void cat_test_cleanup(void)
int cat_perf_miss_val(int cpu_no, int n, char *cache_type) {
- struct sigaction sigact; 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,17 +181,25 @@ 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 {
/** Register CTRL-C handler for parent, as it has to kill* child process 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");Why keep going at this point?
I tried this change but I was not able to trigger ctrlc_handler(). It
I tested this change using kselftest framework, In this case, the timeout command sent a SIGTERM signal, and then ctrlc_handler() was triggered. Since the handling of SIGINT and SIGHUP signals is overridden in run_fill_buf(), ctrl_handler() may be called if ctrl-c is received.
I tested this by running "resctrl_tests -t cat" and when doing so this change does not behave as described.
seems that the handler is changed soon after (see cat_val()->run_fill_buf()) and ctrl_handler() (note the subtle name difference) is run instead when a SIGINT is received. The value of ctrl_handler() is not clear to me though, and it could even be argued that it is broken because it starts with free(startptr) and startptr would likely already be free'd at this point without resetting its value to NULL.
From what I understand ctrl_handler() and its installation from run_fill_buf() could be removed so that it does not override what is being done with this change. Otherwise, from what I can tell, this new handler will never run.
I think removing ctrl_handler() is fine. In CAT test, it overrides ctrlc_handler(). In other tests(CMT,MBA,MBM), the child process used ctrl_handler() to cleanup itself,
Is that explicit cleanup required? All I can see is free(startptr) and that pointer would usually be invalid as I mentioned earlier. If the process crashes while running fill_cache() and thus not get a chance to run free(startptr) then the OS would release the memory, no?
but the parent process cleanup the child process with ctrlc_handler() properly. Also, avoid using signal(). fill_buf.c：run_fill_buf() 201 /* set up ctrl-c handler */ 202 if (signal(SIGINT, ctrl_handler) == SIG_ERR) 203 printf("Failed to catch SIGINT!\n");
I removed ctrl_handler() and ran resctrl_tests with and without the kselftest framework. There is no problem.
Thank you. I only used the CAT test when I found the issue.
Reinette
Hi Reinette,
On 11/24/2022 12:17 AM, Shaopeng Tan (Fujitsu) wrote:
Hi Reinette,
On 11/16/2022 5:05 PM, Shaopeng Tan wrote:
After creating a child process with fork() in CAT test, if there is an error occurs or such as a SIGINT signal is received, the parent process will be terminated immediately, but the child process will not be killed and also umount_resctrlfs() will not be called.
Add a signal handler like other tests to kill child process, umount resctrlfs, cleanup result files, etc. if an error occurs in parent process. To use ctrlc_handler() of other tests to kill child process(bm_pid), using global bm_pid instead of local bm_pid.
Wait for child process to be killed if an error occurs in child process.
Reviewed-by: Shuah Khan skhan@linuxfoundation.org Signed-off-by: Shaopeng Tan tan.shaopeng@jp.fujitsu.com
tools/testing/selftests/resctrl/cat_test.c | 30
++++++++++++++--------
1 file changed, 20 insertions(+), 10 deletions(-)
diff --git a/tools/testing/selftests/resctrl/cat_test.c
b/tools/testing/selftests/resctrl/cat_test.c
index 6a8306b0a109..1f8f5cf94e95 100644 --- a/tools/testing/selftests/resctrl/cat_test.c +++ b/tools/testing/selftests/resctrl/cat_test.c @@ -100,10 +100,10 @@ void cat_test_cleanup(void)
int cat_perf_miss_val(int cpu_no, int n, char *cache_type) {
- struct sigaction sigact; 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,17 +181,25 @@ 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 {
/** Register CTRL-C handler for parent, as it has to kill* child process 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");Why keep going at this point?
I tried this change but I was not able to trigger ctrlc_handler(). It
I tested this change using kselftest framework, In this case, the timeout command sent a SIGTERM signal, and then ctrlc_handler() was triggered. Since the handling of SIGINT and SIGHUP signals is overridden in run_fill_buf(), ctrl_handler() may be called if ctrl-c is received.
I tested this by running "resctrl_tests -t cat" and when doing so this change does not behave as described.
Yes, the fix of v4 cannot satisfy "resctrl_tests -t cat"". I will add new fix in next version.
seems that the handler is changed soon after (see cat_val()->run_fill_buf()) and ctrl_handler() (note the subtle name difference) is run instead when a SIGINT is received. The value of ctrl_handler() is not clear to me though, and it could even be argued that it is broken because it starts with free(startptr) and startptr would likely already be free'd at this point without resetting its value to NULL.
From what I understand ctrl_handler() and its installation from run_fill_buf() could be removed so that it does not override what is being done with this change. Otherwise, from what I can tell, this new handler will never run.
I think removing ctrl_handler() is fine. In CAT test, it overrides ctrlc_handler(). In other tests(CMT,MBA,MBM), the child process used ctrl_handler() to cleanup itself,
Is that explicit cleanup required? All I can see is free(startptr) and that pointer would usually be invalid as I mentioned earlier. If the process crashes while running fill_cache() and thus not get a chance to run free(startptr) then the OS would release the memory, no?
Sorry, my explanation was not clear. I agree with you, I think removing ctrl_handler() is OK.
but the parent process cleanup the child process with ctrlc_handler()
properly.
Also, avoid using signal(). fill_buf.c：run_fill_buf() 201 /* set up ctrl-c handler */ 202 if (signal(SIGINT, ctrl_handler) == SIG_ERR) 203 printf("Failed to catch SIGINT!\n");
I removed ctrl_handler() and ran resctrl_tests with and without the kselftest
framework.
There is no problem.
Thank you. I only used the CAT test when I found the issue.
Removing ctrl_handler() is only part of the fix in the next version(v5). All fixes as follows.
--- a/tools/testing/selftests/resctrl/cat_test.c +++ b/tools/testing/selftests/resctrl/cat_test.c @@ -98,12 +98,17 @@ void cat_test_cleanup(void) remove(RESULT_FILE_NAME2); }
+static void ctrlc_handler_child(int signum, siginfo_t *info, void *ptr) +{ + exit(EXIT_SUCCESS); +} + int cat_perf_miss_val(int cpu_no, int n, char *cache_type) { + struct sigaction sigact; 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,17 +186,33 @@ 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; + + sigfillset(&sigact.sa_mask); + sigact.sa_sigaction = ctrlc_handler_child; + sigact.sa_flags = SA_SIGINFO; + if (sigaction(SIGINT, &sigact, NULL) || + sigaction(SIGTERM, &sigact, NULL) || + sigaction(SIGHUP, &sigact, NULL)) + perror("# sigaction"); + } else { + /* + * Register CTRL-C handler for parent, as it has to kill + * child process 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"); }
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 */ @@ -199,9 +220,11 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type) pipe_message = 1; if (write(pipefd[1], &pipe_message, sizeof(pipe_message)) < sizeof(pipe_message)) { - close(pipefd[1]); + /* + * 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]); @@ -226,5 +249,5 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type) if (bm_pid) umount_resctrlfs();
- return 0; + 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) {
Best regards, Shaopeng Tan
Hi Shaopeng,
On 11/30/2022 12:32 AM, Shaopeng Tan (Fujitsu) wrote:
Removing ctrl_handler() is only part of the fix in the next version(v5). All fixes as follows.
--- a/tools/testing/selftests/resctrl/cat_test.c +++ b/tools/testing/selftests/resctrl/cat_test.c @@ -98,12 +98,17 @@ void cat_test_cleanup(void) remove(RESULT_FILE_NAME2); }
+static void ctrlc_handler_child(int signum, siginfo_t *info, void *ptr) +{
exit(EXIT_SUCCESS);+}
Could you please elaborate why this is necessary?
Reinette
Hi Reinette,
On 11/30/2022 12:32 AM, Shaopeng Tan (Fujitsu) wrote:
Removing ctrl_handler() is only part of the fix in the next version(v5). All fixes as follows.
--- a/tools/testing/selftests/resctrl/cat_test.c +++ b/tools/testing/selftests/resctrl/cat_test.c @@ -98,12 +98,17 @@ void cat_test_cleanup(void) remove(RESULT_FILE_NAME2); }
+static void ctrlc_handler_child(int signum, siginfo_t *info, void +*ptr) {
exit(EXIT_SUCCESS);+}
Could you please elaborate why this is necessary?
If enter "ctrl-c" when running "resctrl_tests -t cat", SIGINT will be sent to all processes (parent&child).
At this time, the child process receives a SIGINT signal, but does not take any action. In this case the parent process may not call ctrlc_handler() as expected. Therefore, ctrlc_handler_child() is necessary.
It may be better to ignore the signal, then code can be simple as follows. ---- if (bm_pid == 0) { param.mask = l_mask_1; strcpy(param.ctrlgrp, "c1"); strcpy(param.mongrp, "m1"); param.span = cache_size * n / count_of_bits; strcpy(param.filename, RESULT_FILE_NAME1); param.num_of_runs = 0; param.cpu_no = sibling_cpu_no; /* Ignore the signal,and wait to be cleaned up by the parent process */ sigfillset(&sigact.sa_mask); sigact.sa_handler = SIG_IGN; //sigact.sa_sigaction = ctrlc_handler_child; //delete if (sigaction(SIGINT, &sigact, NULL) || sigaction(SIGTERM, &sigact, NULL) || sigaction(SIGHUP, &sigact, NULL)) perror("# sigaction"); } else { ----
Best regards, Shaopeng Tan
Hi Shaopeng,
On 12/1/2022 12:20 AM, Shaopeng Tan (Fujitsu) wrote:
Hi Reinette,
On 11/30/2022 12:32 AM, Shaopeng Tan (Fujitsu) wrote:
Removing ctrl_handler() is only part of the fix in the next version(v5). All fixes as follows.
--- a/tools/testing/selftests/resctrl/cat_test.c +++ b/tools/testing/selftests/resctrl/cat_test.c @@ -98,12 +98,17 @@ void cat_test_cleanup(void) remove(RESULT_FILE_NAME2); }
+static void ctrlc_handler_child(int signum, siginfo_t *info, void +*ptr) {
exit(EXIT_SUCCESS);+}
Could you please elaborate why this is necessary?
If enter "ctrl-c" when running "resctrl_tests -t cat", SIGINT will be sent to all processes (parent&child).
At this time, the child process receives a SIGINT signal, but does not take any action. In this case the parent process may not call ctrlc_handler() as expected.
Apologies, but I am not able to follow. My understanding is that the ideal in working an failing case is for the parent to kill the child. Could you please elaborate why the ctrlc_handler() may not be called?
Therefore, ctrlc_handler_child() is necessary.
It may be better to ignore the signal, then code can be simple as follows.
if (bm_pid == 0) { param.mask = l_mask_1; strcpy(param.ctrlgrp, "c1"); strcpy(param.mongrp, "m1"); param.span = cache_size * n / count_of_bits; strcpy(param.filename, RESULT_FILE_NAME1); param.num_of_runs = 0; param.cpu_no = sibling_cpu_no; /* Ignore the signal,and wait to be cleaned up by the parent process */ sigfillset(&sigact.sa_mask); sigact.sa_handler = SIG_IGN; //sigact.sa_sigaction = ctrlc_handler_child; //delete if (sigaction(SIGINT, &sigact, NULL) || sigaction(SIGTERM, &sigact, NULL) || sigaction(SIGHUP, &sigact, NULL)) perror("# sigaction"); } else {
Reinette
Hi Reinette
On 12/1/2022 12:20 AM, Shaopeng Tan (Fujitsu) wrote:
Hi Reinette,
On 11/30/2022 12:32 AM, Shaopeng Tan (Fujitsu) wrote:
Removing ctrl_handler() is only part of the fix in the next version(v5). All fixes as follows.
--- a/tools/testing/selftests/resctrl/cat_test.c +++ b/tools/testing/selftests/resctrl/cat_test.c @@ -98,12 +98,17 @@ void cat_test_cleanup(void) remove(RESULT_FILE_NAME2); }
+static void ctrlc_handler_child(int signum, siginfo_t *info, void +*ptr) {
exit(EXIT_SUCCESS);+}
Could you please elaborate why this is necessary?
If enter "ctrl-c" when running "resctrl_tests -t cat", SIGINT will be sent to all processes (parent&child).
At this time, the child process receives a SIGINT signal, but does not take any
action.
In this case the parent process may not call ctrlc_handler() as expected.
Apologies, but I am not able to follow. My understanding is that the ideal in working an failing case is for the parent to kill the child. Could you please elaborate why the ctrlc_handler() may not be called?
Apologies for the late replay.
The problem is that at the time of running CAT test, previous ctrlc_handler from MBM/MBA/CMT test will be inherited to child process.
Let me explain in detail: In resctrl_tests,the default run order of the tests is MBM->MBA->CMT->CAT. When running MBM, MBA, CMT, signal handler(ctrlc_handler) was set to the parent process. After these tests, when fork() is executed in CAT, the signal handler set by MBM/MBA/CMT is inherited by the parent&child process of CAT. At this time, if "ctrl+c" SIGINT is sent to parent&child process, according to the inherited signal handler, the child process may kill parent process before parent process kills child process. Therefore, when running all tests(MBM->MBA->CMT->CAT), signal handler of child process need to be overridden in CAT.
Also, when running CAT test only, since there are no signal handler that can be inherited from other tests, signal handler of parent process need to be set.
Therefore, ctrlc_handler_child() is necessary.
It may be better to ignore the signal, then code can be simple as follows.
if (bm_pid == 0) { param.mask = l_mask_1; strcpy(param.ctrlgrp, "c1"); strcpy(param.mongrp, "m1"); param.span = cache_size * n / count_of_bits; strcpy(param.filename, RESULT_FILE_NAME1); param.num_of_runs = 0; param.cpu_no = sibling_cpu_no; /* Ignore the signal,and wait to be cleaned up by the parentprocess */
sigfillset(&sigact.sa_mask); sigact.sa_handler = SIG_IGN; //sigact.sa_sigaction = ctrlc_handler_child; //delete if (sigaction(SIGINT, &sigact, NULL) || sigaction(SIGTERM, &sigact, NULL) || sigaction(SIGHUP, &sigact, NULL)) perror("# sigaction"); } else {
Best regards, Shaopeng
Hi Shaopeng,
On 12/16/2022 12:20 AM, Shaopeng Tan (Fujitsu) wrote:
Hi Reinette
On 12/1/2022 12:20 AM, Shaopeng Tan (Fujitsu) wrote:
Hi Reinette,
On 11/30/2022 12:32 AM, Shaopeng Tan (Fujitsu) wrote:
Removing ctrl_handler() is only part of the fix in the next version(v5). All fixes as follows.
--- a/tools/testing/selftests/resctrl/cat_test.c +++ b/tools/testing/selftests/resctrl/cat_test.c @@ -98,12 +98,17 @@ void cat_test_cleanup(void) remove(RESULT_FILE_NAME2); }
+static void ctrlc_handler_child(int signum, siginfo_t *info, void +*ptr) {
exit(EXIT_SUCCESS);+}
Could you please elaborate why this is necessary?
If enter "ctrl-c" when running "resctrl_tests -t cat", SIGINT will be sent to all processes (parent&child).
At this time, the child process receives a SIGINT signal, but does not take any
action.
In this case the parent process may not call ctrlc_handler() as expected.
Apologies, but I am not able to follow. My understanding is that the ideal in working an failing case is for the parent to kill the child. Could you please elaborate why the ctrlc_handler() may not be called?
Apologies for the late replay.
The problem is that at the time of running CAT test, previous ctrlc_handler from MBM/MBA/CMT test will be inherited to child process.
Let me explain in detail: In resctrl_tests,the default run order of the tests is MBM->MBA->CMT->CAT. When running MBM, MBA, CMT, signal handler(ctrlc_handler) was set to the parent process. After these tests, when fork() is executed in CAT, the signal handler set by MBM/MBA/CMT is inherited by the parent&child process of CAT. At this time, if "ctrl+c" SIGINT is sent to parent&child process, according to the inherited signal handler, the child process may kill parent process before parent process kills child process. Therefore, when running all tests(MBM->MBA->CMT->CAT), signal handler of child process need to be overridden in CAT.
Thank you for the additional details. I do not think that this should be handled in the CAT test. The CAT test should not need to work around leftovers from the other tests, that will just leave us with harder code to maintain. Instead, I think this should be addressed in resctrl_val() where the signal handler is set for the MBM/MBA/CMT tests. That is, when the test is complete and the test case specific signal handler no longer needed (after test itself kills the child and handler thus no longer needed), the signal handler should be reset to SIG_DFL.
This should give the CAT test a clean slate to work with.
Also, when running CAT test only, since there are no signal handler that can be inherited from other tests, signal handler of parent process need to be set.
Yes, this is clear. It is the child signal handler that was confusing to me.
Reinette
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.
Signed-off-by: Shaopeng Tan tan.shaopeng@jp.fujitsu.com --- 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)
Hi Shaopeng,
On 11/16/2022 5:05 PM, Shaopeng Tan wrote:
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.
Signed-off-by: Shaopeng Tan tan.shaopeng@jp.fujitsu.com
Thank you very much.
Reviewed-by: Reinette Chatre reinette.chatre@intel.com
Reinette
linux-kselftest-mirror@lists.linaro.org
-
Reinette Chatre -
Shaopeng Tan -
Shaopeng Tan (Fujitsu)