| Message ID | 20230131054655.396270-5-tan.shaopeng@jp.fujitsu.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Some improvements of resctrl selftest | expand |
Hi Shaopeng, On 1/30/2023 9:46 PM, Shaopeng Tan wrote: > After creating a child process with fork() in CAT test, if 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. > Great changelog. ... > @@ -181,28 +180,31 @@ 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); > > 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) > @@ -222,9 +224,11 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type) > kill(bm_pid, SIGKILL); > } > > + signal_handler_unregister(); I expected the code to be symmetrical but found that the signal handler is registered by parent, but unregistered by both the parent and the child. Could signal_handler_unregister() be moved to the parent portion of the "if" statement above? ... > --- a/tools/testing/selftests/resctrl/resctrl_val.c > +++ b/tools/testing/selftests/resctrl/resctrl_val.c > @@ -476,6 +476,45 @@ void ctrlc_handler(int signum, siginfo_t *info, void *ptr) > exit(EXIT_SUCCESS); > } > > +/* > + * Register CTRL-C handler for parent, as it has to kill > + * child process before exiting > + */ > +int signal_handler_register(void) > +{ > + 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 = -1; > + } > + return ret; > +} > + > +/* > + * Reset signal handler to SIG_DFL. > + * Non-Vaule return because the caller should keep Typo in "Non-Vaule" > + * the error code of other path even if sigaction fails. > + */ > +void signal_handler_unregister(void) > +{ > + 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"); > + } > +} > + > /* > * print_results_bw: the memory bandwidth results are stored in a file > * @filename: file that stores the results > @@ -671,39 +710,28 @@ 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; > - goto out; > - } > + ret = signal_handler_register(); > + if (ret) > + goto out1; Please do not use generic "out1" and "out2" goto labels. Could you please change them to reflect what is done at that exit? You could keep "out" but "out2" could be renamed to "unregister" or something more appropriate. ... > @@ -761,7 +789,9 @@ int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param) > } > } > > -out: > +out2: > + signal_handler_unregister(); > +out1: > kill(bm_pid, SIGKILL); > umount_resctrlfs(); > Thank you Reinette
On Tue, 31 Jan 2023, Shaopeng Tan wrote: > After creating a child process with fork() in CAT test, if 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 <tan.shaopeng@jp.fujitsu.com> > --- > tools/testing/selftests/resctrl/cat_test.c | 28 ++++---- > tools/testing/selftests/resctrl/fill_buf.c | 14 ---- > tools/testing/selftests/resctrl/resctrl.h | 2 + > tools/testing/selftests/resctrl/resctrl_val.c | 70 +++++++++++++------ > 4 files changed, 68 insertions(+), 46 deletions(-) > > diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c > index 6a8306b0a109..3524fa88e3a4 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,31 @@ 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); > > ret = cat_val(¶m); > - if (ret) > - return ret; > - > - ret = check_results(¶m); > - if (ret) > - return ret; > + if (ret == 0) > + ret = check_results(¶m); It would be take this program flow fix out of the signal handler change into a separate change.
Hi Ilpo, > On Tue, 31 Jan 2023, Shaopeng Tan wrote: > > > After creating a child process with fork() in CAT test, if 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 <tan.shaopeng@jp.fujitsu.com> > > --- > > tools/testing/selftests/resctrl/cat_test.c | 28 ++++---- > > tools/testing/selftests/resctrl/fill_buf.c | 14 ---- > > tools/testing/selftests/resctrl/resctrl.h | 2 + > > tools/testing/selftests/resctrl/resctrl_val.c | 70 > > +++++++++++++------ > > 4 files changed, 68 insertions(+), 46 deletions(-) > > > > diff --git a/tools/testing/selftests/resctrl/cat_test.c > > b/tools/testing/selftests/resctrl/cat_test.c > > index 6a8306b0a109..3524fa88e3a4 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,31 @@ 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); > > > > > ret = cat_val(¶m); > > - if (ret) > > - return ret; > > - > > - ret = check_results(¶m); > > - if (ret) > > - return ret; > > + if (ret == 0) > > + ret = check_results(¶m); > > It would be take this program flow fix out of the signal handler change into a > separate change. Do you mean this fix should be separated into two patches? To make the child process wait to be killed by parent process in any case(an error occurred or a signal was received), I fixed it like this. This fix was discussed here. https://lore.kernel.org/lkml/2ab9ca20-c757-7dd8-b770-2b84d171cbfb@intel.com/ Best regards, Shaopeng TAN
On Tue, 7 Feb 2023, Shaopeng Tan (Fujitsu) wrote: > > On Tue, 31 Jan 2023, Shaopeng Tan wrote: > > > > > After creating a child process with fork() in CAT test, if 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 <tan.shaopeng@jp.fujitsu.com> > > > --- > > > ret = cat_val(¶m); > > > - if (ret) > > > - return ret; > > > - > > > - ret = check_results(¶m); > > > - if (ret) > > > - return ret; > > > + if (ret == 0) > > > + ret = check_results(¶m); > > > > It would be take this program flow fix out of the signal handler change into a > > separate change. > > Do you mean this fix should be separated into two patches? Yes. Currently, I see your patch doing (mainly) two things: 1) cleaning up the messy signal handler logic 2) fixing the early return in case of error from cat_val() or check_results() Both are good changes and both are needed to fully fix things. But (IMHO) those are indepedent enough that it would warrant to split this change into two.
On Tue, 31 Jan 2023, Shaopeng Tan wrote: > After creating a child process with fork() in CAT test, if 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 <tan.shaopeng@jp.fujitsu.com> > --- > 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"); If the write error is ignored here, won't it just lead to parent hanging forever waiting for the child to send the message through the pipe which will never come?
Hi Ilpo, > On Tue, 31 Jan 2023, Shaopeng Tan wrote: > > > After creating a child process with fork() in CAT test, if 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 <tan.shaopeng@jp.fujitsu.com> > > --- > > > 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"); > > If the write error is ignored here, won't it just lead to parent hanging forever > waiting for the child to send the message through the pipe which will never > come? If the write error is ignored here, the pipe will be closed by "close(pipefd[1]);" and child process will wait to be killed by "while(1)". --- - return errno; - } close(pipefd[1]); while (1) --- If all file descriptors referring to the write end of a pipe have been closed, then an attempt to read(2) from the pipe will see end-of-file (read(2) will return 0). Then, "perror("# failed reading from child process");" occurs. --- } else { /* Parent waits for child to be ready. */ close(pipefd[1]); pipe_message = 0; while (pipe_message != 1) { if (read(pipefd[0], &pipe_message, sizeof(pipe_message)) < sizeof(pipe_message)) { perror("# failed reading from child process"); break; } } close(pipefd[0]); kill(bm_pid, SIGKILL); signal_handler_unregister(); } --- Best regards, Shaopeng TAN
Hi Ilpo, > On Tue, 7 Feb 2023, Shaopeng Tan (Fujitsu) wrote: > > > > On Tue, 31 Jan 2023, Shaopeng Tan wrote: > > > > > > > After creating a child process with fork() in CAT test, if 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 <tan.shaopeng@jp.fujitsu.com> > > > > --- > > > > > ret = cat_val(¶m); > > > > - if (ret) > > > > - return ret; > > > > - > > > > - ret = check_results(¶m); > > > > - if (ret) > > > > - return ret; > > > > + if (ret == 0) > > > > + ret = check_results(¶m); > > > > > > It would be take this program flow fix out of the signal handler > > > change into a separate change. > > > > Do you mean this fix should be separated into two patches? > > Yes. > > Currently, I see your patch doing (mainly) two things: > 1) cleaning up the messy signal handler logic > 2) fixing the early return in case of error from cat_val() or > check_results() > > Both are good changes and both are needed to fully fix things. But (IMHO) > those are indepedent enough that it would warrant to split this change into two. Thanks for your advice, I will split it in next version Best regards, Shaopeng TAN
On Wed, 8 Feb 2023, Shaopeng Tan (Fujitsu) wrote: > > On Tue, 31 Jan 2023, Shaopeng Tan wrote: > > > > > After creating a child process with fork() in CAT test, if 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 <tan.shaopeng@jp.fujitsu.com> > > > --- > > > > > 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"); > > > > If the write error is ignored here, won't it just lead to parent hanging forever > > waiting for the child to send the message through the pipe which will never > > come? > > If the write error is ignored here, the pipe will be closed by "close(pipefd[1]);" and child process will wait to be killed by "while(1)". > --- > - return errno; > - } > > close(pipefd[1]); > while (1) > --- > > If all file descriptors referring to the write end of a pipe have been closed, > then an attempt to read(2) from the pipe will see end-of-file (read(2) will return 0). > Then, "perror("# failed reading from child process");" occurs. > --- > } else { > /* Parent waits for child to be ready. */ > close(pipefd[1]); > pipe_message = 0; > while (pipe_message != 1) { > if (read(pipefd[0], &pipe_message, > sizeof(pipe_message)) < sizeof(pipe_message)) { > perror("# failed reading from child process"); > break; > } > } > close(pipefd[0]); > kill(bm_pid, SIGKILL); > signal_handler_unregister(); > } Ah, indeed read() will pick up the close event. So your code seem fine after all.
diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c index 6a8306b0a109..3524fa88e3a4 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,31 @@ 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); 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) @@ -222,9 +224,11 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type) kill(bm_pid, SIGKILL); } + signal_handler_unregister(); +out: cat_test_cleanup(); 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) { diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h index f0ded31fb3c7..92b59d2f603d 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); +void 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..c2be2f2505a8 100644 --- a/tools/testing/selftests/resctrl/resctrl_val.c +++ b/tools/testing/selftests/resctrl/resctrl_val.c @@ -476,6 +476,45 @@ void ctrlc_handler(int signum, siginfo_t *info, void *ptr) exit(EXIT_SUCCESS); } +/* + * Register CTRL-C handler for parent, as it has to kill + * child process before exiting + */ +int signal_handler_register(void) +{ + 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 = -1; + } + return ret; +} + +/* + * Reset signal handler to SIG_DFL. + * Non-Vaule return because the caller should keep + * the error code of other path even if sigaction fails. + */ +void signal_handler_unregister(void) +{ + 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"); + } +} + /* * print_results_bw: the memory bandwidth results are stored in a file * @filename: file that stores the results @@ -671,39 +710,28 @@ 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; - goto out; - } + ret = signal_handler_register(); + if (ret) + goto out1; value.sival_ptr = benchmark_cmd; /* Taskset benchmark to specified cpu */ ret = taskset_benchmark(bm_pid, param->cpu_no); if (ret) - goto out; + goto out2; /* 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 out; + goto out2; 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 out; + goto out2; initialize_mem_bw_resctrl(param->ctrlgrp, param->mongrp, param->cpu_no, resctrl_val); @@ -718,7 +746,7 @@ int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param) sizeof(pipe_message)) { perror("# failed reading message from child process"); close(pipefd[0]); - goto out; + goto out2; } } close(pipefd[0]); @@ -727,7 +755,7 @@ int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param) if (sigqueue(bm_pid, SIGUSR1, value) == -1) { perror("# sigqueue SIGUSR1 to child"); ret = errno; - goto out; + goto out2; } /* Give benchmark enough time to fully run */ @@ -761,7 +789,9 @@ int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param) } } -out: +out2: + signal_handler_unregister(); +out1: kill(bm_pid, SIGKILL); umount_resctrlfs();
After creating a child process with fork() in CAT test, if 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 <tan.shaopeng@jp.fujitsu.com> --- tools/testing/selftests/resctrl/cat_test.c | 28 ++++---- tools/testing/selftests/resctrl/fill_buf.c | 14 ---- tools/testing/selftests/resctrl/resctrl.h | 2 + tools/testing/selftests/resctrl/resctrl_val.c | 70 +++++++++++++------ 4 files changed, 68 insertions(+), 46 deletions(-)