Re: [PATCH v1 7/7] perf test: Expand top tests for --hybrid-merge

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The hybrid merge test explicitly requests the `cpu-clock` event, contradicting its design to test `--hybrid-merge` on hardware events.
- [Medium] Unnecessary system-wide `sync` commands are used to wait for local file data, causing potential CI performance degradation.
- [Medium] The global `retval` variable is conditionally assigned but not initialized before use, leaking state between consecutive tests.
--

--- Patch [7]: [PATCH v1 7/7] perf test: Expand top tests for --hybrid-merge ---
commit 83b4f9c7a4d40ea456ef8050c21d34919fb655e1
Author: Ian Rogers <irogers@google.com>
perf test: Expand top tests for --hybrid-merge

This commit adds shell test coverage for the top --hybrid-merge flag to
verify correct behavior on topologies featuring hybrid events. It relies
on fallback logic inside perf top.

> diff --git a/tools/perf/tests/shell/top.sh b/tools/perf/tests/shell/top.sh
> index ad7fccd09025d..49820bb2d6b25 100755
> --- a/tools/perf/tests/shell/top.sh
> +++ b/tools/perf/tests/shell/top.sh
[ ... ]
> @@ -35,26 +35,27 @@ test_basic_perf_top() {
[ ... ]
> +	sleep 10 | timeout 5s perf top --stdio -d 1 -e cpu-clock \
> +		-p $PID > "${log_file}" 2>&1 || retval=$?
> +	if [ "${retval:-0}" -ne 124 ] && [ "${retval:-0}" -ne 0 ]; then

[Severity: Medium]
Is it possible for retval to leak state between consecutive tests?

Since retval is conditionally assigned here but not initialized before use, a
successful command execution might leave retval with a value from a previous
test. If test_basic_perf_top fails and leaves a non-zero retval, a subsequent
success in test_hybrid_merge_perf_top could falsely inherit the non-zero
value and incorrectly report a failure.

[ ... ]
>  	kill $PID
>  	wait $PID 2>/dev/null || true
>  
> +	sync
> +

[Severity: Medium]
Does this sync command introduce an unnecessary system-wide delay?

Once the process writes to a file descriptor and exits, the data is
immediately available in the VFS page cache for grep. The sync command
commits all dirty pages system-wide to physical storage rather than just
flushing the process buffers.

[ ... ]
> +test_hybrid_merge_perf_top() {
[ ... ]
> +	# Run without explicitly requesting -e cycles so heavily virtualized
> +	# environments can seamlessly fall back to cpu-clock while real
> +	# hybrid hardware will naturally cover the merge logic.
> +	sleep 10 | timeout 5s perf top -e cpu-clock \
> +			--stdio --hybrid-merge -d 1 -p $PID > "${log_file}" 2>&1 || retval=$?

[Severity: Medium]
Could the explicit -e cpu-clock argument here bypass the hybrid PMU logic
that this function is intended to test?

The inline comment suggests the test runs without requesting cycles to allow
natural hardware merge logic to trigger, yet the command hardcodes cpu-clock,
which is a software event. Software events do not utilize hardware PMUs,
meaning the --hybrid-merge code path for merging hybrid hardware events
might never be exercised.

[ ... ]
> +	kill $PID
> +	wait $PID 2>/dev/null || true
> +
> +	# Wait a tiny bit for the file system to catch up on the logs
> +	sync
> +

[Severity: Medium]
Is this sync command needed to wait for the file system to catch up?

Similar to the basic test, the data written to the log file should already
be available in the page cache once the process exits. Forcing a system-wide
sync might stall the CI environment unnecessarily.

[ ... ]

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