Re: [PATCH v1 3/7] perf ui browsers: Implement interactive 'M' keystroke to toggle hybrid event merging

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Toggling hybrid merging off via 'M' fails to reverse structural changes, leaving the evlist permanently merged.
- [High] If merging hybrid events results in a single event group, the 'M' key becomes unreachable, and pressing it causes a silent exit.
- [High] Intercepting K_RELOAD inside the UI browser breaks critical caller-level session reloads.
- [High] Dynamically calling `evlist__merge_hybrid` from the TUI thread modifies `evlist->core.entries` and rbtrees without locks, causing race conditions.
- [Low] The newly introduced function `evsel__new_dummy()` is dead code.
--

--- Patch [3]: [PATCH v1 3/7] perf ui browsers: Implement interactive 'M' keystroke to toggle hybrid event merging ---
commit 761e0204fddbaaaabd4cf218b352b3c2ddbaf235
Author: Ian Rogers <irogers@google.com>

perf ui browsers: Implement interactive 'M' keystroke to toggle hybrid event merging

Map the 'M' keystroke globally across the interface to toggle boolean state
dynamically rebuilding hybrid core groups independently.
This allows cleanly separating or aggregating hybrid core histograms.

> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index c15874a491b43..07cb06834d0f4 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c

[ ... ]

> @@ -3636,6 +3639,16 @@ static int perf_evsel_menu__run(struct evsel_menu *menu,
>  				goto out;
>  			case K_ESC:
>  			default:
> +				if (key == 'M') {
> +					if (evlist__can_merge_hybrid(evlist)) {
> +						if (!symbol_conf.hybrid_merge)
> +							evlist__merge_hybrid(evlist, true);
> +						symbol_conf.hybrid_merge =
> +							!symbol_conf.hybrid_merge;

[Severity: High]
Does toggling hybrid merging off via 'M' fail to reverse the structural
changes?

When symbol_conf.hybrid_merge is toggled from true to false, the state is
updated and K_RELOAD is returned, but there is no call to an unmerge function.
Since evlist__merge_hybrid() irreversibly mutates pos->core.leader and the
doubly linked evlist->core.entries, wouldn't the histograms remain permanently
merged under the hood?

[Severity: High]
Is it safe to dynamically call evlist__merge_hybrid() from the TUI thread
without locks?

In perf top, a background process_thread continuously mutates evlist and its
child rbtrees while holding hists->lock. This call locklessly manipulates the
doubly linked evlist->core.entries and modifies hists nodes. If the background
thread accesses the evlist concurrently, could this cause list corruption or
crashes?

> +						ui_browser__hide(&menu->b);
> +						return K_RELOAD;
> +					}
> +				}
>  				continue;
>  			}
>  		case K_LEFT:

[ ... ]

> @@ -3720,10 +3747,14 @@ static bool evlist__single_entry(struct evlist *evlist)
>  	return false;
>  }
>  
> +
>  int evlist__tui_browse_hists(struct evlist *evlist, const char *help, struct hist_browser_timer *hbt,
>  			     float min_pcnt, struct perf_env *env, bool warn_lost_event)
>  {
>  	int nr_entries = evlist__nr_entries(evlist);
> +	int ret;
> +
> +retry:
>  
>  	if (evlist__single_entry(evlist)) {
>  single_entry: {
> @@ -3747,8 +3778,13 @@ single_entry: {
>  			goto single_entry;
>  	}
>  
> -	return __evlist__tui_browse_hists(evlist, nr_entries, help, hbt, min_pcnt, env,
> +	ret = __evlist__tui_browse_hists(evlist, nr_entries, help, hbt, min_pcnt, env,
>  					  warn_lost_event);
> +	if (ret == K_RELOAD) {
> +		nr_entries = evlist__nr_entries(evlist);
> +		goto retry;

[Severity: High]
Does this logic trap the user if merging hybrid events results in a single
event group?

If evlist__merge_hybrid() coalesces all events into a single group, nr_entries
will evaluate to 1. On K_RELOAD, this loop goes to retry, which then jumps
directly to the single_entry block and bypasses the perf_evsel_menu__run() menu
loop. Since the single entry view lacks the 'M' handler, pressing 'M' again
would act as an unhandled hotkey and cause a silent exit.

[Severity: High]
Does intercepting K_RELOAD inside the UI browser break critical caller-level
session reloads?

Callers like display_thread_tui() in builtin-top.c previously relied on
receiving K_RELOAD to execute full session re-initialization (e.g., zeroing
histograms). By swallowing the signal with this goto retry loop, does this
force an internal redraw with stale structures instead of yielding control to
the caller?

> +	}
> +	return ret;
>  }
>  
>  static int block_hists_browser__title(struct hist_browser *browser, char *bf,

[ ... ]

> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 3f56a0e6f9d65..a4760cfb7582a 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -4758,3 +4758,15 @@ void evsel__warn_user_requested_cpus(struct evsel *evsel, struct perf_cpu_map *u
>  	perf_cpu_map__put(intersect);
>  	perf_cpu_map__put(online);
>  }
> +
> +struct evsel *evsel__new_dummy(void)
> +{
> +	struct perf_event_attr attr = {
> +		.type	= PERF_TYPE_SOFTWARE,
> +		.config = PERF_COUNT_SW_DUMMY,
> +		.size	= sizeof(attr),
> +		.freq = 0,
> +		.sample_period = 1,
> +	};
> +	return evsel__new(&attr);
> +}

[Severity: Low]
Is this newly introduced function evsel__new_dummy() dead code?

It is defined and exported here, but it doesn't appear to be called anywhere in
the patch series.

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