Re: [PATCH v2 4/4] perf tools: Add test for weight annotation

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The pre-condition check for non-zero weights in `perf report` output is not restricted to the target symbol `test_loop`, causing flaky test failures instead of graceful skips on CPUs with fast L1 caches.
- [Medium] The `grep -q 'Percent Weight'` command silently aborts the script on failure due to `set -e`, suppressing intended diagnostic output.
--

--- Patch [4]: [PATCH v2 4/4] perf tools: Add test for weight annotation ---
commit 4fef6c496fea22a20514f5de2e321ad5a4c0bb3e
Author: Andi Kleen <[email protected]>

perf tools: Add test for weight annotation

This commit adds a test case to check if the --stdio weight annotation works
correctly in perf tools.

> diff --git a/tools/perf/tests/shell/annotate_weight.sh b/tools/perf/tests/shell/annotate_weight.sh
> --- /dev/null
> +++ b/tools/perf/tests/shell/annotate_weight.sh

[ ... ]

> +if ! awk '$1 ~ /^[0-9]/ && ($1 + 0) > 0 { found = 1 } END { exit !found }' \
> +	"${report_out}"; then
> +	echo "[SKIP] PEBS weight sampling produced no nonzero weights"
> +	exit 2
> +fi

[Severity: Medium]
Will this global check cause flaky test failures on CPUs with fast L1 caches?

The awk check validates that any symbol produced non-zero weights. Since the
thloop workload spins on a volatile variable, it heavily hits the L1 cache.
On fast CPUs, these L1 hits might not trigger the PEBS mem-loads event,
meaning test_loop might produce zero samples.

However, thread initialization code can still cause cache misses and produce
samples with non-zero weights, which would pass this check. The script then
runs perf annotate for test_loop, finds zero weight, and fails the test
(exit 1) instead of properly skipping it (exit 2).

Should this condition be restricted to the target symbol test_loop before
proceeding to annotate it?

> +
> +perf annotate --stdio -i "${perfdata}" --symbol "${testsym}" \
> +	> "${annotate_out}" 2> "${record_log}"
> +
> +grep -q 'Percent Weight' "${annotate_out}"

[Severity: Medium]
Because set -e is enabled at the top of the script, will this grep command
silently abort the script if the expected header is missing?

If grep returns an exit code of 1 (e.g., if the symbol had no samples or
annotation failed), the script will immediately exit and run the cleanup trap,
deleting the temporary files.

This bypasses the subsequent awk check which is explicitly designed to output
the diagnostic message "Annotation output contained no nonzero weight" and
print the file for debugging.

Could this be handled in a way that preserves the diagnostic output when it
fails?

> +
> +# The second numeric column is the rendered average weight. This assertion
> +# fails on the original regression because calc_percent() left it at zero.
> +if ! awk '$1 ~ /^[0-9]/ && $2 ~ /^[0-9]/ && ($2 + 0) > 0 { found = 1 } END { exit !found }' \
> +	"${annotate_out}"; then
> +	echo "Annotation output contained no nonzero weight"

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4