From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755203AbcA1JAh (ORCPT ); Thu, 28 Jan 2016 04:00:37 -0500 Received: from szxga03-in.huawei.com ([119.145.14.66]:61608 "EHLO szxga03-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755173AbcA1JAe (ORCPT ); Thu, 28 Jan 2016 04:00:34 -0500 Message-ID: <56A9D887.8030508@huawei.com> Date: Thu, 28 Jan 2016 16:59:51 +0800 From: "Wangnan (F)" User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: Jiri Olsa CC: , , =?UTF-8?B?SsOpcsOpbWll?= =?UTF-8?B?IEdhbGFybmVhdQ==?= , Zefan Li , Subject: Re: [PATCH] perf data: Fix releasing event_class References: <1453893665-20530-1-git-send-email-wangnan0@huawei.com> In-Reply-To: <1453893665-20530-1-git-send-email-wangnan0@huawei.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.111.66.109] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A020203.56A9D896.0198,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2013-05-26 15:14:31, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: 3d416b8a14588e3407ef702dd5111bc0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > 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 | 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); >