Messages in this thread | | | Date | Fri, 20 Nov 2015 11:52:32 +0900 | From | Namhyung Kim <> | Subject | Re: [PATCH perf/core 03/13] perf: Introduce generic refcount APIs with debug feature |
| |
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"); > +}
| |