Re: [PATCH v2 11/14] perf c2c: add function view hierarchy builder

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Stranded entries in `entries_in` are leaked on the error path because `hists__collapse_resort` can abort early.
--

--- Patch [11]: [PATCH v2 11/14] perf c2c: add function view hierarchy builder ---
commit 7fa08f7966668758027f5725f57fcfebac5eaf48
Author: Jiebin Sun <jiebin.sun@intel.com>

perf c2c: add function view hierarchy builder

This patch adds the core algorithm to construct a 3-level function view
for perf c2c. It traverses cacheline entries to build the hierarchy and
aggregates C2C statistics at each level.

> diff --git a/tools/perf/ui/browsers/c2c-function.c b/tools/perf/ui/browsers/c2c-function.c
> index 47d6fdf66b47b..80e032d5e31c5 100644
> --- a/tools/perf/ui/browsers/c2c-function.c
> +++ b/tools/perf/ui/browsers/c2c-function.c

[ ... ]

> +static __maybe_unused int build_function_view_hierarchy(void)
> +{

[ ... ]

> +out_err:
> +	/*
> +	 * On error, migrate any entries still in entries_in to entries and
> +	 * delete them, so a later rebuild does not strand them (the top-level
> +	 * __hists__init() memset would otherwise lose the pointers).
> +	 */
> +	hists__collapse_resort(&c2c_ext.function_hists.hists, NULL);
> +	hists__output_resort(&c2c_ext.function_hists.hists, NULL);
> +	hists__delete_entries(&c2c_ext.function_hists.hists);

[Severity: Medium]
Does this error path leak memory if hists__collapse_resort() aborts early?

Looking at hists__collapse_resort() in tools/perf/util/hist.c, it can exit
early if session_done() is true or if hists__collapse_insert_entry() fails:

tools/perf/util/hist.c:hists__collapse_resort() {
    ...
	while (next) {
		if (session_done())
			break;
		n = rb_entry(next, struct hist_entry, rb_node_in);
		next = rb_next(&n->rb_node_in);

		rb_erase_cached(&n->rb_node_in, root);
		ret = hists__collapse_insert_entry(hists, &hists->entries_collapsed, n);
		if (ret < 0)
			return -1;
    ...
}

If it exits early, the remaining unprocessed nodes are left behind in
entries_in. Since hists__delete_entries() only deletes from the main
entries trees, these stranded nodes will be permanently leaked.

Would it be better to use hists__delete_all_entries(&c2c_ext.function_hists.hists)
here instead to ensure both entries and entries_in are properly cleaned up?

> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710084247.3576706-1-jiebin.sun@intel.com?part=11