Re: [PATCH 09/14] perf c2c: add stats merging and memory management helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Memory leak of nested histogram entries because `hists__delete_entries()` is used instead of `hists__delete_all_entries()`.
--

commit d68acbe4d76a46f5dbb0cda5a255e3f5f919e159
Author: Jiebin Sun <jiebin.sun@intel.com>

perf c2c: add stats merging and memory management helpers

This commit adds statistics aggregation and memory management functions for
function view histogram entries, including c2c_stats_merge(),
c2c_function_he_free(), and c2c_he__free_hierarchy() to handle merging and
recursive cleanup during hierarchy construction.

> diff --git a/tools/perf/ui/browsers/c2c-function.c b/tools/perf/ui/browsers/c2c-function.c
> index c11a81d93c21e..a42378f395f59 100644
> --- a/tools/perf/ui/browsers/c2c-function.c
> +++ b/tools/perf/ui/browsers/c2c-function.c
[ ... ]
> @@ -649,6 +649,164 @@ c2c_function_hists__reinit(struct c2c_hists *c2c_hists,
[ ... ]
> +/*
> + * Free a function-view histogram entry (hist_entry_ops::free).
> + */
> +static void c2c_function_he_free(void *ptr)
> +{
> +	struct hist_entry *he = ptr;
> +	struct c2c_hist_entry *c2c_he;
> +
> +	c2c_he = container_of(he, struct c2c_hist_entry, he);
> +
> +	if (c2c_he->hists) {
> +		perf_hpp__reset_output_field(&c2c_he->hists->list);
> +		hists__delete_entries(&c2c_he->hists->hists);

[Severity: Medium]
Does this code leak nested histogram entries that haven't reached the output
sorted stage?

hists__delete_entries() only iterates over and frees entries in the output
tree. Any entries left in the input or collapsed trees might not be freed.

Should this be hists__delete_all_entries() to prevent leaking these
unprocessed entries, similar to the cleanup handling in builtin-c2c.c?

> +		zfree(&c2c_he->hists);
> +	}
> +
> +	c2c_he__free_hierarchy(he);
> +
[ ... ]
> +static void c2c_he__free_hierarchy(struct hist_entry *he)
> +{
> +	struct rb_node *nd;
> +	struct hist_entry *child_he;
> +	struct c2c_hist_entry *child_c2c;
> +
[ ... ]
> +	nd = rb_first_cached(&he->hroot_out);
> +	while (nd) {
> +		struct rb_node *next = rb_next(nd);
> +
> +		child_he = rb_entry(nd, struct hist_entry, rb_node);
> +		child_c2c = container_of(child_he, struct c2c_hist_entry, he);
> +
> +		if (child_he->stat_acc)
> +			zfree(&child_he->stat_acc);
> +
> +		if (child_he->mem_info)
> +			mem_info__put(child_he->mem_info);
> +
> +		if (child_c2c->hists) {
> +			perf_hpp__reset_output_field(&child_c2c->hists->list);
> +			hists__delete_entries(&child_c2c->hists->hists);

[Severity: Medium]
Could this recursive hierarchy deletion also leak histogram entries?

Like the issue above in c2c_function_he_free(), using hists__delete_entries()
instead of hists__delete_all_entries() skips freeing any entries that are
still in the input or collapsed trees.

> +			zfree(&child_c2c->hists);
> +		}
> +

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