Re: [PATCH v1 39/41] perf sysinfo: Add sysinfo subcommand

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] perf sysinfo: precise-only regs duplicated in print_pmu_json()
- [Medium] perf sysinfo: cmd_sysinfo() collapses heterogeneous core PMUs
--

commit 137020466c84fe37ed29fa37c240863bb116831f
Author: Ian Rogers <irogers@google.com>

perf sysinfo: Add sysinfo subcommand

This commit adds the perf sysinfo subcommand to display and probe PMU and
system performance monitoring capabilities on a running system or from a
perf.data file, supporting both text and JSON output modes.

> diff --git a/tools/perf/builtin-sysinfo.c b/tools/perf/builtin-sysinfo.c
> new file mode 100644
> index 0000000000000..afc4648098dd8
> --- /dev/null
> +++ b/tools/perf/builtin-sysinfo.c
> @@ -0,0 +1,2022 @@

[ ... ]

> +static bool evsel_matches_pmu(const struct evsel *evsel,
> +			      const struct perf_pmu *pmu, bool show_all)
> +{
> +	struct perf_pmu *ev_pmu = evsel__find_pmu(evsel);
> +	const char *ev_pmu_name;
> +
> +	if (ev_pmu == pmu)
> +		return true;
> +
> +	if (ev_pmu) {
> +		if (!show_all && !pmu->is_core && pmu->name && ev_pmu->name) {
> +			size_t base_len = pmu_name_len_no_suffix(pmu->name);
> +
> +			if (pmu_name_len_no_suffix(ev_pmu->name) == base_len &&
> +			    !strncmp(pmu->name, ev_pmu->name, base_len))
> +				return true;
> +		}
> +		return false;
> +	}

[ ... ]

> +static void print_pmu_json(FILE *fp, const struct perf_pmu *pmu,
> +			   const struct perf_session *session, bool show_all)
> +{

[ ... ]

> +			print_probed_regs(fp, &buf, "UserRegisters",
> +					  p->user_regs_mask, p->user_regs_abi,
> +					  p->user_simd_mask, p->user_pred_mask,
> +					  /*intr=*/false, /*json=*/true);
> +			print_probed_regs(fp, &buf, "IntrRegisters",
> +					  p->intr_regs_mask, p->intr_regs_abi,
> +					  p->intr_simd_mask, p->intr_pred_mask,
> +					  /*intr=*/true, /*json=*/true);
> +			print_probed_regs(fp, &buf, "IntrRegistersPreciseOnly",
> +					  p->intr_regs_precise_mask, p->intr_regs_abi,
> +					  p->intr_simd_precise_mask, p->intr_pred_precise_mask,
> +					  /*intr=*/true, /*json=*/true);

[Severity: Medium]
In probe_pmu_regs_one() in tools/perf/util/pmu.c, when an interrupt register
or SIMD/predicate register class can only be sampled with precise_ip > 0, its
bit is set in both intr_regs_mask (or intr_simd_mask / intr_pred_mask) and
intr_regs_precise_mask (or intr_simd_precise_mask / intr_pred_precise_mask).

In print_pmu_text(), the Intr registers output masks out the precise-only
subsets before printing Intr registers (precise sampling only):

	print_probed_regs(fp, NULL, "Intr registers",
			  p->intr_regs_mask & ~p->intr_regs_precise_mask,
			  p->intr_regs_abi,
			  p->intr_simd_mask & ~p->intr_simd_precise_mask,
			  p->intr_pred_mask & ~p->intr_pred_precise_mask,
			  /*intr=*/true, /*json=*/false);
	print_probed_regs(fp, NULL, "Intr registers (precise sampling only)",
			  p->intr_regs_precise_mask, p->intr_regs_abi,
			  p->intr_simd_precise_mask, p->intr_pred_precise_mask,
			  /*intr=*/true, /*json=*/false);

Should print_pmu_json() also mask out intr_regs_precise_mask,
intr_simd_precise_mask, and intr_pred_precise_mask when emitting
IntrRegisters, so that precise-only registers (such as x86 PEBS-only XMM
registers) are not duplicated in both IntrRegisters and
IntrRegistersPreciseOnly?

[ ... ]

> +static void add_pmu_display_name(struct strlist *list, const struct perf_pmu *pmu)
> +{
> +	size_t num_inst = perf_pmu__num_duplicates(pmu);
> +	char buf[128];
> +
> +	if (!pmu->name)
> +		return;
> +	if (num_inst > 1) {
> +		size_t base_len = pmu_name_len_no_suffix(pmu->name);
> +
> +		snprintf(buf, sizeof(buf), "%.*s", (int)base_len, pmu->name);
> +		strlist__add(list, buf);
> +	} else {
> +		strlist__add(list, pmu->name);
> +	}
> +}
> +
> +static void print_system_summary(FILE *fp, const struct perf_session *session,
> +				 bool probed, bool leading_newline)
> +{

[ ... ]

> +	while ((pmu = perf_pmus__scan_skip_duplicates(pmus, pmu)) != NULL) {
> +		size_t pmu_events, sysfs_ev, json_ev;
> +
> +		pmu_event_counts(pmu, &pmu_events, &sysfs_ev, &json_ev);

[ ... ]

> +int cmd_sysinfo(int argc, const char **argv)
> +{

[ ... ]

> +	while ((pmu = show_all ? perf_pmus__scan(pmus, pmu) :
> +				 perf_pmus__scan_skip_duplicates(pmus, pmu)) != NULL) {
> +		if (!pmu_matches_filters(pmu, show_all, argc, argv))
> +			continue;

[Severity: Medium]
On heterogeneous or big.LITTLE systems where distinct core PMUs share a base
prefix before a decimal or 3+ hex-digit suffix (such as armv8_cortex_a53 and
armv8_cortex_a57/a72, or armv8_pmuv3_0 and armv8_pmuv3_1),
pmu_name_len_no_suffix() strips those suffixes and treats the core PMUs as
duplicates.

When running without -v or -a, won't perf_pmus__scan_skip_duplicates() in
cmd_sysinfo() and print_system_summary() skip all but the first core PMU,
while perf_pmu__num_duplicates() in print_pmu_text(), print_pmu_json(), and
add_pmu_display_name() strips the CPU suffix from the displayed core PMU name
(reporting armv8_cortex or armv8_pmuv3) and counts the system as having only
a single core PMU in the summary?

In addition, because evsel_matches_pmu() guards its prefix match with
!pmu->is_core, recorded events belonging to the skipped core PMUs will not
match the first core PMU either and will be omitted from Recorded events.

Should core PMUs be excluded from duplicate skipping and suffix stripping
(similar to how pmu_deduped_name_len() in tools/perf/util/pmu.c handles
pmu->is_core)?

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