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

Groups > linux.kernel > #1287008 > unrolled thread

[PATCH perf/core 00/22] perf refcnt debugger API and fixes

Started byMasami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
First post2015-12-09 03:30 +0100
Last post2015-12-11 23:30 +0100
Articles 20 on this page of 30 — 8 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH perf/core  00/22] perf refcnt debugger API and fixes Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> - 2015-12-09 03:30 +0100
    [PATCH perf/core  14/22] perf: Fix dso__load_sym to put dso Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> - 2015-12-09 03:30 +0100
      Re: [PATCH perf/core  14/22] perf: Fix dso__load_sym to put dso Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-12-09 15:20 +0100
        RE: [PATCH perf/core  14/22] perf: Fix dso__load_sym to put dso 平松雅巳 / HIRAMATU，MASAMI   <masami.hiramatsu.pt@hitachi.com> - 2015-12-10 10:00 +0100
          Re: [PATCH perf/core  14/22] perf: Fix dso__load_sym to put dso 'Arnaldo Carvalho de Melo' <acme@kernel.org> - 2015-12-10 20:30 +0100
    [PATCH perf/core 20/22] perf: Fix maps__fixup_overlappings to put  used maps Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> - 2015-12-09 03:30 +0100
      Re: [PATCH perf/core 20/22] perf: Fix maps__fixup_overlappings to  put used maps Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-12-09 16:20 +0100
      [tip:perf/core] perf tools:   Fix maps__fixup_overlappings to put used maps tip-bot for Masami Hiramatsu <tipbot@zytor.com> - 2015-12-10 09:20 +0100
    [PATCH perf/core 04/22] perf refcnt: refcnt shows summary per object Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> - 2015-12-09 03:30 +0100
    [PATCH perf/core 22/22] perf: Fix write_numa_topology to put  cpu_map instead of free Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> - 2015-12-09 03:30 +0100
      Re: [PATCH perf/core 22/22] perf: Fix write_numa_topology to put  cpu_map instead of free Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-12-09 16:30 +0100
      [tip:perf/core] perf tools:   Fix write_numa_topology to put cpu_map instead of free tip-bot for Masami Hiramatsu <tipbot@zytor.com> - 2015-12-10 09:20 +0100
    [PATCH perf/core  05/22] perf: make map to use refcnt Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> - 2015-12-09 03:30 +0100
    [PATCH perf/core 17/22] perf: Fix __machine__addnew_vdso to put dso  after add to dsos Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> - 2015-12-09 03:30 +0100
      Re: [PATCH perf/core 17/22] perf: Fix __machine__addnew_vdso to put  dso after add to dsos Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-12-09 15:40 +0100
    Re: [PATCH perf/core  00/22] perf refcnt debugger API and fixes Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-12-09 14:50 +0100
      Re: [PATCH perf/core  00/22] perf refcnt debugger API and fixes Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2015-12-10 04:40 +0100
      Re: [PATCH perf/core  00/22] perf refcnt debugger API and fixes Namhyung Kim <namhyung@kernel.org> - 2015-12-10 06:00 +0100
        RE: [PATCH perf/core  00/22] perf refcnt debugger API and fixes 平松雅巳 / HIRAMATU，MASAMI   <masami.hiramatsu.pt@hitachi.com> - 2015-12-10 09:40 +0100
      RE: [PATCH perf/core  00/22] perf refcnt debugger API and fixes 平松雅巳 / HIRAMATU，MASAMI   <masami.hiramatsu.pt@hitachi.com> - 2015-12-10 12:10 +0100
        Re: [PATCH perf/core  00/22] perf refcnt debugger API and fixes "Wangnan (F)" <wangnan0@huawei.com> - 2015-12-10 14:00 +0100
          Re: [PATCH perf/core  00/22] perf refcnt debugger API and fixes 'Arnaldo Carvalho de Melo' <acme@kernel.org> - 2015-12-10 16:20 +0100
            Re: [PATCH perf/core  00/22] perf refcnt debugger API and fixes "Wangnan (F)" <wangnan0@huawei.com> - 2015-12-11 03:00 +0100
              RE: [PATCH perf/core  00/22] perf refcnt debugger API and fixes 平松雅巳 / HIRAMATU，MASAMI   <masami.hiramatsu.pt@hitachi.com> - 2015-12-11 03:10 +0100
                Re: [PATCH perf/core  00/22] perf refcnt debugger API and fixes "Wangnan (F)" <wangnan0@huawei.com> - 2015-12-11 03:30 +0100
            RE: [PATCH perf/core  00/22] perf refcnt debugger API and fixes 平松雅巳 / HIRAMATU，MASAMI   <masami.hiramatsu.pt@hitachi.com> - 2015-12-11 03:20 +0100
              Re: [PATCH perf/core  00/22] perf refcnt debugger API and fixes "Wangnan (F)" <wangnan0@huawei.com> - 2015-12-11 03:50 +0100
                Re: [PATCH perf/core  00/22] perf refcnt debugger API and fixes "Wangnan (F)" <wangnan0@huawei.com> - 2015-12-11 04:00 +0100
                RE: [PATCH perf/core  00/22] perf refcnt debugger API and fixes 平松雅巳 / HIRAMATU，MASAMI   <masami.hiramatsu.pt@hitachi.com> - 2015-12-11 05:00 +0100
      Re: [PATCH perf/core  00/22] perf refcnt debugger API and fixes Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-12-11 23:30 +0100

Page 1 of 2  [1] 2  Next page →

#1287008 — [PATCH perf/core 00/22] perf refcnt debugger API and fixes

FromMasami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Date2015-12-09 03:30 +0100
Subject[PATCH perf/core 00/22] perf refcnt debugger API and fixes
Message-ID<qDCwi-25G-3@gated-at.bofh.it>
Hi Arnaldo,

Here is a series of patches for perf refcnt debugger and
some fixes.

In this series I've replaced all atomic reference counters
with the refcnt interface, including dso, map, map_groups,
thread, cpu_map, comm_str, cgroup_sel, thread_map, and
perf_mmap.

  refcnt debugger (or refcnt leak checker)
  ===============

At first, note that this change doesn't affect any compiled
code unless building with REFCNT_DEBUG=1 (see macros in
refcnt.h). So, this feature is only enabled in the debug binary.
But before releasing, we can ensure that all objects are safely
reclaimed before exit in -rc phase.

To use the refcnt debugger, you just build a perf binary with
REFCNT_DEBUG=1 as follows;
  ----
  # make REFCNT_DEBUG=1
  ----
And run the perf command. If the refcnt debugger finds leaks,
it shows the summary of the bugs. Note that if the command does
not use stdio, it doesn't show anything because refcnt debugger
uses pr_debug. Please use --stdio option in such case.

E.g. with the first 13 patches, perf top shows that many objects
are leaked.
  ----
  # ./perf top --stdio
  q
  exiting.
  REFCNT: BUG: Unreclaimed objects found.
  REFCNT: Total 3595 objects are not reclaimed.
    "map" leaks 3334 objects
    "dso" leaks 231 objects
    "thread" leaks 9 objects
    "comm_str" leaks 13 objects
    "map_groups" leaks 8 objects
     To see all backtraces, rerun with -v option
  ----
You can also dump all the backtrace data with -v option, but I
don't recommend you to do it on your console, because it will
be very very long (for example, above dumps 40MB text logs
on your console).

Instead, you can use PERF_REFCNT_DEBUG_FILTER env. var. to focus
on one object, and also use "2>" to redirect stderr output to file.
E.g.
  ----
  # PERF_REFCNT_DEBUG_FILTER=map ./perf top --stdio -v 2> refcnt.log
  q
  exiting.
  # less refcnt.log
  mmap size 528384B
  Looking at the vmlinux_path (8 entries long)
  Using /lib/modules/4.3.0-rc2+/build/vmlinux for symbols
  REFCNT: BUG: Unreclaimed objects found.
  ==== [0] ====
  Unreclaimed map@0x2157dc0
  Refcount +1 => 1 at
  ...
    ./perf() [0x4226fd]
  REFCNT: Total 3229 objects are not reclaimed.
    "map" leaks 3229 objects
  ----

  Bugfixes
  ========

In this series I've also tried to fix some object leaks in perf top
and perf stat.
After applying this series, this reduced (not vanished) to 1/5.
  ----
  # ./perf top --stdio
  q
  exiting.
  REFCNT: BUG: Unreclaimed objects found.
  REFCNT: Total 866 objects are not reclaimed.
    "dso" leaks 213 objects
    "map" leaks 624 objects
    "comm_str" leaks 12 objects
    "thread" leaks 9 objects
    "map_groups" leaks 8 objects
     To see all backtraces, rerun with -v option
  ----

Actually, I'm still not able to fix all of the bugs. It seems that
hists has a bug that hists__delete_entries doesn't delete all the
entries because some entries are on hists->entries but others on
hists->entries_in. And I'm not so sure about hists.c.
Arnaldo, would you have any idea for this bug?


  General refcnt miscodings
  =========================

BTW, while applying this change, I've found that there are refcnt
coding mismatches in those code and most of the bugs come from those
mismatches.

- The reference counter can be initialized by 0 or 1.
 - If 0 is chosen, caller have to get it and free it if failed to
   register appropriately.
 - If 1 is chosen, caller doesn't need to get it, but when exits the
   caller, it has to put it. (except for returning the object itself)
- The application should choose either one as its policy, to avoid
  confusion.

perf tools mixes it up (moreover, it initializes 2 in a case) and
caller usually forgets to put it (it is not surprising, because too
many "put" usually cause SEGV by accessing freed object.)

As far as I can see, cgroup_sel, perf_mmap(this is initialized 0 or 2...),
thread, and comm_str are initialized by 0. Others are initialized by 1.

So, I'd like to suggest that we choose one policy and cleanup the code.
I recommend to use init by 1 policy, because anyway caller has to get
the refcnt. Suppose the below code;

   ----
obj__new() {
	obj = zalloc(sizeof(*obj));
	refcnt__init(obj, refcnt, 0);
	return obj;
}

caller() {
	obj = obj__new();
	if (parent__add_obj(parent, obj) != SUCCESS) {
		free(obj);
	}
}
   ----

At first glance, this looks good. However, if the parent__add_obj() once
gets the obj(refcnt => 1) and fails to add it to parent list by some reason,
it should put the obj(refcnt => 0). This means the obj is already freed at
that point.

Then, caller() shouldn't free obj in error case? No, because parent__add_obj()
can fail before getting the obj :(. Maybe we can handle it by checking return
code, but it is ugly.

If we choose "init by 1", caller always has to put it before returning. But
the coding rule becomes simpler.
   ----
caller() {
	obj = obj__new();
	if (parent__add_obj(parent, obj) != SUCCESS) {
		ret = errorcode;
	}
	obj__put(obj);
	return ret;
}
   ----

Thank you,

---

Masami Hiramatsu (22):
      [v2] perf refcnt: Introduce generic refcount APIs with debug feature
      perf refcnt: Use a hash for refcnt_root
      perf refcnt: Add refcnt debug filter
      perf refcnt: refcnt shows summary per object
      perf: make map to use refcnt
      perf: Make dso to use refcnt for debug
      perf: Make map_groups to use refcnt
      perf: Make thread uses refcnt for debug
      perf: Make cpu_map to use refcnt for debug
      perf: Make comm_str to use refcnt for debug
      perf: Make cgroup_sel to use refcnt for debug
      perf: Make thread_map to use refcnt for debug
      perf: Make perf_mmap to use refcnt for debug
      perf: Fix dso__load_sym to put dso
      perf: Fix map_groups__clone to put cloned map
      perf: Fix __cmd_top and perf_session__process_events to put the idle thread
      perf: Fix __machine__addnew_vdso to put dso after add to dsos
      perf stat: Fix cmd_stat to release cpu_map
      perf: fix hists_evsel to release hists
      perf: Fix maps__fixup_overlappings to put used maps
      perf: Fix machine.vmlinux_maps to make sure to clear the old one
      perf: Fix write_numa_topology to put cpu_map instead of free


 tools/perf/builtin-stat.c    |   11 ++
 tools/perf/builtin-top.c     |    6 +
 tools/perf/config/Makefile   |    5 +
 tools/perf/util/Build        |    1 
 tools/perf/util/cgroup.c     |   11 +-
 tools/perf/util/comm.c       |    8 +
 tools/perf/util/cpumap.c     |   33 +++---
 tools/perf/util/dso.c        |    7 +
 tools/perf/util/evlist.c     |    8 +
 tools/perf/util/header.c     |    2 
 tools/perf/util/hist.c       |   10 ++
 tools/perf/util/machine.c    |    5 +
 tools/perf/util/map.c        |   15 ++-
 tools/perf/util/map.h        |    5 +
 tools/perf/util/refcnt.c     |  229 ++++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/refcnt.h     |   70 +++++++++++++
 tools/perf/util/session.c    |   10 ++
 tools/perf/util/symbol-elf.c |    2 
 tools/perf/util/thread.c     |    7 +
 tools/perf/util/thread_map.c |   28 +++--
 tools/perf/util/vdso.c       |    2 
 21 files changed, 420 insertions(+), 55 deletions(-)
 create mode 100644 tools/perf/util/refcnt.c
 create mode 100644 tools/perf/util/refcnt.h


-- 
Masami HIRAMATSU
Linux Technology Research Center, System Productivity Research Dept.
Center for Technology Innovation - Systems Engineering 
Hitachi, Ltd., Research & Development Group
E-mail: masami.hiramatsu.pt@hitachi.com
--
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/

[toc] | [next] | [standalone]

#1287009 — [PATCH perf/core 14/22] perf: Fix dso__load_sym to put dso

FromMasami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Date2015-12-09 03:30 +0100
Subject[PATCH perf/core 14/22] perf: Fix dso__load_sym to put dso
Message-ID<qDCFZ-295-19@gated-at.bofh.it>
In reply to#1287008
Fix dso__load_sym to put dso because dsos__add already got it.

Refcnt debugger explain the problem:
  ----
  ==== [0] ====
  Unreclaimed dso: 0x19dd200
  Refcount +1 => 1 at
    ./perf(dso__new+0x1ff) [0x4a62df]
    ./perf(dso__load_sym+0xe89) [0x503509]
    ./perf(dso__load_vmlinux+0xbf) [0x4aa77f]
    ./perf(dso__load_vmlinux_path+0x8c) [0x4aa8dc]
    ./perf() [0x50539a]
    ./perf(convert_perf_probe_events+0xd79) [0x50ad39]
    ./perf() [0x45600f]
    ./perf(cmd_probe+0x6c) [0x4566bc]
    ./perf() [0x47abc5]
    ./perf(main+0x610) [0x421f90]
    /lib64/libc.so.6(__libc_start_main+0xf5) [0x7f74dd0efaf5]
    ./perf() [0x4220a9]
  Refcount +1 => 2 at
    ./perf(dso__get+0x34) [0x4a65f4]
    ./perf(map__new2+0x76) [0x4be216]
    ./perf(dso__load_sym+0xee1) [0x503561]
    ./perf(dso__load_vmlinux+0xbf) [0x4aa77f]
    ./perf(dso__load_vmlinux_path+0x8c) [0x4aa8dc]
    ./perf() [0x50539a]
    ./perf(convert_perf_probe_events+0xd79) [0x50ad39]
    ./perf() [0x45600f]
    ./perf(cmd_probe+0x6c) [0x4566bc]
    ./perf() [0x47abc5]
    ./perf(main+0x610) [0x421f90]
    /lib64/libc.so.6(__libc_start_main+0xf5) [0x7f74dd0efaf5]
    ./perf() [0x4220a9]
  Refcount +1 => 3 at
    ./perf(dsos__add+0xf3) [0x4a6bc3]
    ./perf(dso__load_sym+0xfc1) [0x503641]
    ./perf(dso__load_vmlinux+0xbf) [0x4aa77f]
    ./perf(dso__load_vmlinux_path+0x8c) [0x4aa8dc]
    ./perf() [0x50539a]
    ./perf(convert_perf_probe_events+0xd79) [0x50ad39]
    ./perf() [0x45600f]
    ./perf(cmd_probe+0x6c) [0x4566bc]
    ./perf() [0x47abc5]
    ./perf(main+0x610) [0x421f90]
    /lib64/libc.so.6(__libc_start_main+0xf5) [0x7f74dd0efaf5]
    ./perf() [0x4220a9]
  Refcount -1 => 2 at
    ./perf(dso__put+0x2f) [0x4a664f]
    ./perf(map_groups__exit+0xb9) [0x4bee29]
    ./perf(machine__delete+0xb0) [0x4b93d0]
    ./perf(exit_probe_symbol_maps+0x28) [0x506718]
    ./perf() [0x45628a]
    ./perf(cmd_probe+0x6c) [0x4566bc]
    ./perf() [0x47abc5]
    ./perf(main+0x610) [0x421f90]
    /lib64/libc.so.6(__libc_start_main+0xf5) [0x7f74dd0efaf5]
    ./perf() [0x4220a9]
  Refcount -1 => 1 at
    ./perf(dso__put+0x2f) [0x4a664f]
    ./perf(machine__delete+0xfe) [0x4b941e]
    ./perf(exit_probe_symbol_maps+0x28) [0x506718]
    ./perf() [0x45628a]
    ./perf(cmd_probe+0x6c) [0x4566bc]
    ./perf() [0x47abc5]
    ./perf(main+0x610) [0x421f90]
    /lib64/libc.so.6(__libc_start_main+0xf5) [0x7f74dd0efaf5]
    ./perf() [0x4220a9]
  ----
So, in the dso__load_sym, dso is gotten 3 times, by dso__new,
map__new2, and dsos__add. The last 2 is actually released by
map_groups and machine__delete correspondingly. However, the
first reference by dso__new, is never released.

This solves this issue by putting dso right after dsos__add.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
---
 tools/perf/util/symbol-elf.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index 53f1996..a5703ac 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -1045,6 +1045,8 @@ int dso__load_sym(struct dso *dso, struct map *map,
 				/* kmaps already got it */
 				map__put(curr_map);
 				dsos__add(&map->groups->machine->dsos, curr_dso);
+				/* curr_map and machine->dsos already got it */
+				dso__put(curr_dso);
 				dso__set_loaded(curr_dso, map->type);
 			} else
 				curr_dso = curr_map->dso;

--
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/

[toc] | [prev] | [next] | [standalone]

#1287519 — Re: [PATCH perf/core 14/22] perf: Fix dso__load_sym to put dso

FromArnaldo Carvalho de Melo <acme@kernel.org>
Date2015-12-09 15:20 +0100
SubjectRe: [PATCH perf/core 14/22] perf: Fix dso__load_sym to put dso
Message-ID<qDNL4-116-11@gated-at.bofh.it>
In reply to#1287009
Em Wed, Dec 09, 2015 at 11:11:18AM +0900, Masami Hiramatsu escreveu:
> +++ b/tools/perf/util/symbol-elf.c
> @@ -1045,6 +1045,8 @@ int dso__load_sym(struct dso *dso, struct map *map,
>  				/* kmaps already got it */
>  				map__put(curr_map);
>  				dsos__add(&map->groups->machine->dsos, curr_dso);
> +				/* curr_map and machine->dsos already got it */
> +				dso__put(curr_dso);
>  				dso__set_loaded(curr_dso, map->type);
>  			} else
>  				curr_dso = curr_map->dso;

Right, to make the code smaller, how about doing it this way, i.e. drop
the reference once we have that curr_dso object with a ref held by
curr_map, if curr_map doesn't get it, then we don't need and will drop
it anyway:

diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index 53f19968bfa2..84d787074152 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -1026,8 +1026,8 @@ int dso__load_sym(struct dso *dso, struct map *map,
 				curr_dso->long_name_len = dso->long_name_len;
 				curr_map = map__new2(start, curr_dso,
 						     map->type);
+				dso__put(curr_dso);
 				if (curr_map == NULL) {
-					dso__put(curr_dso);
 					goto out_elf_end;
 				}
 				if (adjust_kernel_syms) {
--
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/

[toc] | [prev] | [next] | [standalone]

#1288366 — RE: [PATCH perf/core 14/22] perf: Fix dso__load_sym to put dso

From平松雅巳 / HIRAMATU，MASAMI <masami.hiramatsu.pt@hitachi.com>
Date2015-12-10 10:00 +0100
SubjectRE: [PATCH perf/core 14/22] perf: Fix dso__load_sym to put dso
Message-ID<qE5eV-3P0-1@gated-at.bofh.it>
In reply to#1287519
From: Arnaldo Carvalho de Melo [mailto:acme@kernel.org]
>
>Em Wed, Dec 09, 2015 at 11:11:18AM +0900, Masami Hiramatsu escreveu:
>> +++ b/tools/perf/util/symbol-elf.c
>> @@ -1045,6 +1045,8 @@ int dso__load_sym(struct dso *dso, struct map *map,
>>  				/* kmaps already got it */
>>  				map__put(curr_map);
>>  				dsos__add(&map->groups->machine->dsos, curr_dso);
>> +				/* curr_map and machine->dsos already got it */
>> +				dso__put(curr_dso);
>>  				dso__set_loaded(curr_dso, map->type);
>>  			} else
>>  				curr_dso = curr_map->dso;
>
>Right, to make the code smaller, how about doing it this way, i.e. drop
>the reference once we have that curr_dso object with a ref held by
>curr_map, if curr_map doesn't get it, then we don't need and will drop
>it anyway:

But as above code, curr_dso is passed to dsos__add after curr_map is put.
Even if the curr_map is hold by kmaps, isn't the kmaps controlled by other pthreads?
If no, I'm OK if we move above dsos__add() before map__put() for safety, because
curr_dso is held by curr_map at that point.

Thank you,

>
>diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
>index 53f19968bfa2..84d787074152 100644
>--- a/tools/perf/util/symbol-elf.c
>+++ b/tools/perf/util/symbol-elf.c
>@@ -1026,8 +1026,8 @@ int dso__load_sym(struct dso *dso, struct map *map,
> 				curr_dso->long_name_len = dso->long_name_len;
> 				curr_map = map__new2(start, curr_dso,
> 						     map->type);
>+				dso__put(curr_dso);
> 				if (curr_map == NULL) {
>-					dso__put(curr_dso);
> 					goto out_elf_end;
> 				}
> 				if (adjust_kernel_syms) {
--
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/

[toc] | [prev] | [next] | [standalone]

#1288866 — Re: [PATCH perf/core 14/22] perf: Fix dso__load_sym to put dso

From'Arnaldo Carvalho de Melo' <acme@kernel.org>
Date2015-12-10 20:30 +0100
SubjectRe: [PATCH perf/core 14/22] perf: Fix dso__load_sym to put dso
Message-ID<qEf4C-1WQ-13@gated-at.bofh.it>
In reply to#1288366
Em Thu, Dec 10, 2015 at 08:52:46AM +0000, 平松雅巳 / HIRAMATU，MASAMI escreveu:
> From: Arnaldo Carvalho de Melo [mailto:acme@kernel.org]
> >
> >Em Wed, Dec 09, 2015 at 11:11:18AM +0900, Masami Hiramatsu escreveu:
> >> +++ b/tools/perf/util/symbol-elf.c
> >> @@ -1045,6 +1045,8 @@ int dso__load_sym(struct dso *dso, struct map *map,
> >>  				/* kmaps already got it */
> >>  				map__put(curr_map);
> >>  				dsos__add(&map->groups->machine->dsos, curr_dso);
> >> +				/* curr_map and machine->dsos already got it */
> >> +				dso__put(curr_dso);
> >>  				dso__set_loaded(curr_dso, map->type);
> >>  			} else
> >>  				curr_dso = curr_map->dso;
> >
> >Right, to make the code smaller, how about doing it this way, i.e. drop
> >the reference once we have that curr_dso object with a ref held by
> >curr_map, if curr_map doesn't get it, then we don't need and will drop
> >it anyway:
> 
> But as above code, curr_dso is passed to dsos__add after curr_map is put.
> Even if the curr_map is hold by kmaps, isn't the kmaps controlled by other pthreads?
> If no, I'm OK if we move above dsos__add() before map__put() for safety, because
> curr_dso is held by curr_map at that point.

Good catch, so I'll do as you suggest and move dsos__add() to before we
drop that map reference, as then we're sure that we hold it and it holds
the dso.
 
> Thank you,
> 
> >
> >diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
> >index 53f19968bfa2..84d787074152 100644
> >--- a/tools/perf/util/symbol-elf.c
> >+++ b/tools/perf/util/symbol-elf.c
> >@@ -1026,8 +1026,8 @@ int dso__load_sym(struct dso *dso, struct map *map,
> > 				curr_dso->long_name_len = dso->long_name_len;
> > 				curr_map = map__new2(start, curr_dso,
> > 						     map->type);
> >+				dso__put(curr_dso);
> > 				if (curr_map == NULL) {
> >-					dso__put(curr_dso);
> > 					goto out_elf_end;
> > 				}
> > 				if (adjust_kernel_syms) {
--
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/

[toc] | [prev] | [next] | [standalone]

#1287010 — [PATCH perf/core 20/22] perf: Fix maps__fixup_overlappings to put used maps

FromMasami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Date2015-12-09 03:30 +0100
Subject[PATCH perf/core 20/22] perf: Fix maps__fixup_overlappings to put used maps
Message-ID<qDCFZ-295-25@gated-at.bofh.it>
In reply to#1287008
Since the __map_groups__insert got the given map, we don't
need to keep it. So put the maps.

Refcnt debugger shows that the map_groups__fixup_overlappings
got a map twice but the group released it once. This pattern
usually indicates the leak happens in caller site.
  ----
  ==== [0] ====
  Unreclaimed map@0x39d3ae0
  Refcount +1 => 1 at
    ./perf(map_groups__fixup_overlappings+0x335) [0x4c1865]
    ./perf(thread__insert_map+0x30) [0x4c8e00]
    ./perf(machine__process_mmap2_event+0x106) [0x4bd876]
    ./perf() [0x4c378e]
    ./perf() [0x4c4393]
    ./perf(perf_session__process_events+0x38a) [0x4c654a]
    ./perf(cmd_record+0xe24) [0x42fc94]
    ./perf() [0x47b745]
    ./perf(main+0x617) [0x422547]
    /lib64/libc.so.6(__libc_start_main+0xf5) [0x7f2eca2deaf5]
    ./perf() [0x4226bd]
  Refcount +1 => 2 at
    ./perf(map_groups__fixup_overlappings+0x3c5) [0x4c18f5]
    ./perf(thread__insert_map+0x30) [0x4c8e00]
    ./perf(machine__process_mmap2_event+0x106) [0x4bd876]
    ./perf() [0x4c378e]
    ./perf() [0x4c4393]
    ./perf(perf_session__process_events+0x38a) [0x4c654a]
    ./perf(cmd_record+0xe24) [0x42fc94]
    ./perf() [0x47b745]
    ./perf(main+0x617) [0x422547]
    /lib64/libc.so.6(__libc_start_main+0xf5) [0x7f2eca2deaf5]
    ./perf() [0x4226bd]
  Refcount -1 => 1 at
    ./perf(map_groups__exit+0x92) [0x4c0962]
    ./perf(map_groups__put+0x60) [0x4c0bc0]
    ./perf(thread__put+0x90) [0x4c8a40]
    ./perf(machine__delete_threads+0x7e) [0x4bad9e]
    ./perf(perf_session__delete+0x4f) [0x4c499f]
    ./perf(cmd_record+0xb6d) [0x42f9dd]
    ./perf() [0x47b745]
    ./perf(main+0x617) [0x422547]
    /lib64/libc.so.6(__libc_start_main+0xf5) [0x7f2eca2deaf5]
    ./perf() [0x4226bd]
  ----

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
---
 tools/perf/util/map.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index 03b7297..89be9c5 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -693,6 +693,7 @@ static int maps__fixup_overlappings(struct maps *maps, struct map *map, FILE *fp
 			__map_groups__insert(pos->groups, before);
 			if (verbose >= 2)
 				map__fprintf(before, fp);
+			map__put(before);
 		}
 
 		if (map->end < pos->end) {
@@ -707,6 +708,7 @@ static int maps__fixup_overlappings(struct maps *maps, struct map *map, FILE *fp
 			__map_groups__insert(pos->groups, after);
 			if (verbose >= 2)
 				map__fprintf(after, fp);
+			map__put(after);
 		}
 put_map:
 		map__put(pos);

--
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/

[toc] | [prev] | [next] | [standalone]

#1287551 — Re: [PATCH perf/core 20/22] perf: Fix maps__fixup_overlappings to put used maps

FromArnaldo Carvalho de Melo <acme@kernel.org>
Date2015-12-09 16:20 +0100
SubjectRe: [PATCH perf/core 20/22] perf: Fix maps__fixup_overlappings to put used maps
Message-ID<qDOH7-1CS-1@gated-at.bofh.it>
In reply to#1287010
Em Wed, Dec 09, 2015 at 11:11:31AM +0900, Masami Hiramatsu escreveu:
> Since the __map_groups__insert got the given map, we don't
> need to keep it. So put the maps.
> 
> Refcnt debugger shows that the map_groups__fixup_overlappings
> got a map twice but the group released it once. This pattern
> usually indicates the leak happens in caller site.

Thanks, applied!

- Arnaldo

>   ----
>   ==== [0] ====
>   Unreclaimed map@0x39d3ae0
>   Refcount +1 => 1 at
>     ./perf(map_groups__fixup_overlappings+0x335) [0x4c1865]
>     ./perf(thread__insert_map+0x30) [0x4c8e00]
>     ./perf(machine__process_mmap2_event+0x106) [0x4bd876]
>     ./perf() [0x4c378e]
>     ./perf() [0x4c4393]
>     ./perf(perf_session__process_events+0x38a) [0x4c654a]
>     ./perf(cmd_record+0xe24) [0x42fc94]
>     ./perf() [0x47b745]
>     ./perf(main+0x617) [0x422547]
>     /lib64/libc.so.6(__libc_start_main+0xf5) [0x7f2eca2deaf5]
>     ./perf() [0x4226bd]
>   Refcount +1 => 2 at
>     ./perf(map_groups__fixup_overlappings+0x3c5) [0x4c18f5]
>     ./perf(thread__insert_map+0x30) [0x4c8e00]
>     ./perf(machine__process_mmap2_event+0x106) [0x4bd876]
>     ./perf() [0x4c378e]
>     ./perf() [0x4c4393]
>     ./perf(perf_session__process_events+0x38a) [0x4c654a]
>     ./perf(cmd_record+0xe24) [0x42fc94]
>     ./perf() [0x47b745]
>     ./perf(main+0x617) [0x422547]
>     /lib64/libc.so.6(__libc_start_main+0xf5) [0x7f2eca2deaf5]
>     ./perf() [0x4226bd]
>   Refcount -1 => 1 at
>     ./perf(map_groups__exit+0x92) [0x4c0962]
>     ./perf(map_groups__put+0x60) [0x4c0bc0]
>     ./perf(thread__put+0x90) [0x4c8a40]
>     ./perf(machine__delete_threads+0x7e) [0x4bad9e]
>     ./perf(perf_session__delete+0x4f) [0x4c499f]
>     ./perf(cmd_record+0xb6d) [0x42f9dd]
>     ./perf() [0x47b745]
>     ./perf(main+0x617) [0x422547]
>     /lib64/libc.so.6(__libc_start_main+0xf5) [0x7f2eca2deaf5]
>     ./perf() [0x4226bd]
>   ----
> 
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> ---
>  tools/perf/util/map.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
> index 03b7297..89be9c5 100644
> --- a/tools/perf/util/map.c
> +++ b/tools/perf/util/map.c
> @@ -693,6 +693,7 @@ static int maps__fixup_overlappings(struct maps *maps, struct map *map, FILE *fp
>  			__map_groups__insert(pos->groups, before);
>  			if (verbose >= 2)
>  				map__fprintf(before, fp);
> +			map__put(before);
>  		}
>  
>  		if (map->end < pos->end) {
> @@ -707,6 +708,7 @@ static int maps__fixup_overlappings(struct maps *maps, struct map *map, FILE *fp
>  			__map_groups__insert(pos->groups, after);
>  			if (verbose >= 2)
>  				map__fprintf(after, fp);
> +			map__put(after);
>  		}
>  put_map:
>  		map__put(pos);
--
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/

[toc] | [prev] | [next] | [standalone]

#1288340 — [tip:perf/core] perf tools: Fix maps__fixup_overlappings to put used maps

Fromtip-bot for Masami Hiramatsu <tipbot@zytor.com>
Date2015-12-10 09:20 +0100
Subject[tip:perf/core] perf tools: Fix maps__fixup_overlappings to put used maps
Message-ID<qE4Cf-3wi-31@gated-at.bofh.it>
In reply to#1287010
Commit-ID:  d91130e90a005876b488b6d52b743149d95b4a59
Gitweb:     http://git.kernel.org/tip/d91130e90a005876b488b6d52b743149d95b4a59
Author:     Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
AuthorDate: Wed, 9 Dec 2015 11:11:31 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 9 Dec 2015 13:42:00 -0300

perf tools: Fix maps__fixup_overlappings to put used maps

Since the __map_groups__insert got the given map, we don't need to keep
it. So put the maps.

Refcnt debugger shows that map_groups__fixup_overlappings() got a map
twice but the group released it just once. This pattern usually
indicates the leak happens in caller site.

  ----
  ==== [0] ====
  Unreclaimed map@0x39d3ae0
  Refcount +1 => 1 at
    ./perf(map_groups__fixup_overlappings+0x335) [0x4c1865]
    ./perf(thread__insert_map+0x30) [0x4c8e00]
    ./perf(machine__process_mmap2_event+0x106) [0x4bd876]
    ./perf() [0x4c378e]
    ./perf() [0x4c4393]
    ./perf(perf_session__process_events+0x38a) [0x4c654a]
    ./perf(cmd_record+0xe24) [0x42fc94]
    ./perf() [0x47b745]
    ./perf(main+0x617) [0x422547]
    /lib64/libc.so.6(__libc_start_main+0xf5) [0x7f2eca2deaf5]
    ./perf() [0x4226bd]
  Refcount +1 => 2 at
    ./perf(map_groups__fixup_overlappings+0x3c5) [0x4c18f5]
    ./perf(thread__insert_map+0x30) [0x4c8e00]
    ./perf(machine__process_mmap2_event+0x106) [0x4bd876]
    ./perf() [0x4c378e]
    ./perf() [0x4c4393]
    ./perf(perf_session__process_events+0x38a) [0x4c654a]
    ./perf(cmd_record+0xe24) [0x42fc94]
    ./perf() [0x47b745]
    ./perf(main+0x617) [0x422547]
    /lib64/libc.so.6(__libc_start_main+0xf5) [0x7f2eca2deaf5]
    ./perf() [0x4226bd]
  Refcount -1 => 1 at
    ./perf(map_groups__exit+0x92) [0x4c0962]
    ./perf(map_groups__put+0x60) [0x4c0bc0]
    ./perf(thread__put+0x90) [0x4c8a40]
    ./perf(machine__delete_threads+0x7e) [0x4bad9e]
    ./perf(perf_session__delete+0x4f) [0x4c499f]
    ./perf(cmd_record+0xb6d) [0x42f9dd]
    ./perf() [0x47b745]
    ./perf(main+0x617) [0x422547]
    /lib64/libc.so.6(__libc_start_main+0xf5) [0x7f2eca2deaf5]
    ./perf() [0x4226bd]
  ----

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/20151209021131.10245.41485.stgit@localhost.localdomain
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/map.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index 7b1c720..171b6d1 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -691,6 +691,7 @@ static int maps__fixup_overlappings(struct maps *maps, struct map *map, FILE *fp
 			__map_groups__insert(pos->groups, before);
 			if (verbose >= 2)
 				map__fprintf(before, fp);
+			map__put(before);
 		}
 
 		if (map->end < pos->end) {
@@ -705,6 +706,7 @@ static int maps__fixup_overlappings(struct maps *maps, struct map *map, FILE *fp
 			__map_groups__insert(pos->groups, after);
 			if (verbose >= 2)
 				map__fprintf(after, fp);
+			map__put(after);
 		}
 put_map:
 		map__put(pos);
--
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/

[toc] | [prev] | [next] | [standalone]

#1287011 — [PATCH perf/core 04/22] perf refcnt: refcnt shows summary per object

FromMasami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Date2015-12-09 03:30 +0100
Subject[PATCH perf/core 04/22] perf refcnt: refcnt shows summary per object
Message-ID<qDCFZ-295-33@gated-at.bofh.it>
In reply to#1287008
Report refcnt per object(class) summary numbers if
any leaks exist, not only the total number.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
---
 tools/perf/util/refcnt.c |   44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/tools/perf/util/refcnt.c b/tools/perf/util/refcnt.c
index e83e9a8..5348406 100644
--- a/tools/perf/util/refcnt.c
+++ b/tools/perf/util/refcnt.c
@@ -157,8 +157,48 @@ static void pr_refcnt_object(struct refcnt_object *ref)
 		pr_refcnt_buffer(buf);
 }
 
+#define REFCNT_STAT_NUM	16
+static struct refcnt_stat_entry {
+	int counter;
+	const char *name;
+} stat_entry[REFCNT_STAT_NUM] = { {.name = "(others)"}, };
+
+static struct refcnt_stat_entry *refcnt_stat__find_entry(const char *name)
+{
+	int i;
+
+	if (!name)
+		goto last;
+
+	for (i = 1; i < REFCNT_STAT_NUM; i++)
+		if (stat_entry[i].name) {
+			if (strcmp(stat_entry[i].name, name) == 0)
+				return &stat_entry[i];
+		} else {
+			stat_entry[i].name = name;
+			return &stat_entry[i];
+		}
+last:
+	/* Here, it shorts the slot, let it fold to others */
+	return &stat_entry[0];
+}
+
+static void pr_refcnt_stat(void)
+{
+	int i;
+
+	for (i = 1; i < REFCNT_STAT_NUM && stat_entry[i].name; i++)
+		pr_warning("  \"%s\" leaks %d objects\n",
+			   stat_entry[i].name, stat_entry[i].counter);
+
+	if (stat_entry[0].counter)
+		pr_warning("  And others leak %d objects\n",
+			stat_entry[0].counter);
+}
+
 static void  __attribute__((destructor)) refcnt__dump_unreclaimed(void)
 {
+	struct refcnt_stat_entry *ent;
 	struct refcnt_object *ref, *n;
 	int h, i = 0;
 
@@ -174,11 +214,15 @@ found:
 				pr_debug("==== [%d] ====\n", i);
 				pr_refcnt_object(ref);
 			}
+			ent = refcnt_stat__find_entry(ref->name);
+			if (ent)
+				ent->counter++;
 			refcnt_object__delete(ref);
 			i++;
 		}
 
 	pr_warning("REFCNT: Total %d objects are not reclaimed.\n", i);
+	pr_refcnt_stat();
 	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/

[toc] | [prev] | [next] | [standalone]

#1287012 — [PATCH perf/core 22/22] perf: Fix write_numa_topology to put cpu_map instead of free

FromMasami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Date2015-12-09 03:30 +0100
Subject[PATCH perf/core 22/22] perf: Fix write_numa_topology to put cpu_map instead of free
Message-ID<qDCFZ-295-27@gated-at.bofh.it>
In reply to#1287008
Fix write_numa_topology to put cpu_map instead of free because
cpu_map is managed based on refcnt.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
---
 tools/perf/util/header.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 4383800..5ac7bdb 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -724,7 +724,7 @@ static int write_numa_topology(int fd, struct perf_header *h __maybe_unused,
 done:
 	free(buf);
 	fclose(fp);
-	free(node_map);
+	cpu_map__put(node_map);
 	return ret;
 }
 

--
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/

[toc] | [prev] | [next] | [standalone]

#1287564 — Re: [PATCH perf/core 22/22] perf: Fix write_numa_topology to put cpu_map instead of free

FromArnaldo Carvalho de Melo <acme@kernel.org>
Date2015-12-09 16:30 +0100
SubjectRe: [PATCH perf/core 22/22] perf: Fix write_numa_topology to put cpu_map instead of free
Message-ID<qDOQN-1FV-1@gated-at.bofh.it>
In reply to#1287012
Em Wed, Dec 09, 2015 at 11:11:35AM +0900, Masami Hiramatsu escreveu:
> Fix write_numa_topology to put cpu_map instead of free because
> cpu_map is managed based on refcnt.

thanks, applied!
 
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> ---
>  tools/perf/util/header.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index 4383800..5ac7bdb 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -724,7 +724,7 @@ static int write_numa_topology(int fd, struct perf_header *h __maybe_unused,
>  done:
>  	free(buf);
>  	fclose(fp);
> -	free(node_map);
> +	cpu_map__put(node_map);
>  	return ret;
>  }
>  
--
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/

[toc] | [prev] | [next] | [standalone]

#1288338 — [tip:perf/core] perf tools: Fix write_numa_topology to put cpu_map instead of free

Fromtip-bot for Masami Hiramatsu <tipbot@zytor.com>
Date2015-12-10 09:20 +0100
Subject[tip:perf/core] perf tools: Fix write_numa_topology to put cpu_map instead of free
Message-ID<qE4Ce-3wi-25@gated-at.bofh.it>
In reply to#1287012
Commit-ID:  5191d887681dd34ba3993a438d5746378952885a
Gitweb:     http://git.kernel.org/tip/5191d887681dd34ba3993a438d5746378952885a
Author:     Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
AuthorDate: Wed, 9 Dec 2015 11:11:35 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 9 Dec 2015 13:42:01 -0300

perf tools: Fix write_numa_topology to put cpu_map instead of free

Fix write_numa_topology to put cpu_map instead of free because cpu_map
is managed based on refcnt.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/20151209021135.10245.79046.stgit@localhost.localdomain
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/header.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 4383800..5ac7bdb 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -724,7 +724,7 @@ static int write_numa_topology(int fd, struct perf_header *h __maybe_unused,
 done:
 	free(buf);
 	fclose(fp);
-	free(node_map);
+	cpu_map__put(node_map);
 	return ret;
 }
 
--
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/

[toc] | [prev] | [next] | [standalone]

#1287013 — [PATCH perf/core 05/22] perf: make map to use refcnt

FromMasami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Date2015-12-09 03:30 +0100
Subject[PATCH perf/core 05/22] perf: make map to use refcnt
Message-ID<qDCFZ-295-29@gated-at.bofh.it>
In reply to#1287008
Make 'map' object to use refcnt interface for debug.
This can find refcnt related memory leaks.
E.g.

  ----
  ./perf probe vfs_read
  Added new event:
    probe:vfs_read       (on vfs_read)

  You can now use it in all perf tools, such as:

          perf record -e probe:vfs_read -aR sleep 1

  REFCNT: BUG: Unreclaimed objects found.
  REFCNT: Total 76 objects are not reclaimed.
     To see all backtraces, rerun with -v option
  ----

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
---
 Changes from v1:
  - Update refcnt__init() call to pass initial value.
---
 tools/perf/util/map.c |    7 ++++---
 tools/perf/util/map.h |    3 ++-
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index 93d9f1c..72fcc54 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -138,7 +138,7 @@ void map__init(struct map *map, enum map_type type,
 	RB_CLEAR_NODE(&map->rb_node);
 	map->groups   = NULL;
 	map->erange_warned = false;
-	atomic_set(&map->refcnt, 1);
+	refcnt__init(map, refcnt, 1);
 }
 
 struct map *map__new(struct machine *machine, u64 start, u64 len,
@@ -241,6 +241,7 @@ bool __map__is_kernel(const struct map *map)
 static void map__exit(struct map *map)
 {
 	BUG_ON(!RB_EMPTY_NODE(&map->rb_node));
+	refcnt__exit(map, refcnt);
 	dso__zput(map->dso);
 }
 
@@ -252,7 +253,7 @@ void map__delete(struct map *map)
 
 void map__put(struct map *map)
 {
-	if (map && atomic_dec_and_test(&map->refcnt))
+	if (map && refcnt__put(map, refcnt))
 		map__delete(map);
 }
 
@@ -353,7 +354,7 @@ struct map *map__clone(struct map *from)
 	struct map *map = memdup(from, sizeof(*map));
 
 	if (map != NULL) {
-		atomic_set(&map->refcnt, 1);
+		refcnt__init(map, refcnt, 1);
 		RB_CLEAR_NODE(&map->rb_node);
 		dso__get(map->dso);
 		map->groups = NULL;
diff --git a/tools/perf/util/map.h b/tools/perf/util/map.h
index 7309d64..fcdc7d6 100644
--- a/tools/perf/util/map.h
+++ b/tools/perf/util/map.h
@@ -9,6 +9,7 @@
 #include <stdio.h>
 #include <stdbool.h>
 #include <linux/types.h>
+#include "refcnt.h"
 
 enum map_type {
 	MAP__FUNCTION = 0,
@@ -153,7 +154,7 @@ struct map *map__clone(struct map *map);
 static inline struct map *map__get(struct map *map)
 {
 	if (map)
-		atomic_inc(&map->refcnt);
+		refcnt__get(map, refcnt);
 	return map;
 }
 

--
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/

[toc] | [prev] | [next] | [standalone]

#1287014 — [PATCH perf/core 17/22] perf: Fix __machine__addnew_vdso to put dso after add to dsos

FromMasami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Date2015-12-09 03:30 +0100
Subject[PATCH perf/core 17/22] perf: Fix __machine__addnew_vdso to put dso after add to dsos
Message-ID<qDCFZ-295-31@gated-at.bofh.it>
In reply to#1287008
Fix __machine__addnew_vdso to put dso after add to dsos because
the dso is already gotten by the dsos via __dsos__add().

This function is called finally from machine__findnew_vdso()
which locks machine->dsos.lock. And before unlock it, the
function gets the dso's refcnt. Thus we can ensure that the
dso is not removed from the machine while this operation,
and we don't need to get the dso except for the machine->dsos.

refcnt debugger shows:
  -----
  $ ./perf top --stdio -v	(note: run by non-root user)
  [...]
  ==== [3] ====
  Unreclaimed dso@0x27a0a30
  Refcount +1 => 1 at
    ./perf(dso__new+0x2bc) [0x4a778c]
    ./perf(machine__findnew_vdso+0x272) [0x4e8792]
    ./perf(map__new+0x2db) [0x4bfb4b]
    ./perf(machine__process_mmap2_event+0xf3) [0x4bda33]
    ./perf(perf_event__synthesize_mmap_events+0x364) [0x484e74]
    ./perf(perf_event__synthesize_threads+0x3ee) [0x48583e]
    ./perf(cmd_top+0xdc2) [0x43cfb2]
    ./perf() [0x47ba35]
    ./perf(main+0x617) [0x4225b7]
    /lib64/libc.so.6(__libc_start_main+0xf5) [0x7f2b01387af5]
    ./perf() [0x42272d]
  Refcount +1 => 2 at
    ./perf(machine__findnew_vdso+0x289) [0x4e87a9]
    ./perf(map__new+0x2db) [0x4bfb4b]
    ./perf(machine__process_mmap2_event+0xf3) [0x4bda33]
    ./perf(perf_event__synthesize_mmap_events+0x364) [0x484e74]
    ./perf(perf_event__synthesize_threads+0x3ee) [0x48583e]
    ./perf(cmd_top+0xdc2) [0x43cfb2]
    ./perf() [0x47ba35]
    ./perf(main+0x617) [0x4225b7]
    /lib64/libc.so.6(__libc_start_main+0xf5) [0x7f2b01387af5]
    ./perf() [0x42272d]
  Refcount +1 => 3 at
    ./perf(dso__get+0x32) [0x4a7b52]
    ./perf(machine__findnew_vdso+0xc1) [0x4e85e1]
    ./perf(map__new+0x2db) [0x4bfb4b]
    ./perf(machine__process_mmap2_event+0xf3) [0x4bda33]
    ./perf(perf_event__synthesize_mmap_events+0x364) [0x484e74]
    ./perf(perf_event__synthesize_threads+0x3ee) [0x48583e]
    ./perf(cmd_top+0xdc2) [0x43cfb2]
    ./perf() [0x47ba35]
    ./perf(main+0x617) [0x4225b7]
    /lib64/libc.so.6(__libc_start_main+0xf5) [0x7f2b01387af5]
    ./perf() [0x42272d]
  [...]
  -----

The log shows that the machine__findnew_vdso gets a dso
so many unnaturally. I've traced the code and found this
bug.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
---
 tools/perf/util/vdso.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/perf/util/vdso.c b/tools/perf/util/vdso.c
index 44d440d..fea0d18 100644
--- a/tools/perf/util/vdso.c
+++ b/tools/perf/util/vdso.c
@@ -130,6 +130,8 @@ static struct dso *__machine__addnew_vdso(struct machine *machine, const char *s
 		__dsos__add(&machine->dsos, dso);
 		dso__set_long_name(dso, long_name, false);
 	}
+	/* Put the dso here because it is already gotten by __dsos__add */
+	dso__put(dso);
 
 	return dso;
 }

--
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/

[toc] | [prev] | [next] | [standalone]

#1287531 — Re: [PATCH perf/core 17/22] perf: Fix __machine__addnew_vdso to put dso after add to dsos

FromArnaldo Carvalho de Melo <acme@kernel.org>
Date2015-12-09 15:40 +0100
SubjectRe: [PATCH perf/core 17/22] perf: Fix __machine__addnew_vdso to put dso after add to dsos
Message-ID<qDO4q-1aj-21@gated-at.bofh.it>
In reply to#1287014
Em Wed, Dec 09, 2015 at 11:11:25AM +0900, Masami Hiramatsu escreveu:
> Fix __machine__addnew_vdso to put dso after add to dsos because
> the dso is already gotten by the dsos via __dsos__add().
> 
> This function is called finally from machine__findnew_vdso()
> which locks machine->dsos.lock. And before unlock it, the
> function gets the dso's refcnt. Thus we can ensure that the
> dso is not removed from the machine while this operation,
> and we don't need to get the dso except for the machine->dsos.
> 
> refcnt debugger shows:
>   -----
>   $ ./perf top --stdio -v	(note: run by non-root user)
>   [...]
>   ==== [3] ====
>   Unreclaimed dso@0x27a0a30
>   Refcount +1 => 1 at
>     ./perf(dso__new+0x2bc) [0x4a778c]
>     ./perf(machine__findnew_vdso+0x272) [0x4e8792]
>     ./perf(map__new+0x2db) [0x4bfb4b]
>     ./perf(machine__process_mmap2_event+0xf3) [0x4bda33]
>     ./perf(perf_event__synthesize_mmap_events+0x364) [0x484e74]
>     ./perf(perf_event__synthesize_threads+0x3ee) [0x48583e]
>     ./perf(cmd_top+0xdc2) [0x43cfb2]
>     ./perf() [0x47ba35]
>     ./perf(main+0x617) [0x4225b7]
>     /lib64/libc.so.6(__libc_start_main+0xf5) [0x7f2b01387af5]
>     ./perf() [0x42272d]
>   Refcount +1 => 2 at
>     ./perf(machine__findnew_vdso+0x289) [0x4e87a9]
>     ./perf(map__new+0x2db) [0x4bfb4b]
>     ./perf(machine__process_mmap2_event+0xf3) [0x4bda33]
>     ./perf(perf_event__synthesize_mmap_events+0x364) [0x484e74]
>     ./perf(perf_event__synthesize_threads+0x3ee) [0x48583e]
>     ./perf(cmd_top+0xdc2) [0x43cfb2]
>     ./perf() [0x47ba35]
>     ./perf(main+0x617) [0x4225b7]
>     /lib64/libc.so.6(__libc_start_main+0xf5) [0x7f2b01387af5]
>     ./perf() [0x42272d]
>   Refcount +1 => 3 at
>     ./perf(dso__get+0x32) [0x4a7b52]
>     ./perf(machine__findnew_vdso+0xc1) [0x4e85e1]
>     ./perf(map__new+0x2db) [0x4bfb4b]
>     ./perf(machine__process_mmap2_event+0xf3) [0x4bda33]
>     ./perf(perf_event__synthesize_mmap_events+0x364) [0x484e74]
>     ./perf(perf_event__synthesize_threads+0x3ee) [0x48583e]
>     ./perf(cmd_top+0xdc2) [0x43cfb2]
>     ./perf() [0x47ba35]
>     ./perf(main+0x617) [0x4225b7]
>     /lib64/libc.so.6(__libc_start_main+0xf5) [0x7f2b01387af5]
>     ./perf() [0x42272d]
>   [...]
>   -----
> 
> The log shows that the machine__findnew_vdso gets a dso
> so many unnaturally. I've traced the code and found this
> bug.
> 
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> ---
>  tools/perf/util/vdso.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tools/perf/util/vdso.c b/tools/perf/util/vdso.c
> index 44d440d..fea0d18 100644
> --- a/tools/perf/util/vdso.c
> +++ b/tools/perf/util/vdso.c
> @@ -130,6 +130,8 @@ static struct dso *__machine__addnew_vdso(struct machine *machine, const char *s
>  		__dsos__add(&machine->dsos, dso);
>  		dso__set_long_name(dso, long_name, false);
>  	}
> +	/* Put the dso here because it is already gotten by __dsos__add */
> +	dso__put(dso);
>  
>  	return dso;
>  }

We cannot put it here, because we're returning a pointer to it, so,
whoever receives this pointer, receives a recfount with it, that it, in
turn, should put.

And indeed, this dso adding code is confusing, will have to look at it
harder :-\

- Arnaldo
--
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/

[toc] | [prev] | [next] | [standalone]

#1287507

FromArnaldo Carvalho de Melo <acme@kernel.org>
Date2015-12-09 14:50 +0100
Message-ID<qDNi2-Bz-17@gated-at.bofh.it>
In reply to#1287008
Em Wed, Dec 09, 2015 at 11:10:48AM +0900, Masami Hiramatsu escreveu:
> Hi Arnaldo,
> 
> Here is a series of patches for perf refcnt debugger and
> some fixes.
> 
> In this series I've replaced all atomic reference counters
> with the refcnt interface, including dso, map, map_groups,
> thread, cpu_map, comm_str, cgroup_sel, thread_map, and
> perf_mmap.
> 
>   refcnt debugger (or refcnt leak checker)
>   ===============
> 
> At first, note that this change doesn't affect any compiled
> code unless building with REFCNT_DEBUG=1 (see macros in
> refcnt.h). So, this feature is only enabled in the debug binary.
> But before releasing, we can ensure that all objects are safely
> reclaimed before exit in -rc phase.

That helps and is finding bugs and is really great stuff, thank you!

But I wonder if we couldn't get the same results on an unmodified binary
by using things like 'perf probe', the BPF code we're introducing, have
you thought about this possibility?

I.e. trying to use 'perf probe' to do this would help in using the same
technique in other code bases where we can't change the sources, etc.

For perf we could perhaps use a 'noinline' on the __get/__put
operations, so that we could have the right places to hook using
uprobes, other codebases would have to rely in the 'perf probe'
infrastructure that knows where inlines were expanded, etc.

Such a toold could work like:

   perf dbgrefcnt ~/bin/perf thread 

And it would look up thread__get and thread__put(), create an eBPF map
where to store the needed tracking data structures, and use the same
techniques you used, asking for backtraces using the perf
infrastructure, etc.

We would be using this opportunity to improve the 'perf probe' and the
eBPF infrastructures we're putting in place, and having something that
could be used in other codebases, not just perf.

Comments about other issues are below:
 
> To use the refcnt debugger, you just build a perf binary with
> REFCNT_DEBUG=1 as follows;
>   ----
>   # make REFCNT_DEBUG=1
>   ----
> And run the perf command. If the refcnt debugger finds leaks,
> it shows the summary of the bugs. Note that if the command does
> not use stdio, it doesn't show anything because refcnt debugger
> uses pr_debug. Please use --stdio option in such case.

It would be interesting to have this writing to a file instead, so that
we could use the same technique for the TUI and other UIs.
 
> E.g. with the first 13 patches, perf top shows that many objects
> are leaked.
>   ----
>   # ./perf top --stdio
>   q
>   exiting.
>   REFCNT: BUG: Unreclaimed objects found.
>   REFCNT: Total 3595 objects are not reclaimed.
>     "map" leaks 3334 objects
>     "dso" leaks 231 objects
>     "thread" leaks 9 objects
>     "comm_str" leaks 13 objects
>     "map_groups" leaks 8 objects
>      To see all backtraces, rerun with -v option
>   ----
> You can also dump all the backtrace data with -v option, but I
> don't recommend you to do it on your console, because it will
> be very very long (for example, above dumps 40MB text logs
> on your console).
> 
> Instead, you can use PERF_REFCNT_DEBUG_FILTER env. var. to focus
> on one object, and also use "2>" to redirect stderr output to file.
> E.g.
>   ----
>   # PERF_REFCNT_DEBUG_FILTER=map ./perf top --stdio -v 2> refcnt.log
>   q
>   exiting.
>   # less refcnt.log
>   mmap size 528384B
>   Looking at the vmlinux_path (8 entries long)
>   Using /lib/modules/4.3.0-rc2+/build/vmlinux for symbols
>   REFCNT: BUG: Unreclaimed objects found.
>   ==== [0] ====
>   Unreclaimed map@0x2157dc0
>   Refcount +1 => 1 at
>   ...
>     ./perf() [0x4226fd]
>   REFCNT: Total 3229 objects are not reclaimed.
>     "map" leaks 3229 objects
>   ----
> 
>   Bugfixes
>   ========
> 
> In this series I've also tried to fix some object leaks in perf top
> and perf stat.
> After applying this series, this reduced (not vanished) to 1/5.
>   ----
>   # ./perf top --stdio
>   q
>   exiting.
>   REFCNT: BUG: Unreclaimed objects found.
>   REFCNT: Total 866 objects are not reclaimed.
>     "dso" leaks 213 objects
>     "map" leaks 624 objects
>     "comm_str" leaks 12 objects
>     "thread" leaks 9 objects
>     "map_groups" leaks 8 objects
>      To see all backtraces, rerun with -v option
>   ----
> 
> Actually, I'm still not able to fix all of the bugs. It seems that
> hists has a bug that hists__delete_entries doesn't delete all the
> entries because some entries are on hists->entries but others on
> hists->entries_in. And I'm not so sure about hists.c.
> Arnaldo, would you have any idea for this bug?

I'll will audit the hists code, its all about collecting new entries
into an rbtree to then move the last batch for merging with the
previous, decayed ones while continuing to process a new batch in
another thread.
 
> 
>   General refcnt miscodings
>   =========================
> 
> BTW, while applying this change, I've found that there are refcnt
> coding mismatches in those code and most of the bugs come from those
> mismatches.
> 
> - The reference counter can be initialized by 0 or 1.
>  - If 0 is chosen, caller have to get it and free it if failed to
>    register appropriately.
>  - If 1 is chosen, caller doesn't need to get it, but when exits the
>    caller, it has to put it. (except for returning the object itself)
> - The application should choose either one as its policy, to avoid
>   confusion.
> 
> perf tools mixes it up (moreover, it initializes 2 in a case) and
> caller usually forgets to put it (it is not surprising, because too
> many "put" usually cause SEGV by accessing freed object.)

Well, we should fix the bugs and document why some initialization is
deemed better than a single initialization style.

For instance, if we know that we will keep multiple references straight
away, then we could init it with some value different that preferred,
that I aggee, is 1, i.e. the usual way for some constructor is:


struct foo *foo__new()
{
	struct foo *f = malloc(sizeof (*f));

	if (f) {
		atomic_set(&f->refcnt, 1);
	}

	return f;
}

void *foo__delete(struct foo *f)
{
	free(f);
}

void foo__put(struct foo *f)
{
	if (f && atomic_dec_and_test(f->refcnt))
		foo__delete(f);
}


Then, when using if, and before adding it to any other tree, list, i.e.
a container, we do:

	struct foo *f = foo__new();

	/*
	 * assume f was allocated and then the function allocating it
	 * failed, when it bails out it should just do:
 	 */
out_error:
	foo__put(f);

And that will make it hit zero, which will call foo__delete(), case
closed.
 
> As far as I can see, cgroup_sel, perf_mmap(this is initialized 0 or 2...),
> thread, and comm_str are initialized by 0. Others are initialized by 1.
> 
> So, I'd like to suggest that we choose one policy and cleanup the code.
> I recommend to use init by 1 policy, because anyway caller has to get

See above, if nothing else recommends using a different value, use 1.

> the refcnt. Suppose the below code;
> 
>    ----
> obj__new() {
> 	obj = zalloc(sizeof(*obj));
> 	refcnt__init(obj, refcnt, 0);
> 	return obj;
> }
> 
> caller() {
> 	obj = obj__new();
> 	if (parent__add_obj(parent, obj) != SUCCESS) {
> 		free(obj);
> 	}
> }
>    ----
> 
> At first glance, this looks good. However, if the parent__add_obj() once

The caller never should call free or the object destructor (i.e.
foo__delete() in my example above), it should _always_ just drop the
referece it has to the object, i.e. call foo__put()

> gets the obj(refcnt => 1) and fails to add it to parent list by some reason,
> it should put the obj(refcnt => 0). This means the obj is already freed at
> that point.
> 
> Then, caller() shouldn't free obj in error case? No, because parent__add_obj()
> can fail before getting the obj :(. Maybe we can handle it by checking return
> code, but it is ugly.
> 
> If we choose "init by 1", caller always has to put it before returning. But

Right, if you don't keep a pointer to the object that at a later point
you will call obj__put() on it, then you should do it straight away
after adding it to some list, tree, array, i.e. a container, and the
container adding operation should grab a refcount, that will be dropped
when the object is removed from that data structure.

> the coding rule becomes simpler.
>    ----
> caller() {
> 	obj = obj__new();
> 	if (parent__add_obj(parent, obj) != SUCCESS) {
> 		ret = errorcode;
> 	}
> 	obj__put(obj);
> 	return ret;
> }
>    ----
> 
> Thank you,
> 
> ---
> 
> Masami Hiramatsu (22):
>       [v2] perf refcnt: Introduce generic refcount APIs with debug feature
>       perf refcnt: Use a hash for refcnt_root
>       perf refcnt: Add refcnt debug filter
>       perf refcnt: refcnt shows summary per object
>       perf: make map to use refcnt
>       perf: Make dso to use refcnt for debug
>       perf: Make map_groups to use refcnt
>       perf: Make thread uses refcnt for debug
>       perf: Make cpu_map to use refcnt for debug
>       perf: Make comm_str to use refcnt for debug
>       perf: Make cgroup_sel to use refcnt for debug
>       perf: Make thread_map to use refcnt for debug
>       perf: Make perf_mmap to use refcnt for debug
>       perf: Fix dso__load_sym to put dso
>       perf: Fix map_groups__clone to put cloned map
>       perf: Fix __cmd_top and perf_session__process_events to put the idle thread
>       perf: Fix __machine__addnew_vdso to put dso after add to dsos
>       perf stat: Fix cmd_stat to release cpu_map
>       perf: fix hists_evsel to release hists
>       perf: Fix maps__fixup_overlappings to put used maps
>       perf: Fix machine.vmlinux_maps to make sure to clear the old one
>       perf: Fix write_numa_topology to put cpu_map instead of free
> 
> 
>  tools/perf/builtin-stat.c    |   11 ++
>  tools/perf/builtin-top.c     |    6 +
>  tools/perf/config/Makefile   |    5 +
>  tools/perf/util/Build        |    1 
>  tools/perf/util/cgroup.c     |   11 +-
>  tools/perf/util/comm.c       |    8 +
>  tools/perf/util/cpumap.c     |   33 +++---
>  tools/perf/util/dso.c        |    7 +
>  tools/perf/util/evlist.c     |    8 +
>  tools/perf/util/header.c     |    2 
>  tools/perf/util/hist.c       |   10 ++
>  tools/perf/util/machine.c    |    5 +
>  tools/perf/util/map.c        |   15 ++-
>  tools/perf/util/map.h        |    5 +
>  tools/perf/util/refcnt.c     |  229 ++++++++++++++++++++++++++++++++++++++++++
>  tools/perf/util/refcnt.h     |   70 +++++++++++++
>  tools/perf/util/session.c    |   10 ++
>  tools/perf/util/symbol-elf.c |    2 
>  tools/perf/util/thread.c     |    7 +
>  tools/perf/util/thread_map.c |   28 +++--
>  tools/perf/util/vdso.c       |    2 
>  21 files changed, 420 insertions(+), 55 deletions(-)
>  create mode 100644 tools/perf/util/refcnt.c
>  create mode 100644 tools/perf/util/refcnt.h
> 
> 
> -- 
> Masami HIRAMATSU
> Linux Technology Research Center, System Productivity Research Dept.
> Center for Technology Innovation - Systems Engineering 
> Hitachi, Ltd., Research & Development Group
> E-mail: masami.hiramatsu.pt@hitachi.com
--
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/

[toc] | [prev] | [next] | [standalone]

#1288171

FromAlexei Starovoitov <alexei.starovoitov@gmail.com>
Date2015-12-10 04:40 +0100
Message-ID<qE0fg-AZ-9@gated-at.bofh.it>
In reply to#1287507
On Wed, Dec 09, 2015 at 10:41:38AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, Dec 09, 2015 at 11:10:48AM +0900, Masami Hiramatsu escreveu:
> > Hi Arnaldo,
> > 
> > Here is a series of patches for perf refcnt debugger and
> > some fixes.
> > 
> > In this series I've replaced all atomic reference counters
> > with the refcnt interface, including dso, map, map_groups,
> > thread, cpu_map, comm_str, cgroup_sel, thread_map, and
> > perf_mmap.
> > 
> >   refcnt debugger (or refcnt leak checker)
> >   ===============
> > 
> > At first, note that this change doesn't affect any compiled
> > code unless building with REFCNT_DEBUG=1 (see macros in
> > refcnt.h). So, this feature is only enabled in the debug binary.
> > But before releasing, we can ensure that all objects are safely
> > reclaimed before exit in -rc phase.
> 
> That helps and is finding bugs and is really great stuff, thank you!
> 
> But I wonder if we couldn't get the same results on an unmodified binary
> by using things like 'perf probe', the BPF code we're introducing, have
> you thought about this possibility?
> 
> I.e. trying to use 'perf probe' to do this would help in using the same
> technique in other code bases where we can't change the sources, etc.
> 
> For perf we could perhaps use a 'noinline' on the __get/__put
> operations, so that we could have the right places to hook using
> uprobes, other codebases would have to rely in the 'perf probe'
> infrastructure that knows where inlines were expanded, etc.
> 
> Such a toold could work like:
> 
>    perf dbgrefcnt ~/bin/perf thread 
> 
> And it would look up thread__get and thread__put(), create an eBPF map
> where to store the needed tracking data structures, and use the same
> techniques you used, asking for backtraces using the perf
> infrastructure, etc.

I really like the idea. It's doable with minimal changes.
The only question is the speed of uprobes.
I just haven't benchmarked them. If uprobe+bpf is within 10% slower
than this native refcnt debugger then I think we can build some really
cool tools on top of it. Like generic debugging of std::shared_ptr
or dead lock detection of unmodified binaries.
We can uprobe into pthread_mutex_lock, etc.
Infinite possibilites.

--
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/

[toc] | [prev] | [next] | [standalone]

#1288206

FromNamhyung Kim <namhyung@kernel.org>
Date2015-12-10 06:00 +0100
Message-ID<qE1uF-1j4-13@gated-at.bofh.it>
In reply to#1287507
Hi Arnaldo and Masami,

On Wed, Dec 09, 2015 at 10:41:38AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, Dec 09, 2015 at 11:10:48AM +0900, Masami Hiramatsu escreveu:
> > In this series I've also tried to fix some object leaks in perf top
> > and perf stat.
> > After applying this series, this reduced (not vanished) to 1/5.
> >   ----
> >   # ./perf top --stdio
> >   q
> >   exiting.
> >   REFCNT: BUG: Unreclaimed objects found.
> >   REFCNT: Total 866 objects are not reclaimed.
> >     "dso" leaks 213 objects
> >     "map" leaks 624 objects
> >     "comm_str" leaks 12 objects
> >     "thread" leaks 9 objects
> >     "map_groups" leaks 8 objects
> >      To see all backtraces, rerun with -v option
> >   ----
> > 
> > Actually, I'm still not able to fix all of the bugs. It seems that
> > hists has a bug that hists__delete_entries doesn't delete all the
> > entries because some entries are on hists->entries but others on
> > hists->entries_in. And I'm not so sure about hists.c.
> > Arnaldo, would you have any idea for this bug?
> 
> I'll will audit the hists code, its all about collecting new entries
> into an rbtree to then move the last batch for merging with the
> previous, decayed ones while continuing to process a new batch in
> another thread.

Right.  After processing is done, hist entries are in both of
hists->entries and hists->entries_in (or hists->entries_collapsed).
So I guess perf report does not have leaks on hists.

But for perf top, it's possible to have half-processed entries which
are only in hists->entries_in.  Eventually they will go to the
hists->entries and get freed but they cannot be deleted by current
hists__delete_entries().  If we really care deleting all of those
half-processed entries, how about this?

Thanks,
Namhyung



diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index fd179a068df7..08396a7fea23 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -270,6 +270,8 @@ static void hists__delete_entry(struct hists *hists, struct hist_entry *he)
 
 	if (sort__need_collapse)
 		rb_erase(&he->rb_node_in, &hists->entries_collapsed);
+	else
+		rb_erase(&he->rb_node_in, hists->entries_in);
 
 	--hists->nr_entries;
 	if (!he->filtered)
@@ -1589,11 +1591,33 @@ int __hists__init(struct hists *hists)
 	return 0;
 }
 
+static void hists__delete_remaining_entries(struct rb_root *root)
+{
+	struct rb_node *node;
+	struct hist_entry *he;
+
+	while (!RB_EMPTY_ROOT(root)) {
+		node = rb_first(root);
+		rb_erase(node, root);
+
+		he = rb_entry(node, struct hist_entry, rb_node_in);
+		hist_entry__delete(he);
+	}
+}
+
+static void hists__delete_all_entries(struct hists *hists)
+{
+	hists__delete_entries(hists);
+	hists__delete_remaining_entries(&hists->entries_in_array[0]);
+	hists__delete_remaining_entries(&hists->entries_in_array[1]);
+	hists__delete_remaining_entries(&hists->entries_collapsed);
+}
+
 static void hists_evsel__exit(struct perf_evsel *evsel)
 {
 	struct hists *hists = evsel__hists(evsel);
 
-	hists__delete_entries(hists);
+	hists__delete_all_entries(hists);
 }
 
 static int hists_evsel__init(struct perf_evsel *evsel)
--
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/

[toc] | [prev] | [next] | [standalone]

#1288360

From平松雅巳 / HIRAMATU，MASAMI <masami.hiramatsu.pt@hitachi.com>
Date2015-12-10 09:40 +0100
Message-ID<qE4VA-3E8-17@gated-at.bofh.it>
In reply to#1288206
PkZyb206IE5hbWh5dW5nIEtpbSBbbWFpbHRvOm5hbWh5dW5nQGtlcm5lbC5vcmddDQo+DQo+SGkg
QXJuYWxkbyBhbmQgTWFzYW1pLA0KPg0KPk9uIFdlZCwgRGVjIDA5LCAyMDE1IGF0IDEwOjQxOjM4
QU0gLTAzMDAsIEFybmFsZG8gQ2FydmFsaG8gZGUgTWVsbyB3cm90ZToNCj4+IEVtIFdlZCwgRGVj
IDA5LCAyMDE1IGF0IDExOjEwOjQ4QU0gKzA5MDAsIE1hc2FtaSBIaXJhbWF0c3UgZXNjcmV2ZXU6
DQo+PiA+IEluIHRoaXMgc2VyaWVzIEkndmUgYWxzbyB0cmllZCB0byBmaXggc29tZSBvYmplY3Qg
bGVha3MgaW4gcGVyZiB0b3ANCj4+ID4gYW5kIHBlcmYgc3RhdC4NCj4+ID4gQWZ0ZXIgYXBwbHlp
bmcgdGhpcyBzZXJpZXMsIHRoaXMgcmVkdWNlZCAobm90IHZhbmlzaGVkKSB0byAxLzUuDQo+PiA+
ICAgLS0tLQ0KPj4gPiAgICMgLi9wZXJmIHRvcCAtLXN0ZGlvDQo+PiA+ICAgcQ0KPj4gPiAgIGV4
aXRpbmcuDQo+PiA+ICAgUkVGQ05UOiBCVUc6IFVucmVjbGFpbWVkIG9iamVjdHMgZm91bmQuDQo+
PiA+ICAgUkVGQ05UOiBUb3RhbCA4NjYgb2JqZWN0cyBhcmUgbm90IHJlY2xhaW1lZC4NCj4+ID4g
ICAgICJkc28iIGxlYWtzIDIxMyBvYmplY3RzDQo+PiA+ICAgICAibWFwIiBsZWFrcyA2MjQgb2Jq
ZWN0cw0KPj4gPiAgICAgImNvbW1fc3RyIiBsZWFrcyAxMiBvYmplY3RzDQo+PiA+ICAgICAidGhy
ZWFkIiBsZWFrcyA5IG9iamVjdHMNCj4+ID4gICAgICJtYXBfZ3JvdXBzIiBsZWFrcyA4IG9iamVj
dHMNCj4+ID4gICAgICBUbyBzZWUgYWxsIGJhY2t0cmFjZXMsIHJlcnVuIHdpdGggLXYgb3B0aW9u
DQo+PiA+ICAgLS0tLQ0KPj4gPg0KPj4gPiBBY3R1YWxseSwgSSdtIHN0aWxsIG5vdCBhYmxlIHRv
IGZpeCBhbGwgb2YgdGhlIGJ1Z3MuIEl0IHNlZW1zIHRoYXQNCj4+ID4gaGlzdHMgaGFzIGEgYnVn
IHRoYXQgaGlzdHNfX2RlbGV0ZV9lbnRyaWVzIGRvZXNuJ3QgZGVsZXRlIGFsbCB0aGUNCj4+ID4g
ZW50cmllcyBiZWNhdXNlIHNvbWUgZW50cmllcyBhcmUgb24gaGlzdHMtPmVudHJpZXMgYnV0IG90
aGVycyBvbg0KPj4gPiBoaXN0cy0+ZW50cmllc19pbi4gQW5kIEknbSBub3Qgc28gc3VyZSBhYm91
dCBoaXN0cy5jLg0KPj4gPiBBcm5hbGRvLCB3b3VsZCB5b3UgaGF2ZSBhbnkgaWRlYSBmb3IgdGhp
cyBidWc/DQo+Pg0KPj4gSSdsbCB3aWxsIGF1ZGl0IHRoZSBoaXN0cyBjb2RlLCBpdHMgYWxsIGFi
b3V0IGNvbGxlY3RpbmcgbmV3IGVudHJpZXMNCj4+IGludG8gYW4gcmJ0cmVlIHRvIHRoZW4gbW92
ZSB0aGUgbGFzdCBiYXRjaCBmb3IgbWVyZ2luZyB3aXRoIHRoZQ0KPj4gcHJldmlvdXMsIGRlY2F5
ZWQgb25lcyB3aGlsZSBjb250aW51aW5nIHRvIHByb2Nlc3MgYSBuZXcgYmF0Y2ggaW4NCj4+IGFu
b3RoZXIgdGhyZWFkLg0KPg0KPlJpZ2h0LiAgQWZ0ZXIgcHJvY2Vzc2luZyBpcyBkb25lLCBoaXN0
IGVudHJpZXMgYXJlIGluIGJvdGggb2YNCj5oaXN0cy0+ZW50cmllcyBhbmQgaGlzdHMtPmVudHJp
ZXNfaW4gKG9yIGhpc3RzLT5lbnRyaWVzX2NvbGxhcHNlZCkuDQo+U28gSSBndWVzcyBwZXJmIHJl
cG9ydCBkb2VzIG5vdCBoYXZlIGxlYWtzIG9uIGhpc3RzLg0KPg0KPkJ1dCBmb3IgcGVyZiB0b3As
IGl0J3MgcG9zc2libGUgdG8gaGF2ZSBoYWxmLXByb2Nlc3NlZCBlbnRyaWVzIHdoaWNoDQo+YXJl
IG9ubHkgaW4gaGlzdHMtPmVudHJpZXNfaW4uICBFdmVudHVhbGx5IHRoZXkgd2lsbCBnbyB0byB0
aGUNCj5oaXN0cy0+ZW50cmllcyBhbmQgZ2V0IGZyZWVkIGJ1dCB0aGV5IGNhbm5vdCBiZSBkZWxl
dGVkIGJ5IGN1cnJlbnQNCj5oaXN0c19fZGVsZXRlX2VudHJpZXMoKS4gIElmIHdlIHJlYWxseSBj
YXJlIGRlbGV0aW5nIGFsbCBvZiB0aG9zZQ0KPmhhbGYtcHJvY2Vzc2VkIGVudHJpZXMsIGhvdyBh
Ym91dCB0aGlzPw0KPg0KDQpOaWNlISBJJ3ZlIGFwcGxpZWQgYmVsb3cgcGF0Y2ggb24gdGhlIHRv
cCBvZiBteSBzZXJpZXMgYW5kDQpjaGVja2VkIHRoYXQgbm8gbGVhayBpcyBkZXRlY3RlZC4gOikN
Cg0KWW91IGNhbiBhZGQgbXkgYWNrZWQtYnkgYW5kIHRlc3RlZC1ieS4gOikNCg0KQWNrZWQtYnk6
IE1hc2FtaSBIaXJhbWF0c3UgPG1hc2FtaS5oaXJhbWF0c3UucHRAaGl0YWNoaS5jb20+DQpUZXN0
ZWQtYnk6IE1hc2FtaSBIaXJhbWF0c3UgPG1hc2FtaS5oaXJhbWF0c3UucHRAaGl0YWNoaS5jb20+
DQoNClRoYW5rcyENCg0KPg0KPg0KPmRpZmYgLS1naXQgYS90b29scy9wZXJmL3V0aWwvaGlzdC5j
IGIvdG9vbHMvcGVyZi91dGlsL2hpc3QuYw0KPmluZGV4IGZkMTc5YTA2OGRmNy4uMDgzOTZhN2Zl
YTIzIDEwMDY0NA0KPi0tLSBhL3Rvb2xzL3BlcmYvdXRpbC9oaXN0LmMNCj4rKysgYi90b29scy9w
ZXJmL3V0aWwvaGlzdC5jDQo+QEAgLTI3MCw2ICsyNzAsOCBAQCBzdGF0aWMgdm9pZCBoaXN0c19f
ZGVsZXRlX2VudHJ5KHN0cnVjdCBoaXN0cyAqaGlzdHMsIHN0cnVjdCBoaXN0X2VudHJ5ICpoZSkN
Cj4NCj4gCWlmIChzb3J0X19uZWVkX2NvbGxhcHNlKQ0KPiAJCXJiX2VyYXNlKCZoZS0+cmJfbm9k
ZV9pbiwgJmhpc3RzLT5lbnRyaWVzX2NvbGxhcHNlZCk7DQo+KwllbHNlDQo+KwkJcmJfZXJhc2Uo
JmhlLT5yYl9ub2RlX2luLCBoaXN0cy0+ZW50cmllc19pbik7DQo+DQo+IAktLWhpc3RzLT5ucl9l
bnRyaWVzOw0KPiAJaWYgKCFoZS0+ZmlsdGVyZWQpDQo+QEAgLTE1ODksMTEgKzE1OTEsMzMgQEAg
aW50IF9faGlzdHNfX2luaXQoc3RydWN0IGhpc3RzICpoaXN0cykNCj4gCXJldHVybiAwOw0KPiB9
DQo+DQo+K3N0YXRpYyB2b2lkIGhpc3RzX19kZWxldGVfcmVtYWluaW5nX2VudHJpZXMoc3RydWN0
IHJiX3Jvb3QgKnJvb3QpDQo+K3sNCj4rCXN0cnVjdCByYl9ub2RlICpub2RlOw0KPisJc3RydWN0
IGhpc3RfZW50cnkgKmhlOw0KPisNCj4rCXdoaWxlICghUkJfRU1QVFlfUk9PVChyb290KSkgew0K
PisJCW5vZGUgPSByYl9maXJzdChyb290KTsNCj4rCQlyYl9lcmFzZShub2RlLCByb290KTsNCj4r
DQo+KwkJaGUgPSByYl9lbnRyeShub2RlLCBzdHJ1Y3QgaGlzdF9lbnRyeSwgcmJfbm9kZV9pbik7
DQo+KwkJaGlzdF9lbnRyeV9fZGVsZXRlKGhlKTsNCj4rCX0NCj4rfQ0KPisNCj4rc3RhdGljIHZv
aWQgaGlzdHNfX2RlbGV0ZV9hbGxfZW50cmllcyhzdHJ1Y3QgaGlzdHMgKmhpc3RzKQ0KPit7DQo+
KwloaXN0c19fZGVsZXRlX2VudHJpZXMoaGlzdHMpOw0KPisJaGlzdHNfX2RlbGV0ZV9yZW1haW5p
bmdfZW50cmllcygmaGlzdHMtPmVudHJpZXNfaW5fYXJyYXlbMF0pOw0KPisJaGlzdHNfX2RlbGV0
ZV9yZW1haW5pbmdfZW50cmllcygmaGlzdHMtPmVudHJpZXNfaW5fYXJyYXlbMV0pOw0KPisJaGlz
dHNfX2RlbGV0ZV9yZW1haW5pbmdfZW50cmllcygmaGlzdHMtPmVudHJpZXNfY29sbGFwc2VkKTsN
Cj4rfQ0KPisNCj4gc3RhdGljIHZvaWQgaGlzdHNfZXZzZWxfX2V4aXQoc3RydWN0IHBlcmZfZXZz
ZWwgKmV2c2VsKQ0KPiB7DQo+IAlzdHJ1Y3QgaGlzdHMgKmhpc3RzID0gZXZzZWxfX2hpc3RzKGV2
c2VsKTsNCj4NCj4tCWhpc3RzX19kZWxldGVfZW50cmllcyhoaXN0cyk7DQo+KwloaXN0c19fZGVs
ZXRlX2FsbF9lbnRyaWVzKGhpc3RzKTsNCj4gfQ0KPg0KPiBzdGF0aWMgaW50IGhpc3RzX2V2c2Vs
X19pbml0KHN0cnVjdCBwZXJmX2V2c2VsICpldnNlbCkNCg==
--
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/

[toc] | [prev] | [next] | [standalone]

#1288445

From平松雅巳 / HIRAMATU，MASAMI <masami.hiramatsu.pt@hitachi.com>
Date2015-12-10 12:10 +0100
Message-ID<qE7gJ-5pn-5@gated-at.bofh.it>
In reply to#1287507
>From: Arnaldo Carvalho de Melo [mailto:acme@kernel.org]
>
>Em Wed, Dec 09, 2015 at 11:10:48AM +0900, Masami Hiramatsu escreveu:
>> Hi Arnaldo,
>>
>> Here is a series of patches for perf refcnt debugger and
>> some fixes.
>>
>> In this series I've replaced all atomic reference counters
>> with the refcnt interface, including dso, map, map_groups,
>> thread, cpu_map, comm_str, cgroup_sel, thread_map, and
>> perf_mmap.
>>
>>   refcnt debugger (or refcnt leak checker)
>>   ===============
>>
>> At first, note that this change doesn't affect any compiled
>> code unless building with REFCNT_DEBUG=1 (see macros in
>> refcnt.h). So, this feature is only enabled in the debug binary.
>> But before releasing, we can ensure that all objects are safely
>> reclaimed before exit in -rc phase.
>
>That helps and is finding bugs and is really great stuff, thank you!
>
>But I wonder if we couldn't get the same results on an unmodified binary
>by using things like 'perf probe', the BPF code we're introducing, have
>you thought about this possibility?

That's possible, but it will require pre-analysis of the binary, because
refcnt interface is not fixed API like a "systemcall" (moreover, it could
be just a non-atomic variable).

Thus we need a kind of "annotation" event by source code level.

>
>I.e. trying to use 'perf probe' to do this would help in using the same
>technique in other code bases where we can't change the sources, etc.
>
>For perf we could perhaps use a 'noinline' on the __get/__put
>operations, so that we could have the right places to hook using
>uprobes, other codebases would have to rely in the 'perf probe'
>infrastructure that knows where inlines were expanded, etc.
>
>Such a toold could work like:
>
>   perf dbgrefcnt ~/bin/perf thread

This works only for the binary which is coded as you said.
I actually doubt that this is universal solution. We'd better introduce
librefcnt.so if you need more general solution, so that we can fix the
refcnt API and we can also hook the interface easily.

But with this way, we don't need ebpf/uprobes anymore, since we've already
have LD_PRELOAD (like valgrind does). :(

So, IMHO, using ebpf and perf probe for this issue sounds like using
a sledge‐hammer...

Thanks,

>
>And it would look up thread__get and thread__put(), create an eBPF map
>where to store the needed tracking data structures, and use the same
>techniques you used, asking for backtraces using the perf
>infrastructure, etc.
>
>We would be using this opportunity to improve the 'perf probe' and the
>eBPF infrastructures we're putting in place, and having something that
>could be used in other codebases, not just perf.
>




--
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/

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web