From: Riccardo Mancini <rickyman7@gmail.com>
To: Jiri Olsa <jolsa@redhat.com>, Ian Rogers <irogers@google.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>,
Namhyung Kim <namhyung@kernel.org>,
Peter Zijlstra <peterz@infradead.org>,
Ingo Molnar <mingo@redhat.com>,
Mark Rutland <mark.rutland@arm.com>,
Alexander Shishkin <alexander.shishkin@linux.intel.com>,
Adrian Hunter <adrian.hunter@intel.com>,
Kan Liang <kan.liang@linux.intel.com>,
Leo Yan <leo.yan@linaro.org>,
linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] perf session: add missing evlist__delete when deleting a session
Date: Fri, 25 Jun 2021 17:45:59 +0200 [thread overview]
Message-ID: <4725ef2249540b6876daea3714a38a6be59223e6.camel@gmail.com> (raw)
In-Reply-To: <YNXD2xN2wrZgdbwO@krava>
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
> > >
> >
>
next prev parent reply other threads:[~2021-06-25 15:46 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2021-06-27 17:29 ` Jiri Olsa
2021-07-01 18:04 ` Arnaldo Carvalho de Melo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4725ef2249540b6876daea3714a38a6be59223e6.camel@gmail.com \
--to=rickyman7@gmail.com \
--cc=acme@kernel.org \
--cc=adrian.hunter@intel.com \
--cc=alexander.shishkin@linux.intel.com \
--cc=irogers@google.com \
--cc=jolsa@redhat.com \
--cc=kan.liang@linux.intel.com \
--cc=leo.yan@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mingo@redhat.com \
--cc=namhyung@kernel.org \
--cc=peterz@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.