[v2] selftests/powerpc: Avoid remaining process/threads

Message ID 1533307039-13744-1-git-send-email-leitao@debian.org
State Superseded
Headers show
Series
  • [v2] selftests/powerpc: Avoid remaining process/threads
Related show

Checks

Context Check Description
snowpatch_ozlabs/build-ppc32 success Test build-ppc32 on branch next
snowpatch_ozlabs/build-ppc64e success Test build-ppc64e on branch next
snowpatch_ozlabs/build-ppc64be success Test build-ppc64be on branch next
snowpatch_ozlabs/build-ppc64le success Test build-ppc64le on branch next
snowpatch_ozlabs/checkpatch success Test checkpatch on branch next
snowpatch_ozlabs/apply_patch success next/apply_patch Successfully applied

Commit Message

Breno Leitao Aug. 3, 2018, 2:37 p.m.
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(-)

Comments

Michael Ellerman Aug. 6, 2018, 11:06 a.m. | #1
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
Breno Leitao Aug. 6, 2018, 6:24 p.m. | #2
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. :-)
Michael Ellerman Aug. 14, 2018, 4:16 a.m. | #3
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

Patch

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;
 	}