Re: [PATCH v3 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 3 potential issue(s) to consider:
- [Medium] Inconsistent disabled state on early return in evsel__enable() and evsel__disable().
- [Medium] evsel__tool_pmu_disable_cpu() leaves start_time intact if reading stats fails, causing disabled CPUs to keep counting.
- [Medium] Disabling a duration_time group leader prematurely sets its disabled flag, causing __evlist__disable() to skip member events on CPUs > 0.
--

commit c37afe120a59773ddf6b54a63fa825fceec20137
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
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
[ ... ]
> @@ -1810,12 +1867,65 @@ int evsel__enable(struct evsel *evsel)
>  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_non_perf_event_open_pmu(evsel) ||
> +			    evsel__is_non_perf_event_open_pmu(member)) {
[ ... ]
> +				int mem_err = evsel__enable(member);
> +
> +				if (mem_err)
> +					return mem_err;

Does this early return on mem_err skip setting evsel->disabled = false at
the end of the function? It looks like this could leave the group leader with
its software disabled flag set to true, even though the kernel already
successfully enabled it via perf_evsel__enable(&evsel->core).

> +			}
> +			member->disabled = false;
> +		}
> +	}
>  
>  	if (!err)
>  		evsel->disabled = false;
>  	return err;
>  }
[ ... ]
>  int evsel__disable(struct evsel *evsel)
>  {
> -	int err = perf_evsel__disable(&evsel->core);
> +	int err;
> +
> +	if (evsel__is_tool(evsel))
> +		err = evsel__tool_pmu_disable(evsel);
> +	else
> +		err = perf_evsel__disable(&evsel->core);
> +
> +	if (!err && evsel__is_group_leader(evsel)) {
> +		struct evsel *member;
> +
> +		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__disable(member);
> +
> +				if (mem_err)
> +					return mem_err;

A similar early return happens here. If evsel__disable(member) fails, it
skips marking the group leader evsel->disabled = true at the bottom of the
function. Could this leave the state inconsistent for the leader?

> +			}
> +			member->disabled = true;
> +		}
> +	}

> diff --git a/tools/perf/util/tool_pmu.c b/tools/perf/util/tool_pmu.c
> --- a/tools/perf/util/tool_pmu.c
> +++ b/tools/perf/util/tool_pmu.c
[ ... ]
> +int evsel__tool_pmu_disable_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) {
> +			__u64 delta = rdclock() - evsel->duration_time.start_time;
> +
> +			evsel->duration_time.accumulated_time += delta;
> +			evsel->disabled = true;

Since evsel->disabled = true is set globally here when processing
cpu_map_idx 0, will __evlist__disable() see pos->disabled == true for this
group leader when iterating over the remaining CPUs?

If so, would it skip calling evsel__disable_cpu() for CPUs > 0, failing to
disable the associated member events on those CPUs?

> +		}
> +		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 *accumulated_time = xyarray__entry(
> +				evsel->process_time.accumulated_times, cpu_map_idx, thread);
> +			__u64 val;
> +			int err;
> +
> +			err = tool_pmu__read_stat(evsel, cpu_map_idx, thread, &val);
> +			if (!err) {
> +				if (*start_time != INVALID_START_TIME && val >= *start_time)
> +					*accumulated_time += (val - *start_time);
> +				*start_time = INVALID_START_TIME;
> +			}

If tool_pmu__read_stat() fails here (for instance, due to transient /proc
errors or a terminating thread), does leaving *start_time intact cause
subsequent calls to evsel__tool_pmu_read() to continue calculating deltas
using the lingering *start_time and erroneously add them to accumulated_time?

Should *start_time = INVALID_START_TIME; be done unconditionally outside
the if (!err) block?

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

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