* [PATCH] powerpc/selftests: Avoid backgroud process/threads @ 2018-07-31 14:10 Breno Leitao 2018-08-03 10:36 ` Michael Ellerman 0 siblings, 1 reply; 8+ messages in thread From: Breno Leitao @ 2018-07-31 14:10 UTC (permalink / raw) To: linuxppc-dev; +Cc: Breno Leitao, Gustavo Romero Current tm-unavailable test runs for a long period (>120 seconds), and if it is interrupted, as pressing CRTL-C (SIGINT), the foreground process (harness) dies but the child process and threads continue to execute (with PPID = 1 now). In this case, you'd think the test is gone, but there are two threads being executed in background, one of the thread ('pong') consumes 100% of the CPU and the other one ('ping') dumps output message, from time to time, in the STDOUT, which is annoying. This patch simply gets the child process to be SIGTERMed when the parent dies. Signed-off-by: Breno Leitao <leitao@debian.org> Signed-off-by: Gustavo Romero <gromero@linux.vnet.ibm.com> --- tools/testing/selftests/powerpc/tm/tm-unavailable.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tools/testing/selftests/powerpc/tm/tm-unavailable.c b/tools/testing/selftests/powerpc/tm/tm-unavailable.c index 156c8e750259..c42f8b60063c 100644 --- a/tools/testing/selftests/powerpc/tm/tm-unavailable.c +++ b/tools/testing/selftests/powerpc/tm/tm-unavailable.c @@ -23,6 +23,8 @@ #include <stdbool.h> #include <pthread.h> #include <sched.h> +#include <signal.h> +#include <sys/prctl.h> #include "tm.h" @@ -342,6 +344,9 @@ int tm_unavailable_test(void) SKIP_IF(!have_htm()); + /* Send me SIGTERM if PPID is dead (as SIGINTed) */ + prctl(PR_SET_PDEATHSIG, SIGTERM); + /* Set only CPU 0 in the mask. Both threads will be bound to CPU 0. */ CPU_ZERO(&cpuset); CPU_SET(0, &cpuset); -- 2.16.3 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] powerpc/selftests: Avoid backgroud process/threads 2018-07-31 14:10 [PATCH] powerpc/selftests: Avoid backgroud process/threads Breno Leitao @ 2018-08-03 10:36 ` Michael Ellerman 2018-08-03 14:37 ` [PATCH v2] selftests/powerpc: Avoid remaining process/threads Breno Leitao 0 siblings, 1 reply; 8+ messages in thread From: Michael Ellerman @ 2018-08-03 10:36 UTC (permalink / raw) To: Breno Leitao, linuxppc-dev; +Cc: Breno Leitao, Gustavo Romero Breno Leitao <leitao@debian.org> writes: > Current tm-unavailable test runs for a long period (>120 seconds), and if it is > interrupted, as pressing CRTL-C (SIGINT), the foreground process (harness) dies > but the child process and threads continue to execute (with PPID = 1 now). > > In this case, you'd think the test is gone, but there are two threads being > executed in background, one of the thread ('pong') consumes 100% of the CPU and > the other one ('ping') dumps output message, from time to time, in the STDOUT, > which is annoying. > > This patch simply gets the child process to be SIGTERMed when the parent dies. Hmm, I think we should fix this in the harness if possible. In run_test() it does: /* Kill anything else in the process group that is still running */ kill(-pid, SIGTERM); But that doesn't work if the harness has been killed with Ctrl-C. I think the harness could have a SIGINT handler that basically does the above and then exits? cheers ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] selftests/powerpc: Avoid remaining process/threads 2018-08-03 10:36 ` Michael Ellerman @ 2018-08-03 14:37 ` Breno Leitao 2018-08-06 11:06 ` Michael Ellerman 0 siblings, 1 reply; 8+ messages in thread From: Breno Leitao @ 2018-08-03 14:37 UTC (permalink / raw) To: linuxppc-dev; +Cc: Breno Leitao, Gustavo Romero There are some powerpc selftests, as tm/tm-unavailable, that run for a long period (>120 seconds), and if it is interrupted, as pressing CRTL-C (SIGINT), the foreground process (harness) dies but the child process and threads continue to execute (with PPID = 1 now) in background. In this case, you'd think the whole test exited, but there are remaining threads and processes being executed in background. Sometimes these zoombies processes are doing annoying things, as consuming the whole CPU or dumping things to STDOUT. This patch fixes this problem by creating a SIGINT handler in the harness process, which will kill the child process group once a SIGINT is caught. Signed-off-by: Breno Leitao <leitao@debian.org> Signed-off-by: Gustavo Romero <gromero@linux.vnet.ibm.com> --- tools/testing/selftests/powerpc/harness.c | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/tools/testing/selftests/powerpc/harness.c b/tools/testing/selftests/powerpc/harness.c index 66d31de60b9a..06c51e8d8ccb 100644 --- a/tools/testing/selftests/powerpc/harness.c +++ b/tools/testing/selftests/powerpc/harness.c @@ -22,12 +22,12 @@ #define KILL_TIMEOUT 5 static uint64_t timeout = 120; +pid_t pid; int run_test(int (test_function)(void), char *name) { bool terminated; int rc, status; - pid_t pid; /* Make sure output is flushed before forking */ fflush(stdout); @@ -85,13 +85,16 @@ int run_test(int (test_function)(void), char *name) return status; } -static void alarm_handler(int signum) +static void sig_handler(int signum) { - /* Jut wake us up from waitpid */ + if (signum == SIGINT) + kill(-pid, SIGTERM); + + /* if SIGALRM, just wake us up from waitpid */ } -static struct sigaction alarm_action = { - .sa_handler = alarm_handler, +static struct sigaction sig_action = { + .sa_handler = sig_handler, }; void test_harness_set_timeout(uint64_t time) @@ -106,8 +109,14 @@ int test_harness(int (test_function)(void), char *name) test_start(name); test_set_git_version(GIT_VERSION); - if (sigaction(SIGALRM, &alarm_action, NULL)) { - perror("sigaction"); + if (sigaction(SIGINT, &sig_action, NULL)) { + perror("sigaction (sigint)"); + test_error(name); + return 1; + } + + if (sigaction(SIGALRM, &sig_action, NULL)) { + perror("sigaction (sigalrm)"); test_error(name); return 1; } -- 2.16.3 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] selftests/powerpc: Avoid remaining process/threads 2018-08-03 14:37 ` [PATCH v2] selftests/powerpc: Avoid remaining process/threads Breno Leitao @ 2018-08-06 11:06 ` Michael Ellerman 2018-08-06 18:24 ` Breno Leitao 0 siblings, 1 reply; 8+ messages in thread From: Michael Ellerman @ 2018-08-06 11:06 UTC (permalink / raw) To: Breno Leitao, linuxppc-dev; +Cc: Breno Leitao, Gustavo Romero Breno Leitao <leitao@debian.org> writes: > diff --git a/tools/testing/selftests/powerpc/harness.c b/tools/testing/selftests/powerpc/harness.c > index 66d31de60b9a..06c51e8d8ccb 100644 > --- a/tools/testing/selftests/powerpc/harness.c > +++ b/tools/testing/selftests/powerpc/harness.c > @@ -85,13 +85,16 @@ int run_test(int (test_function)(void), char *name) > return status; > } > > -static void alarm_handler(int signum) > +static void sig_handler(int signum) > { > - /* Jut wake us up from waitpid */ > + if (signum == SIGINT) > + kill(-pid, SIGTERM); I don't think we need to do that here, if we just return then we'll pop out of the waitpid() and go via the normal path. Can you test with the existing signal handler, but wired up to SIGINT? cheers ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] selftests/powerpc: Avoid remaining process/threads 2018-08-06 11:06 ` Michael Ellerman @ 2018-08-06 18:24 ` Breno Leitao 2018-08-07 14:15 ` [PATCH v3] selftests/powerpc: Kill child processes on SIGINT Breno Leitao 2018-08-14 4:16 ` [PATCH v2] selftests/powerpc: Avoid remaining process/threads Michael Ellerman 0 siblings, 2 replies; 8+ messages in thread From: Breno Leitao @ 2018-08-06 18:24 UTC (permalink / raw) To: Michael Ellerman, linuxppc-dev; +Cc: Gustavo Romero Hello Michael, On 08/06/2018 08:06 AM, Michael Ellerman wrote: > Breno Leitao <leitao@debian.org> writes: > >> diff --git a/tools/testing/selftests/powerpc/harness.c b/tools/testing/selftests/powerpc/harness.c >> index 66d31de60b9a..06c51e8d8ccb 100644 >> --- a/tools/testing/selftests/powerpc/harness.c >> +++ b/tools/testing/selftests/powerpc/harness.c >> @@ -85,13 +85,16 @@ int run_test(int (test_function)(void), char *name) >> return status; >> } >> >> -static void alarm_handler(int signum) >> +static void sig_handler(int signum) >> { >> - /* Jut wake us up from waitpid */ >> + if (signum == SIGINT) >> + kill(-pid, SIGTERM); > > I don't think we need to do that here, if we just return then we'll pop > out of the waitpid() and go via the normal path. Correct, if we press ^C while the parent process is waiting at waitpid(), then waitpid() syscall will be interrupted (EINTR) and never restarted again (unless we set sa_flags = SA_RESTART), thus, the code will restart to execute the next instruction when the signal handler is done, as we had skipped waitpid(). >From a theoretical point of view, the user can press ^C before the process executes waitpid() syscall. In this case and the process will not 'skip' the waitpid(), which will continue to wait. We can clearly force this behavior putting a sleep(1) before waitpid() and pressing ^C in the very first second, it will 'skip' the nanosleep() syscall instead of waitpid() which will be there, and the ^C will be ignored (thus not calling kill(-pid, SIGTERM)). >From a practical point of view, I will prepare a v3 patch. :-) ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v3] selftests/powerpc: Kill child processes on SIGINT 2018-08-06 18:24 ` Breno Leitao @ 2018-08-07 14:15 ` Breno Leitao 2018-08-13 11:23 ` [v3] " Michael Ellerman 2018-08-14 4:16 ` [PATCH v2] selftests/powerpc: Avoid remaining process/threads Michael Ellerman 1 sibling, 1 reply; 8+ messages in thread From: Breno Leitao @ 2018-08-07 14:15 UTC (permalink / raw) To: linuxppc-dev; +Cc: mpe, Breno Leitao, Gustavo Romero There are some powerpc selftests, as tm/tm-unavailable, that run for a long period (>120 seconds), and if it is interrupted, as pressing CRTL-C (SIGINT), the foreground process (harness) dies but the child process and threads continue to execute (with PPID = 1 now) in background. In this case, you'd think the whole test exited, but there are remaining threads and processes being executed in background. Sometimes these zombies processes are doing annoying things, as consuming the whole CPU or dumping things to STDOUT. This patch fixes this problem by attaching an empty signal handler to SIGINT in the harness process. This handler will interrupt (EINTR) the parent process waitpid() call, letting the code to follow through the normal flow, which will kill all the processes in the child process group. This patch also fixes a typo. Signed-off-by: Breno Leitao <leitao@debian.org> Signed-off-by: Gustavo Romero <gromero@linux.vnet.ibm.com> --- tools/testing/selftests/powerpc/harness.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/tools/testing/selftests/powerpc/harness.c b/tools/testing/selftests/powerpc/harness.c index 66d31de60b9a..9d7166dfad1e 100644 --- a/tools/testing/selftests/powerpc/harness.c +++ b/tools/testing/selftests/powerpc/harness.c @@ -85,13 +85,13 @@ int run_test(int (test_function)(void), char *name) return status; } -static void alarm_handler(int signum) +static void sig_handler(int signum) { - /* Jut wake us up from waitpid */ + /* Just wake us up from waitpid */ } -static struct sigaction alarm_action = { - .sa_handler = alarm_handler, +static struct sigaction sig_action = { + .sa_handler = sig_handler, }; void test_harness_set_timeout(uint64_t time) @@ -106,8 +106,14 @@ int test_harness(int (test_function)(void), char *name) test_start(name); test_set_git_version(GIT_VERSION); - if (sigaction(SIGALRM, &alarm_action, NULL)) { - perror("sigaction"); + if (sigaction(SIGINT, &sig_action, NULL)) { + perror("sigaction (sigint)"); + test_error(name); + return 1; + } + + if (sigaction(SIGALRM, &sig_action, NULL)) { + perror("sigaction (sigalrm)"); test_error(name); return 1; } -- 2.16.3 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [v3] selftests/powerpc: Kill child processes on SIGINT 2018-08-07 14:15 ` [PATCH v3] selftests/powerpc: Kill child processes on SIGINT Breno Leitao @ 2018-08-13 11:23 ` Michael Ellerman 0 siblings, 0 replies; 8+ messages in thread From: Michael Ellerman @ 2018-08-13 11:23 UTC (permalink / raw) To: Breno Leitao, linuxppc-dev; +Cc: Breno Leitao, Gustavo Romero On Tue, 2018-08-07 at 14:15:39 UTC, Breno Leitao wrote: > There are some powerpc selftests, as tm/tm-unavailable, that run for a long > period (>120 seconds), and if it is interrupted, as pressing CRTL-C > (SIGINT), the foreground process (harness) dies but the child process and > threads continue to execute (with PPID = 1 now) in background. > > In this case, you'd think the whole test exited, but there are remaining > threads and processes being executed in background. Sometimes these > zombies processes are doing annoying things, as consuming the whole CPU or > dumping things to STDOUT. > > This patch fixes this problem by attaching an empty signal handler to > SIGINT in the harness process. This handler will interrupt (EINTR) the > parent process waitpid() call, letting the code to follow through the > normal flow, which will kill all the processes in the child process group. > > This patch also fixes a typo. > > Signed-off-by: Breno Leitao <leitao@debian.org> > Signed-off-by: Gustavo Romero <gromero@linux.vnet.ibm.com> Applied to powerpc next, thanks. https://git.kernel.org/powerpc/c/7c27a26e1ed5a7dd709aa19685d2c9 cheers ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] selftests/powerpc: Avoid remaining process/threads 2018-08-06 18:24 ` Breno Leitao 2018-08-07 14:15 ` [PATCH v3] selftests/powerpc: Kill child processes on SIGINT Breno Leitao @ 2018-08-14 4:16 ` Michael Ellerman 1 sibling, 0 replies; 8+ messages in thread From: Michael Ellerman @ 2018-08-14 4:16 UTC (permalink / raw) To: Breno Leitao, linuxppc-dev; +Cc: Gustavo Romero Breno Leitao <leitao@debian.org> writes: > Hello Michael, > > On 08/06/2018 08:06 AM, Michael Ellerman wrote: >> Breno Leitao <leitao@debian.org> writes: >> >>> diff --git a/tools/testing/selftests/powerpc/harness.c b/tools/testing/selftests/powerpc/harness.c >>> index 66d31de60b9a..06c51e8d8ccb 100644 >>> --- a/tools/testing/selftests/powerpc/harness.c >>> +++ b/tools/testing/selftests/powerpc/harness.c >>> @@ -85,13 +85,16 @@ int run_test(int (test_function)(void), char *name) >>> return status; >>> } >>> >>> -static void alarm_handler(int signum) >>> +static void sig_handler(int signum) >>> { >>> - /* Jut wake us up from waitpid */ >>> + if (signum == SIGINT) >>> + kill(-pid, SIGTERM); >> >> I don't think we need to do that here, if we just return then we'll pop >> out of the waitpid() and go via the normal path. > > Correct, if we press ^C while the parent process is waiting at waitpid(), > then waitpid() syscall will be interrupted (EINTR) and never restarted again > (unless we set sa_flags = SA_RESTART), thus, the code will restart to execute > the next instruction when the signal handler is done, as we had skipped > waitpid(). > > From a theoretical point of view, the user can press ^C before the process > executes waitpid() syscall. In this case and the process will not 'skip' the > waitpid(), which will continue to wait. We can clearly force this behavior > putting a sleep(1) before waitpid() and pressing ^C in the very first > second, it will 'skip' the nanosleep() syscall instead of waitpid() which > will be there, and the ^C will be ignored (thus not calling kill(-pid, SIGTERM)). True. Though that race also exists vs us registering the SIGINT handler, so it's basically not solvable, the user can always press ^C before we're ready. cheers ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-08-14 4:16 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-07-31 14:10 [PATCH] powerpc/selftests: Avoid backgroud process/threads Breno Leitao 2018-08-03 10:36 ` Michael Ellerman 2018-08-03 14:37 ` [PATCH v2] selftests/powerpc: Avoid remaining process/threads Breno Leitao 2018-08-06 11:06 ` Michael Ellerman 2018-08-06 18:24 ` Breno Leitao 2018-08-07 14:15 ` [PATCH v3] selftests/powerpc: Kill child processes on SIGINT Breno Leitao 2018-08-13 11:23 ` [v3] " Michael Ellerman 2018-08-14 4:16 ` [PATCH v2] selftests/powerpc: Avoid remaining process/threads Michael Ellerman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).