[PATCH v2 1/2] perf report: Use map_symbol__copy() when copying callchians

[PATCH v2 1/2] perf report: Use map_symbol__copy() when copying callchians

[Namhyung Kim]

It seems there are places to miss updating refcount of maps.
Let's use map_symbol__copy() helper to properly copy them with
refcounts updated.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/callchain.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
index 0c7564747a14e539..d7b7eef740b9d6ed 100644
--- a/tools/perf/util/callchain.c
+++ b/tools/perf/util/callchain.c
@@ -589,9 +589,7 @@ fill_node(struct callchain_node *node, struct callchain_cursor *cursor)
 			return -ENOMEM;
 		}
 		call->ip = cursor_node->ip;
-		call->ms = cursor_node->ms;
-		call->ms.map = map__get(call->ms.map);
-		call->ms.maps = maps__get(call->ms.maps);
+		map_symbol__copy(&call->ms, &cursor_node->ms);
 		call->srcline = cursor_node->srcline;
 
 		if (cursor_node->branch) {
@@ -1094,9 +1092,7 @@ int callchain_cursor_append(struct callchain_cursor *cursor,
 
 	node->ip = ip;
 	map_symbol__exit(&node->ms);
-	node->ms = *ms;
-	node->ms.maps = maps__get(ms->maps);
-	node->ms.map = map__get(ms->map);
+	map_symbol__copy(&node->ms, ms);
 	node->branch = branch;
 	node->nr_loop_iter = nr_loop_iter;
 	node->iter_cycles = iter_cycles;
@@ -1564,7 +1560,7 @@ int callchain_node__make_parent_list(struct callchain_node *node)
 				goto out;
 			*new = *chain;
 			new->has_children = false;
-			new->ms.map = map__get(new->ms.map);
+			map_symbol__copy(&new->ms, &chain->ms);
 			list_add_tail(&new->list, &head);
 		}
 		parent = parent->parent;
-- 
2.49.0.rc0.332.g42c0ae87b1-goog

[PATCH v2 2/2] perf report: Fix memory leaks in the hierarchy mode

[Namhyung Kim]

Ian told me that there are many memory leaks in the hierarchy mode.  I
can easily reproduce it with the follwing command.

  $ make DEBUG=1 EXTRA_CFLAGS=-fsanitize=leak

  $ perf record --latency -g -- ./perf test -w thloop

  $ perf report -H --stdio
  ...
  Indirect leak of 168 byte(s) in 21 object(s) allocated from:
      #0 0x7f3414c16c65 in malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:75
      #1 0x55ed3602346e in map__get util/map.h:189
      #2 0x55ed36024cc4 in hist_entry__init util/hist.c:476
      #3 0x55ed36025208 in hist_entry__new util/hist.c:588
      #4 0x55ed36027c05 in hierarchy_insert_entry util/hist.c:1587
      #5 0x55ed36027e2e in hists__hierarchy_insert_entry util/hist.c:1638
      #6 0x55ed36027fa4 in hists__collapse_insert_entry util/hist.c:1685
      #7 0x55ed360283e8 in hists__collapse_resort util/hist.c:1776
      #8 0x55ed35de0323 in report__collapse_hists /home/namhyung/project/linux/tools/perf/builtin-report.c:735
      #9 0x55ed35de15b4 in __cmd_report /home/namhyung/project/linux/tools/perf/builtin-report.c:1119
      #10 0x55ed35de43dc in cmd_report /home/namhyung/project/linux/tools/perf/builtin-report.c:1867
      #11 0x55ed35e66767 in run_builtin /home/namhyung/project/linux/tools/perf/perf.c:351
      #12 0x55ed35e66a0e in handle_internal_command /home/namhyung/project/linux/tools/perf/perf.c:404
      #13 0x55ed35e66b67 in run_argv /home/namhyung/project/linux/tools/perf/perf.c:448
      #14 0x55ed35e66eb0 in main /home/namhyung/project/linux/tools/perf/perf.c:556
      #15 0x7f340ac33d67 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
  ...

  $ perf report -H --stdio 2>&1 | grep -c '^Indirect leak'
  93

I found that hist_entry__delete() missed to release child entries in the
hierarchy tree (hroot_{in,out}).  It needs to iterate the child entries
and call hist_entry__delete() recursively.

After this change:

  $ perf report -H --stdio 2>&1 | grep -c '^Indirect leak'
  0

Reported-by: Ian Rogers <irogers@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
v2) Use rbtree_postorder_for_each_entry_safe()  (Ian)

 tools/perf/util/hist.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index fbf131aeae7ffe9b..d65228c1141251fb 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -1385,6 +1385,16 @@ void hist_entry__delete(struct hist_entry *he)
 {
 	struct hist_entry_ops *ops = he->ops;
 
+	if (symbol_conf.report_hierarchy) {
+		struct rb_root *root = &he->hroot_out.rb_root;
+		struct hist_entry *child, *tmp;
+
+		rbtree_postorder_for_each_entry_safe(child, tmp, root, rb_node)
+			hist_entry__delete(child);
+
+		*root = RB_ROOT;
+	}
+
 	thread__zput(he->thread);
 	map_symbol__exit(&he->ms);
 
-- 
2.49.0.rc0.332.g42c0ae87b1-goog

Re: [PATCH v2 2/2] perf report: Fix memory leaks in the hierarchy mode

[Falcon, Thomas]

On Thu, 2025-03-06 at 22:12 -0800, Namhyung Kim wrote:
> Ian told me that there are many memory leaks in the hierarchy mode. 
> I
> can easily reproduce it with the follwing command.
> 
>   $ make DEBUG=1 EXTRA_CFLAGS=-fsanitize=leak
> 
>   $ perf record --latency -g -- ./perf test -w thloop
> 
>   $ perf report -H --stdio
>   ...
>   Indirect leak of 168 byte(s) in 21 object(s) allocated from:
>       #0 0x7f3414c16c65 in malloc
> ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:75
>       #1 0x55ed3602346e in map__get util/map.h:189
>       #2 0x55ed36024cc4 in hist_entry__init util/hist.c:476
>       #3 0x55ed36025208 in hist_entry__new util/hist.c:588
>       #4 0x55ed36027c05 in hierarchy_insert_entry util/hist.c:1587
>       #5 0x55ed36027e2e in hists__hierarchy_insert_entry
> util/hist.c:1638
>       #6 0x55ed36027fa4 in hists__collapse_insert_entry
> util/hist.c:1685
>       #7 0x55ed360283e8 in hists__collapse_resort util/hist.c:1776
>       #8 0x55ed35de0323 in report__collapse_hists
> /home/namhyung/project/linux/tools/perf/builtin-report.c:735
>       #9 0x55ed35de15b4 in __cmd_report
> /home/namhyung/project/linux/tools/perf/builtin-report.c:1119
>       #10 0x55ed35de43dc in cmd_report
> /home/namhyung/project/linux/tools/perf/builtin-report.c:1867
>       #11 0x55ed35e66767 in run_builtin
> /home/namhyung/project/linux/tools/perf/perf.c:351
>       #12 0x55ed35e66a0e in handle_internal_command
> /home/namhyung/project/linux/tools/perf/perf.c:404
>       #13 0x55ed35e66b67 in run_argv
> /home/namhyung/project/linux/tools/perf/perf.c:448
>       #14 0x55ed35e66eb0 in main
> /home/namhyung/project/linux/tools/perf/perf.c:556
>       #15 0x7f340ac33d67 in __libc_start_call_main
> ../sysdeps/nptl/libc_start_call_main.h:58
>   ...
> 
>   $ perf report -H --stdio 2>&1 | grep -c '^Indirect leak'
>   93
> 
> I found that hist_entry__delete() missed to release child entries in
> the
> hierarchy tree (hroot_{in,out}).  It needs to iterate the child
> entries
> and call hist_entry__delete() recursively.
> 
> After this change:
> 
>   $ perf report -H --stdio 2>&1 | grep -c '^Indirect leak'
>   0
> 
> Reported-by: Ian Rogers <irogers@google.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Tested on an Arrow Lake system.

Tested-by Thomas Falcon <thomas.falcon@intel.com>

> ---
> v2) Use rbtree_postorder_for_each_entry_safe()  (Ian)
> 
>  tools/perf/util/hist.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index fbf131aeae7ffe9b..d65228c1141251fb 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -1385,6 +1385,16 @@ void hist_entry__delete(struct hist_entry *he)
>  {
>  	struct hist_entry_ops *ops = he->ops;
>  
> +	if (symbol_conf.report_hierarchy) {
> +		struct rb_root *root = &he->hroot_out.rb_root;
> +		struct hist_entry *child, *tmp;
> +
> +		rbtree_postorder_for_each_entry_safe(child, tmp,
> root, rb_node)
> +			hist_entry__delete(child);
> +
> +		*root = RB_ROOT;
> +	}
> +
>  	thread__zput(he->thread);
>  	map_symbol__exit(&he->ms);
>  

Re: [PATCH v2 2/2] perf report: Fix memory leaks in the hierarchy mode

[Ian Rogers]

On Fri, Mar 7, 2025 at 8:28 AM Falcon, Thomas <thomas.falcon@intel.com> wrote:
>
> On Thu, 2025-03-06 at 22:12 -0800, Namhyung Kim wrote:
> > Ian told me that there are many memory leaks in the hierarchy mode.
> > I
> > can easily reproduce it with the follwing command.
> >
> >   $ make DEBUG=1 EXTRA_CFLAGS=-fsanitize=leak
> >
> >   $ perf record --latency -g -- ./perf test -w thloop
> >
> >   $ perf report -H --stdio
> >   ...
> >   Indirect leak of 168 byte(s) in 21 object(s) allocated from:
> >       #0 0x7f3414c16c65 in malloc
> > ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:75
> >       #1 0x55ed3602346e in map__get util/map.h:189
> >       #2 0x55ed36024cc4 in hist_entry__init util/hist.c:476
> >       #3 0x55ed36025208 in hist_entry__new util/hist.c:588
> >       #4 0x55ed36027c05 in hierarchy_insert_entry util/hist.c:1587
> >       #5 0x55ed36027e2e in hists__hierarchy_insert_entry
> > util/hist.c:1638
> >       #6 0x55ed36027fa4 in hists__collapse_insert_entry
> > util/hist.c:1685
> >       #7 0x55ed360283e8 in hists__collapse_resort util/hist.c:1776
> >       #8 0x55ed35de0323 in report__collapse_hists
> > /home/namhyung/project/linux/tools/perf/builtin-report.c:735
> >       #9 0x55ed35de15b4 in __cmd_report
> > /home/namhyung/project/linux/tools/perf/builtin-report.c:1119
> >       #10 0x55ed35de43dc in cmd_report
> > /home/namhyung/project/linux/tools/perf/builtin-report.c:1867
> >       #11 0x55ed35e66767 in run_builtin
> > /home/namhyung/project/linux/tools/perf/perf.c:351
> >       #12 0x55ed35e66a0e in handle_internal_command
> > /home/namhyung/project/linux/tools/perf/perf.c:404
> >       #13 0x55ed35e66b67 in run_argv
> > /home/namhyung/project/linux/tools/perf/perf.c:448
> >       #14 0x55ed35e66eb0 in main
> > /home/namhyung/project/linux/tools/perf/perf.c:556
> >       #15 0x7f340ac33d67 in __libc_start_call_main
> > ../sysdeps/nptl/libc_start_call_main.h:58
> >   ...
> >
> >   $ perf report -H --stdio 2>&1 | grep -c '^Indirect leak'
> >   93
> >
> > I found that hist_entry__delete() missed to release child entries in
> > the
> > hierarchy tree (hroot_{in,out}).  It needs to iterate the child
> > entries
> > and call hist_entry__delete() recursively.
> >
> > After this change:
> >
> >   $ perf report -H --stdio 2>&1 | grep -c '^Indirect leak'
> >   0
> >
> > Reported-by: Ian Rogers <irogers@google.com>
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>
> Tested on an Arrow Lake system.
>
> Tested-by Thomas Falcon <thomas.falcon@intel.com>

Reviewed-by: Ian Rogers <irogers@google.com>

Thanks!
Ian

Re: [PATCH v2 1/2] perf report: Use map_symbol__copy() when copying callchians

[Markus Elfring]

* Please avoid a typo in the summary phrase.

* Can a cover letter be helpful for such a patch series?


Regards,
Markus

Re: [PATCH v2 1/2] perf report: Use map_symbol__copy() when copying callchians

[Namhyung Kim]

Hello,

On Fri, Mar 07, 2025 at 07:55:25PM +0100, Markus Elfring wrote:
> * Please avoid a typo in the summary phrase.

Thanks for pointing this out, will fix.

> 
> * Can a cover letter be helpful for such a patch series?

It's just two patches and they are independent.  I don't think it needs
a cover letter.

Thanks,
Namhyung

Re: [v2 1/2] perf report: Use map_symbol__copy() when copying call chains

[Markus Elfring]

> It's just two patches and they are independent.

How do you think about to offer the desired changes as single items then
(without the context of a patch series in this case)?

Regards,
Markus

Re: [v2 1/2] perf report: Use map_symbol__copy() when copying call chains

[Namhyung Kim]

On Sat, Mar 08, 2025 at 08:56:37AM +0100, Markus Elfring wrote:
> > It's just two patches and they are independent.
> 
> How do you think about to offer the desired changes as single items then
> (without the context of a patch series in this case)?

It could be.  But they are loosely related as I found the issue during
investigation of memory leaks so I sent them together.  I think I added
enough explanation in each commit.

Thanks,
Namhyung

Re: [PATCH v2 1/2] perf report: Use map_symbol__copy() when copying callchians

[Namhyung Kim]

On Thu, 06 Mar 2025 22:12:49 -0800, Namhyung Kim wrote:
> It seems there are places to miss updating refcount of maps.
> Let's use map_symbol__copy() helper to properly copy them with
> refcounts updated.
> 
> 
Applied to perf-tools-next, thanks!

Best regards,
Namhyung