Messages in this thread | | | Date | Fri, 12 Feb 2016 13:42:10 -0200 | From | Arnaldo Carvalho de Melo <> | Subject | Re: [PATCH 05/54] perf data: Fix releasing event_class |
| |
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
| |