| Message ID | 20220216022641.2998318-2-tan.shaopeng@jp.fujitsu.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f54b3278164405fdd4f5f1b53f0d04e34ade27a6 |
| Headers | show |
| Series | selftests/resctrl: Add resctrl_tests into kselftest set | expand |
On 2/15/22 7:26 PM, Shaopeng Tan wrote: > In kselftest framework, a sub test is run using the timeout utility > and it will send SIGTERM to the test upon timeout. > > In resctrl_tests, a child process is created by fork() to > run benchmark but SIGTERM is not set in sigaction(). > If SIGTERM signal is received, the parent process will be killed, > but the child process still exists. > > kill child process before parent process terminates > if SIGTERM signal is received. > > Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com> > --- > Some important feedbacks from v1&v2 are addressed as follows: > > - Change the order so that current patch 3/3 becomes 1/3. Since without > the SIGTERM fix, the test would hang if run from the kselftest framework. > => I changed the order and the SIGTERM fix now becomes patch [1/5]. > > - Describe that the test is run using the timeout utility and > it will send SIGTERM to the test upon timeout. > => I updated the changelog to include this information. > > - Describe changes in imperative mood, and address this in all patches. > See Documentation/process/submitting-patches.rst for more details. > => I described all my patches' changelog in imperative mood and > deleted "This commit". > > - + sigaction(SIGTERM, &sigact, NULL) || > This snippet is preceded with a comment that describes its usage > you could also update it with the expanded use of the kselftest framework. > => I don't think it is necessary to add other comments. > Since the current comment already states "Register CTRL-C handler for parent, > as it has to kill benchmark before exiting", So, when SIGTERM comes, > the benchmark(child process) should be killed before parent process terminates, > but it was missing. > > tools/testing/selftests/resctrl/resctrl_val.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c > index 95224345c78e..b32b96356ec7 100644 > --- a/tools/testing/selftests/resctrl/resctrl_val.c > +++ b/tools/testing/selftests/resctrl/resctrl_val.c > @@ -678,6 +678,7 @@ int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param) > 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; > This looks good to me. Reviewed-by: Shuah Khan <skhan@linuxfoundation.org> thanks, -- Shuah
diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c index 95224345c78e..b32b96356ec7 100644 --- a/tools/testing/selftests/resctrl/resctrl_val.c +++ b/tools/testing/selftests/resctrl/resctrl_val.c @@ -678,6 +678,7 @@ int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param) 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;
In kselftest framework, a sub test is run using the timeout utility and it will send SIGTERM to the test upon timeout. In resctrl_tests, a child process is created by fork() to run benchmark but SIGTERM is not set in sigaction(). If SIGTERM signal is received, the parent process will be killed, but the child process still exists. kill child process before parent process terminates if SIGTERM signal is received. Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com> --- Some important feedbacks from v1&v2 are addressed as follows: - Change the order so that current patch 3/3 becomes 1/3. Since without the SIGTERM fix, the test would hang if run from the kselftest framework. => I changed the order and the SIGTERM fix now becomes patch [1/5]. - Describe that the test is run using the timeout utility and it will send SIGTERM to the test upon timeout. => I updated the changelog to include this information. - Describe changes in imperative mood, and address this in all patches. See Documentation/process/submitting-patches.rst for more details. => I described all my patches' changelog in imperative mood and deleted "This commit". - + sigaction(SIGTERM, &sigact, NULL) || This snippet is preceded with a comment that describes its usage you could also update it with the expanded use of the kselftest framework. => I don't think it is necessary to add other comments. Since the current comment already states "Register CTRL-C handler for parent, as it has to kill benchmark before exiting", So, when SIGTERM comes, the benchmark(child process) should be killed before parent process terminates, but it was missing. tools/testing/selftests/resctrl/resctrl_val.c | 1 + 1 file changed, 1 insertion(+)