Re: [PATCH v1 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 4 potential issue(s) to consider:
- [Medium] The parallel test harness enters a 100% CPU busy loop if a child process closes its stderr pipe (reaching EOF) but continues executing.
- [Medium] If the test harness hits the open file limit (RLIMIT_NOFILE), it enters an infinite deadlock unable to reap finished children.
- [Medium] The error and signal cleanup path in `__cmd_test` relies on the legacy `finish_test()` function, causing memory leaks and false failure reporting.
- [Low] The patch introduces a malformed function declaration that breaks compilation (Syntax Error).
--

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

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

This commit refactors the perf test harness to poll and drain output pipes
from all running children simultaneously into per-child buffers. This prevents
child processes from blocking indefinitely when their pipe buffers fill up.

> diff --git a/tools/lib/subcmd/run-command.c b/tools/lib/subcmd/run-command.c
> index b7510f83209a0..60e7df3673169 100644
> --- a/tools/lib/subcmd/run-command.c
> +++ b/tools/lib/subcmd/run-command.c
> @@ -241,8 +241,8 @@ 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;
> +		/* Open failed. Only assume finished if process no longer exists. */
> +		return errno == ENOENT ? 1 : 0;
>  	}

If finish_tests_parallel() launches many tests concurrently and hits the open
file limit (RLIMIT_NOFILE), fopen() will fail with EMFILE.

Since this now returns 0, the test is assumed to still be running. Could this
lead to an infinite loop where tests are never marked as finished, and no file
descriptors are ever freed to break the deadlock?

> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> index a350db071674a..ad1b941731ca1 100644
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c
[ ... ]
> @@ -508,6 +511,187 @@ static void finish_test(struct child_test **child_tests, int running_test, int c
>  	zfree(&child_tests[running_test]);
>  }
>  
> +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;
> +static void drain_child_process_err(struct child_test *child)

This appears to be a duplicate partial declaration of finish_tests_parallel()
left over from a merge or refactor. Does this syntax error break the build?

> +{
> +	char buf[512];
> +	ssize_t len;
> +
> +	while ((len = read(child->process.err, buf, sizeof(buf) - 1)) > 0) {
> +		buf[len] = '\0';
> +		strbuf_addstr(&child->err_output, buf);
> +	}
> +}
[ ... ]
> +		if (num_pfds == 0) {
> +			if (running_count > 0)
> +				usleep(10 * 1000);
> +		} else {
> +			int pret = poll(pfds, num_pfds, 100);
> +
> +			if (pret > 0) {
> +				for (p = 0; p < num_pfds; p++) {
> +					if (pfds[p].revents) {
> +						size_t idx = pfd_indices[p];
> +						struct child_test *child = child_tests[idx];
> +
> +						drain_child_process_err(child);
> +					}
> +				}
> +			}
> +		}

If a child process closes its stderr pipe (reaching EOF) but continues
executing, poll() returns POLLHUP. drain_child_process_err() will read 0 bytes,
but it does not close child->process.err or set it to -1.

Will the next iteration add this open file descriptor back to the poll() array,
causing poll() to return immediately and create a tight busy loop until the
child finally exits?

[ ... ]
> @@ -670,8 +854,9 @@ static int __cmd_test(struct test_suite **suites, int argc, const char *argv[],
>  		}
>  		if (!sequential) {
>  			/* Parallel mode starts tests but doesn't finish them. Do that now. */
> -			for (size_t x = 0; x < num_tests; x++)
> -				finish_test(child_tests, x, num_tests, width);
> +			err = finish_tests_parallel(child_tests, num_tests, width);
> +			if (err)
> +				goto err_out;
>  		}
>  	}
>  err_out:

If finish_tests_parallel() encounters an error and jumps to err_out, the cleanup
path loops over all children and calls the legacy finish_test() function. 

Because finish_test() uses a local string buffer, it does not release the
dynamically allocated child->err_output populated by finish_tests_parallel().
Does this leak memory?

Additionally, finish_test() unconditionally calls finish_command(). If a child
was already reaped in finish_tests_parallel() (where child->done is true),
could this second call lead to ECHILD errors and corrupted test results?

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