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@xxxxxxxxxxxxxxxxxxx> > > Signed-off-by: Shaopeng Tan <tan.shaopeng@xxxxxxxxxxxxxx> > > --- > > 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