Re: [PATCH v2 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 2 potential issue(s) to consider:
- [High] Tool PMU events (user_time and system_time) fail to update their `disabled` state when acting as event group members, leading to missing data (reporting 0).
- [Medium] File descriptor leak in `evsel__tool_pmu_open()` error path.
--

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

perf tool_pmu: Make tool PMU events respect enable/disable

This commit implements enable and disable support for tool PMU events
(duration_time, user_time, system_time) to make them behave more like regular
counters. It adds accumulated_time tracking to prevent the events from
counting time while paused.

Closes: 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..63bfb03b6b13a 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
[ ... ]
> @@ -1795,12 +1798,63 @@ int evsel__append_addr_filter(struct evsel *evsel, const char *filter)
[ ... ]
>  int evsel__enable(struct evsel *evsel)
>  {
> -	int err = perf_evsel__enable(&evsel->core);
> +	int err;
> +
> +	if (evsel__is_tool(evsel))
> +		err = evsel__tool_pmu_enable(evsel);
> +	else
> +		err = perf_evsel__enable(&evsel->core);
> +
> +	if (!err && evsel__is_group_leader(evsel)) {
> +		struct evsel *member;
> +
> +		for_each_group_member(member, evsel) {
> +			if (evsel__is_tool(member)) {
[ ... ]
> +				int mem_err = evsel__tool_pmu_enable(member);
> +
> +				if (mem_err)
> +					return mem_err;
> +			}
> +		}
> +	}
>  
>  	if (!err)
>  		evsel->disabled = false;

When evsel__enable() iterates over group members to enable them, does the
disabled flag get updated correctly for user_time and system_time events?

Since evsel->disabled = false at the end of this function only applies to
the group leader, it seems the group members are reliant on
evsel__tool_pmu_enable_cpu() to update their state.

> diff --git a/tools/perf/util/tool_pmu.c b/tools/perf/util/tool_pmu.c
> index 6a9df3dc0e076..bb398fe82f6d9 100644
> --- a/tools/perf/util/tool_pmu.c
> +++ b/tools/perf/util/tool_pmu.c
[ ... ]
> @@ -246,8 +292,8 @@ int evsel__tool_pmu_open(struct evsel *evsel,
[ ... ]
> +				if (evsel->core.attr.disabled) {
> +					evsel->disabled = true;
> +					*start_time = INVALID_START_TIME;
>  				} else {
> -					struct perf_cpu cpu;
> -
> -					cpu = perf_cpu_map__cpu(evsel->core.cpus, idx);
> -					err = read_stat_field(fd, cpu, system ? 3 : 1,
> -							      start_time);
> +					evsel->disabled = false;
> +					err = tool_pmu__read_stat(evsel, idx, thread, start_time);
> +					if (err)
> +						goto out_close;
>  				}

If tool_pmu__read_stat() fails here and jumps to out_close, is the file
descriptor we just successfully opened and saved in FD(evsel, idx, thread)
leaked?

Looking at the out_close cleanup logic:

out_close:
        ...
        do {
                while (--thread >= 0) {
                        if (FD(evsel, idx, thread) >= 0)
                                close(FD(evsel, idx, thread));
                        FD(evsel, idx, thread) = -1;
                }
                thread = nthreads;
        } while (--idx >= 0);

Since the loop pre-decrements the thread index, it seems to skip closing
the file descriptor for the currently failing thread.

[ ... ]
> @@ -467,10 +514,108 @@ static void perf_counts__update(struct perf_counts_values *count,
[ ... ]
> +int evsel__tool_pmu_enable_cpu(struct evsel *evsel, int cpu_map_idx)
> +{
> +	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;
> +			else
> +				*start_time = INVALID_START_TIME;
> +		}
> +	}
> +	return 0;
> +}

Unlike the duration_time branch, it looks like evsel->disabled is not set
to false when processing user_time or system_time events here.

Will this cause evsel__tool_pmu_read() to see evsel->disabled as still true
and unconditionally return the frozen accumulated time for these members?
A similar pattern appears to be present in the disable path as well.

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