From: Riccardo Mancini <rickyman7@gmail.com>
To: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
Cc: Ian Rogers <irogers@google.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>,
Adrian Hunter <adrian.hunter@intel.com>,
Andi Kleen <ak@linux.intel.com>,
Tommi Rantala <tommi.t.rantala@nokia.com>,
linux-perf-users <linux-perf-users@vger.kernel.org>,
LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] perf ksymbol: fix memory leak: decrease refcount of map and dso
Date: Fri, 18 Jun 2021 12:01:28 +0200 [thread overview]
Message-ID: <da5b052d2c94db91c0bf8cb794c5cad299f19e57.camel@gmail.com> (raw)
In-Reply-To: <YLpxDf6+YOxYI5z3@kernel.org>
Hi,
On Fri, 2021-06-04 at 15:29 -0300, Arnaldo Carvalho de Melo wrote:
<SNIP>
> > > But looking at this code now I realize that maps__find() should grab a
> > > refcount for the map it returns, because in this
> > > machine__process_ksymbol_register() function we use reference that 'map'
> > > after the if block, i.e. we use it if it came from maps__find() or if we
> > > created it machine__process_ksymbol_register, so there is a possible
> > > race where other thread removes it from the list and map__put()s it
> > > ending up in map__delete() while we still use it in
> > > machine__process_ksymbol_register(), right?
> >
> > Agree. It should be placed before up_read to avoid races, right?
>
> Yes, we have to grab a refcount while we are sure its not going away,
> then return that as the lookup result, whoever receives that refcounted
> entry should use it and then drop the refcount.
>
> > Then we would need to see where it's called and add the appropriate
> > map__put.
>
> yes
This function has quite a number of callers (direct and indirect) so the the
patch is becoming huge.
One of these callers is thread__find_map, which returns an addr_location
(actually it's an output argument). This addr_location holds references to map,
maps and thread without getting any refcnt (actually in one function it gets it
on the thread and a comment tells to put it once done). If I'm not wrong, this
addr_location is never malloced (always a local variable) and, is should be
present in parts of the code where there should be a refcnt on the thread.
Therefore, maybe it does not get the refcnts since it assumes that thread (upon
which depends maps and as a consequence map) is always refcnted in its context.
However, I think that it should get all refcnts anyways for clarity and to
prevent possible misuses (if I understood correctly, Ian is of the same
opinion).
My solution would be to add the refcnt grabbing for map, maps and thread in
thread__find_map, releasing them in addr_location__put, and then making sure all
callers call it when no longer in use.
Following the same reasoning, I added refcnt grabbing also to mem_info,
branch_info (map was already refcnted, I added it also to maps for coherency),
map_symbol (as in branch_info, I added it to maps), and in other places in which
I saw a pointer was passed without refcounting.
Most changes are quite trivial, however, the changelog is huge:
48 files changed, 472 insertions(+), 157 deletions(-)
Most of them are just returns converted to goto for calling the __put functions.
Doing so, I managed to remove memory leaks caused by refcounting also in perf-
report (I wanted to try also perf top but I encountered another memory-related
issue). However, the changelog is huge and testing all of it is challenging
(especially since I can test missing puts only with ASan's LeakSanitizer and its
reports are usually full of leaks, which I am trying to fix along the way, I
will send some patches in the following days). How would you go about it? Do you
have any suggestions?
>
> > In addition, having a look at other possible concurrency issues in map.c:
>
> Its good to have new eyes looking at this, exactly at a time we're
> discussing further parallelizing perf :-)
>
> > - maps__for_each_entry should always be called with either read or write
> > lock,
> > am I right? It looks like this is not done in certain parts of the code. If
> > such
>
> Right.
>
> > lock is taken, then grabbing the refcount on the looping variable is not
> > needed
> > unless we need to return it, right?
>
> Right, returning an entry needs to take a refcount.
>
> > - maps__first and map__next do not grab a refcount and neither a lock. If
> > they're used through a lock-protected loop, it's not a problem, but maybe
> > it's
>
> yes
>
> > worth making explicit that they are not to be used directly (through either
> > a
> > comment or adding some underscores in their names).
>
> yes, __ in front means, in kernel style, that it does less than the non
> __ prefixed, same name, function.
>
> > - maps__empty: should probably take a reader lock.
>
> Indeed.
>
> > - maps__find_symbol: the returned symbol is not protected (the caller does
> > not
> > receive a refcount to neither map or dso, so if dso is deleted, his
> > reference to
> > the symbol gets invalidated). Depending on how it's being used it might not
> > be a
> > problem, but in the general scenario I think it's not thread-safe.
>
> Yes, that function is also problematic.
This issue is easier to solve than expected since the map is returned as **mapp,
so it's just a matter of making sure that the caller always passes it and then
puts the refcnt.
Thanks,
Riccardo
>
> Thanks for looking into this, please consider sending patches for these
> issues,
>
> - Arnaldo
next prev parent reply other threads:[~2021-06-18 10:01 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-02 23:10 [PATCH] perf ksymbol: fix memory leak: decrease refcount of map and dso Riccardo Mancini
2021-06-04 4:26 ` Ian Rogers
2021-06-04 13:22 ` Arnaldo Carvalho de Melo
2021-06-04 15:16 ` Riccardo Mancini
2021-06-04 18:29 ` Arnaldo Carvalho de Melo
2021-06-12 17:37 ` [PATCH v2] " Riccardo Mancini
2021-06-16 18:12 ` Arnaldo Carvalho de Melo
2021-06-18 10:01 ` Riccardo Mancini [this message]
2021-06-18 13:25 ` [PATCH] " 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=da5b052d2c94db91c0bf8cb794c5cad299f19e57.camel@gmail.com \
--to=rickyman7@gmail.com \
--cc=adrian.hunter@intel.com \
--cc=ak@linux.intel.com \
--cc=alexander.shishkin@linux.intel.com \
--cc=arnaldo.melo@gmail.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=tommi.t.rantala@nokia.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).