Re: [PATCH] perf stat: Count kernel events in a group led by a tool event

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] perf stat: missing disabled/enable_on_exec in
  create_perf_stat_counter()
- [Medium] perf evsel: get_group_fd() ignores !leader->supported for
  userspace PMU
- [Medium] perf stat: incomplete enable/disable for members under tool
  leader
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Medium] perf stat: mixed group read and disabled flaw in
  create_perf_stat_counter() [1]
--

commit 498a1414507f8fb0fa9be7cd1e97fec9e69bc393
Author: Hang Zhengyang <hang.zhengyang1010@gmail.com>

perf stat: Count kernel events in a group led by a tool event

This commit opens kernel member events with group_fd = -1 and omits
PERF_FORMAT_GROUP when their group leader is a non-perf_event_open PMU
event, allowing them to be opened and read individually.

> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 37752d6bfef45..59890916dcc4a 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -721,7 +721,7 @@ static int create_perf_stat_counter(struct evsel *evsel,
>  	 * the group read (for leader) and ID retrieval for all
>  	 * members.
>  	 */
> -	if (leader->core.nr_members > 1)
> +	if (leader->core.nr_members > 1 && !evsel__is_non_perf_event_open_pmu(leader))
>  		attr->read_format |= PERF_FORMAT_ID|PERF_FORMAT_GROUP;
>  
>  	attr->inherit = !config->no_inherit && list_empty(&evsel->bpf_counter_list);

[Severity: Medium]
Should create_perf_stat_counter() (and evsel__config()) also set
attr->disabled = 1 and attr->enable_on_exec = 1 when
evsel__is_non_perf_event_open_pmu(leader) is true?

tools/perf/builtin-stat.c:create_perf_stat_counter() {
	...
	if (evsel__is_group_leader(evsel)) {
		attr->disabled = 1;

		if (target__enable_on_exec(&target))
			attr->enable_on_exec = 1;
	}
	...
}

Because non-leader members of a group led by a tool event have
evsel__is_group_leader(evsel) == false while get_group_fd() now returns
-1, sys_perf_event_open() opens them as standalone kernel events with
attr->disabled = 0 and attr->enable_on_exec = 0.  This regression causes
those counters to start counting immediately inside sys_perf_event_open(),
recording events during setup, across initial delays (-D), and before
execvp() when profiling a child workload.

[Severity: Medium]
This problem wasn't introduced by this patch, but when a kernel PMU event
leads a mixed group containing a tool PMU member [1], does
PERF_FORMAT_GROUP still fail to read the group?

evsel__group_read_nr_members() and evsel__group_has_tpebs() only exclude
retire_lat events and do not exclude evsel__is_non_perf_event_open_pmu()
members, so evsel__group_read_size() and evsel__process_group_data()
expect more members than the kernel group contains and fail the read.  In
addition, because create_perf_stat_counter() only sets attr->disabled = 1
on the group leader, a subordinate tool PMU member opens with
attr->disabled = 0 in evsel__tool_pmu_open() and begins timing immediately
before evlist__enable() runs.

> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 560741d9599d2..5a919303562a0 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -2378,6 +2378,10 @@ static int get_group_fd(struct evsel *evsel, int cpu_map_idx, int thread)
>  	if (!evsel->supported || evsel__is_group_leader(evsel))
>  		return -1;
>  
> +	/* A userspace PMU leader has no perf event to group with */
> +	if (evsel__is_non_perf_event_open_pmu(leader))
> +		return -1;
> +
>  	/*
>  	 * Leader must be already processed/open,
>  	 * if not it's a bug.

[Severity: Medium]
What happens in get_group_fd() if the userspace PMU leader failed to open
and leader->supported is false?

Returning -1 instead of -2 bypasses the broken-leader check in
evsel__open_cpu(), so the member events still open with core.leader
pointing to the unsupported leader.  In perf stat, __run_perf_stat() frees
leader->core.fd and in perf record (e.g. perf record -e
'{user_time,cycles}'), record__open() removes the failed leader via
evlist__remove(), while __evlist__enable() and __evlist__disable() skip
the non-leader members and leave them impossible to enable or disable.

[Severity: Medium]
Can kernel members on CPUs 1..N-1 be enabled or disabled when the group
leader is a tool event like duration_time?

perf_pmu__reads_only_on_cpu_idx0() is true for duration_time, so
__perf_evlist__propagate_maps() restricts leader->core.cpus to CPU index 0
while kernel members keep all target CPUs.  Because __evlist__enable() and
__evlist__disable() skip non-leader events (!evsel__is_group_leader(pos))
and only delegate to members from evsel__enable_cpu() and
evsel__disable_cpu() on the leader's CPUs, member counters opened on CPUs
1..N-1 are never enabled or disabled.

[1] https://sashiko.dev/#/bug/linux-ff5d792a-1b0a-41d1-9760-725926f4f860

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-tool-pmu-leader-v1-1-6173b5569f59@gmail.com?part=1