Re: [RFC PATCH v2 7/9] 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: non-first numbered PMU instances skipped in cmd_sysinfo() filter
- [Medium] perf sysinfo: unchecked open_supported in print_system_summary()
--

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

perf sysinfo: Add sysinfo subcommand

This commit adds the perf sysinfo subcommand to display and probe PMUs and
system performance monitoring capabilities on the running 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..ea544b3d70a04
> --- /dev/null
> +++ b/tools/perf/builtin-sysinfo.c
> @@ -0,0 +1,2010 @@

[ ... ]

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

[ ... ]

> +		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.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 (probed) {
> +			fprintf(fp, "    User-space counter reading (rdpmc): ");
> +			color_fprintf(fp, core_rdpmc ? PERF_COLOR_GREEN : PERF_COLOR_YELLOW,
> +				      "%s", core_rdpmc ? "supported" : "missing/disabled");
> +			fputc('\n', fp);
> +			if (!core_rdpmc)
> +				strlist__add(missing, "user-space counter reading (rdpmc)");
> +
> +			fprintf(fp, "    Leader sampling: ");
> +			color_fprintf(fp,
> +				      core_leader_sampling ? PERF_COLOR_GREEN : PERF_COLOR_RED,
> +				      "%s",
> +				      core_leader_sampling ? "supported" : "missing");
> +			fputc('\n', fp);
> +		}

[ ... ]

> +	color_fprintf(fp, PERF_COLOR_BOLD, "  System rating:");
> +	if (nr_core == 0) {
> +		const char *min_desc =
> +			"no hardware core PMU; profiling limited to software timer events";
> +
> +		fprintf(fp, " ");
> +		color_fprintf(fp, PERF_COLOR_RED, "Minimal");
> +		fprintf(fp, " (%s)\n", min_desc);
> +	} else if (core_precise && core_branch_stack && (!probed || core_rdpmc) &&
> +		   !strlist__empty(mem_bw_pmus) && max_drill_lvl >= 2) {

[Severity: Medium]
What happens in print_system_summary() when probing is enabled (probed ==
true), but perf_event_open() fails on the core PMUs (for example, when
kernel.perf_event_paranoid > 2 or in an unprivileged container)?

In perf_pmu__probe(), failure to open any candidate event sets
pmu->probed.open_supported = false and returns early without probing
can_user_rdpmc or can_leader_sampling.

In the PMU scan loop above, print_system_summary() checks
pmu->probed.open_supported and falls back to static sysfs properties for
core_can_sample, core_precise, core_branch_stack, and core_mem_events,
leaving core_rdpmc and core_leader_sampling false.

However, the summary output and rating checks here only test probed rather
than whether any core PMU had open_supported == true. Should this track
whether core PMU probing actually succeeded so that rdpmc and leader
sampling are not reported as missing (and the system rating is not
downgraded) when perf_event_open() is unavailable?

[ ... ]

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

[ ... ]

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

[Severity: Medium]
When a user passes a filter argument matching a non-first numbered uncore
PMU instance (such as uncore_imc_1 or "*_1") without specifying -a/--all
or -v/--verbose, will cmd_sysinfo() fail to match it?

When show_all is false, perf_pmus__scan_skip_duplicates() visits only the
first instance (e.g., uncore_imc_0) and skips all subsequent instances
sharing the same base prefix before pmu_matches_filters() can inspect
their pmu->name:

tools/perf/builtin-sysinfo.c:pmu_matches_filters() {
	...
	for (int i = 0; i < argc; i++) {
		const char *pat = argv[i];

		if (perf_pmu__wildcard_match(pmu, pat))
			return true;
		if (pmu->name && (strglobmatch(pmu->name, pat) || !strcmp(pmu->name, pat)))
			return true;
	...
}

As a result, running perf sysinfo uncore_imc_0 matches and prints the
collapsed PMU group, whereas perf sysinfo uncore_imc_1 produces no output.

> +
> +		if (do_probe && !input_name &&
> +		    (show_pmus || pmu->is_core || pmu->mem_events ||
> +		     pmu->type == PERF_TYPE_SOFTWARE))
> +			perf_pmu__probe(pmu);

[ ... ]

> +}

[ ... ]

完美结束，确保最后有一个空行。]

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