Re: [PATCH v3] 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 1 potential
issue to consider:
- [Medium] perf stat: broken tool event enable/disable for -D,
  --control, and exec in create_perf_stat_counter()
--

commit 0e981d5b2ece1f32ac47445309993697c6cdb096
Author: Hang Zhengyang <hang.zhengyang1010@gmail.com>

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

This commit opens members of a group led by a tool event on their own
with group_fd = -1 and without PERF_FORMAT_GROUP, while opening kernel
event members initially disabled.

> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 37752d6bfef45..57956f55ea09b 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
[ ... ]
> @@ -748,9 +749,16 @@ static int create_perf_stat_counter(struct evsel *evsel,
>  	/*
>  	 * Disabling all counters initially, they will be enabled
>  	 * either manually by us or by kernel via enable_on_exec
> -	 * set later.
> +	 * set later.  A kernel event that a tool event leader has as
> +	 * a member is not in the kernel's group either, so it has to
> +	 * be opened disabled to start together with the workload.
> +	 * A tool event member counts from open and is only stopped by
> +	 * an explicit disable, so leaving it enabled here keeps its
> +	 * count; disabling it would leave nobody to start it when
> +	 * perf relies on enable_on_exec.
>  	 */
> -	if (evsel__is_group_leader(evsel)) {
> +	if (evsel__is_group_leader(evsel) ||
> +	    (evsel__is_tool(leader) && !evsel__is_tool(evsel))) {
>  		attr->disabled = 1;
>  
>  		if (target__enable_on_exec(&target))

[Severity: Medium]
Does conditioning attr->disabled on leader vs. member rather than
target__enable_on_exec(&target) leave tool event members enabled during
-D / --control and leave tool event leaders disabled during exec
workloads?

In evsel__tool_pmu_open(), attr->disabled sets evsel->disabled = true and
start_time = INVALID_START_TIME; otherwise evsel->disabled is false and
start_time is sampled immediately at open.

When !target__enable_on_exec(&target) (such as with -D or --control),
enable_counters() calls evlist__enable(), and evsel__enable_cpu() enables
each member of a tool-led group via evsel__tool_pmu_enable_cpu():

tools/perf/util/tool_pmu.c:evsel__tool_pmu_enable_cpu() {
	if (!evsel->disabled)
		return 0;
    ...
}

Because a tool event member in a group like '{user_time,system_time}' is
skipped by !evsel__is_tool(evsel) and opened with attr->disabled == 0,
evsel__tool_pmu_enable_cpu() returns early without resetting start_time,
so the member counts continuously through the disabled delay window.

Conversely, when target__enable_on_exec(&target) is true (such as
'perf stat -e "{user_time,cycles}" -- sleep 0.2'), enable_counters() skips
evlist__enable() because kernel events are enabled on exec.  A tool event
leader still matches evsel__is_group_leader(evsel) and opens with
evsel->disabled = true; nothing enables it on exec, so
evsel__tool_pmu_read() returns 0.

Should tool events instead set attr->disabled = 1 only when
!target__enable_on_exec(&target), regardless of whether they are the
group leader or a member?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111110616.2249764-1-takano32@gmail.com?part=1