[PATCH] perf data: Fix releasing event_class

[PATCH] perf data: Fix releasing event_class

[Wang Nan]

A new patch of libbabeltrace [1] reveals a object leak problem in
perf data CTF support: perf code never release event_class which is
allocated in add_event() and stored in evsel's private field.

If libbabeltrace has the above patch applied, leaking event_class
prevent writer being destroied and flushing metadata. For example:

 $ ./perf record ls
 Lowering default frequency rate to 500.
 Please consider tweaking /proc/sys/kernel/perf_event_max_sample_rate.
 perf.data
 [ perf record: Woken up 1 times to write data ]
 [ perf record: Captured and wrote 0.012 MB perf.data (12 samples) ]
 $ ./perf data convert --to-ctf ./out.ctf
 [ perf data convert: Converted 'perf.data' into CTF data './out.ctf' ]
 [ perf data convert: Converted and wrote 0.000 MB (12 samples) ]
 $ cat ./out.ctf/metadata
 $ ls -l  ./out.ctf/metadata
 -rw-r----- 1 w00229757 mm 0 Jan 27 10:49 ./out.ctf/metadata

The correct result should be:
 ...
 $ cat ./out.ctf/metadata
 /* CTF 1.8 */

 trace {
 [SNIP]

 $ ls -l  ./out.ctf/metadata
 -rw-r----- 1 w00229757 mm 2446 Jan 27 10:52 ./out.ctf/metadata

The full story is:

 Patch [1] of babeltrace redesign reference counting scheme. In that
 patch:

  * writer <- trace (bt_ctf_writer_create)
  * trace <- stream_class (bt_ctf_trace_add_stream_class)
  * stream_class <- event_class (bt_ctf_stream_class_add_event_class)
  ('<-' means 'is a parent of')

  Holding of event_class causes reference count of corresponding
  'writer' increases through parent chain. Perf expect 'writer' is
  released (so metadata is flushed) through bt_ctf_writer_put() in
  ctf_writer__cleanup(). However, since it never release event_class,
  the reference of 'writer' won't be reduced, so bt_ctf_writer_put()
  won't lead releasing of writer.

 Before this CTF patch, !(writer <- trace). Even event_class leak,
 writer is able to be released.

[1] https://github.com/efficios/babeltrace/commit/e6a8e8e4744633807083a077ff9f101eb97d9801

Signed-off-by: Wang Nan <wangnan0@huawei.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Jérémie Galarneau <jeremie.galarneau@efficios.com>
Cc: Zefan Li <lizefan@huawei.com>
Cc: pi3orama@163.com
---
 tools/perf/util/data-convert-bt.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/tools/perf/util/data-convert-bt.c b/tools/perf/util/data-convert-bt.c
index 34cd1e4..e553321 100644
--- a/tools/perf/util/data-convert-bt.c
+++ b/tools/perf/util/data-convert-bt.c
@@ -858,6 +858,19 @@ static int setup_events(struct ctf_writer *cw, struct perf_session *session)
 	return 0;
 }
 
+static void cleanup_events(struct perf_session *session)
+{
+	struct perf_evlist *evlist = session->evlist;
+	struct perf_evsel *evsel;
+
+	evlist__for_each(evlist, evsel) {
+		struct evsel_priv *priv;
+
+		priv = evsel->priv;
+		bt_ctf_event_class_put(priv->event_class);
+	}
+}
+
 static int setup_streams(struct ctf_writer *cw, struct perf_session *session)
 {
 	struct ctf_stream **stream;
@@ -1171,6 +1184,7 @@ int bt_convert__perf2ctf(const char *input, const char *path, bool force)
 		(double) c.events_size / 1024.0 / 1024.0,
 		c.events_count);
 
+	cleanup_events(session);
 	perf_session__delete(session);
 	ctf_writer__cleanup(cw);
 
-- 
1.8.3.4

Re: [PATCH] perf data: Fix releasing event_class

[Wangnan (F)]

Hi Jiri, What's your opinion on this patch?

Thank you.

On 2016/1/27 19:21, Wang Nan wrote:
> A new patch of libbabeltrace [1] reveals a object leak problem in
> perf data CTF support: perf code never release event_class which is
> allocated in add_event() and stored in evsel's private field.
>
> If libbabeltrace has the above patch applied, leaking event_class
> prevent writer being destroied and flushing metadata. For example:
>
>   $ ./perf record ls
>   Lowering default frequency rate to 500.
>   Please consider tweaking /proc/sys/kernel/perf_event_max_sample_rate.
>   perf.data
>   [ perf record: Woken up 1 times to write data ]
>   [ perf record: Captured and wrote 0.012 MB perf.data (12 samples) ]
>   $ ./perf data convert --to-ctf ./out.ctf
>   [ perf data convert: Converted 'perf.data' into CTF data './out.ctf' ]
>   [ perf data convert: Converted and wrote 0.000 MB (12 samples) ]
>   $ cat ./out.ctf/metadata
>   $ ls -l  ./out.ctf/metadata
>   -rw-r----- 1 w00229757 mm 0 Jan 27 10:49 ./out.ctf/metadata
>
> The correct result should be:
>   ...
>   $ cat ./out.ctf/metadata
>   /* CTF 1.8 */
>
>   trace {
>   [SNIP]
>
>   $ ls -l  ./out.ctf/metadata
>   -rw-r----- 1 w00229757 mm 2446 Jan 27 10:52 ./out.ctf/metadata
>
> The full story is:
>
>   Patch [1] of babeltrace redesign reference counting scheme. In that
>   patch:
>
>    * writer <- trace (bt_ctf_writer_create)
>    * trace <- stream_class (bt_ctf_trace_add_stream_class)
>    * stream_class <- event_class (bt_ctf_stream_class_add_event_class)
>    ('<-' means 'is a parent of')
>
>    Holding of event_class causes reference count of corresponding
>    'writer' increases through parent chain. Perf expect 'writer' is
>    released (so metadata is flushed) through bt_ctf_writer_put() in
>    ctf_writer__cleanup(). However, since it never release event_class,
>    the reference of 'writer' won't be reduced, so bt_ctf_writer_put()
>    won't lead releasing of writer.
>
>   Before this CTF patch, !(writer <- trace). Even event_class leak,
>   writer is able to be released.
>
> [1] https://github.com/efficios/babeltrace/commit/e6a8e8e4744633807083a077ff9f101eb97d9801
>
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Jérémie Galarneau <jeremie.galarneau@efficios.com>
> Cc: Zefan Li <lizefan@huawei.com>
> Cc: pi3orama@163.com
> ---
>   tools/perf/util/data-convert-bt.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
>
> diff --git a/tools/perf/util/data-convert-bt.c b/tools/perf/util/data-convert-bt.c
> index 34cd1e4..e553321 100644
> --- a/tools/perf/util/data-convert-bt.c
> +++ b/tools/perf/util/data-convert-bt.c
> @@ -858,6 +858,19 @@ static int setup_events(struct ctf_writer *cw, struct perf_session *session)
>   	return 0;
>   }
>   
> +static void cleanup_events(struct perf_session *session)
> +{
> +	struct perf_evlist *evlist = session->evlist;
> +	struct perf_evsel *evsel;
> +
> +	evlist__for_each(evlist, evsel) {
> +		struct evsel_priv *priv;
> +
> +		priv = evsel->priv;
> +		bt_ctf_event_class_put(priv->event_class);
> +	}
> +}
> +
>   static int setup_streams(struct ctf_writer *cw, struct perf_session *session)
>   {
>   	struct ctf_stream **stream;
> @@ -1171,6 +1184,7 @@ int bt_convert__perf2ctf(const char *input, const char *path, bool force)
>   		(double) c.events_size / 1024.0 / 1024.0,
>   		c.events_count);
>   
> +	cleanup_events(session);
>   	perf_session__delete(session);
>   	ctf_writer__cleanup(cw);
>   

Re: [PATCH] perf data: Fix releasing event_class

[Jiri Olsa]

On Thu, Jan 28, 2016 at 04:59:51PM +0800, Wangnan (F) wrote:
> Hi Jiri, What's your opinion on this patch?

> 
> Thank you.
> 
> On 2016/1/27 19:21, Wang Nan wrote:
> >A new patch of libbabeltrace [1] reveals a object leak problem in
> >perf data CTF support: perf code never release event_class which is
> >allocated in add_event() and stored in evsel's private field.
> >
> >If libbabeltrace has the above patch applied, leaking event_class
> >prevent writer being destroied and flushing metadata. For example:

oops, missed this one.. sry.. nice feature ;-)

> >
> >  $ ./perf record ls
> >  Lowering default frequency rate to 500.
> >  Please consider tweaking /proc/sys/kernel/perf_event_max_sample_rate.
> >  perf.data
> >  [ perf record: Woken up 1 times to write data ]
> >  [ perf record: Captured and wrote 0.012 MB perf.data (12 samples) ]
> >  $ ./perf data convert --to-ctf ./out.ctf
> >  [ perf data convert: Converted 'perf.data' into CTF data './out.ctf' ]
> >  [ perf data convert: Converted and wrote 0.000 MB (12 samples) ]
> >  $ cat ./out.ctf/metadata
> >  $ ls -l  ./out.ctf/metadata
> >  -rw-r----- 1 w00229757 mm 0 Jan 27 10:49 ./out.ctf/metadata

SNIP

> >+static void cleanup_events(struct perf_session *session)
> >+{
> >+	struct perf_evlist *evlist = session->evlist;
> >+	struct perf_evsel *evsel;
> >+
> >+	evlist__for_each(evlist, evsel) {
> >+		struct evsel_priv *priv;
> >+
> >+		priv = evsel->priv;
> >+		bt_ctf_event_class_put(priv->event_class);

right, we missed that.. but we should also call free(priv) then

also I can't see also perf_evlist__delete being called..
not sure perf_session__delete will take care of that

thanks,
jirka

[PATCH v2] perf data: Fix releasing event_class

[Wang Nan]

A new patch of libbabeltrace [1] reveals a object leak problem in
perf data CTF support: perf code never release event_class which is
allocated in add_event() and stored in evsel's private field.

If libbabeltrace has the above patch applied, leaking event_class
prevent writer being destroied and flushing metadata. For example:

 $ ./perf record ls
 Lowering default frequency rate to 500.
 Please consider tweaking /proc/sys/kernel/perf_event_max_sample_rate.
 perf.data
 [ perf record: Woken up 1 times to write data ]
 [ perf record: Captured and wrote 0.012 MB perf.data (12 samples) ]
 $ ./perf data convert --to-ctf ./out.ctf
 [ perf data convert: Converted 'perf.data' into CTF data './out.ctf' ]
 [ perf data convert: Converted and wrote 0.000 MB (12 samples) ]
 $ cat ./out.ctf/metadata
 $ ls -l  ./out.ctf/metadata
 -rw-r----- 1 w00229757 mm 0 Jan 27 10:49 ./out.ctf/metadata

The correct result should be:
 ...
 $ cat ./out.ctf/metadata
 /* CTF 1.8 */

 trace {
 [SNIP]

 $ ls -l  ./out.ctf/metadata
 -rw-r----- 1 w00229757 mm 2446 Jan 27 10:52 ./out.ctf/metadata

The full story is:

 Patch [1] of babeltrace redesign reference counting scheme. In that
 patch:

  * writer <- trace (bt_ctf_writer_create)
  * trace <- stream_class (bt_ctf_trace_add_stream_class)
  * stream_class <- event_class (bt_ctf_stream_class_add_event_class)
  ('<-' means 'is a parent of')

  Holding of event_class causes reference count of corresponding
  'writer' increases through parent chain. Perf expect 'writer' is
  released (so metadata is flushed) through bt_ctf_writer_put() in
  ctf_writer__cleanup(). However, since it never release event_class,
  the reference of 'writer' won't be reduced, so bt_ctf_writer_put()
  won't lead releasing of writer.

 Before this CTF patch, !(writer <- trace). Even event_class leak,
 writer is able to be released.

[1] https://github.com/efficios/babeltrace/commit/e6a8e8e4744633807083a077ff9f101eb97d9801

Signed-off-by: Wang Nan <wangnan0@huawei.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Jérémie Galarneau <jeremie.galarneau@efficios.com>
Cc: Zefan Li <lizefan@huawei.com>
Cc: pi3orama@163.com
---

v1 -> v2: Free evsel->priv, destroy evlist
          (even 'perf report' doesn't destroy evlist created by
	   perf_session__open)
---
 tools/perf/util/data-convert-bt.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/tools/perf/util/data-convert-bt.c b/tools/perf/util/data-convert-bt.c
index 34cd1e4..b722e57 100644
--- a/tools/perf/util/data-convert-bt.c
+++ b/tools/perf/util/data-convert-bt.c
@@ -858,6 +858,23 @@ static int setup_events(struct ctf_writer *cw, struct perf_session *session)
 	return 0;
 }
 
+static void cleanup_events(struct perf_session *session)
+{
+	struct perf_evlist *evlist = session->evlist;
+	struct perf_evsel *evsel;
+
+	evlist__for_each(evlist, evsel) {
+		struct evsel_priv *priv;
+
+		priv = evsel->priv;
+		bt_ctf_event_class_put(priv->event_class);
+		zfree(&evsel->priv);
+	}
+
+	perf_evlist__delete(evlist);
+	session->evlist = NULL;
+}
+
 static int setup_streams(struct ctf_writer *cw, struct perf_session *session)
 {
 	struct ctf_stream **stream;
@@ -1171,6 +1188,7 @@ int bt_convert__perf2ctf(const char *input, const char *path, bool force)
 		(double) c.events_size / 1024.0 / 1024.0,
 		c.events_count);
 
+	cleanup_events(session);
 	perf_session__delete(session);
 	ctf_writer__cleanup(cw);
 
-- 
1.8.3.4

Re: [PATCH v2] perf data: Fix releasing event_class

[Jiri Olsa]

On Thu, Jan 28, 2016 at 01:20:28PM +0000, Wang Nan wrote:
> A new patch of libbabeltrace [1] reveals a object leak problem in
> perf data CTF support: perf code never release event_class which is
> allocated in add_event() and stored in evsel's private field.
> 
> If libbabeltrace has the above patch applied, leaking event_class
> prevent writer being destroied and flushing metadata. For example:
> 
>  $ ./perf record ls
>  Lowering default frequency rate to 500.
>  Please consider tweaking /proc/sys/kernel/perf_event_max_sample_rate.
>  perf.data
>  [ perf record: Woken up 1 times to write data ]
>  [ perf record: Captured and wrote 0.012 MB perf.data (12 samples) ]
>  $ ./perf data convert --to-ctf ./out.ctf
>  [ perf data convert: Converted 'perf.data' into CTF data './out.ctf' ]
>  [ perf data convert: Converted and wrote 0.000 MB (12 samples) ]
>  $ cat ./out.ctf/metadata
>  $ ls -l  ./out.ctf/metadata
>  -rw-r----- 1 w00229757 mm 0 Jan 27 10:49 ./out.ctf/metadata
> 
> The correct result should be:
>  ...
>  $ cat ./out.ctf/metadata
>  /* CTF 1.8 */
> 
>  trace {
>  [SNIP]
> 
>  $ ls -l  ./out.ctf/metadata
>  -rw-r----- 1 w00229757 mm 2446 Jan 27 10:52 ./out.ctf/metadata
> 
> The full story is:
> 
>  Patch [1] of babeltrace redesign reference counting scheme. In that
>  patch:
> 
>   * writer <- trace (bt_ctf_writer_create)
>   * trace <- stream_class (bt_ctf_trace_add_stream_class)
>   * stream_class <- event_class (bt_ctf_stream_class_add_event_class)
>   ('<-' means 'is a parent of')
> 
>   Holding of event_class causes reference count of corresponding
>   'writer' increases through parent chain. Perf expect 'writer' is
>   released (so metadata is flushed) through bt_ctf_writer_put() in
>   ctf_writer__cleanup(). However, since it never release event_class,
>   the reference of 'writer' won't be reduced, so bt_ctf_writer_put()
>   won't lead releasing of writer.
> 
>  Before this CTF patch, !(writer <- trace). Even event_class leak,
>  writer is able to be released.
> 
> [1] https://github.com/efficios/babeltrace/commit/e6a8e8e4744633807083a077ff9f101eb97d9801
> 
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Jérémie Galarneau <jeremie.galarneau@efficios.com>
> Cc: Zefan Li <lizefan@huawei.com>
> Cc: pi3orama@163.com
> ---
> 
> v1 -> v2: Free evsel->priv, destroy evlist
>           (even 'perf report' doesn't destroy evlist created by
> 	   perf_session__open)

Acked-by: Jiri Olsa <jolsa@kernel.org>


Arnaldo,
we're missing perf_evlist__delete in report command

I remember there were several cleanups skipped intentionaly,
but I dont think this one is the case.. also report TUI provides
the 's' key to load new data file.. which makes this leak
important

however even with this patch I can see perf report getting
more memory every time it reloads the data.. either there's
something else, or I'm looking at it wrong ;-)

thanks,
jirka


---
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 2bf537f190a0..7a4a27a6f053 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -970,12 +970,14 @@ repeat:
 
 	ret = __cmd_report(&report);
 	if (ret == K_SWITCH_INPUT_DATA) {
+		perf_evlist__delete(session->evlist);
 		perf_session__delete(session);
 		goto repeat;
 	} else
 		ret = 0;
 
 error:
+	perf_evlist__delete(session->evlist);
 	perf_session__delete(session);
 	return ret;
 }

Re: [PATCH v2] perf data: Fix releasing event_class

[Arnaldo Carvalho de Melo]

Em Thu, Jan 28, 2016 at 03:59:23PM +0100, Jiri Olsa escreveu:
> On Thu, Jan 28, 2016 at 01:20:28PM +0000, Wang Nan wrote:
> > A new patch of libbabeltrace [1] reveals a object leak problem in
> > perf data CTF support: perf code never release event_class which is
> > allocated in add_event() and stored in evsel's private field.
> > 
> > If libbabeltrace has the above patch applied, leaking event_class
> > prevent writer being destroied and flushing metadata. For example:
> > 
> >  $ ./perf record ls
> >  Lowering default frequency rate to 500.
> >  Please consider tweaking /proc/sys/kernel/perf_event_max_sample_rate.
> >  perf.data
> >  [ perf record: Woken up 1 times to write data ]
> >  [ perf record: Captured and wrote 0.012 MB perf.data (12 samples) ]
> >  $ ./perf data convert --to-ctf ./out.ctf
> >  [ perf data convert: Converted 'perf.data' into CTF data './out.ctf' ]
> >  [ perf data convert: Converted and wrote 0.000 MB (12 samples) ]
> >  $ cat ./out.ctf/metadata
> >  $ ls -l  ./out.ctf/metadata
> >  -rw-r----- 1 w00229757 mm 0 Jan 27 10:49 ./out.ctf/metadata
> > 
> > The correct result should be:
> >  ...
> >  $ cat ./out.ctf/metadata
> >  /* CTF 1.8 */
> > 
> >  trace {
> >  [SNIP]
> > 
> >  $ ls -l  ./out.ctf/metadata
> >  -rw-r----- 1 w00229757 mm 2446 Jan 27 10:52 ./out.ctf/metadata
> > 
> > The full story is:
> > 
> >  Patch [1] of babeltrace redesign reference counting scheme. In that
> >  patch:
> > 
> >   * writer <- trace (bt_ctf_writer_create)
> >   * trace <- stream_class (bt_ctf_trace_add_stream_class)
> >   * stream_class <- event_class (bt_ctf_stream_class_add_event_class)
> >   ('<-' means 'is a parent of')
> > 
> >   Holding of event_class causes reference count of corresponding
> >   'writer' increases through parent chain. Perf expect 'writer' is
> >   released (so metadata is flushed) through bt_ctf_writer_put() in
> >   ctf_writer__cleanup(). However, since it never release event_class,
> >   the reference of 'writer' won't be reduced, so bt_ctf_writer_put()
> >   won't lead releasing of writer.
> > 
> >  Before this CTF patch, !(writer <- trace). Even event_class leak,
> >  writer is able to be released.
> > 
> > [1] https://github.com/efficios/babeltrace/commit/e6a8e8e4744633807083a077ff9f101eb97d9801
> > 
> > Signed-off-by: Wang Nan <wangnan0@huawei.com>
> > Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> > Cc: Jiri Olsa <jolsa@kernel.org>
> > Cc: Jérémie Galarneau <jeremie.galarneau@efficios.com>
> > Cc: Zefan Li <lizefan@huawei.com>
> > Cc: pi3orama@163.com
> > ---
> > 
> > v1 -> v2: Free evsel->priv, destroy evlist
> >           (even 'perf report' doesn't destroy evlist created by
> > 	   perf_session__open)
> 
> Acked-by: Jiri Olsa <jolsa@kernel.org>
> 
> 
> Arnaldo,
> we're missing perf_evlist__delete in report command
> 
> I remember there were several cleanups skipped intentionaly,
> but I dont think this one is the case.. also report TUI provides
> the 's' key to load new data file.. which makes this leak
> important
> 
> however even with this patch I can see perf report getting
> more memory every time it reloads the data.. either there's
> something else, or I'm looking at it wrong ;-)

So, for the last delete, IIRC that teardown was removed because it made
the exit operation to be slower than just not calling
perf_evlist__delete (or was it perf_session__delete, don't recall right
now, probably the later, because it would delete all the threads, mmaps,
etc).

But yeah, for switching perf.data files on the fly, we better free all
that up...
 
> thanks,
> jirka
> 
> 
> ---
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index 2bf537f190a0..7a4a27a6f053 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -970,12 +970,14 @@ repeat:
>  
>  	ret = __cmd_report(&report);
>  	if (ret == K_SWITCH_INPUT_DATA) {
> +		perf_evlist__delete(session->evlist);
>  		perf_session__delete(session);
>  		goto repeat;
>  	} else
>  		ret = 0;
>  
>  error:
> +	perf_evlist__delete(session->evlist);
>  	perf_session__delete(session);
>  	return ret;
>  }

Re: [PATCH v2] perf data: Fix releasing event_class

[Jiri Olsa]

On Thu, Jan 28, 2016 at 05:09:30PM -0200, Arnaldo Carvalho de Melo wrote:

SNIP

> > however even with this patch I can see perf report getting
> > more memory every time it reloads the data.. either there's
> > something else, or I'm looking at it wrong ;-)
> 
> So, for the last delete, IIRC that teardown was removed because it made
> the exit operation to be slower than just not calling
> perf_evlist__delete (or was it perf_session__delete, don't recall right
> now, probably the later, because it would delete all the threads, mmaps,
> etc).
> 
> But yeah, for switching perf.data files on the fly, we better free all
> that up...

ook, I'll check some more and queue it in my next post

jirka