Re: [PATCH perf/core 03/13] perf: Introduce generic refcount APIs with debug feature

[Namhyung Kim <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..


> ---

[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.

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");
> +}