Em Thu, Jul 15, 2021 at 04:02:16PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Thu, Jul 15, 2021 at 06:07:06PM +0200, Riccardo Mancini escreveu:
> > ASan reports a memory leak of nsinfo during the execution of the perf
> > test "31: Lookup mmap thread".
> > The leak is caused by a refcounted variable being replaced without
> > dropping the refcount.
> > This patch makes sure that the refcnt of nsinfo is decreased whenever
> > a refcounted variable is replaced with a new value.
> So, there are multiple fixes in just one patch, I'll split it into
> three, no need to resend.
> I'll try and check if finding Fixes: for the three is easy, that way
> [email protected] will figure out which of the supported releases
> need each of them.
The second is at the end of this message, and looking at it I think
there is a problem with where this nsinfo reference is stored, it should
be in the 'struct map', that is per-thread, not in the 'struct dso',
that may be shared by multiple 'struct map's in different cgroups, so we
can't set dso->nsinfo in map__new(), right? Kristen?
- Arnaldo
commit 280f3933661f9ca9563ed04ce82313fdbcbb8f75
Author: Riccardo Mancini <[email protected]>
Date: Thu Jul 15 18:07:06 2021 +0200
perf map: Fix dso->nsinfo refcounting
ASan reports a memory leak of nsinfo during the execution of
# perf test "31: Lookup mmap thread"
The leak is caused by a refcounted variable being replaced without
dropping the refcount.
This patch makes sure that the refcnt of nsinfo is decreased whenever a
refcounted variable is replaced with a new value.
Signed-off-by: Riccardo Mancini <[email protected]>
Fixes: bf2e710b3cb8445c ("perf maps: Lookup maps in both intitial mountns and inner mountns.")
Cc: Ian Rogers <[email protected]>
Cc: Jiri Olsa <[email protected]>
Cc: Krister Johansen <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Link: http://lore.kernel.org/lkml/55223bc8821b34ccb01f92ef1401c02b6a32e61f.1626343282.git.rickyman7@gmail.com
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index 8af693d9678cefe0..72e7f3616157ead4 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -192,6 +192,8 @@ struct map *map__new(struct machine *machine, u64 start, u64 len,
if (!(prot & PROT_EXEC))
dso__set_loaded(dso);
}
+
+ nsinfo__put(dso->nsinfo);
dso->nsinfo = nsi;
if (build_id__is_defined(bid))
On Thu, Jul 15, 2021 at 04:20:24PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, Jul 15, 2021 at 04:02:16PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Thu, Jul 15, 2021 at 06:07:06PM +0200, Riccardo Mancini escreveu:
> > > ASan reports a memory leak of nsinfo during the execution of the perf
> > > test "31: Lookup mmap thread".
> > > The leak is caused by a refcounted variable being replaced without
> > > dropping the refcount.
>
> > > This patch makes sure that the refcnt of nsinfo is decreased whenever
> > > a refcounted variable is replaced with a new value.
>
> > So, there are multiple fixes in just one patch, I'll split it into
> > three, no need to resend.
>
> > I'll try and check if finding Fixes: for the three is easy, that way
> > [email protected] will figure out which of the supported releases
> > need each of them.
>
> The second is at the end of this message, and looking at it I think
> there is a problem with where this nsinfo reference is stored, it should
> be in the 'struct map', that is per-thread, not in the 'struct dso',
> that may be shared by multiple 'struct map's in different cgroups, so we
> can't set dso->nsinfo in map__new(), right? Kristen?
Apologies for the long delayed reply, and for the hassle that this has
caused.
You're right, Arnaldo, that namespace mapping could differ on a per
thread basis. If the dso objects are intended to be read by multiple
threads with different views of the target process's namespaces. With
containers, though, there might be many different versions of the same
object with the same filename, and the only ways we can disambiguate are
through the build-ids or by ensuring that the dso refers to a single
pid and mount namespace. I may have erred in assuming that all of
perf's threads would be able to access the target namespace via the same
identifiers. Is that what's going on here?
-K
> commit 280f3933661f9ca9563ed04ce82313fdbcbb8f75
> Author: Riccardo Mancini <[email protected]>
> Date: Thu Jul 15 18:07:06 2021 +0200
>
> perf map: Fix dso->nsinfo refcounting
>
> ASan reports a memory leak of nsinfo during the execution of
>
> # perf test "31: Lookup mmap thread"
>
> The leak is caused by a refcounted variable being replaced without
> dropping the refcount.
>
> This patch makes sure that the refcnt of nsinfo is decreased whenever a
> refcounted variable is replaced with a new value.
>
> Signed-off-by: Riccardo Mancini <[email protected]>
> Fixes: bf2e710b3cb8445c ("perf maps: Lookup maps in both intitial mountns and inner mountns.")
> Cc: Ian Rogers <[email protected]>
> Cc: Jiri Olsa <[email protected]>
> Cc: Krister Johansen <[email protected]>
> Cc: Mark Rutland <[email protected]>
> Cc: Namhyung Kim <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Link: http://lore.kernel.org/lkml/55223bc8821b34ccb01f92ef1401c02b6a32e61f.1626343282.git.rickyman7@gmail.com
> Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
>
> diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
> index 8af693d9678cefe0..72e7f3616157ead4 100644
> --- a/tools/perf/util/map.c
> +++ b/tools/perf/util/map.c
> @@ -192,6 +192,8 @@ struct map *map__new(struct machine *machine, u64 start, u64 len,
> if (!(prot & PROT_EXEC))
> dso__set_loaded(dso);
> }
> +
> + nsinfo__put(dso->nsinfo);
> dso->nsinfo = nsi;
>
> if (build_id__is_defined(bid))
>
Em Tue, Jan 04, 2022 at 10:19:42PM -0800, Krister Johansen escreveu:
> On Thu, Jul 15, 2021 at 04:20:24PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Thu, Jul 15, 2021 at 04:02:16PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > Em Thu, Jul 15, 2021 at 06:07:06PM +0200, Riccardo Mancini escreveu:
> > > > ASan reports a memory leak of nsinfo during the execution of the perf
> > > > test "31: Lookup mmap thread".
> > > > The leak is caused by a refcounted variable being replaced without
> > > > dropping the refcount.
> >
> > > > This patch makes sure that the refcnt of nsinfo is decreased whenever
> > > > a refcounted variable is replaced with a new value.
> >
> > > So, there are multiple fixes in just one patch, I'll split it into
> > > three, no need to resend.
> >
> > > I'll try and check if finding Fixes: for the three is easy, that way
> > > [email protected] will figure out which of the supported releases
> > > need each of them.
> >
> > The second is at the end of this message, and looking at it I think
> > there is a problem with where this nsinfo reference is stored, it should
> > be in the 'struct map', that is per-thread, not in the 'struct dso',
> > that may be shared by multiple 'struct map's in different cgroups, so we
> > can't set dso->nsinfo in map__new(), right? Kristen?
>
> Apologies for the long delayed reply, and for the hassle that this has
> caused.
>
> You're right, Arnaldo, that namespace mapping could differ on a per
> thread basis. If the dso objects are intended to be read by multiple
> threads with different views of the target process's namespaces. With
> containers, though, there might be many different versions of the same
> object with the same filename, and the only ways we can disambiguate are
> through the build-ids or by ensuring that the dso refers to a single
> pid and mount namespace. I may have erred in assuming that all of
> perf's threads would be able to access the target namespace via the same
> identifiers. Is that what's going on here?
yeah, I think using build-ids, when available is the way to go, and
since we can almost take it for granted (haven't checked) that it will
be present, as I think gcc/clang adds them by default, seems the way to
go.
That or somehow have the DSO list to be perf namespace.
- Arnaldo
> -K
>
>
> > commit 280f3933661f9ca9563ed04ce82313fdbcbb8f75
> > Author: Riccardo Mancini <[email protected]>
> > Date: Thu Jul 15 18:07:06 2021 +0200
> >
> > perf map: Fix dso->nsinfo refcounting
> >
> > ASan reports a memory leak of nsinfo during the execution of
> >
> > # perf test "31: Lookup mmap thread"
> >
> > The leak is caused by a refcounted variable being replaced without
> > dropping the refcount.
> >
> > This patch makes sure that the refcnt of nsinfo is decreased whenever a
> > refcounted variable is replaced with a new value.
> >
> > Signed-off-by: Riccardo Mancini <[email protected]>
> > Fixes: bf2e710b3cb8445c ("perf maps: Lookup maps in both intitial mountns and inner mountns.")
> > Cc: Ian Rogers <[email protected]>
> > Cc: Jiri Olsa <[email protected]>
> > Cc: Krister Johansen <[email protected]>
> > Cc: Mark Rutland <[email protected]>
> > Cc: Namhyung Kim <[email protected]>
> > Cc: Peter Zijlstra <[email protected]>
> > Link: http://lore.kernel.org/lkml/55223bc8821b34ccb01f92ef1401c02b6a32e61f.1626343282.git.rickyman7@gmail.com
> > Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
> >
> > diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
> > index 8af693d9678cefe0..72e7f3616157ead4 100644
> > --- a/tools/perf/util/map.c
> > +++ b/tools/perf/util/map.c
> > @@ -192,6 +192,8 @@ struct map *map__new(struct machine *machine, u64 start, u64 len,
> > if (!(prot & PROT_EXEC))
> > dso__set_loaded(dso);
> > }
> > +
> > + nsinfo__put(dso->nsinfo);
> > dso->nsinfo = nsi;
> >
> > if (build_id__is_defined(bid))
> >
--
- Arnaldo
Em Thu, Jan 06, 2022 at 05:27:43PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Tue, Jan 04, 2022 at 10:19:42PM -0800, Krister Johansen escreveu:
> > On Thu, Jul 15, 2021 at 04:20:24PM -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Thu, Jul 15, 2021 at 04:02:16PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > > Em Thu, Jul 15, 2021 at 06:07:06PM +0200, Riccardo Mancini escreveu:
> > > > > ASan reports a memory leak of nsinfo during the execution of the perf
> > > > > test "31: Lookup mmap thread".
> > > > > The leak is caused by a refcounted variable being replaced without
> > > > > dropping the refcount.
> > >
> > > > > This patch makes sure that the refcnt of nsinfo is decreased whenever
> > > > > a refcounted variable is replaced with a new value.
> > >
> > > > So, there are multiple fixes in just one patch, I'll split it into
> > > > three, no need to resend.
> > >
> > > > I'll try and check if finding Fixes: for the three is easy, that way
> > > > [email protected] will figure out which of the supported releases
> > > > need each of them.
> > >
> > > The second is at the end of this message, and looking at it I think
> > > there is a problem with where this nsinfo reference is stored, it should
> > > be in the 'struct map', that is per-thread, not in the 'struct dso',
> > > that may be shared by multiple 'struct map's in different cgroups, so we
> > > can't set dso->nsinfo in map__new(), right? Kristen?
> >
> > Apologies for the long delayed reply, and for the hassle that this has
> > caused.
> >
> > You're right, Arnaldo, that namespace mapping could differ on a per
> > thread basis. If the dso objects are intended to be read by multiple
> > threads with different views of the target process's namespaces. With
> > containers, though, there might be many different versions of the same
> > object with the same filename, and the only ways we can disambiguate are
> > through the build-ids or by ensuring that the dso refers to a single
> > pid and mount namespace. I may have erred in assuming that all of
> > perf's threads would be able to access the target namespace via the same
> > identifiers. Is that what's going on here?
>
> yeah, I think using build-ids, when available is the way to go, and
> since we can almost take it for granted (haven't checked) that it will
> be present, as I think gcc/clang adds them by default, seems the way to
> go.
>
> That or somehow have the DSO list to be perf namespace.
To be by namespace, sorry.
But having it by build-id seems the more sensible way to go, so that we
can avoid having multiple instances of the same DSO in multiple
namespaces, which should be common.
I.e. continue to have a perf 'struct machine' DSO list instead of moving
to having the namespace hierarchy represented under 'struct machine'
since we have a way to uniquify DSOs, the build id.
One way or the other, nsinfo shouldn't be stored in DSOs, its a map
property, which in turn is a thread property.
I.e. please consider thinking about moving it to 'struct map', would you
try it, please?
- Arnaldo
> - Arnaldo
>
> > -K
> >
> >
> > > commit 280f3933661f9ca9563ed04ce82313fdbcbb8f75
> > > Author: Riccardo Mancini <[email protected]>
> > > Date: Thu Jul 15 18:07:06 2021 +0200
> > >
> > > perf map: Fix dso->nsinfo refcounting
> > >
> > > ASan reports a memory leak of nsinfo during the execution of
> > >
> > > # perf test "31: Lookup mmap thread"
> > >
> > > The leak is caused by a refcounted variable being replaced without
> > > dropping the refcount.
> > >
> > > This patch makes sure that the refcnt of nsinfo is decreased whenever a
> > > refcounted variable is replaced with a new value.
> > >
> > > Signed-off-by: Riccardo Mancini <[email protected]>
> > > Fixes: bf2e710b3cb8445c ("perf maps: Lookup maps in both intitial mountns and inner mountns.")
> > > Cc: Ian Rogers <[email protected]>
> > > Cc: Jiri Olsa <[email protected]>
> > > Cc: Krister Johansen <[email protected]>
> > > Cc: Mark Rutland <[email protected]>
> > > Cc: Namhyung Kim <[email protected]>
> > > Cc: Peter Zijlstra <[email protected]>
> > > Link: http://lore.kernel.org/lkml/55223bc8821b34ccb01f92ef1401c02b6a32e61f.1626343282.git.rickyman7@gmail.com
> > > Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
> > >
> > > diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
> > > index 8af693d9678cefe0..72e7f3616157ead4 100644
> > > --- a/tools/perf/util/map.c
> > > +++ b/tools/perf/util/map.c
> > > @@ -192,6 +192,8 @@ struct map *map__new(struct machine *machine, u64 start, u64 len,
> > > if (!(prot & PROT_EXEC))
> > > dso__set_loaded(dso);
> > > }
> > > +
> > > + nsinfo__put(dso->nsinfo);
> > > dso->nsinfo = nsi;
> > >
> > > if (build_id__is_defined(bid))
> > >
>
> --
>
> - Arnaldo
--
- Arnaldo