Re: [PATCH v5 1/3] perf test: Add per-test timeout support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf test: pipe drained before kill_timed_out_group() kills subprocesses
--

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

perf test: Add per-test timeout support

This commit adds per-test timeout support to perf test, defaulting to 120
seconds and configurable via -t/--timeout. Forked test children are placed in
their own process groups so timed-out tests and their subprocesses can be
terminated and reported as timeouts in the test summary and JUnit output.

> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> index 880eece0bc123..b870ca0bd8262 100644
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c

[ ... ]

> @@ -815,6 +893,106 @@ static void print_test_failure_snippet(FILE *fp, const char *buf)
>  	free(line_lens);
>  }
>  

[ ... ]

> +static void kill_timed_out_group(struct child_test *child)
> +{
> +	if (!child->timed_out || child->process.finished || child->process.pid <= 0)
> +		return;
> +
> +	pr_debug3("Sending SIGKILL to remaining processes of timed out test %d.%d (pgid %d)\n",
> +		  child->suite_num + 1, child->test_case_num + 1, child->process.pid);
> +	kill(-child->process.pid, SIGKILL);
> +}

[ ... ]

> +static int reap_child_test(struct child_test *child)
> +{
> +	sigset_t oldset;
> +	int ret;
> +
> +	block_term_signals(&oldset);
> +	kill_timed_out_group(child);
> +	ret = finish_command(&child->process);
> +	child->process.pid = 0;
> +	sigprocmask(SIG_SETMASK, &oldset, NULL);
> +	return child->timed_out ? TEST_TIMEOUT : ret;
> +}
> +
>  static void finish_test(struct child_test **child_tests, int running_test, int child_test_num,
>  		int width)
>  {

[ ... ]

> @@ -904,11 +1088,15 @@ static void finish_test(struct child_test **child_tests, int running_test, int c
>  					err_done = false;
>  					buf[len] = '\0';
>  					strbuf_addstr_safe(&err_output, buf);
> +				} else if (pfds[0].revents & (POLLHUP | POLLERR | POLLNVAL)) {
> +					close(err);
> +					err = -1;
> +					child_test->process.err = -1;
>  				}
>  			}
>  		}
> -		if (err_done)
> -			err_done = check_if_command_finished(&child_test->process);
> +		if (err_done || child_test->timed_out)
> +			err_done = test_command_finished(child_test);
>  	}
>  	/* Drain any remaining data from the pipe. */
>  	if (err >= 0) {

[ ... ]

> @@ -925,8 +1113,7 @@ static void finish_test(struct child_test **child_tests, int running_test, int c
>  		fprintf(debug_file(), PERF_COLOR_DELETE_LINE);
>  	}
>  	/* Clean up child process. */
> -	ret = finish_command(&child_test->process);
> -	child_test->process.pid = 0;
> +	ret = reap_child_test(child_test);

[ ... ]

> @@ -1157,14 +1350,13 @@ static int finish_tests_parallel(struct child_test **child_tests, size_t num_tes
>  			if (!child || child->done)
>  				continue;
>  
> -			if (check_if_command_finished(&child->process)) {
> +			if (test_command_finished(child)) {
>  				if (child->process.err >= 0) {
>  					drain_child_process_err(child);
>  					close(child->process.err);
>  					child->process.err = -1;
>  				}
> -				child->result = finish_command(&child->process);
> -				child->process.pid = 0;
> +				child->result = reap_child_test(child);
>  				clock_gettime(CLOCK_MONOTONIC, &child->end_time);
>  				child->done = true;
>  			}

[Severity: Medium]
In tools/perf/tests/builtin-test.c, when running with verbose >= 1 so child
stdout and stderr are piped through process.err, both finish_test() and
finish_tests_parallel() drain the pipe in an unbounded read loop before
calling reap_child_test() -> kill_timed_out_group():

tools/perf/tests/builtin-test.c:finish_test() {
	...
		if (err_done || child_test->timed_out)
			err_done = test_command_finished(child_test);
	}
	/* Drain any remaining data from the pipe. */
	if (err >= 0) {
		char buf[512];
		ssize_t len;

		while ((len = read(err, buf, sizeof(buf) - 1)) > 0) {
			buf[len] = '\0';
			strbuf_addstr_safe(&err_output, buf);
		}
	}
	...
	ret = reap_child_test(child_test);
}

And in parallel mode:

tools/perf/tests/builtin-test.c:finish_tests_parallel() {
	...
	if (test_command_finished(child)) {
		if (child->process.err >= 0) {
			drain_child_process_err(child);
			close(child->process.err);
			child->process.err = -1;
		}
		child->result = reap_child_test(child);
	...
}

If a timed-out test's main child process exits after the initial termination
signal while a subprocess in its process group survives and continuously
writes to stdout or stderr, test_command_finished() returns true once the
main child becomes a zombie. However, kill_timed_out_group() has not yet been
called to terminate the remaining process group members.

Could a noisy surviving subprocess keep feeding the pipe so that the read()
loop in finish_test() or drain_child_process_err() never sees -EAGAIN or
end-of-file, hanging the test runner and growing err_output without bound
before reap_child_test() can call kill_timed_out_group()? Should
kill_timed_out_group() be invoked before draining the pipe when a timed-out
child has finished?

[ ... ]

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