From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S966356AbcA1O73 (ORCPT ); Thu, 28 Jan 2016 09:59:29 -0500 Received: from mx1.redhat.com ([209.132.183.28]:50948 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965438AbcA1O70 (ORCPT ); Thu, 28 Jan 2016 09:59:26 -0500 Date: Thu, 28 Jan 2016 15:59:23 +0100 From: Jiri Olsa To: Wang Nan Cc: acme@redhat.com, jolsa@kernel.org, linux-kernel@vger.kernel.org, =?iso-8859-1?Q?J=E9r=E9mie?= Galarneau , Zefan Li , pi3orama@163.com Subject: Re: [PATCH v2] perf data: Fix releasing event_class Message-ID: <20160128145923.GB20826@krava.brq.redhat.com> References: <20160128092639.GE2322@krava.brq.redhat.com> <1453987228-31418-1-git-send-email-wangnan0@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1453987228-31418-1-git-send-email-wangnan0@huawei.com> 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, Jan 28, 2016 at 01:20:28PM +0000, 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 > --- > > v1 -> v2: Free evsel->priv, destroy evlist > (even 'perf report' doesn't destroy evlist created by > perf_session__open) Acked-by: Jiri Olsa Arnaldo, we're missing perf_evlist__delete in report command I remember there were several cleanups skipped intentionaly, but I dont think this one is the case.. also report TUI provides the 's' key to load new data file.. which makes this leak important however even with this patch I can see perf report getting more memory every time it reloads the data.. either there's something else, or I'm looking at it wrong ;-) thanks, jirka --- diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c index 2bf537f190a0..7a4a27a6f053 100644 --- a/tools/perf/builtin-report.c +++ b/tools/perf/builtin-report.c @@ -970,12 +970,14 @@ repeat: ret = __cmd_report(&report); if (ret == K_SWITCH_INPUT_DATA) { + perf_evlist__delete(session->evlist); perf_session__delete(session); goto repeat; } else ret = 0; error: + perf_evlist__delete(session->evlist); perf_session__delete(session); return ret; }