[PATCH] perf stat: Start tool events when enable_on_exec is used

[PATCH] perf stat: Start tool events when enable_on_exec is used

[TAKANO Mitsuhiro]

duration_time is a tool PMU event emulated in perf, so the kernel never
sees it and enable_on_exec cannot start it.  perf stat enables the events
itself only when enable_on_exec is not used; with two or more events
target__enable_on_exec() is true, so the tool event is never started and
reads as zero:

  # perf stat -e duration_time -- sleep 0.1
          103813345      duration_time
  # perf stat -e task-clock,duration_time -- sleep 0.1
                    0      duration_time

The zero value also reaches the default CPUs_utilized metric, which
divides by duration_time, so a plain `perf stat` prints "nan CPUs".

Start the tool events explicitly when enable_on_exec is used.  The second
command above then reports 103991394 and the default output prints
"0.0 CPUs" rather than "nan CPUs".

Verified on an arm64 box (Ampere Altra, Neoverse-N1), including the -r,
-I and group forms of the command; x86 was not tested.

Fixes: 0b18bced5444 ("perf tool_pmu: Make tool PMU events respect enable/disable")
Assisted-by: LLM
Signed-off-by: TAKANO Mitsuhiro <takano32@gmail.com>
---
 tools/perf/builtin-stat.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index c50cf230f16e..2062623682fb 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -510,6 +510,15 @@ static int enable_counters(void)
 	if (!target__enable_on_exec(&target)) {
 		if (!all_counters_use_bpf)
 			evlist__enable(evsel_list);
+	} else {
+		/*
+		 * Tool events are emulated in perf, so enable_on_exec above
+		 * does not start them.
+		 */
+		evlist__for_each_entry(evsel_list, evsel) {
+			if (evsel__is_tool(evsel))
+				evsel__enable(evsel);
+		}
 	}
 	return 0;
 }

base-commit: f47d223627f80cb7a1eb14c585fe6cb35de5e984
-- 
2.53.0

Re: [PATCH] perf stat: Start tool events when enable_on_exec is used

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf stat: NULL dereference in enable_counters() on unsupported events
--

commit 63681c497cb81f27ece4b03357873947bd854a0c
Author: TAKANO Mitsuhiro <takano32@gmail.com>

perf stat: Start tool events when enable_on_exec is used

This commit explicitly enables tool events in enable_counters() when
target__enable_on_exec() is true, since tool events are emulated in userspace
and are not started by the kernel on exec.

> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index c50cf230f16ed..2062623682fb2 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -510,6 +510,15 @@ static int enable_counters(void)
>  	if (!target__enable_on_exec(&target)) {
>  		if (!all_counters_use_bpf)
>  			evlist__enable(evsel_list);
> +	} else {
> +		/*
> +		 * Tool events are emulated in perf, so enable_on_exec above
> +		 * does not start them.
> +		 */
> +		evlist__for_each_entry(evsel_list, evsel) {
> +			if (evsel__is_tool(evsel))
> +				evsel__enable(evsel);
> +		}
>  	}
>  	return 0;
>  }

[Severity: Medium]
Can calling evsel__enable(evsel) here without checking evsel->supported or
evsel->core.fd dereference a null pointer when an event fails to open?

Prior to calling enable_counters(), __run_perf_stat() frees core.fd for any
unsupported event:

tools/perf/builtin-stat.c:__run_perf_stat() {
	...
	evlist__for_each_entry(evsel_list, counter) {
		if (!counter->supported) {
			perf_evsel__free_fd(&counter->core);
			continue;
		}
	...
}

When target__enable_on_exec() is false, evlist__enable() ->
__evlist__enable() skips events where pos->core.fd is null:

tools/perf/util/evlist.c:__evlist__enable() {
	...
		if (!evsel__is_group_leader(pos) || !pos->core.fd)
			continue;
	...
}

In the new else branch, if a user_time or system_time event fails in
evsel__tool_pmu_open() after evsel->disabled = true is set (for example on a
subsequent thread or repeat run), evsel->supported is false and
evsel->core.fd is freed. Calling evsel__enable() then follows:

evsel__enable()
  -> evsel__tool_pmu_enable()
    -> evsel__tool_pmu_enable_cpu()
      -> tool_pmu__read_stat()

where tool_pmu__read_stat() dereferences evsel->core.fd via:

	int fd = FD(evsel, cpu_map_idx, thread);

Similarly, if a tool event such as user_time or system_time is the group
leader of a group whose member failed to open (such as
perf stat -e "{user_time,cycles}" -- sleep 0.1, where opening cycles with the
procfs fd as group_fd fails and frees cycles->core.fd), evsel__enable()
iterates over the group members:

tools/perf/util/evsel.c:evsel__enable() {
	...
		for_each_group_member(member, evsel) {
			if (evsel__is_non_perf_event_open_pmu(evsel) ||
			    evsel__is_non_perf_event_open_pmu(member)) {
				int mem_err = evsel__enable(member);
	...
}

which calls evsel__enable(member) -> perf_evsel__enable(&member->core) and
dereferences member->core.fd when evaluating xyarray__max_x(evsel->fd).

Should enable_counters() and the group member loop in evsel__enable() check
supported (or core.fd) before enabling?

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