Re: [PATCH v1] perf top: Merge hybrid common events

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Incorrect `total_period` calculation in `hist_entry__get_percent_limit` for hybrid merged entries artificially inflates symbol percentages and breaks the `--percent-limit` filter.
- [Medium] Dummy events are improperly added to the hybrid merged event group, resulting in unwanted empty columns in the perf top display.
--

--- Patch [1]: [PATCH v1] perf top: Merge hybrid common events ---
commit 49ec7e29ca8d51aba244d3542196da5fbe98b440
Author: Andi Kleen <[email protected]>

    perf top: Merge hybrid common events
    
    This patch automatically merges the two PMUs when the event is present
    in both PMUs for simple TYPE_HARDWARE cases, like cycles. The behavior
    can be disabled with --no-hybrid-merge.

> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index 1211401616ee3..19dec094683b3 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
[ ... ]
> @@ -1334,6 +1334,19 @@ static int __cmd_top(struct perf_top *top)
>  	 */
>          if (!target__none(&opts->target))
>  		evlist__enable(top->evlist);
> +	if (top->hybrid_merge && !symbol_conf.report_hierarchy &&
> +	    evlist__can_merge_hybrid(top->evlist)) {
> +		struct evsel *leader = evlist__first(top->evlist);
> +
> +		/*
> +		 * Merged events are not true groups, but can use
> +		 * the existing group display code to display them
> +		 * anyways.
> +		 */
> +		__perf_evlist__set_leader(&top->evlist->core.entries, &leader->core);

[Severity: Medium]
Does __perf_evlist__set_leader() unconditionally force all events in the
evlist, including dummy events, into the merged group?

While evlist__can_merge_hybrid() correctly skips dummy events, this assignment
appears to include them anyway. This results in the dummy events improperly
being added to the group, creating unwanted empty columns in the perf top
display.

> +		evsel__hists(leader)->merge_entries = true;
> +		symbol_conf.event_group = true;
> +	}
>  
>  	ret = -1;
>  	if (pthread_create(&thread_process, NULL, process_thread, top)) {
[ ... ]
> diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
> index b830cbe7f95bf..a9dd423ded5e1 100644
> --- a/tools/perf/util/hist.h
> +++ b/tools/perf/util/hist.h
[ ... ]
> @@ -435,14 +436,26 @@ int hists__unlink(struct hists *hists);
>  
>  static inline float hist_entry__get_percent_limit(struct hist_entry *he)
>  {
> +	struct hist_entry *pair;
>  	u64 period = he->stat.period;
>  	u64 total_period = hists__total_period(he->hists);
>  
> +	if (he->hists->merge_entries) {
> +		list_for_each_entry(pair, &he->pairs.head, pairs.node) {
> +			period += pair->stat.period;
> +			total_period += hists__total_period(pair->hists);
> +		}
> +	}
> +

[Severity: Medium]
Does this loop conditionally sum total_period based on the existence of a
paired entry, thereby under-calculating the global denominator for symbols
that were not hit on all PMUs?

If the denominator shrinks dynamically for asymmetric hits, it would
artificially inflate symbol percentages and break the --percent-limit filter.

>  	if (unlikely(total_period == 0))
>  		return 0;
>  
> -	if (symbol_conf.cumulate_callchain)
> +	if (symbol_conf.cumulate_callchain) {
>  		period = he->stat_acc->period;
> +		if (he->hists->merge_entries)
> +			list_for_each_entry(pair, &he->pairs.head, pairs.node)
> +				period += pair->stat_acc->period;
> +	}
>  
>  	return period * 100.0 / total_period;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1