Re: [PATCH v2 06/14] perf test: Refactor parallel poll loop to drain all pipes simultaneously

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Fallback check `kill(pid, 0)` returns 0 for zombie processes, potentially causing an infinite hang.
- [Medium] Memory leak of strbuf and loss of buffered stderr logs during signal-triggered error cleanup.
- [Medium] Memory leak of `pfds` and `pfd_indices` arrays when parallel tests are interrupted by a signal.
--

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

perf test: Refactor parallel poll loop to drain all pipes simultaneously

This patch implements finish_tests_parallel() to poll and drain output pipes
from all running children simultaneously. Reaping occurs out of order as
children finish, while final result printing remains strictly in order, speeding
up parallel test execution.

> diff --git a/tools/lib/subcmd/run-command.c b/tools/lib/subcmd/run-command.c
> index b7510f83209a0..4f8ec001b9486 100644
> --- a/tools/lib/subcmd/run-command.c
> +++ b/tools/lib/subcmd/run-command.c
> @@ -241,8 +241,18 @@ int check_if_command_finished(struct child_process *cmd)
>  	sprintf(filename, "/proc/%u/status", cmd->pid);
>  	status_file = fopen(filename, "r");
>  	if (status_file == NULL) {
> -		/* Open failed assume finish_command was called. */
> -		return true;
> +		/*
> +		 * fopen() can fail with ENOENT if the process has been reaped.
> +		 * It can also fail with EMFILE/ENFILE if RLIMIT_NOFILE is reached,
> +		 * or with EINTR/ENOMEM. Use kill(pid, 0) as a robust fallback
> +		 * to distinguish between active processes and dead ones without
> +		 * consuming file descriptors.
> +		 */
> +		if (errno == ENOENT)
> +			return 1;
> +		if (kill(cmd->pid, 0) == -1 && errno == ESRCH)
> +			return 1;
> +		return 0;

[Severity: High]
Can this cause an infinite hang if a process becomes a zombie?

If fopen("/proc/<pid>/status", "r") fails persistently (e.g. due to EACCES),
this fallback uses kill(cmd->pid, 0). However, kill() returns 0 for zombie
processes since they still exist in the process table. 

If this happens, check_if_command_finished() incorrectly returns 0. Over in
finish_tests_parallel(), the parent loop will see POLLHUP, close the child's
pipe, but because check_if_command_finished() still returns 0, finish_command()
is never called to reap the zombie.

With num_pfds at 0 and running_count > 0, does the parent enter an infinite
usleep(10 * 1000) loop?

>  	}
>  	while (fgets(status_line, sizeof(status_line), status_file) != NULL) {
>  		char *p;

[ ... ]

> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> index 2ccb52a776ccb..87c6a67beb10e 100644
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c
[ ... ]
> @@ -509,6 +512,192 @@ static void finish_test(struct child_test **child_tests, int running_test, int c
>  	zfree(&child_tests[running_test]);

[Severity: Medium]
Does this legacy cleanup path leak the newly added strbuf?

If a signal interrupts the test, siglongjmp() transfers control to the err_out
label in __cmd_test(), which calls finish_test() to clean up. finish_test()
uses zfree(&child_tests[running_test]) but doesn't call strbuf_release() on
child->err_output.

Since child->err_output is dynamically allocated in finish_tests_parallel(),
does this leak the internal buffer and lose any buffered stderr logs during
signal-triggered cleanup?

>  }
>  
> +static void drain_child_process_err(struct child_test *child)
> +{
[ ... ]
> +static int finish_tests_parallel(struct child_test **child_tests, size_t num_tests, int width)
> +{
> +	size_t next_to_print = 0;
> +	struct pollfd *pfds;
> +	size_t *pfd_indices;
> +	size_t num_pfds = 0;
> +	int last_running = -1;
> +	size_t i;
> +	int last_suite_printed = -1;
> +
> +	pfds = calloc(num_tests, sizeof(*pfds));
> +	pfd_indices = calloc(num_tests, sizeof(*pfd_indices));

[Severity: Medium]
Will these allocations leak if a signal is received?

If a signal like SIGINT is received during the polling loop,
cmd_test_sig_handler() invokes siglongjmp() to jump back to __cmd_test().
This abruptly unwinds the stack, completely bypassing the free(pfds) and
free(pfd_indices) calls at the end of this function.

[ ... ]

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