From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Ian Rogers <irogers@google.com>
Cc: Riccardo Mancini <rickyman7@gmail.com>,
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>,
Jiri Olsa <jolsa@redhat.com>,
linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] perf script: delete evlist when deleting session
Date: Tue, 22 Jun 2021 14:42:04 -0300 [thread overview]
Message-ID: <YNIg7OkPi7YgsBZ3@kernel.org> (raw)
In-Reply-To: <CAP-5=fXGJXmpddPVEjmCCv9oC7bmum3p+1m1m6rem8Pdy+XaXA@mail.gmail.com>
Em Tue, Jun 22, 2021 at 09:33:23AM -0700, Ian Rogers escreveu:
> On Tue, Jun 22, 2021 at 12:44 AM Riccardo Mancini <rickyman7@gmail.com> wrote:
> >
> > Hi,
> >
> > thanks for your comments.
> >
> > On Mon, 2021-06-21 at 22:14 -0700, Ian Rogers wrote:
> > > On Mon, Jun 21, 2021 at 4:44 PM Riccardo Mancini <rickyman7@gmail.com> wrote:
> > > >
> > > > ASan reports a memory leak related to session->evlist never being deleted.
> > > > The evlist member is not deleted in perf_session__delete, so it should be
> > > > deleted separately.
> > > > This patch adds the missing deletion in perf-script.
> > > >
> > > > Signed-off-by: Riccardo Mancini <rickyman7@gmail.com>
> > > > ---
> > > > tools/perf/builtin-script.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> > > > index 1280cbfad4db..635a1d9cfc88 100644
> > > > --- a/tools/perf/builtin-script.c
> > > > +++ b/tools/perf/builtin-script.c
> > > > @@ -3991,7 +3991,7 @@ int cmd_script(int argc, const char **argv)
> > > > zfree(&script.ptime_range);
> > > > }
> > > >
> > > > - evlist__free_stats(session->evlist);
> > >
> > > Should this be removed?
> >
> > Probably not. I originally thought this was already taken care of by
> > evlist__delete, but it's not.
> > Oddly, this issue is not causing a memory leak in my simple test.
> >
> > >
> > > > + evlist__delete(session->evlist);
This looks like a bug, if it is a 'session' member, its a session method
that should delete it, probably perf_session__delete().
> > > If the perf session "owns" the evlist, would it be cleaner to add this
> > > to perf_session__delete?
> >
> > I thought about that too, but that's not always true.
> > E.g., in perf-record, __cmd_record calls perf_session__delete,then cmd_record
> > calls evlist__delete on rec->evlist, which points to the same location to which
> > session->evlist pointed.
>
> Agreed. I find it hard to understand the ownership properties in the
> perf code. The missing delete is an example of the owner of the evlist
> (the caller) not "knowing" it needed cleaning up. I'd like it if we
> documented things like perf_sessions' evlist to say not owned, user
> must clean up. The makes it unambiguous who has to take
> responsibility. Having things clean up after themselves is of course
> easiest, hence wanting this to be in perf_session__delete.
This specific case, from just reading the description on this message,
looks just like a bug/thinko.
> Fwiw, I've been reading around things like sparse [1, 2] and Clang's
> similar analysis [3] that people have looked to use like sparse [4]. I
> don't see anything that handles memory allocation lifetimes, but
> perhaps something will feed into C's standards by way of C++ [5].
> Perhaps people have ideas to rewrite in checked C or Rust :-)
>
> Some thoughts:
> 1) we can't have C++ as we're trying to follow kernel conventions [6]
> 2) we can't annotate code for things like sparse or thread safety
> analysis, as checking for memory errors is out of scope for them, the
> annotations don't exist, etc.
> 3) we can add comments, document the rules around pointers, perhaps
> even invent empty annotations that may one day help with automated
> checking.
> 4) we can try to clean up the ownership model to make bugs less likely.
>
> I've heard concerns on non-kernel projects about annotation litter and
> comments adding to complexity. I think your patch is good, it follows
> the existing conventions. I wonder if we can learn something from the
> fact the code was wrong to make it less likely we have wrong code in
> the future. I'd be interested to hear what others think.
>
> Thanks,
> Ian
>
> [1] https://lore.kernel.org/lkml/Pine.LNX.4.58.0410302005270.28839@ppc970.osdl.org/
> [2] https://lwn.net/Articles/689907/
> [3] https://clang.llvm.org/docs/ThreadSafetyAnalysis.html
> [4] https://www.openwall.com/lists/kernel-hardening/2019/05/20/3
> [5] https://github.com/isocpp/CppCoreGuidelines/blob/master/docs/Lifetime.pdf
> [6] even concatenating a string is error prone in C :-(
> https://lore.kernel.org/lkml/YMzOpgZPJeC2jGKf@kernel.org/
>
> > Thanks,
> > Riccardo
> >
> > >
> > > Thanks,
> > > Ian
> > >
> > > > perf_session__delete(session);
> > > >
> > > > if (script_started)
> > > > --
> > > > 2.31.1
> > > >
> >
> >
--
- Arnaldo
next prev parent reply other threads:[~2021-06-22 17:42 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-21 23:43 [PATCH 0/2] perf: add missing evlist__delete when deleting Riccardo Mancini
2021-06-21 23:43 ` [PATCH 1/2] perf report: delete evlist when deleting session Riccardo Mancini
2021-06-21 23:43 ` [PATCH 2/2] perf script: " Riccardo Mancini
2021-06-22 5:14 ` Ian Rogers
2021-06-22 7:44 ` Riccardo Mancini
2021-06-22 16:33 ` Ian Rogers
2021-06-22 17:42 ` Arnaldo Carvalho de Melo [this message]
2021-06-22 18:28 ` Ian Rogers
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=YNIg7OkPi7YgsBZ3@kernel.org \
--to=acme@kernel.org \
--cc=alexander.shishkin@linux.intel.com \
--cc=irogers@google.com \
--cc=jolsa@redhat.com \
--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 \
--cc=rickyman7@gmail.com \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).