Re: [PATCH V2 2/6] tools/perf: Add AUXTRACE recording support for powerpc HTM

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Uninitialized auxtrace_info->type causes perf.data corruption and breaks bisectability.
- [High] aux_watermark is set to the full size of the ring buffer, risking trace data loss.
--

--- Patch [2]: [PATCH V2 2/6] tools/perf: Add AUXTRACE recording support for powerpc HTM ---
commit 90e77eef6274098986791263120e8c79ee34c0b9
Author: Athira Rajeev <atrajeev@linux.ibm.com>

tools/perf: Add AUXTRACE recording support for powerpc HTM

Adds powerpc perf tool support for HTM PMU AUXTRACE recording. Introduces
htm_recording_init() and related auxtrace callbacks to create
PERF_RECORD_AUXTRACE records for HTM data. Updates auxtrace_record__init()
to detect HTM events.

> diff --git a/tools/perf/arch/powerpc/util/htm.c b/tools/perf/arch/powerpc/util/htm.c
> new file mode 100644
> index 0000000000000..07b496e4be2fb
> --- /dev/null
> +++ b/tools/perf/arch/powerpc/util/htm.c

[ ... ]

> +static int
> +htm_recording_options(struct auxtrace_record *itr __maybe_unused,
> +		      struct evlist *evlist,
> +		      struct record_opts *opts)
> +{
> +	struct evsel *pos;
> +
> +	opts->full_auxtrace = true;
> +
> +	if (!opts->auxtrace_mmap_pages) {
> +		opts->auxtrace_mmap_pages = KiB(128) / page_size;
> +		if (opts->mmap_pages == UINT_MAX)
> +			opts->mmap_pages = KiB(256) / page_size;
> +	}
> +
> +	evlist__for_each_entry(evlist, pos) {
> +		if (!pos->name || !strstarts(pos->name, "htm"))
> +			continue;
> +		pos->core.attr.aux_watermark =
> +			opts->auxtrace_mmap_pages * (size_t)page_size;

[Severity: High]
Does setting the watermark to the full size of the ring buffer risk hardware
trace data loss?

By setting the watermark equal to the entire buffer size, the wakeup only
fires when the buffer is 100% full. Because scheduling the user-space perf
tool and reading the buffer introduces latency, any hardware trace data
generated during this window might be dropped due to a lack of buffer
headroom.

> +		pos->core.attr.freq = 0;
> +		pos->core.attr.sample_type |= PERF_SAMPLE_RAW;
> +		pos->needs_auxtrace_mmap = true;
> +	}
> +
> +	return 0;
> +}

[ ... ]

> +static int
> +htm_info_fill(struct auxtrace_record *itr,
> +	      struct perf_session *session,
> +	      struct perf_record_auxtrace_info *auxtrace_info,
> +	      size_t priv_size)
> +{
> +	struct htm_recording *htm_r = container_of(itr, struct htm_recording, itr);
> +	struct evlist *evlist = session->evlist;
> +	struct evsel *pos;
> +	int n = 0;
> +	int expected_n = htm_nr_events(evlist);
> +
> +	if (priv_size != HTM_AUXTRACE_PRIV_SIZE(expected_n))
> +		return -EINVAL;
> +
> +	// To set: auxtrace_info->type
> +	auxtrace_info->priv[POWERPC_HTM_PMU_TYPE] = htm_r->evsel->core.attr.type;

[Severity: High]
Does leaving auxtrace_info->type uninitialized break bisectability by
generating corrupted perf.data files?

Because the type is left uninitialized (defaulting to PERF_AUXTRACE_UNKNOWN)
before the auxtrace info data is written to the stream, this might create
trace files that cannot be read by `perf report` at this commit.

> +	auxtrace_info->priv[POWERPC_HTM_NUM_EVENTS] = expected_n;
> +

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