Re: [RFC PATCH v1 2/2] perf sysinfo: Add sysinfo subcommand, documentation, and test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] perf sysinfo: broken perf.data metrics lookup in print_system_summary()
- [Medium] perf sysinfo: inaccurate mem_events detection in print_system_summary()
--

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

perf sysinfo: Add sysinfo subcommand, documentation, and test

This commit adds the perf sysinfo subcommand to display and probe PMU and
system performance monitoring capabilities on a live system or from a
perf.data file.

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

[ ... ]

> +static void print_system_summary(FILE *fp, const struct perf_session *session,
> +				 bool probed, bool leading_newline)
> +{
> +	const struct pmu_metrics_table *mtable;

[ ... ]

> +	while ((pmu = perf_pmus__scan_skip_duplicates(pmu)) != NULL) {
> +		size_t pmu_events = perf_pmu__num_events(pmu);
> +		size_t sysfs_ev = pmu->sysfs_aliases;
> +		size_t json_ev = pmu_events > sysfs_ev ? pmu_events - sysfs_ev : 0;
> +
> +		total_pmus++;
> +		total_events += pmu_events;
> +		total_sysfs_events += sysfs_ev;
> +		total_json_events += json_ev;
> +
> +		if (pmu->is_core) {
> +			struct perf_pmu_caps *br_cap;
> +
> +			add_pmu_display_name(core_pmus, pmu);
> +			perf_pmu__caps_parse(pmu);
> +			br_cap = perf_pmu__get_cap(pmu, "branches");
> +			if (br_cap && br_cap->value) {
> +				int depth = atoi(br_cap->value);
> +
> +				if (depth > max_br_depth)
> +					max_br_depth = depth;
> +			}
> +			if (probed && pmu->probed.checked && pmu->probed.open_supported) {
> +				if (pmu->probed.can_sample)
> +					core_can_sample = true;
> +				if (pmu->probed.max_precise > 0) {
> +					core_precise = true;
> +					if (pmu->probed.max_precise > max_precise)
> +						max_precise = pmu->probed.max_precise;
> +				}
> +				if (pmu->probed.can_user_rdpmc)
> +					core_rdpmc = true;
> +				if (pmu->probed.can_branch_stack)
> +					core_branch_stack = true;
> +				if (pmu->probed.mem_events_mask)
> +					core_mem_events = true;
> +				if (pmu->probed.can_leader_sampling)
> +					core_leader_sampling = true;
> +			} else {
> +				core_can_sample = true;
> +				if (pmu->max_precise > 0) {
> +					core_precise = true;
> +					if (pmu->max_precise > max_precise)
> +						max_precise = pmu->max_precise;
> +				}
> +				if (max_br_depth > 0)
> +					core_branch_stack = true;
> +				if (pmu->mem_events)
> +					core_mem_events = true;
> +			}

[Severity: Medium]
Can this misreport core_mem_events in print_system_summary()?

On AMD x86 and ARM64 systems, pmu->mem_events is attached to non-core PMUs
(ibs_op and arm_spe_*, where pmu->is_core is false), while the core PMU has
pmu->mem_events == NULL. Checking mem_events only inside the if (pmu->is_core)
block causes the summary to always report memory profiling events as missing
on those architectures.

Conversely, in the unprobed else branch, checking if (pmu->mem_events)
without calling perf_pmu__mem_events_init_pmu(pmu) (unlike
print_mem_events_static()) will set core_mem_events = true whenever
perf_pmu__arch_init() assigns a static perf_mem_events array to a core PMU,
even if none of the events (such as events/mem-loads or events/mem-stores)
are actually supported in sysfs.

> +		} else if (pmu->type == PERF_TYPE_SOFTWARE) {
> +			if (!probed || !pmu->probed.checked || pmu->probed.can_sample)
> +				has_sw_sampling = true;
> +		}
> +
> +		if (pmu->auxtrace)
> +			add_pmu_display_name(aux_pmus, pmu);
> +		if (is_mem_bw_pmu_name(pmu->name))
> +			add_pmu_display_name(mem_bw_pmus, pmu);
> +		if (is_io_bw_pmu_name(pmu->name))
> +			add_pmu_display_name(io_bw_pmus, pmu);
> +	}
> +
> +	if (session && session->header.env.arch && session->header.env.cpuid)
> +		mtable = find_core_metrics_table(session->header.env.arch,
> +						 session->header.env.cpuid);
> +	else
> +		mtable = pmu_metrics_table__find();

[Severity: Medium]
Will find_core_metrics_table() fail to match the metrics table when reading
from a perf.data file here?

session->header.env.arch contains the raw uts.machine string from
HEADER_ARCH (such as "x86_64", "aarch64", or "ppc64le"), whereas
pmu_events_map[].arch uses the normalized architecture name returned by
perf_env__arch(&session->header.env) ("x86", "arm64", "powerpc", etc.).

In addition, on x86, session->header.env.cpuid stores "vendor,family,model,step"
in decimal, whereas pmu_events_map and strcmp_cpuid_str() expect the
"vendor-family-HEXMODEL-HEXSTEP" format. In perf_pmus__read_from_env() in
tools/perf/util/pmus.c, both conversions are performed before calling
find_core_events_table():

	if (env->cpuid) {
		char cpuid_str[128];
		char vendor[32];
		unsigned int family, model, step;
		const char *cpuid = env->cpuid;

		if (sscanf(env->cpuid, "%31[^,],%u,%u,%u",
			   vendor, &family, &model, &step) == 4) {
			snprintf(cpuid_str, sizeof(cpuid_str), "%s-%u-%X-%X",
				 vendor, family, model, step);
			cpuid = cpuid_str;
		}
		core_table = find_core_events_table(perf_env__arch(env), cpuid);
	}

Also, if session is non-NULL (-i perf.data) but either arch or cpuid is NULL
in the header, falling into the else branch calls pmu_metrics_table__find(),
which queries the live host CPU's metrics table instead of treating the
recorded environment as having no core metrics table.

[ ... ]

> +	while ((pmu = show_all ? perf_pmus__scan(pmu) :
> +				 perf_pmus__scan_skip_duplicates(pmu)) != NULL) {
> +		if (!pmu_matches_filters(pmu, argc, argv))
> +			continue;
> +
> +		if (do_probe && !input_name &&
> +		    (show_pmus || pmu->is_core || pmu->type == PERF_TYPE_SOFTWARE))
> +			perf_pmu__probe(pmu);

[Severity: Medium]
Related to the core_mem_events check in print_system_summary(), when
show_pmus is false, should this condition also probe PMUs that have
pmu->mem_events != NULL so that non-core memory profiling PMUs (such as
ibs_op and arm_spe_*) are probed for the default summary output?

[ ... ]

> +}

[ ... ]

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