Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1273738

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

Show all headers | View raw

On Fri, Nov 20, 2015 at 04:12:06AM +0000, 平松雅巳 / HIRAMATU，MASAMI wrote:
> 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.

OK, I think we need to improve the backtrace code in general.  I'll
send a related patch soon.

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");
> >> +}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH perf/core  00/13] perf memory/refcnt leak fixes Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> - 2015-11-18 07:50 +0100
  [PATCH perf/core 12/13] perf: Fix machine__findnew_module_map to  put dso Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> - 2015-11-18 07:50 +0100
    [tip:perf/core] perf machine:   Fix machine__findnew_module_map to put dso tip-bot for Masami Hiramatsu <tipbot@zytor.com> - 2015-11-23 17:20 +0100
  [PATCH perf/core  13/13] perf: Fix dso__load_sym to put dso Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> - 2015-11-18 07:50 +0100
  [PATCH perf/core 10/13] perf: Fix __dsos__addnew to put dso after  adding it to the list Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> - 2015-11-18 07:50 +0100
    [tip:perf/core] perf tools:   Fix __dsos__addnew to put dso after adding it to the list tip-bot for Masami Hiramatsu <tipbot@zytor.com> - 2015-11-23 17:20 +0100
  [PATCH perf/core 05/13] perf: Fix machine__findnew_module_map to  put registered map Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> - 2015-11-18 07:50 +0100
    Re: [PATCH perf/core 05/13] perf: Fix machine__findnew_module_map to  put registered map Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-11-18 23:40 +0100
    [tip:perf/core] perf machine:   Fix machine__findnew_module_map to put registered map tip-bot for Masami Hiramatsu <tipbot@zytor.com> - 2015-11-23 17:20 +0100
  [PATCH perf/core  09/13] perf: Make dso to use refcnt for debug Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> - 2015-11-18 07:50 +0100
  [PATCH perf/core 02/13] perf: Make perf_exec_path always returns  malloc'd string Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> - 2015-11-18 08:00 +0100
  [PATCH perf/core 03/13] perf: Introduce generic refcount APIs with  debug feature Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> - 2015-11-18 08:00 +0100
    Re: [PATCH perf/core 03/13] perf: Introduce generic refcount APIs  with debug feature Namhyung Kim <namhyung@kernel.org> - 2015-11-20 04:00 +0100
      RE: [PATCH perf/core 03/13] perf: Introduce generic refcount APIs  with debug feature 平松雅巳 / HIRAMATU，MASAMI   <masami.hiramatsu.pt@hitachi.com> - 2015-11-20 05:20 +0100
        Re: [PATCH perf/core 03/13] perf: Introduce generic refcount APIs  with debug feature Namhyung Kim <namhyung@kernel.org> - 2015-11-20 07:00 +0100
  Re: [PATCH perf/core  00/13] perf memory/refcnt leak fixes Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-11-18 13:50 +0100
    RE: [PATCH perf/core  00/13] perf memory/refcnt leak fixes 平松雅巳 / HIRAMATU，MASAMI   <masami.hiramatsu.pt@hitachi.com> - 2015-11-19 04:00 +0100

csiph-web