Re: [PATCH v2 5/9] perf evlist: Toggle merged_hybrid_group properly in enable/disable

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf: dead code in evlist enable/disable due to flag misunderstanding
--

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

perf evlist: Toggle merged_hybrid_group properly in enable/disable

Merging makes the merged event the leader of the events of the other
core PMUs so their histograms can be linked, but the result isn't a
real event group. The commit treats an evsel with merged_hybrid_group
set as a leader so that it is enabled and disabled in its own right,
and updates the logic to avoid walking the members of a merged leader.

> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index 930efbb66bc8c..034032955d332 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
[ ... ]
> @@ -789,16 +796,21 @@ static void __evlist__disable(struct evlist *evlist, char *evsel_name, bool excl
>  	}
>  
>  	evlist__for_each_entry(evlist, pos) {
> -		if (evsel__strcmp(pos, evsel_name))
> +		match = !evsel__strcmp(pos, evsel_name);
> +		if (!match && pos->merged_hybrid_group && evsel__leader(pos))
> +			match = !evsel__strcmp(evsel__leader(pos), evsel_name);
> +		if (!match)
>  			continue;
> -		if (!evsel__is_group_leader(pos) || !pos->core.fd)
> +		if ((!evsel__is_group_leader(pos) && !pos->merged_hybrid_group) || !pos->core.fd)
>  			continue;
>  		if (excl_dummy && evsel__is_dummy_event(pos))
>  			continue;
>  		pos->disabled = true;
>  
> -		for_each_group_member(member, pos)
> -			member->disabled = true;
> +		if (!pos->merged_hybrid_group) {
> +			for_each_group_member(member, pos)
> +				member->disabled = true;
> +		}

[Severity: Medium]
Does this logic correctly prevent walking the members of a merged leader?

The commit message states "don't walk the members of a merged leader".
However, the merged_hybrid_group flag is only set on merged members, never on
the leader. When __evlist__disable() processes a merged leader,
pos->merged_hybrid_group is false, so this code will walk the members anyway.

Conversely, when processing a merged member, the flag is true, skipping the
block. But since a member has no members of its own, the walk would have been
empty anyway.

Does this check correctly match the intent, or should it be testing if the
current group leader is a merged leader instead?

>  	}
>  
>  	/*
[ ... ]
> @@ -827,28 +839,37 @@ static void __evlist__enable(struct evlist *evlist, char *evsel_name, bool excl_
[ ... ]
>  	evlist__for_each_entry(evlist, pos) {
> -		if (evsel__strcmp(pos, evsel_name))
> +		match = !evsel__strcmp(pos, evsel_name);
> +		if (!match && pos->merged_hybrid_group && evsel__leader(pos))
> +			match = !evsel__strcmp(evsel__leader(pos), evsel_name);
> +		if (!match)
>  			continue;
> -		if (!evsel__is_group_leader(pos) || !pos->core.fd)
> +		if ((!evsel__is_group_leader(pos) && !pos->merged_hybrid_group) || !pos->core.fd)
>  			continue;
>  		if (excl_dummy && evsel__is_dummy_event(pos))
>  			continue;
>  		pos->disabled = false;
> +		if (!pos->merged_hybrid_group) {
> +			for_each_group_member(member, pos)
> +				member->disabled = false;
> +		}

[Severity: Medium]
Does this code also suffer from the same flag misunderstanding in
__evlist__enable()?

If pos->merged_hybrid_group is only set on members, this check will similarly
fail to prevent the merged leader from walking its members.

>  
> -		for_each_group_member(member, pos)
> -			member->disabled = false;
>  	}
>  
>  	/*

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