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

[Arnaldo Carvalho de Melo <acme@redhat.com>]

Em Fri, Feb 12, 2016 at 01:19:42PM +0100, Jiri Olsa escreveu:
> 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

Ok, Wang, if you notice such Acked-by, collect them, i.e. add them to
new versions of your patchkits, that helps me in processing them, i.e.
knowing that there was already discussion/acknowledgement from people
directly involved in the affected code.

thanks,

- Arnaldo
 
> 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@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 | 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