From: =?utf-8?B?5bmz5p2+6ZuF5bezIC8gSElSQU1BVFXvvIxNQVNBTUk=?= 
	<masami.hiramatsu.pt@hitachi.com>
To: "'Namhyung Kim'" <namhyung@kernel.org>
CC: Arnaldo Carvalho de Melo <acme@kernel.org>,
        Peter Zijlstra <a.p.zijlstra@chello.nl>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Adrian Hunter <adrian.hunter@intel.com>,
        "Ingo Molnar" <mingo@redhat.com>, Jiri Olsa <jolsa@redhat.com>
Subject: RE: [PATCH perf/core 03/13] perf: Introduce generic refcount APIs
 with debug feature
Thread-Topic: [PATCH perf/core 03/13] perf: Introduce generic refcount APIs
 with debug feature
Thread-Index: AQHRIcz2qhuGHmGd5kW1cSQDqDSVTZ6joocAgACnyiA=
Date: Fri, 20 Nov 2015 04:12:06 +0000
Message-ID: <50399556C9727B4D88A595C8584AAB375262738C@GSjpTKYDCembx32.service.hitachi.net>
References: <20151118064009.30709.74354.stgit@localhost.localdomain>
 <20151118064016.30709.10199.stgit@localhost.localdomain>
 <20151120025232.GF23310@sejong>
In-Reply-To: <20151120025232.GF23310@sejong>
Accept-Language: ja-JP, en-US
Content-Language: ja-JP
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
Content-Transfer-Encoding: 8bit
Content-Length: 4217
Lines: 136

From: Namhyung Kim [mailto:namhyung@kernel.org]
>
>On Wed, Nov 18, 2015 at 03:40:16PM +0900, Masami Hiramatsu wrote:
>> This is a kind of debugging feature for atomic reference counter.
>> The reference counters are widely used in perf tools but not well
>> debugged. It sometimes causes memory leaks but no one has noticed
>> the issue, since it is hard to debug such un-reclaimed objects.
>>
>> This refcnt interface provides fully backtrace debug feature to
>> analyze such issue. User just replace atomic_t ops with refcnt
>> APIs and add refcnt__exit() where the object is released.
>>
>> /* At object initializing */
>> refcnt__init(obj, refcnt); /* <- atomic_set(&obj->refcnt, 1); */
>>
>> /* At object get method */
>> refcnt__get(obj, refcnt); /* <- atomic_inc(&obj->refcnt); */
>>
>> /* At object put method */
>> if (obj && refcnt__put(obj, refcnt)) /* <-atmoic_dec_and_test(&obj->refcnt)*/
>>
>> /* At object releasing */
>> refcnt__exit(obj, refcnt); /* <- Newly added */
>>
>> The debugging feature is enabled when building perf with
>> REFCNT_DEBUG=1. Otherwides it is just translated as normal
>> atomic ops.
>>
>> Debugging binary warns you if it finds leaks when the perf exits.
>> If you give -v (or --verbose) to the perf, it also shows backtrace
>> logs on all refcnt operations of leaked objects.
>>
>> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>
>Looks really useful!
>
>Acked-by: Namhyung Kim <namhyung@kernel.org>
>
>Just a nitpick below..

Thanks!

>> ---
>
>[SNIP]
>> +void refcnt_object__record(void *obj, const char *name, int count)
>> +{
>> +	struct refcnt_object *ref = refcnt__find_object(obj);
>> +	struct refcnt_buffer *buf;
>> +
>> +	/* If no entry, allocate new one */
>> +	if (!ref) {
>> +		ref = malloc(sizeof(*ref));
>> +		if (!ref) {
>> +			pr_debug("REFCNT: Out of memory for %p (%s)\n",
>> +				 obj, name);
>> +			return;
>> +		}
>> +		INIT_LIST_HEAD(&ref->list);
>> +		INIT_LIST_HEAD(&ref->head);
>> +		ref->name = name;
>> +		ref->obj = obj;
>> +		list_add_tail(&ref->list, &refcnt_root);
>> +	}
>> +
>> +	buf = malloc(sizeof(*buf));
>> +	if (!buf) {
>> +		pr_debug("REFCNT: Out of memory for %p (%s)\n", obj, ref->name);
>> +		return;
>> +	}
>> +
>> +	INIT_LIST_HEAD(&buf->list);
>> +	buf->count = count;
>> +	buf->nr = backtrace(buf->buf, BACKTRACE_SIZE);
>> +	list_add_tail(&buf->list, &ref->head);
>> +}
>> +
>> +static void pr_refcnt_buffer(struct refcnt_buffer *buf)
>> +{
>> +	char **symbuf;
>> +	int i;
>> +
>> +	if (!buf)
>> +		return;
>> +	symbuf = backtrace_symbols(buf->buf, buf->nr);
>
>It seems you need to check the return value.  Maybe we can use
>backtrace_symbols_fd() instead, or just in case of an error.

Yeah, it should be checked and in that case we can fall back to
backtrace_symbols_fd(as the last resort), but I don’t like
backtrace_symbols_fd replacing because it doesn't allow us to
indent the backtrace result.

Thank you,


>
>Thanks,
>Namhyung
>
>
>> +	/* Skip the first one because it is always btrace__record */
>> +	for (i = 1; i < buf->nr; i++)
>> +		pr_debug("  %s\n", symbuf[i]);
>> +	free(symbuf);
>> +}
>> +
>> +void refcnt__dump_unreclaimed(void) __attribute__((destructor));
>> +void refcnt__dump_unreclaimed(void)
>> +{
>> +	struct refcnt_object *ref, *n;
>> +	struct refcnt_buffer *buf;
>> +	int i = 0;
>> +
>> +	if (list_empty(&refcnt_root))
>> +		return;
>> +
>> +	pr_warning("REFCNT: BUG: Unreclaimed objects found.\n");
>> +	list_for_each_entry_safe(ref, n, &refcnt_root, list) {
>> +		pr_debug("==== [%d] ====\nUnreclaimed %s: %p\n", i,
>> +			 ref->name ? ref->name : "(object)", ref->obj);
>> +		list_for_each_entry(buf, &ref->head, list) {
>> +			pr_debug("Refcount %s => %d at\n",
>> +				 buf->count > 0 ? "+1" : "-1",
>> +				 buf->count > 0 ? buf->count : -buf->count - 1);
>> +			pr_refcnt_buffer(buf);
>> +		}
>> +		__refcnt_object__delete(ref);
>> +		i++;
>> +	}
>> +	pr_warning("REFCNT: Total %d objects are not reclaimed.\n", i);
>> +	if (!verbose)
>> +		pr_warning("   To see all backtraces, rerun with -v option\n");
>> +}
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m????????????I?