Re: [PATCH perf/core 03/13] perf: Introduce generic refcount APIs with debug feature
On Fri, Nov 20, 2015 at 04:12:06AM +, 平松雅巳 / HIRAMATU，MASAMI wrote:
> From: Namhyung Kim [mailto:namhy...@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
> >
> >Looks really useful!
> >
> >Acked-by: Namhyung Kim
> >
> >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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH perf/core 03/13] perf: Introduce generic refcount APIs with debug feature
From: Namhyung Kim [mailto:namhy...@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
>
>Looks really useful!
>
>Acked-by: Namhyung Kim
>
>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");
>> +}
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
Looks really useful!
Acked-by: Namhyung Kim
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");
> +}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
