Re: [PATCH v2 2/4] perf tool_pmu: Add network events

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] perf tool_pmu: grouped tool events stay disabled when
  enable_on_exec is set
--

commit 8a5f1d91c8102e028647607058772cadeb3a4c93
Author: Ian Rogers <irogers@google.com>

perf tool_pmu: Add network events

This commit adds system-wide network tool PMU events read from /proc/net/dev
and updates tool PMU enable, disable, and group leader handling.

> diff --git a/tools/perf/util/tool_pmu.c b/tools/perf/util/tool_pmu.c
> index 0ae08cd910bc4..655b221043201 100644
> --- a/tools/perf/util/tool_pmu.c
> +++ b/tools/perf/util/tool_pmu.c
[ ... ]
> @@ -539,40 +676,58 @@ static int open_memory_pid_dir(enum tool_pmu_event ev, int pid,
>  	return 0;
>  }
>  
> +static bool tool_pmu__is_initially_disabled(const struct evsel *evsel)
> +{
> +	const struct evsel *leader = evsel__leader(evsel);
> +
> +	return evsel->core.attr.disabled || (leader && leader->core.attr.disabled);
> +}
> +
>  static int evsel__tool_pmu_open_one(struct evsel *evsel, enum tool_pmu_event ev,
>  				    int pid, int idx, int thread,
>  				    int *seen_tgids, int *nr_seen_tgids)
>  {
[ ... ]
> -		if (evsel->core.attr.disabled) {
> +		if (tool_pmu__is_initially_disabled(evsel)) {
>  			evsel->disabled = true;
>  			*start_time = INVALID_START_TIME;
>  		} else {

[Severity: Medium]
Does tool_pmu__is_initially_disabled() leave grouped tool events permanently
disabled when profiling a forked workload?

When target__enable_on_exec(&target) is true (for example, perf stat -e
'{cycles,duration_time}' -- <workload>), create_perf_stat_counter() sets both
attr->disabled = 1 and attr->enable_on_exec = 1 on the group leader:

tools/perf/builtin-stat.c:create_perf_stat_counter() {
	...
	if (evsel__is_group_leader(evsel)) {
		attr->disabled = 1;

		if (target__enable_on_exec(&target))
			attr->enable_on_exec = 1;
	}
	...
}

With this change, tool_pmu__is_initially_disabled() returns true for the
grouped tool event, so evsel__tool_pmu_open() and evsel__tool_pmu_open_one()
set evsel->disabled = true and start_time = INVALID_START_TIME. Because
enable_counters() skips evlist__enable() when target__enable_on_exec(&target)
is true and userspace tool events have no kernel exec hook, evsel->disabled
remains true for the entire run and perf stat reports 0 for the grouped tool
counter.

Checking !leader->core.attr.enable_on_exec (and !evsel->core.attr.enable_on_exec)
in tool_pmu__is_initially_disabled() would avoid leaving these counters
disabled.

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