Messages in this thread | | | Subject | Re: [PATCH v3 1/5] selftests/resctrl: Kill child process before parent process terminates if SIGTERM is received | From | Shuah Khan <> | Date | Fri, 18 Feb 2022 13:10:10 -0700 |
| |
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
| |