Re: [PATCH 05/54] perf data: Fix releasing event_class

[Jiri Olsa]

On Thu, Feb 11, 2016 at 07:04:13PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Feb 05, 2016 at 02:01:30PM +0000, Wang Nan escreveu:
> > 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:
> 
> Ok, so if the user has an older version of this libbabeltrace, what
> happens? 

it's standard cleanup that should be there even for old version,

IIUC the problem is that new version of libbabeltrace started
to check on refcounts on some exit function and gets crazy
if there's a mess/leak

IIRC I've already acked this one

jirka

> 
> Would he/she gets some warning about the requirement of a later version?
> 
> - Arnaldo
>  
> >  $ ./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@xxxxxxxxxx>
> > Cc: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
> > Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
> > Cc: Jérémie Galarneau <jeremie.galarneau@xxxxxxxxxxxx>
> > Cc: Zefan Li <lizefan@xxxxxxxxxx>
> > Cc: pi3orama@xxxxxxx
> > ---
> >  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