Re: [PATCH v1 1/2] perf tool_pmu: Make tool PMU events respect enable/disable

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Tool PMU events configured as members of an event group are never enabled and will incorrectly report zero.
- [Medium] The high-level `evsel__enable()` and `evsel__disable()` API functions bypass the tool PMU logic, causing `-ENOTTY` failures.
- [Medium] Silent error masking in `evsel__tool_pmu_enable_cpu` can lead to massive time calculation errors.
- [Low] Incorrect parameter ordering in `lseek` call within `tool_pmu__read_stat()`.
--

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

perf tool_pmu: Make tool PMU events respect enable/disable

Tool PMU events (duration_time, user_time, system_time) currently
count from when the event is opened to when it is read. This causes
issues with features like the delay option (-D) or control fd, where
events are opened but should not start counting immediately.

Make these events behave more like regular counters by implementing
proper enable and disable support.

Link: https://lore.kernel.org/linux-perf-users/20260517093650.2540920-1-nigro.fra@gmail.com/

> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 2ee87fd84d3ea..3646156500f6e 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
[ ... ]
> @@ -1795,6 +1798,8 @@ int evsel__append_addr_filter(struct evsel *evsel, const char *filter)
>  /* Caller has to clear disabled after going through all CPUs. */
>  int evsel__enable_cpu(struct evsel *evsel, int cpu_map_idx)
>  {
> +	if (evsel__is_tool(evsel))
> +		return evsel__tool_pmu_enable_cpu(evsel, cpu_map_idx);
>  	return perf_evsel__enable_cpu(&evsel->core, cpu_map_idx);
>  }
>  
> @@ -1810,6 +1815,8 @@ int evsel__enable(struct evsel *evsel)

Does evsel__enable() also need to be updated to handle tool PMUs?

The per-CPU functions are updated here, but the batch functions
evsel__enable() and evsel__disable() directly call perf_evsel__enable().

Since perf_evsel__enable() executes an ioctl(fd, PERF_EVENT_IOC_ENABLE)
and tool events use plain text files like /proc/stat, won't this result in
an -ENOTTY failure and leave the batch-enabled events disabled?

>  /* Caller has to set disabled after going through all CPUs. */
>  int evsel__disable_cpu(struct evsel *evsel, int cpu_map_idx)
>  {
> +	if (evsel__is_tool(evsel))
> +		return evsel__tool_pmu_disable_cpu(evsel, cpu_map_idx);
>  	return perf_evsel__disable_cpu(&evsel->core, cpu_map_idx);
>  }
[ ... ]
> diff --git a/tools/perf/util/tool_pmu.c b/tools/perf/util/tool_pmu.c
> index 6a9df3dc0e076..cfefd97cb95bc 100644
> --- a/tools/perf/util/tool_pmu.c
> +++ b/tools/perf/util/tool_pmu.c
[ ... ]
> +static int tool_pmu__read_stat(struct evsel *evsel, int cpu_map_idx, int thread, __u64 *val)
> +{
> +	enum tool_pmu_event ev = evsel__tool_event(evsel);
> +	bool system = ev == TOOL_PMU__EVENT_SYSTEM_TIME;
> +	int fd = FD(evsel, cpu_map_idx, thread);
> +	int err = 0;
> +
> +	if (fd < 0) {
> +		*val = 0;
> +		return 0;
> +	}
> +
> +	lseek(fd, SEEK_SET, 0);

This isn't a bug, but are the offset and whence arguments to lseek swapped
here? 

The POSIX prototype expects lseek(fd, offset, whence). This happens to
work because SEEK_SET is 0, but it misuses the API.

> +	if (evsel->pid_stat) {
> +		if (cpu_map_idx == 0)
> +			err = read_pid_stat_field(fd, system ? 15 : 14, val);
[ ... ]
> @@ -467,10 +512,79 @@ static void perf_counts__update(struct perf_counts_values *count,
>  		count->lost = 0;
>  	}
>  }
> +int evsel__tool_pmu_enable_cpu(struct evsel *evsel, int cpu_map_idx)
> +{

Will tool PMU events configured as members of an event group be successfully
enabled?

In the core event iteration logic like __evlist__enable(), the code
explicitly skips processing any event that is not a group leader. For kernel
PMU events, the kernel automatically enables the entire group when the leader
is enabled.

Since the kernel has no concept of synthetic userspace tool PMU events, if a
tool PMU event is grouped as a non-leader (e.g. 
perf stat -e '{cycles,duration_time}'), doesn't userspace skip it, leaving
evsel__tool_pmu_enable_cpu() uncalled and the event erroneously reporting 0?

> +	enum tool_pmu_event ev = evsel__tool_event(evsel);
> +	int thread, nthreads;
> +
> +	if (!evsel->disabled)
> +		return 0;
> +
> +	if (ev == TOOL_PMU__EVENT_DURATION_TIME) {
> +		if (cpu_map_idx == 0) {
> +			evsel->duration_time.start_time = rdclock();
> +			evsel->disabled = false;
> +		}
> +		return 0;
> +	}
> +
> +	if (ev == TOOL_PMU__EVENT_USER_TIME || ev == TOOL_PMU__EVENT_SYSTEM_TIME) {
> +		nthreads = xyarray__max_y(evsel->process_time.start_times);
> +		for (thread = 0; thread < nthreads; thread++) {
> +			__u64 *start_time = xyarray__entry(evsel->process_time.start_times,
> +							   cpu_map_idx, thread);
> +			__u64 val;
> +			int err;
> +
> +			err = tool_pmu__read_stat(evsel, cpu_map_idx, thread, &val);
> +			if (!err)
> +				*start_time = val;

What happens if tool_pmu__read_stat() fails here?

If reading /proc/<pid>/stat fails because the monitored process just exited,
the error is ignored and *start_time remains 0.

If a subsequent read in evsel__tool_pmu_read() succeeds, could this cause the
delta calculation to subtract the stale 0 start time from the current system
time, incorrectly accumulating massive values and breaking the counter?

> +		}
> +	}
> +	return 0;
> +}

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