* [PATCH v2] perf session: add missing evlist__delete when deleting a session
@ 2021-06-24 23:19 Riccardo Mancini
2021-06-25 5:39 ` Ian Rogers
0 siblings, 1 reply; 6+ messages in thread
From: Riccardo Mancini @ 2021-06-24 23:19 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Namhyung Kim, Ian Rogers, Riccardo Mancini, Peter Zijlstra,
Ingo Molnar, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Adrian Hunter, Kan Liang, Leo Yan, linux-perf-users,
linux-kernel
ASan reports a memory leak caused by evlist not being deleted on exit in
perf-report, perf-script and perf-data.
The problem is caused by evlist->session not being deleted, which is
allocated in perf_session__read_header, called in perf_session__new if
perf_data is in read mode.
In case of write mode, the session->evlist is filled by the caller.
This patch solves the problem by calling evlist__delete in
perf_session__delete if perf_data is in read mode.
Changes in v2:
- call evlist__delete from within perf_session__delete
v1: https://lore.kernel.org/lkml/20210621234317.235545-1-rickyman7@gmail.com/
ASan report follows:
$ ./perf script report flamegraph
=================================================================
==227640==ERROR: LeakSanitizer: detected memory leaks
<SNIP unrelated>
Indirect leak of 2704 byte(s) in 1 object(s) allocated from:
#0 0x4f4137 in calloc (/home/user/linux/tools/perf/perf+0x4f4137)
#1 0xbe3d56 in zalloc /home/user/linux/tools/lib/perf/../../lib/zalloc.c:8:9
#2 0x7f999e in evlist__new /home/user/linux/tools/perf/util/evlist.c:77:26
#3 0x8ad938 in perf_session__read_header /home/user/linux/tools/perf/util/header.c:3797:20
#4 0x8ec714 in perf_session__open /home/user/linux/tools/perf/util/session.c:109:6
#5 0x8ebe83 in perf_session__new /home/user/linux/tools/perf/util/session.c:213:10
#6 0x60c6de in cmd_script /home/user/linux/tools/perf/builtin-script.c:3856:12
#7 0x7b2930 in run_builtin /home/user/linux/tools/perf/perf.c:313:11
#8 0x7b120f in handle_internal_command /home/user/linux/tools/perf/perf.c:365:8
#9 0x7b2493 in run_argv /home/user/linux/tools/perf/perf.c:409:2
#10 0x7b0c89 in main /home/user/linux/tools/perf/perf.c:539:3
#11 0x7f5260654b74 (/lib64/libc.so.6+0x27b74)
Indirect leak of 568 byte(s) in 1 object(s) allocated from:
#0 0x4f4137 in calloc (/home/user/linux/tools/perf/perf+0x4f4137)
#1 0xbe3d56 in zalloc /home/user/linux/tools/lib/perf/../../lib/zalloc.c:8:9
#2 0x80ce88 in evsel__new_idx /home/user/linux/tools/perf/util/evsel.c:268:24
#3 0x8aed93 in evsel__new /home/user/linux/tools/perf/util/evsel.h:210:9
#4 0x8ae07e in perf_session__read_header /home/user/linux/tools/perf/util/header.c:3853:11
#5 0x8ec714 in perf_session__open /home/user/linux/tools/perf/util/session.c:109:6
#6 0x8ebe83 in perf_session__new /home/user/linux/tools/perf/util/session.c:213:10
#7 0x60c6de in cmd_script /home/user/linux/tools/perf/builtin-script.c:3856:12
#8 0x7b2930 in run_builtin /home/user/linux/tools/perf/perf.c:313:11
#9 0x7b120f in handle_internal_command /home/user/linux/tools/perf/perf.c:365:8
#10 0x7b2493 in run_argv /home/user/linux/tools/perf/perf.c:409:2
#11 0x7b0c89 in main /home/user/linux/tools/perf/perf.c:539:3
#12 0x7f5260654b74 (/lib64/libc.so.6+0x27b74)
Indirect leak of 264 byte(s) in 1 object(s) allocated from:
#0 0x4f4137 in calloc (/home/user/linux/tools/perf/perf+0x4f4137)
#1 0xbe3d56 in zalloc /home/user/linux/tools/lib/perf/../../lib/zalloc.c:8:9
#2 0xbe3e70 in xyarray__new /home/user/linux/tools/lib/perf/xyarray.c:10:23
#3 0xbd7754 in perf_evsel__alloc_id /home/user/linux/tools/lib/perf/evsel.c:361:21
#4 0x8ae201 in perf_session__read_header /home/user/linux/tools/perf/util/header.c:3871:7
#5 0x8ec714 in perf_session__open /home/user/linux/tools/perf/util/session.c:109:6
#6 0x8ebe83 in perf_session__new /home/user/linux/tools/perf/util/session.c:213:10
#7 0x60c6de in cmd_script /home/user/linux/tools/perf/builtin-script.c:3856:12
#8 0x7b2930 in run_builtin /home/user/linux/tools/perf/perf.c:313:11
#9 0x7b120f in handle_internal_command /home/user/linux/tools/perf/perf.c:365:8
#10 0x7b2493 in run_argv /home/user/linux/tools/perf/perf.c:409:2
#11 0x7b0c89 in main /home/user/linux/tools/perf/perf.c:539:3
#12 0x7f5260654b74 (/lib64/libc.so.6+0x27b74)
Indirect leak of 32 byte(s) in 1 object(s) allocated from:
#0 0x4f4137 in calloc (/home/user/linux/tools/perf/perf+0x4f4137)
#1 0xbe3d56 in zalloc /home/user/linux/tools/lib/perf/../../lib/zalloc.c:8:9
#2 0xbd77e0 in perf_evsel__alloc_id /home/user/linux/tools/lib/perf/evsel.c:365:14
#3 0x8ae201 in perf_session__read_header /home/user/linux/tools/perf/util/header.c:3871:7
#4 0x8ec714 in perf_session__open /home/user/linux/tools/perf/util/session.c:109:6
#5 0x8ebe83 in perf_session__new /home/user/linux/tools/perf/util/session.c:213:10
#6 0x60c6de in cmd_script /home/user/linux/tools/perf/builtin-script.c:3856:12
#7 0x7b2930 in run_builtin /home/user/linux/tools/perf/perf.c:313:11
#8 0x7b120f in handle_internal_command /home/user/linux/tools/perf/perf.c:365:8
#9 0x7b2493 in run_argv /home/user/linux/tools/perf/perf.c:409:2
#10 0x7b0c89 in main /home/user/linux/tools/perf/perf.c:539:3
#11 0x7f5260654b74 (/lib64/libc.so.6+0x27b74)
Indirect leak of 7 byte(s) in 1 object(s) allocated from:
#0 0x4b8207 in strdup (/home/user/linux/tools/perf/perf+0x4b8207)
#1 0x8b4459 in evlist__set_event_name /home/user/linux/tools/perf/util/header.c:2292:16
#2 0x89d862 in process_event_desc /home/user/linux/tools/perf/util/header.c:2313:3
#3 0x8af319 in perf_file_section__process /home/user/linux/tools/perf/util/header.c:3651:9
#4 0x8aa6e9 in perf_header__process_sections /home/user/linux/tools/perf/util/header.c:3427:9
#5 0x8ae3e7 in perf_session__read_header /home/user/linux/tools/perf/util/header.c:3886:2
#6 0x8ec714 in perf_session__open /home/user/linux/tools/perf/util/session.c:109:6
#7 0x8ebe83 in perf_session__new /home/user/linux/tools/perf/util/session.c:213:10
#8 0x60c6de in cmd_script /home/user/linux/tools/perf/builtin-script.c:3856:12
#9 0x7b2930 in run_builtin /home/user/linux/tools/perf/perf.c:313:11
#10 0x7b120f in handle_internal_command /home/user/linux/tools/perf/perf.c:365:8
#11 0x7b2493 in run_argv /home/user/linux/tools/perf/perf.c:409:2
#12 0x7b0c89 in main /home/user/linux/tools/perf/perf.c:539:3
#13 0x7f5260654b74 (/lib64/libc.so.6+0x27b74)
SUMMARY: AddressSanitizer: 3728 byte(s) leaked in 7 allocation(s).
Signed-off-by: Riccardo Mancini <rickyman7@gmail.com>
---
tools/perf/util/session.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index e59242c361ce..c36464d94387 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -301,8 +301,11 @@ void perf_session__delete(struct perf_session *session)
perf_session__release_decomp_events(session);
perf_env__exit(&session->header.env);
machines__exit(&session->machines);
- if (session->data)
+ if (session->data) {
+ if (perf_data__is_read(session->data))
+ evlist__delete(session->evlist);
perf_data__close(session->data);
+ }
free(session);
}
--
2.31.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] perf session: add missing evlist__delete when deleting a session
2021-06-24 23:19 [PATCH v2] perf session: add missing evlist__delete when deleting a session Riccardo Mancini
@ 2021-06-25 5:39 ` Ian Rogers
2021-06-25 11:54 ` Jiri Olsa
0 siblings, 1 reply; 6+ messages in thread
From: Ian Rogers @ 2021-06-25 5:39 UTC (permalink / raw)
To: Riccardo Mancini
Cc: Arnaldo Carvalho de Melo, Namhyung Kim, Peter Zijlstra,
Ingo Molnar, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Adrian Hunter, Kan Liang, Leo Yan, linux-perf-users,
linux-kernel
On Thu, Jun 24, 2021 at 4:20 PM Riccardo Mancini <rickyman7@gmail.com> wrote:
>
> ASan reports a memory leak caused by evlist not being deleted on exit in
> perf-report, perf-script and perf-data.
> The problem is caused by evlist->session not being deleted, which is
> allocated in perf_session__read_header, called in perf_session__new if
> perf_data is in read mode.
> In case of write mode, the session->evlist is filled by the caller.
> This patch solves the problem by calling evlist__delete in
> perf_session__delete if perf_data is in read mode.
Acked-by: Ian Rogers <irogers@google.com>
It is messy that in read mode the session owns the evlist, but
otherwise not. Imo, it'd be nice to make the ownership unconditional.
Thanks,
Ian
> Changes in v2:
> - call evlist__delete from within perf_session__delete
>
> v1: https://lore.kernel.org/lkml/20210621234317.235545-1-rickyman7@gmail.com/
>
> ASan report follows:
>
> $ ./perf script report flamegraph
> =================================================================
> ==227640==ERROR: LeakSanitizer: detected memory leaks
>
> <SNIP unrelated>
>
> Indirect leak of 2704 byte(s) in 1 object(s) allocated from:
> #0 0x4f4137 in calloc (/home/user/linux/tools/perf/perf+0x4f4137)
> #1 0xbe3d56 in zalloc /home/user/linux/tools/lib/perf/../../lib/zalloc.c:8:9
> #2 0x7f999e in evlist__new /home/user/linux/tools/perf/util/evlist.c:77:26
> #3 0x8ad938 in perf_session__read_header /home/user/linux/tools/perf/util/header.c:3797:20
> #4 0x8ec714 in perf_session__open /home/user/linux/tools/perf/util/session.c:109:6
> #5 0x8ebe83 in perf_session__new /home/user/linux/tools/perf/util/session.c:213:10
> #6 0x60c6de in cmd_script /home/user/linux/tools/perf/builtin-script.c:3856:12
> #7 0x7b2930 in run_builtin /home/user/linux/tools/perf/perf.c:313:11
> #8 0x7b120f in handle_internal_command /home/user/linux/tools/perf/perf.c:365:8
> #9 0x7b2493 in run_argv /home/user/linux/tools/perf/perf.c:409:2
> #10 0x7b0c89 in main /home/user/linux/tools/perf/perf.c:539:3
> #11 0x7f5260654b74 (/lib64/libc.so.6+0x27b74)
>
> Indirect leak of 568 byte(s) in 1 object(s) allocated from:
> #0 0x4f4137 in calloc (/home/user/linux/tools/perf/perf+0x4f4137)
> #1 0xbe3d56 in zalloc /home/user/linux/tools/lib/perf/../../lib/zalloc.c:8:9
> #2 0x80ce88 in evsel__new_idx /home/user/linux/tools/perf/util/evsel.c:268:24
> #3 0x8aed93 in evsel__new /home/user/linux/tools/perf/util/evsel.h:210:9
> #4 0x8ae07e in perf_session__read_header /home/user/linux/tools/perf/util/header.c:3853:11
> #5 0x8ec714 in perf_session__open /home/user/linux/tools/perf/util/session.c:109:6
> #6 0x8ebe83 in perf_session__new /home/user/linux/tools/perf/util/session.c:213:10
> #7 0x60c6de in cmd_script /home/user/linux/tools/perf/builtin-script.c:3856:12
> #8 0x7b2930 in run_builtin /home/user/linux/tools/perf/perf.c:313:11
> #9 0x7b120f in handle_internal_command /home/user/linux/tools/perf/perf.c:365:8
> #10 0x7b2493 in run_argv /home/user/linux/tools/perf/perf.c:409:2
> #11 0x7b0c89 in main /home/user/linux/tools/perf/perf.c:539:3
> #12 0x7f5260654b74 (/lib64/libc.so.6+0x27b74)
>
> Indirect leak of 264 byte(s) in 1 object(s) allocated from:
> #0 0x4f4137 in calloc (/home/user/linux/tools/perf/perf+0x4f4137)
> #1 0xbe3d56 in zalloc /home/user/linux/tools/lib/perf/../../lib/zalloc.c:8:9
> #2 0xbe3e70 in xyarray__new /home/user/linux/tools/lib/perf/xyarray.c:10:23
> #3 0xbd7754 in perf_evsel__alloc_id /home/user/linux/tools/lib/perf/evsel.c:361:21
> #4 0x8ae201 in perf_session__read_header /home/user/linux/tools/perf/util/header.c:3871:7
> #5 0x8ec714 in perf_session__open /home/user/linux/tools/perf/util/session.c:109:6
> #6 0x8ebe83 in perf_session__new /home/user/linux/tools/perf/util/session.c:213:10
> #7 0x60c6de in cmd_script /home/user/linux/tools/perf/builtin-script.c:3856:12
> #8 0x7b2930 in run_builtin /home/user/linux/tools/perf/perf.c:313:11
> #9 0x7b120f in handle_internal_command /home/user/linux/tools/perf/perf.c:365:8
> #10 0x7b2493 in run_argv /home/user/linux/tools/perf/perf.c:409:2
> #11 0x7b0c89 in main /home/user/linux/tools/perf/perf.c:539:3
> #12 0x7f5260654b74 (/lib64/libc.so.6+0x27b74)
>
> Indirect leak of 32 byte(s) in 1 object(s) allocated from:
> #0 0x4f4137 in calloc (/home/user/linux/tools/perf/perf+0x4f4137)
> #1 0xbe3d56 in zalloc /home/user/linux/tools/lib/perf/../../lib/zalloc.c:8:9
> #2 0xbd77e0 in perf_evsel__alloc_id /home/user/linux/tools/lib/perf/evsel.c:365:14
> #3 0x8ae201 in perf_session__read_header /home/user/linux/tools/perf/util/header.c:3871:7
> #4 0x8ec714 in perf_session__open /home/user/linux/tools/perf/util/session.c:109:6
> #5 0x8ebe83 in perf_session__new /home/user/linux/tools/perf/util/session.c:213:10
> #6 0x60c6de in cmd_script /home/user/linux/tools/perf/builtin-script.c:3856:12
> #7 0x7b2930 in run_builtin /home/user/linux/tools/perf/perf.c:313:11
> #8 0x7b120f in handle_internal_command /home/user/linux/tools/perf/perf.c:365:8
> #9 0x7b2493 in run_argv /home/user/linux/tools/perf/perf.c:409:2
> #10 0x7b0c89 in main /home/user/linux/tools/perf/perf.c:539:3
> #11 0x7f5260654b74 (/lib64/libc.so.6+0x27b74)
>
> Indirect leak of 7 byte(s) in 1 object(s) allocated from:
> #0 0x4b8207 in strdup (/home/user/linux/tools/perf/perf+0x4b8207)
> #1 0x8b4459 in evlist__set_event_name /home/user/linux/tools/perf/util/header.c:2292:16
> #2 0x89d862 in process_event_desc /home/user/linux/tools/perf/util/header.c:2313:3
> #3 0x8af319 in perf_file_section__process /home/user/linux/tools/perf/util/header.c:3651:9
> #4 0x8aa6e9 in perf_header__process_sections /home/user/linux/tools/perf/util/header.c:3427:9
> #5 0x8ae3e7 in perf_session__read_header /home/user/linux/tools/perf/util/header.c:3886:2
> #6 0x8ec714 in perf_session__open /home/user/linux/tools/perf/util/session.c:109:6
> #7 0x8ebe83 in perf_session__new /home/user/linux/tools/perf/util/session.c:213:10
> #8 0x60c6de in cmd_script /home/user/linux/tools/perf/builtin-script.c:3856:12
> #9 0x7b2930 in run_builtin /home/user/linux/tools/perf/perf.c:313:11
> #10 0x7b120f in handle_internal_command /home/user/linux/tools/perf/perf.c:365:8
> #11 0x7b2493 in run_argv /home/user/linux/tools/perf/perf.c:409:2
> #12 0x7b0c89 in main /home/user/linux/tools/perf/perf.c:539:3
> #13 0x7f5260654b74 (/lib64/libc.so.6+0x27b74)
>
> SUMMARY: AddressSanitizer: 3728 byte(s) leaked in 7 allocation(s).
>
> Signed-off-by: Riccardo Mancini <rickyman7@gmail.com>
> ---
> tools/perf/util/session.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index e59242c361ce..c36464d94387 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -301,8 +301,11 @@ void perf_session__delete(struct perf_session *session)
> perf_session__release_decomp_events(session);
> perf_env__exit(&session->header.env);
> machines__exit(&session->machines);
> - if (session->data)
> + if (session->data) {
> + if (perf_data__is_read(session->data))
> + evlist__delete(session->evlist);
> perf_data__close(session->data);
> + }
> free(session);
> }
>
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] perf session: add missing evlist__delete when deleting a session
2021-06-25 5:39 ` Ian Rogers
@ 2021-06-25 11:54 ` Jiri Olsa
2021-06-25 15:45 ` Riccardo Mancini
2021-07-01 18:04 ` Arnaldo Carvalho de Melo
0 siblings, 2 replies; 6+ messages in thread
From: Jiri Olsa @ 2021-06-25 11:54 UTC (permalink / raw)
To: Ian Rogers
Cc: Riccardo Mancini, Arnaldo Carvalho de Melo, Namhyung Kim,
Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
Adrian Hunter, Kan Liang, Leo Yan, linux-perf-users,
linux-kernel
On Thu, Jun 24, 2021 at 10:39:34PM -0700, Ian Rogers wrote:
> On Thu, Jun 24, 2021 at 4:20 PM Riccardo Mancini <rickyman7@gmail.com> wrote:
> >
> > ASan reports a memory leak caused by evlist not being deleted on exit in
> > perf-report, perf-script and perf-data.
> > The problem is caused by evlist->session not being deleted, which is
> > allocated in perf_session__read_header, called in perf_session__new if
> > perf_data is in read mode.
> > In case of write mode, the session->evlist is filled by the caller.
> > This patch solves the problem by calling evlist__delete in
> > perf_session__delete if perf_data is in read mode.
ugh, I'm surprised we did not free that.. and can't find
in git log we ever did ;-) I briefly check commands using
sessions and looks like it's correct
Acked-by: Jiri Olsa <jolsa@redhat.com>
>
> Acked-by: Ian Rogers <irogers@google.com>
>
> It is messy that in read mode the session owns the evlist, but
> otherwise not. Imo, it'd be nice to make the ownership unconditional.
yep, would be nice
thanks,
jirka
>
> Thanks,
> Ian
>
> > Changes in v2:
> > - call evlist__delete from within perf_session__delete
> >
> > v1: https://lore.kernel.org/lkml/20210621234317.235545-1-rickyman7@gmail.com/
> >
> > ASan report follows:
> >
> > $ ./perf script report flamegraph
> > =================================================================
> > ==227640==ERROR: LeakSanitizer: detected memory leaks
> >
> > <SNIP unrelated>
> >
> > Indirect leak of 2704 byte(s) in 1 object(s) allocated from:
> > #0 0x4f4137 in calloc (/home/user/linux/tools/perf/perf+0x4f4137)
> > #1 0xbe3d56 in zalloc /home/user/linux/tools/lib/perf/../../lib/zalloc.c:8:9
> > #2 0x7f999e in evlist__new /home/user/linux/tools/perf/util/evlist.c:77:26
> > #3 0x8ad938 in perf_session__read_header /home/user/linux/tools/perf/util/header.c:3797:20
> > #4 0x8ec714 in perf_session__open /home/user/linux/tools/perf/util/session.c:109:6
> > #5 0x8ebe83 in perf_session__new /home/user/linux/tools/perf/util/session.c:213:10
> > #6 0x60c6de in cmd_script /home/user/linux/tools/perf/builtin-script.c:3856:12
> > #7 0x7b2930 in run_builtin /home/user/linux/tools/perf/perf.c:313:11
> > #8 0x7b120f in handle_internal_command /home/user/linux/tools/perf/perf.c:365:8
> > #9 0x7b2493 in run_argv /home/user/linux/tools/perf/perf.c:409:2
> > #10 0x7b0c89 in main /home/user/linux/tools/perf/perf.c:539:3
> > #11 0x7f5260654b74 (/lib64/libc.so.6+0x27b74)
> >
> > Indirect leak of 568 byte(s) in 1 object(s) allocated from:
> > #0 0x4f4137 in calloc (/home/user/linux/tools/perf/perf+0x4f4137)
> > #1 0xbe3d56 in zalloc /home/user/linux/tools/lib/perf/../../lib/zalloc.c:8:9
> > #2 0x80ce88 in evsel__new_idx /home/user/linux/tools/perf/util/evsel.c:268:24
> > #3 0x8aed93 in evsel__new /home/user/linux/tools/perf/util/evsel.h:210:9
> > #4 0x8ae07e in perf_session__read_header /home/user/linux/tools/perf/util/header.c:3853:11
> > #5 0x8ec714 in perf_session__open /home/user/linux/tools/perf/util/session.c:109:6
> > #6 0x8ebe83 in perf_session__new /home/user/linux/tools/perf/util/session.c:213:10
> > #7 0x60c6de in cmd_script /home/user/linux/tools/perf/builtin-script.c:3856:12
> > #8 0x7b2930 in run_builtin /home/user/linux/tools/perf/perf.c:313:11
> > #9 0x7b120f in handle_internal_command /home/user/linux/tools/perf/perf.c:365:8
> > #10 0x7b2493 in run_argv /home/user/linux/tools/perf/perf.c:409:2
> > #11 0x7b0c89 in main /home/user/linux/tools/perf/perf.c:539:3
> > #12 0x7f5260654b74 (/lib64/libc.so.6+0x27b74)
> >
> > Indirect leak of 264 byte(s) in 1 object(s) allocated from:
> > #0 0x4f4137 in calloc (/home/user/linux/tools/perf/perf+0x4f4137)
> > #1 0xbe3d56 in zalloc /home/user/linux/tools/lib/perf/../../lib/zalloc.c:8:9
> > #2 0xbe3e70 in xyarray__new /home/user/linux/tools/lib/perf/xyarray.c:10:23
> > #3 0xbd7754 in perf_evsel__alloc_id /home/user/linux/tools/lib/perf/evsel.c:361:21
> > #4 0x8ae201 in perf_session__read_header /home/user/linux/tools/perf/util/header.c:3871:7
> > #5 0x8ec714 in perf_session__open /home/user/linux/tools/perf/util/session.c:109:6
> > #6 0x8ebe83 in perf_session__new /home/user/linux/tools/perf/util/session.c:213:10
> > #7 0x60c6de in cmd_script /home/user/linux/tools/perf/builtin-script.c:3856:12
> > #8 0x7b2930 in run_builtin /home/user/linux/tools/perf/perf.c:313:11
> > #9 0x7b120f in handle_internal_command /home/user/linux/tools/perf/perf.c:365:8
> > #10 0x7b2493 in run_argv /home/user/linux/tools/perf/perf.c:409:2
> > #11 0x7b0c89 in main /home/user/linux/tools/perf/perf.c:539:3
> > #12 0x7f5260654b74 (/lib64/libc.so.6+0x27b74)
> >
> > Indirect leak of 32 byte(s) in 1 object(s) allocated from:
> > #0 0x4f4137 in calloc (/home/user/linux/tools/perf/perf+0x4f4137)
> > #1 0xbe3d56 in zalloc /home/user/linux/tools/lib/perf/../../lib/zalloc.c:8:9
> > #2 0xbd77e0 in perf_evsel__alloc_id /home/user/linux/tools/lib/perf/evsel.c:365:14
> > #3 0x8ae201 in perf_session__read_header /home/user/linux/tools/perf/util/header.c:3871:7
> > #4 0x8ec714 in perf_session__open /home/user/linux/tools/perf/util/session.c:109:6
> > #5 0x8ebe83 in perf_session__new /home/user/linux/tools/perf/util/session.c:213:10
> > #6 0x60c6de in cmd_script /home/user/linux/tools/perf/builtin-script.c:3856:12
> > #7 0x7b2930 in run_builtin /home/user/linux/tools/perf/perf.c:313:11
> > #8 0x7b120f in handle_internal_command /home/user/linux/tools/perf/perf.c:365:8
> > #9 0x7b2493 in run_argv /home/user/linux/tools/perf/perf.c:409:2
> > #10 0x7b0c89 in main /home/user/linux/tools/perf/perf.c:539:3
> > #11 0x7f5260654b74 (/lib64/libc.so.6+0x27b74)
> >
> > Indirect leak of 7 byte(s) in 1 object(s) allocated from:
> > #0 0x4b8207 in strdup (/home/user/linux/tools/perf/perf+0x4b8207)
> > #1 0x8b4459 in evlist__set_event_name /home/user/linux/tools/perf/util/header.c:2292:16
> > #2 0x89d862 in process_event_desc /home/user/linux/tools/perf/util/header.c:2313:3
> > #3 0x8af319 in perf_file_section__process /home/user/linux/tools/perf/util/header.c:3651:9
> > #4 0x8aa6e9 in perf_header__process_sections /home/user/linux/tools/perf/util/header.c:3427:9
> > #5 0x8ae3e7 in perf_session__read_header /home/user/linux/tools/perf/util/header.c:3886:2
> > #6 0x8ec714 in perf_session__open /home/user/linux/tools/perf/util/session.c:109:6
> > #7 0x8ebe83 in perf_session__new /home/user/linux/tools/perf/util/session.c:213:10
> > #8 0x60c6de in cmd_script /home/user/linux/tools/perf/builtin-script.c:3856:12
> > #9 0x7b2930 in run_builtin /home/user/linux/tools/perf/perf.c:313:11
> > #10 0x7b120f in handle_internal_command /home/user/linux/tools/perf/perf.c:365:8
> > #11 0x7b2493 in run_argv /home/user/linux/tools/perf/perf.c:409:2
> > #12 0x7b0c89 in main /home/user/linux/tools/perf/perf.c:539:3
> > #13 0x7f5260654b74 (/lib64/libc.so.6+0x27b74)
> >
> > SUMMARY: AddressSanitizer: 3728 byte(s) leaked in 7 allocation(s).
> >
> > Signed-off-by: Riccardo Mancini <rickyman7@gmail.com>
> > ---
> > tools/perf/util/session.c | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> > index e59242c361ce..c36464d94387 100644
> > --- a/tools/perf/util/session.c
> > +++ b/tools/perf/util/session.c
> > @@ -301,8 +301,11 @@ void perf_session__delete(struct perf_session *session)
> > perf_session__release_decomp_events(session);
> > perf_env__exit(&session->header.env);
> > machines__exit(&session->machines);
> > - if (session->data)
> > + if (session->data) {
> > + if (perf_data__is_read(session->data))
> > + evlist__delete(session->evlist);
> > perf_data__close(session->data);
> > + }
> > free(session);
> > }
> >
> > --
> > 2.31.1
> >
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] perf session: add missing evlist__delete when deleting a session
2021-06-25 11:54 ` Jiri Olsa
@ 2021-06-25 15:45 ` Riccardo Mancini
2021-06-27 17:29 ` Jiri Olsa
2021-07-01 18:04 ` Arnaldo Carvalho de Melo
1 sibling, 1 reply; 6+ messages in thread
From: Riccardo Mancini @ 2021-06-25 15:45 UTC (permalink / raw)
To: Jiri Olsa, Ian Rogers
Cc: Arnaldo Carvalho de Melo, Namhyung Kim, Peter Zijlstra,
Ingo Molnar, Mark Rutland, Alexander Shishkin, Adrian Hunter,
Kan Liang, Leo Yan, linux-perf-users, linux-kernel
Hi,
thank you both for your comments.
On Fri, 2021-06-25 at 13:54 +0200, Jiri Olsa wrote:
> On Thu, Jun 24, 2021 at 10:39:34PM -0700, Ian Rogers wrote:
> > On Thu, Jun 24, 2021 at 4:20 PM Riccardo Mancini <rickyman7@gmail.com>
> > wrote:
> > >
> > > ASan reports a memory leak caused by evlist not being deleted on exit in
> > > perf-report, perf-script and perf-data.
> > > The problem is caused by evlist->session not being deleted, which is
> > > allocated in perf_session__read_header, called in perf_session__new if
> > > perf_data is in read mode.
> > > In case of write mode, the session->evlist is filled by the caller.
> > > This patch solves the problem by calling evlist__delete in
> > > perf_session__delete if perf_data is in read mode.
>
> ugh, I'm surprised we did not free that.. and can't find
> in git log we ever did ;-) I briefly check commands using
> sessions and looks like it's correct
>
> Acked-by: Jiri Olsa <jolsa@redhat.com>
>
> >
> > Acked-by: Ian Rogers <irogers@google.com>
> >
> > It is messy that in read mode the session owns the evlist, but
> > otherwise not. Imo, it'd be nice to make the ownership unconditional.
>
> yep, would be nice
I think the root problem is that perf_session__new has different behaviours
depending on perf_data and perf_tool and that it probably does too many things
for a __new function.
If we split it into multiple functions and then, say, we create two helpers
perf_session__init_read and perf_session__init_write, with the corresponding
perf_session__fini_read and perf_session__fini_write, then the conditional
ownership won't be a big problem due to having these two cases clearly
separated.
What do you think?
Thanks,
Riccardo
>
> thanks,
> jirka
>
> >
> > Thanks,
> > Ian
> >
> > > Changes in v2:
> > > - call evlist__delete from within perf_session__delete
> > >
> > > v1:
> > > https://lore.kernel.org/lkml/20210621234317.235545-1-rickyman7@gmail.com/
> > >
> > > ASan report follows:
> > >
> > > $ ./perf script report flamegraph
> > > =================================================================
> > > ==227640==ERROR: LeakSanitizer: detected memory leaks
> > >
> > > <SNIP unrelated>
> > >
> > > Indirect leak of 2704 byte(s) in 1 object(s) allocated from:
> > > #0 0x4f4137 in calloc (/home/user/linux/tools/perf/perf+0x4f4137)
> > > #1 0xbe3d56 in zalloc
> > > /home/user/linux/tools/lib/perf/../../lib/zalloc.c:8:9
> > > #2 0x7f999e in evlist__new
> > > /home/user/linux/tools/perf/util/evlist.c:77:26
> > > #3 0x8ad938 in perf_session__read_header
> > > /home/user/linux/tools/perf/util/header.c:3797:20
> > > #4 0x8ec714 in perf_session__open
> > > /home/user/linux/tools/perf/util/session.c:109:6
> > > #5 0x8ebe83 in perf_session__new
> > > /home/user/linux/tools/perf/util/session.c:213:10
> > > #6 0x60c6de in cmd_script /home/user/linux/tools/perf/builtin-
> > > script.c:3856:12
> > > #7 0x7b2930 in run_builtin /home/user/linux/tools/perf/perf.c:313:11
> > > #8 0x7b120f in handle_internal_command
> > > /home/user/linux/tools/perf/perf.c:365:8
> > > #9 0x7b2493 in run_argv /home/user/linux/tools/perf/perf.c:409:2
> > > #10 0x7b0c89 in main /home/user/linux/tools/perf/perf.c:539:3
> > > #11 0x7f5260654b74 (/lib64/libc.so.6+0x27b74)
> > >
> > > Indirect leak of 568 byte(s) in 1 object(s) allocated from:
> > > #0 0x4f4137 in calloc (/home/user/linux/tools/perf/perf+0x4f4137)
> > > #1 0xbe3d56 in zalloc
> > > /home/user/linux/tools/lib/perf/../../lib/zalloc.c:8:9
> > > #2 0x80ce88 in evsel__new_idx
> > > /home/user/linux/tools/perf/util/evsel.c:268:24
> > > #3 0x8aed93 in evsel__new /home/user/linux/tools/perf/util/evsel.h:210:9
> > > #4 0x8ae07e in perf_session__read_header
> > > /home/user/linux/tools/perf/util/header.c:3853:11
> > > #5 0x8ec714 in perf_session__open
> > > /home/user/linux/tools/perf/util/session.c:109:6
> > > #6 0x8ebe83 in perf_session__new
> > > /home/user/linux/tools/perf/util/session.c:213:10
> > > #7 0x60c6de in cmd_script /home/user/linux/tools/perf/builtin-
> > > script.c:3856:12
> > > #8 0x7b2930 in run_builtin /home/user/linux/tools/perf/perf.c:313:11
> > > #9 0x7b120f in handle_internal_command
> > > /home/user/linux/tools/perf/perf.c:365:8
> > > #10 0x7b2493 in run_argv /home/user/linux/tools/perf/perf.c:409:2
> > > #11 0x7b0c89 in main /home/user/linux/tools/perf/perf.c:539:3
> > > #12 0x7f5260654b74 (/lib64/libc.so.6+0x27b74)
> > >
> > > Indirect leak of 264 byte(s) in 1 object(s) allocated from:
> > > #0 0x4f4137 in calloc (/home/user/linux/tools/perf/perf+0x4f4137)
> > > #1 0xbe3d56 in zalloc
> > > /home/user/linux/tools/lib/perf/../../lib/zalloc.c:8:9
> > > #2 0xbe3e70 in xyarray__new
> > > /home/user/linux/tools/lib/perf/xyarray.c:10:23
> > > #3 0xbd7754 in perf_evsel__alloc_id
> > > /home/user/linux/tools/lib/perf/evsel.c:361:21
> > > #4 0x8ae201 in perf_session__read_header
> > > /home/user/linux/tools/perf/util/header.c:3871:7
> > > #5 0x8ec714 in perf_session__open
> > > /home/user/linux/tools/perf/util/session.c:109:6
> > > #6 0x8ebe83 in perf_session__new
> > > /home/user/linux/tools/perf/util/session.c:213:10
> > > #7 0x60c6de in cmd_script /home/user/linux/tools/perf/builtin-
> > > script.c:3856:12
> > > #8 0x7b2930 in run_builtin /home/user/linux/tools/perf/perf.c:313:11
> > > #9 0x7b120f in handle_internal_command
> > > /home/user/linux/tools/perf/perf.c:365:8
> > > #10 0x7b2493 in run_argv /home/user/linux/tools/perf/perf.c:409:2
> > > #11 0x7b0c89 in main /home/user/linux/tools/perf/perf.c:539:3
> > > #12 0x7f5260654b74 (/lib64/libc.so.6+0x27b74)
> > >
> > > Indirect leak of 32 byte(s) in 1 object(s) allocated from:
> > > #0 0x4f4137 in calloc (/home/user/linux/tools/perf/perf+0x4f4137)
> > > #1 0xbe3d56 in zalloc
> > > /home/user/linux/tools/lib/perf/../../lib/zalloc.c:8:9
> > > #2 0xbd77e0 in perf_evsel__alloc_id
> > > /home/user/linux/tools/lib/perf/evsel.c:365:14
> > > #3 0x8ae201 in perf_session__read_header
> > > /home/user/linux/tools/perf/util/header.c:3871:7
> > > #4 0x8ec714 in perf_session__open
> > > /home/user/linux/tools/perf/util/session.c:109:6
> > > #5 0x8ebe83 in perf_session__new
> > > /home/user/linux/tools/perf/util/session.c:213:10
> > > #6 0x60c6de in cmd_script /home/user/linux/tools/perf/builtin-
> > > script.c:3856:12
> > > #7 0x7b2930 in run_builtin /home/user/linux/tools/perf/perf.c:313:11
> > > #8 0x7b120f in handle_internal_command
> > > /home/user/linux/tools/perf/perf.c:365:8
> > > #9 0x7b2493 in run_argv /home/user/linux/tools/perf/perf.c:409:2
> > > #10 0x7b0c89 in main /home/user/linux/tools/perf/perf.c:539:3
> > > #11 0x7f5260654b74 (/lib64/libc.so.6+0x27b74)
> > >
> > > Indirect leak of 7 byte(s) in 1 object(s) allocated from:
> > > #0 0x4b8207 in strdup (/home/user/linux/tools/perf/perf+0x4b8207)
> > > #1 0x8b4459 in evlist__set_event_name
> > > /home/user/linux/tools/perf/util/header.c:2292:16
> > > #2 0x89d862 in process_event_desc
> > > /home/user/linux/tools/perf/util/header.c:2313:3
> > > #3 0x8af319 in perf_file_section__process
> > > /home/user/linux/tools/perf/util/header.c:3651:9
> > > #4 0x8aa6e9 in perf_header__process_sections
> > > /home/user/linux/tools/perf/util/header.c:3427:9
> > > #5 0x8ae3e7 in perf_session__read_header
> > > /home/user/linux/tools/perf/util/header.c:3886:2
> > > #6 0x8ec714 in perf_session__open
> > > /home/user/linux/tools/perf/util/session.c:109:6
> > > #7 0x8ebe83 in perf_session__new
> > > /home/user/linux/tools/perf/util/session.c:213:10
> > > #8 0x60c6de in cmd_script /home/user/linux/tools/perf/builtin-
> > > script.c:3856:12
> > > #9 0x7b2930 in run_builtin /home/user/linux/tools/perf/perf.c:313:11
> > > #10 0x7b120f in handle_internal_command
> > > /home/user/linux/tools/perf/perf.c:365:8
> > > #11 0x7b2493 in run_argv /home/user/linux/tools/perf/perf.c:409:2
> > > #12 0x7b0c89 in main /home/user/linux/tools/perf/perf.c:539:3
> > > #13 0x7f5260654b74 (/lib64/libc.so.6+0x27b74)
> > >
> > > SUMMARY: AddressSanitizer: 3728 byte(s) leaked in 7 allocation(s).
> > >
> > > Signed-off-by: Riccardo Mancini <rickyman7@gmail.com>
> > > ---
> > > tools/perf/util/session.c | 5 ++++-
> > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> > > index e59242c361ce..c36464d94387 100644
> > > --- a/tools/perf/util/session.c
> > > +++ b/tools/perf/util/session.c
> > > @@ -301,8 +301,11 @@ void perf_session__delete(struct perf_session *session)
> > > perf_session__release_decomp_events(session);
> > > perf_env__exit(&session->header.env);
> > > machines__exit(&session->machines);
> > > - if (session->data)
> > > + if (session->data) {
> > > + if (perf_data__is_read(session->data))
> > > + evlist__delete(session->evlist);
> > > perf_data__close(session->data);
> > > + }
> > > free(session);
> > > }
> > >
> > > --
> > > 2.31.1
> > >
> >
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] perf session: add missing evlist__delete when deleting a session
2021-06-25 15:45 ` Riccardo Mancini
@ 2021-06-27 17:29 ` Jiri Olsa
0 siblings, 0 replies; 6+ messages in thread
From: Jiri Olsa @ 2021-06-27 17:29 UTC (permalink / raw)
To: Riccardo Mancini
Cc: Ian Rogers, Arnaldo Carvalho de Melo, Namhyung Kim,
Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
Adrian Hunter, Kan Liang, Leo Yan, linux-perf-users,
linux-kernel
On Fri, Jun 25, 2021 at 05:45:59PM +0200, Riccardo Mancini wrote:
> Hi,
>
> thank you both for your comments.
>
> On Fri, 2021-06-25 at 13:54 +0200, Jiri Olsa wrote:
> > On Thu, Jun 24, 2021 at 10:39:34PM -0700, Ian Rogers wrote:
> > > On Thu, Jun 24, 2021 at 4:20 PM Riccardo Mancini <rickyman7@gmail.com>
> > > wrote:
> > > >
> > > > ASan reports a memory leak caused by evlist not being deleted on exit in
> > > > perf-report, perf-script and perf-data.
> > > > The problem is caused by evlist->session not being deleted, which is
> > > > allocated in perf_session__read_header, called in perf_session__new if
> > > > perf_data is in read mode.
> > > > In case of write mode, the session->evlist is filled by the caller.
> > > > This patch solves the problem by calling evlist__delete in
> > > > perf_session__delete if perf_data is in read mode.
> >
> > ugh, I'm surprised we did not free that.. and can't find
> > in git log we ever did ;-) I briefly check commands using
> > sessions and looks like it's correct
> >
> > Acked-by: Jiri Olsa <jolsa@redhat.com>
> >
> > >
> > > Acked-by: Ian Rogers <irogers@google.com>
> > >
> > > It is messy that in read mode the session owns the evlist, but
> > > otherwise not. Imo, it'd be nice to make the ownership unconditional.
> >
> > yep, would be nice
>
> I think the root problem is that perf_session__new has different behaviours
> depending on perf_data and perf_tool and that it probably does too many things
> for a __new function.
> If we split it into multiple functions and then, say, we create two helpers
> perf_session__init_read and perf_session__init_write, with the corresponding
> perf_session__fini_read and perf_session__fini_write, then the conditional
> ownership won't be a big problem due to having these two cases clearly
> separated.
> What do you think?
yes, interesting idea, let's see how the code looks like ;-)
thanks,
jirka
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] perf session: add missing evlist__delete when deleting a session
2021-06-25 11:54 ` Jiri Olsa
2021-06-25 15:45 ` Riccardo Mancini
@ 2021-07-01 18:04 ` Arnaldo Carvalho de Melo
1 sibling, 0 replies; 6+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-07-01 18:04 UTC (permalink / raw)
To: Jiri Olsa
Cc: Ian Rogers, Riccardo Mancini, Namhyung Kim, Peter Zijlstra,
Ingo Molnar, Mark Rutland, Alexander Shishkin, Adrian Hunter,
Kan Liang, Leo Yan, linux-perf-users, linux-kernel
Em Fri, Jun 25, 2021 at 01:54:03PM +0200, Jiri Olsa escreveu:
> On Thu, Jun 24, 2021 at 10:39:34PM -0700, Ian Rogers wrote:
> > On Thu, Jun 24, 2021 at 4:20 PM Riccardo Mancini <rickyman7@gmail.com> wrote:
> > >
> > > ASan reports a memory leak caused by evlist not being deleted on exit in
> > > perf-report, perf-script and perf-data.
> > > The problem is caused by evlist->session not being deleted, which is
> > > allocated in perf_session__read_header, called in perf_session__new if
> > > perf_data is in read mode.
> > > In case of write mode, the session->evlist is filled by the caller.
> > > This patch solves the problem by calling evlist__delete in
> > > perf_session__delete if perf_data is in read mode.
>
> ugh, I'm surprised we did not free that.. and can't find
> in git log we ever did ;-) I briefly check commands using
> sessions and looks like it's correct
>
> Acked-by: Jiri Olsa <jolsa@redhat.com>
>
> >
> > Acked-by: Ian Rogers <irogers@google.com>
> >
> > It is messy that in read mode the session owns the evlist, but
> > otherwise not. Imo, it'd be nice to make the ownership unconditional.
>
> yep, would be nice
Thanks, applied.
Riccardo, next time please consider adding a Fixes: tag so that the
stable@kernel.org guys can pick this for stable releases.
- Arnaldo
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-07-01 18:05 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-24 23:19 [PATCH v2] perf session: add missing evlist__delete when deleting a session Riccardo Mancini
2021-06-25 5:39 ` Ian Rogers
2021-06-25 11:54 ` Jiri Olsa
2021-06-25 15:45 ` Riccardo Mancini
2021-06-27 17:29 ` Jiri Olsa
2021-07-01 18:04 ` Arnaldo Carvalho de Melo
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.