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 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.