Messages in this thread | | | From | 平松雅巳 / HIRAMATU,MASAMI <> | Subject | RE: [PATCH perf/core 03/13] perf: Introduce generic refcount APIs with debug feature | Date | Fri, 20 Nov 2015 04:12:06 +0000 |
| |
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"); >> +}
| |