From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752470AbcBLMTy (ORCPT ); Fri, 12 Feb 2016 07:19:54 -0500 Received: from mx1.redhat.com ([209.132.183.28]:51893 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752364AbcBLMTw (ORCPT ); Fri, 12 Feb 2016 07:19:52 -0500 Date: Fri, 12 Feb 2016 13:19:42 +0100 From: Jiri Olsa To: Arnaldo Carvalho de Melo Cc: Wang Nan , Alexei Starovoitov , Brendan Gregg , Adrian Hunter , Cody P Schafer , "David S. Miller" , He Kuang , =?iso-8859-1?Q?J=E9r=E9mie?= Galarneau , Jiri Olsa , Kirill Smelkov , Li Zefan , Masami Hiramatsu , Namhyung Kim , Peter Zijlstra , pi3orama@163.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH 05/54] perf data: Fix releasing event_class Message-ID: <20160212121942.GA16190@krava.redhat.com> References: <1454680939-24963-1-git-send-email-wangnan0@huawei.com> <1454680939-24963-6-git-send-email-wangnan0@huawei.com> <20160211220413.GF32168@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20160211220413.GF32168@kernel.org> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > > Cc: Arnaldo Carvalho de Melo > > Cc: Jiri Olsa > > Cc: Jérémie Galarneau > > Cc: Zefan Li > > 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