Re: [PATCH V4 4/6] tools/perf: Add powerpc callback support for arch_perf_record__need_read

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The use of `perf_evsel__read()` causes silent read failures for sibling HTM events in a group, leading to premature termination of the record loop.
--

--- Patch [4]: [PATCH V4 4/6] tools/perf: Add powerpc callback support for arch_perf_record__need_read ---
commit aa9cd81264b9ba7389f3532c4f16fea15643247b
Author: Athira Rajeev <atrajeev@linux.ibm.com>

tools/perf: Add powerpc callback support for arch_perf_record__need_read

This commit implements the arch_perf_record__need_read() hook for powerpc.
It reads the count for every open htm evsel via perf_evsel__read() and
accumulates the values into total_pending_records. A non-zero total means
at least one HTM target still has records pending.

> diff --git a/tools/perf/arch/powerpc/util/evsel.c b/tools/perf/arch/powerpc/util/evsel.c
> index 2f733cdc8dbbe..3c7ccc955b0af 100644
> --- a/tools/perf/arch/powerpc/util/evsel.c
> +++ b/tools/perf/arch/powerpc/util/evsel.c

[ ... ]

> +		for (x = 0; x < xyarray__max_x(xy); x++) {
> +			for (y = 0; y < xyarray__max_y(xy); y++) {
> +				struct perf_counts_values count = { .val = 0 };
> +
> +				if (perf_evsel__read(&evsel->core, x, y, &count) == 0)

[Severity: High]
Does this code silently ignore read failures when the HTM event is a
sibling in a group?

If a user groups the HTM event with other events (e.g., using
perf record -e '{cycles,htm}:S'), evsel__config() sets the
PERF_FORMAT_GROUP flag for all members of the group.

When perf_evsel__read() is called on the HTM sibling, it will delegate to
perf_evsel__read_group(). The userspace buffer size is calculated using the
sibling's nr_members (which is 0), but the kernel enforces event->read_size
based on the group leader's nr_siblings.

The kernel then rejects the read with -ENOSPC because the userspace buffer
size is smaller than the required group payload size.

Since the error is silently ignored here, count.val remains 0, which could
abort the drain loop prematurely and drop the remaining records.

> +					total_pending_records += count.val;
> +			}
> +		}
> +	}
> +
> +	/* Collection is complete only when ALL hardware queues have no pending records */
> +	return (total_pending_records > 0) ? 1 : 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729124400.65009-1-atrajeev@linux.ibm.com?part=4