Re: [PATCH v1 1/2] perf test: Add per-test timeout support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] perf test: overly broad waitpid() condition hangs test suite on normal failures
- [Medium] perf test: child tests inherit SA_RESTART SIGALRM handler, breaking alarm()
- [Medium] perf test: SIGKILL escalation for timed-out process groups is aborted prematurely
--

commit efae32fde876d5c28dcaff79950fdf6547005215
Author: Ian Rogers <irogers@google.com>

perf test: Add per-test timeout support

This commit adds a new per-test timeout to perf test, defaulting to 60
seconds. Tests are placed into separate process groups, and timeouts are
enforced using an ITIMER_REAL periodic timer that sends SIGTERM followed
by a SIGKILL escalation after 1 second.

> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c
[ ... ]
> @@ -378,20 +395,31 @@ static int run_test_child(struct child_process *process)
>  
>  	check_leaks();
>  err_out:
> -	fflush(NULL);
>  	for (size_t i = 0; i < ARRAY_SIZE(signals); i++)
>  		signal(signals[i], SIG_DFL);

[Severity: Medium]
Does this omit SIGALRM from being reset to SIG_DFL?

Since the parent __cmd_test() sets up a no-op handler with SA_RESTART for
SIGALRM before forking, children will inherit that handler. Will builtin tests
that rely on the default SIGALRM behavior (like using alarm() to prevent
internal hangs) fail to terminate because the no-op handler overrides it?

> +	if (err == TEST_FAIL) {
> +		/*
> +		 * Wait for any spawned child processes (such as shell scripts
> +		 * running trap cleanup handlers on SIGTERM) to finish before
> +		 * exiting.
> +		 */
> +		while (waitpid(-1, NULL, 0) > 0 || errno == EINTR)
> +			;
> +	}

[Severity: High]
Can this loop hang the test suite indefinitely on normal failures?

This condition catches all normal test failures (err == TEST_FAIL), not just
tests interrupted by a timeout SIGTERM. If a test spawns child processes and
fails normally, it will block on waitpid(-1) because the child processes were
never signaled to exit.

Will this hang the entire test run until the global timeout triggers, or
deadlock completely if timeouts are disabled via the dont-fork path?

> +	fflush(NULL);
>  	return -err;
>  }
[ ... ]
> @@ -748,6 +792,47 @@ static void print_test_failure_snippet(FILE *fp, const char *buf)
>  	free(line_lens);
>  }
>  
> +static void cmd_test_alarm_handler(int sig __maybe_unused)
> +{
> +}
> +
> +static void check_child_timeout(struct child_test *child, const struct timespec *now)
> +{
> +	double elapsed;
> +	pid_t pid;
> +
> +	if (!test_timeout || !child || child->done || child->process.finished)
> +		return;

[Severity: Medium]
Does returning early when child->done is true bypass the SIGKILL escalation?

If the immediate child process exits after receiving SIGTERM, the parent
marks the entire test case as done (setting child->done to true in
finish_tests_parallel).

Will checking child->done here completely bypass the 1-second timer and the
kill(-pid, SIGKILL) call for the broader process group, leaving any
subprocesses that ignored SIGTERM to leak?

> +
> +	pid = child->process.pid;
> +	if (pid <= 0)
> +		return;
> +
> +	if (!child->timed_out) {
> +		elapsed = (now->tv_sec - child->start_time.tv_sec) +
> +			  (now->tv_nsec - child->start_time.tv_nsec) / 1000000000.0;
> +		if (elapsed >= test_timeout) {
> +			child->timed_out = true;
> +			child->timeout_time = *now;
> +			pr_debug3("Timeout (%us) for test %d.%d (pid %d), sending SIGTERM\n",
> +				  test_timeout, child->suite_num + 1,
> +				  child->test_case_num + 1, pid);
> +			kill(-pid, SIGTERM);
> +			kill(pid, SIGTERM);
> +		}
> +	} else if (!child->sigkill_sent) {
> +		elapsed = (now->tv_sec - child->timeout_time.tv_sec) +
> +			  (now->tv_nsec - child->timeout_time.tv_nsec) / 1000000000.0;
> +		if (elapsed >= 1.0) {
> +			child->sigkill_sent = true;
> +			pr_debug3("Sending SIGKILL to timed out test %d.%d (pid %d)\n",
> +				  child->suite_num + 1, child->test_case_num + 1, pid);
> +			kill(-pid, SIGKILL);
> +			kill(pid, SIGKILL);
> +		}
> +	}
> +}
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260928210144.3925224-1-irogers@google.com?part=1