Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1287008 > unrolled thread
| Started by | Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> |
|---|---|
| First post | 2015-12-09 03:30 +0100 |
| Last post | 2015-12-11 23:30 +0100 |
| Articles | 10 on this page of 30 — 8 participants |
Back to article view | Back to linux.kernel
[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 2 of 2 — ← Prev page 1 [2]
| From | "Wangnan (F)" <wangnan0@huawei.com> |
|---|---|
| Date | 2015-12-10 14:00 +0100 |
| Message-ID | <qE8Zd-6lY-21@gated-at.bofh.it> |
| In reply to | #1288445 |
On 2015/12/10 19:04, 平松雅巳 / HIRAMATU，MASAMI wrote:
>> 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...
But this is an interesting problem and can inspire us the direction
for eBPF improvement. I guess if we can solve this problem with eBPF
we can also solve many similar problems with much lower cost than what
you have done in first 5 patches?
This is what we have done today:
With a much simpler patch which create 4 stub functions:
diff --git a/tools/perf/util/Build b/tools/perf/util/Build
index 65fef59..2c45478 100644
--- a/tools/perf/util/Build
+++ b/tools/perf/util/Build
@@ -87,6 +87,7 @@ libperf-$(CONFIG_AUXTRACE) += intel-bts.o
libperf-y += parse-branch-options.o
libperf-y += parse-regs-options.o
libperf-y += term.o
+libperf-y += refcnt.o
libperf-$(CONFIG_LIBBPF) += bpf-loader.o
libperf-$(CONFIG_BPF_PROLOGUE) += bpf-prologue.o
diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index e8e9a9d..de52ae8 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -1,6 +1,7 @@
#include <asm/bug.h>
#include <sys/time.h>
#include <sys/resource.h>
+#include "refcnt.h"
#include "symbol.h"
#include "dso.h"
#include "machine.h"
diff --git a/tools/perf/util/refcnt.c b/tools/perf/util/refcnt.c
new file mode 100644
index 0000000..f5a6659
--- /dev/null
+++ b/tools/perf/util/refcnt.c
@@ -0,0 +1,29 @@
+#include <linux/compiler.h>
+#include "util/refcnt.h"
+
+void __attribute__ ((noinline))
+__refcnt__init(atomic_t *refcnt, int n,
+ void *obj __maybe_unused,
+ const char *name __maybe_unused)
+{
+ atomic_set(refcnt, n);
+}
+
+void __attribute__ ((noinline))
+refcnt__delete(void *obj __maybe_unused)
+{
+}
+
+void __attribute__ ((noinline))
+__refcnt__get(atomic_t *refcnt, void *obj __maybe_unused,
+ const char *name __maybe_unused)
+{
+ atomic_inc(refcnt);
+}
+
+int __attribute__ ((noinline))
+__refcnt__put(atomic_t *refcnt, void *obj __maybe_unused,
+ const char *name __maybe_unused)
+{
+ return atomic_dec_and_test(refcnt);
+}
diff --git a/tools/perf/util/refcnt.h b/tools/perf/util/refcnt.h
new file mode 100644
index 0000000..04f5390
--- /dev/null
+++ b/tools/perf/util/refcnt.h
@@ -0,0 +1,21 @@
+#ifndef PERF_REFCNT_H
+#define PERF_REFCNT_H
+#include <linux/atomic.h>
+
+void __refcnt__init(atomic_t *refcnt, int n, void *obj, const char *name);
+void refcnt__delete(void *obj);
+void __refcnt__get(atomic_t *refcnt, void *obj, const char *name);
+int __refcnt__put(atomic_t *refcnt, void *obj, const char *name);
+
+#define refcnt__init(obj, member, n) \
+ __refcnt__init(&(obj)->member, n, obj, #obj)
+#define refcnt__init_as(obj, member, n, name) \
+ __refcnt__init(&(obj)->member, n, obj, name)
+#define refcnt__exit(obj, member) \
+ refcnt__delete(obj)
+#define refcnt__get(obj, member) \
+ __refcnt__get(&(obj)->member, obj, #obj)
+#define refcnt__put(obj, member) \
+ __refcnt__put(&(obj)->member, obj, #obj)
+
+#endif
And a relative complex eBPF script attached at the end of
this mail, with following cmdline:
# ./perf record -e ./refcnt.c ./perf probe vfs_read
# cat /sys/kernel/debug/tracing/trace
...
perf-18419 [004] d... 613572.513083: : Type 0 leak 2
perf-18419 [004] d... 613572.513084: : Type 1 leak 1
I know we have 2 dsos and 1 map get leak.
However I have to analysis full stack trace from 'perf script' to find
which one get leak, because currently my eBPF script is unable to report
which object is leak. I know I can use a hashtable with object address
as key, but currently I don't know how to enumerate keys in a hash table,
except maintaining a relationship between index and object address.
Now I'm waiting for Daniel's persistent map to be enforced for that. When
it ready we can create a tool with the following eBPF script embedded into
perf as a small subcommand, and report call stack of 'alloc' method of
leak object in 'perf report' style, so we can solve similar problem easier.
To make it genereic, we can even make it attach to '{m,c}alloc%return' and
'free', or 'mmap/munmap'.
Thank you.
-------------- eBPF script --------------
typedef int u32;
typedef unsigned long long u64;
#define NULL ((void *)(0))
#define BPF_ANY 0 /* create new element or update existing */
#define BPF_NOEXIST 1 /* create new element if it didn't exist */
#define BPF_EXIST 2 /* update existing element */
enum bpf_map_type {
BPF_MAP_TYPE_UNSPEC,
BPF_MAP_TYPE_HASH,
BPF_MAP_TYPE_ARRAY,
BPF_MAP_TYPE_PROG_ARRAY,
BPF_MAP_TYPE_PERF_EVENT_ARRAY,
};
struct bpf_map_def {
unsigned int type;
unsigned int key_size;
unsigned int value_size;
unsigned int max_entries;
};
#define SEC(NAME) __attribute__((section(NAME), used))
static int (*bpf_probe_read)(void *dst, int size, void *src) =
(void *)4;
static int (*bpf_trace_printk)(const char *fmt, int fmt_size, ...) =
(void *)6;
static int (*bpf_get_smp_processor_id)(void) =
(void *)8;
static int (*map_update_elem)(struct bpf_map_def *, void *, void *,
unsigned long long flags) =
(void *)2;
static void *(*map_lookup_elem)(struct bpf_map_def *, void *) =
(void *)1;
static unsigned long long (*get_current_pid_tgid)(void) =
(void *)14;
static unsigned long long (*get_current_comm)(char *buf, int size_of_buf) =
(void *)16;
char _license[] SEC("license") = "GPL";
int _version SEC("version") = LINUX_VERSION_CODE;
enum global_var {
G_pid,
G_LEAK_START,
G_dso_leak = G_LEAK_START,
G_map_group_leak,
G_LEAK_END,
G_NR = G_LEAK_END,
};
struct bpf_map_def SEC("maps") global_vars = {
.type = BPF_MAP_TYPE_ARRAY,
.key_size = sizeof(int),
.value_size = sizeof(u64),
.max_entries = G_NR,
};
static inline int filter_pid(void)
{
int key_pid = G_pid;
unsigned long long *p_pid, pid;
char fmt[] = "%d vs %d\n";
p_pid = map_lookup_elem(&global_vars, &key_pid);
if (!p_pid)
return 0;
pid = get_current_pid_tgid() & 0xffffffff;
if (*p_pid != pid)
return 0;
return 1;
}
static inline void print_leak(int type)
{
unsigned long long *p_cnt;
char fmt[] = "Type %d leak %llu\n";
p_cnt = map_lookup_elem(&global_vars, &type);
if (!p_cnt)
return;
bpf_trace_printk(fmt, sizeof(fmt), type - G_LEAK_START, *p_cnt);
}
SEC("execve=sys_execve")
int execve(void *ctx)
{
char name[sizeof(u64)] = "";
char name_perf[sizeof(u64)] = "perf";
unsigned long long *p_pid, pid;
int key = G_pid;
p_pid = map_lookup_elem(&global_vars, &key);
if (!p_pid)
return 0;
pid = *p_pid;
if (pid)
return 0;
if (get_current_comm(name, sizeof(name)))
return 0;
if (*(u32*)name != *(u32*)name_perf)
return 0;
pid = get_current_pid_tgid() & 0xffffffff;
map_update_elem(&global_vars, &key, &pid, BPF_ANY);
return 0;
}
static inline int func_exit(void *ctx)
{
if (!filter_pid())
return 0;
print_leak(G_dso_leak);
print_leak(G_map_group_leak);
return 0;
}
SEC("exit_group=sys_exit_group")
int exit_group(void *ctx)
{
return func_exit(ctx);
}
SEC("exit_=sys_exit")
int exit_(void *ctx)
{
return func_exit(ctx);
}
static inline void inc_leak_from_type(int type, int n)
{
u64 *p_cnt, cnt;
type += G_LEAK_START;
if (type >= G_LEAK_END)
return;
p_cnt = map_lookup_elem(&global_vars, &type);
if (!p_cnt)
cnt = n;
else
cnt = *p_cnt + n;
map_update_elem(&global_vars, &type, &cnt, BPF_ANY);
return;
}
SEC("exec=/home/wangnan/perf;"
"refcnt_init=__refcnt__init n obj type")
int refcnt_init(void *ctx, int err, int n, void *obj, int type)
{
if (!filter_pid())
return 0;
inc_leak_from_type(type, n);
return 0;
}
SEC("exec=/home/wangnan/perf;"
"refcnt_del=refcnt__delete obj type")
int refcnt_del(void *ctx, int err, void *obj, int type)
{
if (!filter_pid())
return 0;
return 0;
}
SEC("exec=/home/wangnan/perf;"
"refcnt_get=__refcnt__get obj type")
int refcnt_get(void *ctx, int err, void *obj, int type)
{
if (!filter_pid())
return 0;
inc_leak_from_type(type, 1);
return 0;
}
SEC("exec=/home/wangnan/perf;"
"refcnt_put=__refcnt__put refcnt obj type")
int refcnt_put(void *ctx, int err, void *refcnt, void *obj, int type)
{
int old_cnt = -1;
if (!filter_pid())
return 0;
if (bpf_probe_read(&old_cnt, sizeof(int), refcnt))
return 0;
if (old_cnt)
inc_leak_from_type(type, -1);
return 0;
}
--
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]
| From | 'Arnaldo Carvalho de Melo' <acme@kernel.org> |
|---|---|
| Date | 2015-12-10 16:20 +0100 |
| Message-ID | <qEbaH-7V7-47@gated-at.bofh.it> |
| In reply to | #1288499 |
Em Thu, Dec 10, 2015 at 08:52:02PM +0800, Wangnan (F) escreveu:
> On 2015/12/10 19:04, 平松雅巳 / HIRAMATU，MASAMI wrote:
> >>From: Arnaldo Carvalho de Melo [mailto:acme@kernel.org]
> >>Em Wed, Dec 09, 2015 at 11:10:48AM +0900, Masami Hiramatsu escreveu:
> >>>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...
>
> But this is an interesting problem and can inspire us the direction
> for eBPF improvement. I guess if we can solve this problem with eBPF
That is the spirit! All those efforts in the kernel to have always
available tracing facilities, kprobes, uprobes, eBPF, jump labels, you
name it, leaves me thinking that doing things by having to rebuild from
sources using defines, and even having to restart a workload to use
LD_PRELOAD tricks looks backwards :-)
> we can also solve many similar problems with much lower cost than what
> you have done in first 5 patches?
>
> This is what we have done today:
>
> With a much simpler patch which create 4 stub functions:
>
> diff --git a/tools/perf/util/Build b/tools/perf/util/Build
> index 65fef59..2c45478 100644
> --- a/tools/perf/util/Build
> +++ b/tools/perf/util/Build
> @@ -87,6 +87,7 @@ libperf-$(CONFIG_AUXTRACE) += intel-bts.o
> libperf-y += parse-branch-options.o
> libperf-y += parse-regs-options.o
> libperf-y += term.o
> +libperf-y += refcnt.o
>
> libperf-$(CONFIG_LIBBPF) += bpf-loader.o
> libperf-$(CONFIG_BPF_PROLOGUE) += bpf-prologue.o
> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index e8e9a9d..de52ae8 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -1,6 +1,7 @@
> #include <asm/bug.h>
> #include <sys/time.h>
> #include <sys/resource.h>
> +#include "refcnt.h"
> #include "symbol.h"
> #include "dso.h"
> #include "machine.h"
> diff --git a/tools/perf/util/refcnt.c b/tools/perf/util/refcnt.c
> new file mode 100644
> index 0000000..f5a6659
> --- /dev/null
> +++ b/tools/perf/util/refcnt.c
> @@ -0,0 +1,29 @@
> +#include <linux/compiler.h>
> +#include "util/refcnt.h"
> +
> +void __attribute__ ((noinline))
> +__refcnt__init(atomic_t *refcnt, int n,
> + void *obj __maybe_unused,
> + const char *name __maybe_unused)
> +{
> + atomic_set(refcnt, n);
> +}
> +
> +void __attribute__ ((noinline))
> +refcnt__delete(void *obj __maybe_unused)
> +{
> +}
> +
> +void __attribute__ ((noinline))
> +__refcnt__get(atomic_t *refcnt, void *obj __maybe_unused,
> + const char *name __maybe_unused)
> +{
> + atomic_inc(refcnt);
> +}
> +
> +int __attribute__ ((noinline))
> +__refcnt__put(atomic_t *refcnt, void *obj __maybe_unused,
> + const char *name __maybe_unused)
> +{
> + return atomic_dec_and_test(refcnt);
> +}
> diff --git a/tools/perf/util/refcnt.h b/tools/perf/util/refcnt.h
> new file mode 100644
> index 0000000..04f5390
> --- /dev/null
> +++ b/tools/perf/util/refcnt.h
> @@ -0,0 +1,21 @@
> +#ifndef PERF_REFCNT_H
> +#define PERF_REFCNT_H
> +#include <linux/atomic.h>
> +
> +void __refcnt__init(atomic_t *refcnt, int n, void *obj, const char *name);
> +void refcnt__delete(void *obj);
> +void __refcnt__get(atomic_t *refcnt, void *obj, const char *name);
> +int __refcnt__put(atomic_t *refcnt, void *obj, const char *name);
> +
> +#define refcnt__init(obj, member, n) \
> + __refcnt__init(&(obj)->member, n, obj, #obj)
> +#define refcnt__init_as(obj, member, n, name) \
> + __refcnt__init(&(obj)->member, n, obj, name)
> +#define refcnt__exit(obj, member) \
> + refcnt__delete(obj)
> +#define refcnt__get(obj, member) \
> + __refcnt__get(&(obj)->member, obj, #obj)
> +#define refcnt__put(obj, member) \
> + __refcnt__put(&(obj)->member, obj, #obj)
> +
> +#endif
But this requires having these special refcnt__ routines, that will make
tools/perf/ code patterns for reference counts look different that the
refcount patterns in the kernel :-\
And would be a requirement to change the observed workload :-\
Is this _strictly_ required? Can't we, for a project like perf, where we
know where some refcount (say, the one for 'struct thread') gets
initialized, use that (thread__new()) and then hook into thread__get and
thread__put and then use the destructor, thread__delete() as the place
to dump leaks?
> And a relative complex eBPF script attached at the end of
> this mail, with following cmdline:
>
> # ./perf record -e ./refcnt.c ./perf probe vfs_read
> # cat /sys/kernel/debug/tracing/trace
> ...
> perf-18419 [004] d... 613572.513083: : Type 0 leak 2
> perf-18419 [004] d... 613572.513084: : Type 1 leak 1
>
> I know we have 2 dsos and 1 map get leak.
>
> However I have to analysis full stack trace from 'perf script' to find
> which one get leak, because currently my eBPF script is unable to report
> which object is leak. I know I can use a hashtable with object address
> as key, but currently I don't know how to enumerate keys in a hash table,
> except maintaining a relationship between index and object address.
> Now I'm waiting for Daniel's persistent map to be enforced for that. When
> it ready we can create a tool with the following eBPF script embedded into
> perf as a small subcommand, and report call stack of 'alloc' method of
> leak object in 'perf report' style, so we can solve similar problem easier.
> To make it genereic, we can even make it attach to '{m,c}alloc%return' and
> 'free', or 'mmap/munmap'.
>
> Thank you.
>
>
> -------------- eBPF script --------------
>
> typedef int u32;
> typedef unsigned long long u64;
> #define NULL ((void *)(0))
>
> #define BPF_ANY 0 /* create new element or update existing */
> #define BPF_NOEXIST 1 /* create new element if it didn't exist */
> #define BPF_EXIST 2 /* update existing element */
>
> enum bpf_map_type {
> BPF_MAP_TYPE_UNSPEC,
> BPF_MAP_TYPE_HASH,
> BPF_MAP_TYPE_ARRAY,
> BPF_MAP_TYPE_PROG_ARRAY,
> BPF_MAP_TYPE_PERF_EVENT_ARRAY,
> };
>
> struct bpf_map_def {
> unsigned int type;
> unsigned int key_size;
> unsigned int value_size;
> unsigned int max_entries;
> };
>
> #define SEC(NAME) __attribute__((section(NAME), used))
> static int (*bpf_probe_read)(void *dst, int size, void *src) =
> (void *)4;
> static int (*bpf_trace_printk)(const char *fmt, int fmt_size, ...) =
> (void *)6;
> static int (*bpf_get_smp_processor_id)(void) =
> (void *)8;
> static int (*map_update_elem)(struct bpf_map_def *, void *, void *, unsigned
> long long flags) =
> (void *)2;
> static void *(*map_lookup_elem)(struct bpf_map_def *, void *) =
> (void *)1;
> static unsigned long long (*get_current_pid_tgid)(void) =
> (void *)14;
> static unsigned long long (*get_current_comm)(char *buf, int size_of_buf) =
> (void *)16;
>
> char _license[] SEC("license") = "GPL";
> int _version SEC("version") = LINUX_VERSION_CODE;
>
> enum global_var {
> G_pid,
> G_LEAK_START,
> G_dso_leak = G_LEAK_START,
> G_map_group_leak,
> G_LEAK_END,
> G_NR = G_LEAK_END,
> };
>
> struct bpf_map_def SEC("maps") global_vars = {
> .type = BPF_MAP_TYPE_ARRAY,
> .key_size = sizeof(int),
> .value_size = sizeof(u64),
> .max_entries = G_NR,
> };
>
> static inline int filter_pid(void)
> {
> int key_pid = G_pid;
> unsigned long long *p_pid, pid;
>
> char fmt[] = "%d vs %d\n";
>
> p_pid = map_lookup_elem(&global_vars, &key_pid);
> if (!p_pid)
> return 0;
>
> pid = get_current_pid_tgid() & 0xffffffff;
>
> if (*p_pid != pid)
> return 0;
> return 1;
> }
>
> static inline void print_leak(int type)
> {
> unsigned long long *p_cnt;
> char fmt[] = "Type %d leak %llu\n";
>
> p_cnt = map_lookup_elem(&global_vars, &type);
> if (!p_cnt)
> return;
> bpf_trace_printk(fmt, sizeof(fmt), type - G_LEAK_START, *p_cnt);
> }
>
> SEC("execve=sys_execve")
> int execve(void *ctx)
> {
> char name[sizeof(u64)] = "";
> char name_perf[sizeof(u64)] = "perf";
> unsigned long long *p_pid, pid;
> int key = G_pid;
>
> p_pid = map_lookup_elem(&global_vars, &key);
> if (!p_pid)
> return 0;
> pid = *p_pid;
> if (pid)
> return 0;
> if (get_current_comm(name, sizeof(name)))
> return 0;
> if (*(u32*)name != *(u32*)name_perf)
> return 0;
>
> pid = get_current_pid_tgid() & 0xffffffff;
> map_update_elem(&global_vars, &key, &pid, BPF_ANY);
> return 0;
> }
>
> static inline int func_exit(void *ctx)
> {
> if (!filter_pid())
> return 0;
> print_leak(G_dso_leak);
> print_leak(G_map_group_leak);
> return 0;
> }
>
> SEC("exit_group=sys_exit_group")
> int exit_group(void *ctx)
> {
> return func_exit(ctx);
> }
>
> SEC("exit_=sys_exit")
> int exit_(void *ctx)
> {
> return func_exit(ctx);
> }
> static inline void inc_leak_from_type(int type, int n)
> {
> u64 *p_cnt, cnt;
>
> type += G_LEAK_START;
> if (type >= G_LEAK_END)
> return;
>
> p_cnt = map_lookup_elem(&global_vars, &type);
> if (!p_cnt)
> cnt = n;
> else
> cnt = *p_cnt + n;
>
> map_update_elem(&global_vars, &type, &cnt, BPF_ANY);
> return;
> }
>
> SEC("exec=/home/wangnan/perf;"
> "refcnt_init=__refcnt__init n obj type")
> int refcnt_init(void *ctx, int err, int n, void *obj, int type)
> {
> if (!filter_pid())
> return 0;
> inc_leak_from_type(type, n);
> return 0;
> }
> SEC("exec=/home/wangnan/perf;"
> "refcnt_del=refcnt__delete obj type")
> int refcnt_del(void *ctx, int err, void *obj, int type)
> {
> if (!filter_pid())
> return 0;
> return 0;
> }
>
> SEC("exec=/home/wangnan/perf;"
> "refcnt_get=__refcnt__get obj type")
> int refcnt_get(void *ctx, int err, void *obj, int type)
> {
> if (!filter_pid())
> return 0;
> inc_leak_from_type(type, 1);
> return 0;
> }
>
> SEC("exec=/home/wangnan/perf;"
> "refcnt_put=__refcnt__put refcnt obj type")
> int refcnt_put(void *ctx, int err, void *refcnt, void *obj, int type)
> {
> int old_cnt = -1;
>
> if (!filter_pid())
> return 0;
> if (bpf_probe_read(&old_cnt, sizeof(int), refcnt))
> return 0;
> if (old_cnt)
> inc_leak_from_type(type, -1);
> return 0;
> }
>
--
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]
| From | "Wangnan (F)" <wangnan0@huawei.com> |
|---|---|
| Date | 2015-12-11 03:00 +0100 |
| Message-ID | <qEla2-5TY-19@gated-at.bofh.it> |
| In reply to | #1288586 |
On 2015/12/10 23:12, 'Arnaldo Carvalho de Melo' wrote:
[SNIP]
> But this requires having these special refcnt__ routines, that will make
> tools/perf/ code patterns for reference counts look different that the
> refcount patterns in the kernel :-\
>
> And would be a requirement to change the observed workload :-\
>
> Is this _strictly_ required?
No. The requirement should be:
1. The create/get/put/delete functions are non-inline (because dwarf info
is not as reliable as symbol);
2. From their argument list, we can always get the variable we need (the
pointer of objects, the value of refcnt, etc.)
We don't have to use this refcnt things.
> Can't we, for a project like perf, where we
> know where some refcount (say, the one for 'struct thread') gets
> initialized, use that (thread__new()) and then hook into thread__get and
> thread__put and then use the destructor, thread__delete() as the place
> to dump leaks?
>
I think it is possible. If we can abstract a common pattern about it,
we can provide a perf subcommand which we can deal with generic alloc/free
pattern. I'll put it on my todo-list. Currently we are focusing on perf
daemonization.
Thank you.
--
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]
| From | 平松雅巳 / HIRAMATU，MASAMI <masami.hiramatsu.pt@hitachi.com> |
|---|---|
| Date | 2015-12-11 03:10 +0100 |
| Message-ID | <qEljH-6cz-7@gated-at.bofh.it> |
| In reply to | #1289056 |
RnJvbTogV2FuZ25hbiAoRikgW21haWx0bzp3YW5nbmFuMEBodWF3ZWkuY29tXQ0KPk9uIDIwMTUv MTIvMTAgMjM6MTIsICdBcm5hbGRvIENhcnZhbGhvIGRlIE1lbG8nIHdyb3RlOg0KPg0KPltTTklQ XQ0KPj4gQnV0IHRoaXMgcmVxdWlyZXMgaGF2aW5nIHRoZXNlIHNwZWNpYWwgcmVmY250X18gcm91 dGluZXMsIHRoYXQgd2lsbCBtYWtlDQo+PiB0b29scy9wZXJmLyBjb2RlIHBhdHRlcm5zIGZvciBy ZWZlcmVuY2UgY291bnRzIGxvb2sgZGlmZmVyZW50IHRoYXQgdGhlDQo+PiByZWZjb3VudCBwYXR0 ZXJucyBpbiB0aGUga2VybmVsIDotXA0KPj4NCj4+IEFuZCB3b3VsZCBiZSBhIHJlcXVpcmVtZW50 IHRvIGNoYW5nZSB0aGUgb2JzZXJ2ZWQgd29ya2xvYWQgOi1cDQo+Pg0KPj4gSXMgdGhpcyBfc3Ry aWN0bHlfIHJlcXVpcmVkPw0KPg0KPk5vLiBUaGUgcmVxdWlyZW1lbnQgc2hvdWxkIGJlOg0KPg0K PiAgMS4gVGhlIGNyZWF0ZS9nZXQvcHV0L2RlbGV0ZSBmdW5jdGlvbnMgYXJlIG5vbi1pbmxpbmUg KGJlY2F1c2UgZHdhcmYgaW5mbw0KPiAgICAgaXMgbm90IGFzIHJlbGlhYmxlIGFzIHN5bWJvbCk7 DQo+ICAyLiBGcm9tIHRoZWlyIGFyZ3VtZW50IGxpc3QsIHdlIGNhbiBhbHdheXMgZ2V0IHRoZSB2 YXJpYWJsZSB3ZSBuZWVkICh0aGUNCj4gICAgIHBvaW50ZXIgb2Ygb2JqZWN0cywgdGhlIHZhbHVl IG9mIHJlZmNudCwgZXRjLikNCg0KSG93ZXZlciwgd2UgaGF2ZSB0byBjdXN0b21pemUgaXQgZm9y IGVhY2ggYXBwbGljYXRpb24uIFBlcmYgaXRzZWxmIG1pZ2h0IGJlIE9LDQpidXQgb3RoZXJzIG1p Z2h0IGhhdmUgZGlmZmVyZW50IGltcGxlbWVudGF0aW9uLg0KDQpUaGFua3MsDQoNCg== -- 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]
| From | "Wangnan (F)" <wangnan0@huawei.com> |
|---|---|
| Date | 2015-12-11 03:30 +0100 |
| Message-ID | <qElD3-6k8-11@gated-at.bofh.it> |
| In reply to | #1289060 |
On 2015/12/11 10:08, 平松雅巳 / HIRAMATU，MASAMI wrote:
> From: Wangnan (F) [mailto:wangnan0@huawei.com]
>> On 2015/12/10 23:12, 'Arnaldo Carvalho de Melo' wrote:
>>
>> [SNIP]
>>> But this requires having these special refcnt__ routines, that will make
>>> tools/perf/ code patterns for reference counts look different that the
>>> refcount patterns in the kernel :-\
>>>
>>> And would be a requirement to change the observed workload :-\
>>>
>>> Is this _strictly_ required?
>> No. The requirement should be:
>>
>> 1. The create/get/put/delete functions are non-inline (because dwarf info
>> is not as reliable as symbol);
>> 2. From their argument list, we can always get the variable we need (the
>> pointer of objects, the value of refcnt, etc.)
> However, we have to customize it for each application. Perf itself might be OK
> but others might have different implementation.
If limited to pairwise operations ({{m,c}alloc,strdup} vs free, open vs
close),
I think it is possible to abstract a uniformed pattern.
Thank you.
--
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]
| From | 平松雅巳 / HIRAMATU，MASAMI <masami.hiramatsu.pt@hitachi.com> |
|---|---|
| Date | 2015-12-11 03:20 +0100 |
| Message-ID | <qEltn-6fP-3@gated-at.bofh.it> |
| In reply to | #1288586 |
RnJvbTogJ0FybmFsZG8gQ2FydmFsaG8gZGUgTWVsbycgW21haWx0bzphY21lQGtlcm5lbC5vcmdd DQo+DQo+QnV0IHRoaXMgcmVxdWlyZXMgaGF2aW5nIHRoZXNlIHNwZWNpYWwgcmVmY250X18gcm91 dGluZXMsIHRoYXQgd2lsbCBtYWtlDQo+dG9vbHMvcGVyZi8gY29kZSBwYXR0ZXJucyBmb3IgcmVm ZXJlbmNlIGNvdW50cyBsb29rIGRpZmZlcmVudCB0aGF0IHRoZQ0KPnJlZmNvdW50IHBhdHRlcm5z IGluIHRoZSBrZXJuZWwgOi1cDQoNCkJUVywgSSB0aGluayBldmVuIHdpdGhvdXQgdGhlIHJlZmNu dCBkZWJ1Z2dlciwgd2UnZCBiZXR0ZXIgaW50cm9kdWNlIHRoaXMNCmtpbmQgQVBJIHRvIHVuaWZ5 IHRoZSByZWZjbnQgb3BlcmF0aW9uIGluIHBlcmYgY29kZS4gQXMgSSBzYWlkLCB3ZSBoYXZlIG1h bnkNCm1pc2NvZGluZ3Mgb24gY3VycmVudCBpbXBsZW1lbnRhdGlvbi4gVW5pZnlpbmcgdGhlIEFQ SSBjYW4gZW5mb3JjZSBkZXZlbG9wZXJzDQp0byBhdm9pZCBzdWNoIG1pc2NvZGluZ3MuDQoNClRo YW5rIHlvdSwNCg0KDQo= -- 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]
| From | "Wangnan (F)" <wangnan0@huawei.com> |
|---|---|
| Date | 2015-12-11 03:50 +0100 |
| Message-ID | <qElWp-6r8-7@gated-at.bofh.it> |
| In reply to | #1289062 |
On 2015/12/11 10:15, 平松雅巳 / HIRAMATU，MASAMI wrote:
> From: 'Arnaldo Carvalho de Melo' [mailto:acme@kernel.org]
>> But this requires having these special refcnt__ routines, that will make
>> tools/perf/ code patterns for reference counts look different that the
>> refcount patterns in the kernel :-\
> BTW, I think even without the refcnt debugger, we'd better introduce this
> kind API to unify the refcnt operation in perf code. As I said, we have many
> miscodings on current implementation. Unifying the API can enforce developers
> to avoid such miscodings.
>
> Thank you,
>
I tried this problem in another way, I'd like to share it here.
First: create two uprobes:
# ./perf probe --exec /home/wangnan/perf dso__new%return %ax
Added new event:
probe_perf:dso__new (on dso__new%return in /home/wangnan/perf with %ax)
You can now use it in all perf tools, such as:
perf record -e probe_perf:dso__new -aR sleep 1
# ./perf probe --exec /home/wangnan/perf dso__delete dso
Added new event:
probe_perf:dso__delete (on dso__delete in /home/wangnan/perf with dso)
You can now use it in all perf tools, such as:
perf record -e probe_perf:dso__delete -aR sleep 1
Then start test:
# ./perf record -g -e probe_perf:dso__new -e probe_perf:dso__delete
./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
[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.048 MB perf.data (178 samples) ]
From the perf report result I know two dso objects are leak:
90 probe_perf:dso__new `
88 probe_perf:dso__delete
Then convert output to CTF:
$ ./perf data convert --to-ctf ./out.ctf
With a python script, try to find the exact leak objects (I'm not good
at python,
I believe we can do this with much shorter script):
$ cat refcnt.py
from babeltrace import TraceCollection
tc = TraceCollection();
tc.add_trace('./out.ctf', 'ctf')
objs = {}
for event in tc.events:
if event.name.startswith('probe_perf:dso__new'):
if event['arg1'] not in objs:
objs[event['arg1']] = 1
if event.name.startswith('probe_perf:dso__delete'):
if event['dso'] in objs:
objs[event['dso']] = 0
for x in objs:
if objs[x] is 0:
continue
print("0x%x" % x)
$ python3 ./refcnt.py
0x34cb350
0x34d4640
Then from perf script's result, search for the two address:
perf 23203 [004] 665244.170387: probe_perf:dso__new: (4aaee0 <- 50a5eb)
arg1=0x34cb350
10a5eb dso__load_sym (/home/w00229757/perf)
af42d dso__load_vmlinux (/home/w00229757/perf)
af58c dso__load_vmlinux_path (/home/w00229757/perf)
10c40a open_debuginfo (/home/w00229757/perf)
111d39 convert_perf_probe_events (/home/w00229757/perf)
603a7 __cmd_probe.isra.3 (/home/w00229757/perf)
60a44 cmd_probe (/home/w00229757/perf)
7f6d1 run_builtin (/home/w00229757/perf)
33056 main (/home/w00229757/perf)
21bd5 __libc_start_main
(/tmp/oxygen_root-w00229757/lib64/libc-2.18.so)
perf 23203 [004] 665244.170679: probe_perf:dso__new: (4aaee0 <- 50a5eb)
arg1=0x34d4640
10a5eb dso__load_sym (/home/w00229757/perf)
af42d dso__load_vmlinux (/home/w00229757/perf)
af58c dso__load_vmlinux_path (/home/w00229757/perf)
10c40a open_debuginfo (/home/w00229757/perf)
111d39 convert_perf_probe_events (/home/w00229757/perf)
603a7 __cmd_probe.isra.3 (/home/w00229757/perf)
60a44 cmd_probe (/home/w00229757/perf)
7f6d1 run_builtin (/home/w00229757/perf)
33056 main (/home/w00229757/perf)
21bd5 __libc_start_main
(/tmp/oxygen_root-w00229757/lib64/libc-2.18.so)
Thank you.
--
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]
| From | "Wangnan (F)" <wangnan0@huawei.com> |
|---|---|
| Date | 2015-12-11 04:00 +0100 |
| Message-ID | <qEm66-6uo-19@gated-at.bofh.it> |
| In reply to | #1289071 |
On 2015/12/11 10:42, Wangnan (F) wrote: > > > On 2015/12/11 10:15, 平松雅巳 / HIRAMATU，MASAMI wrote: >> From: 'Arnaldo Carvalho de Melo' [mailto:acme@kernel.org] >>> But this requires having these special refcnt__ routines, that will >>> make >>> tools/perf/ code patterns for reference counts look different that the >>> refcount patterns in the kernel :-\ >> BTW, I think even without the refcnt debugger, we'd better introduce >> this >> kind API to unify the refcnt operation in perf code. As I said, we >> have many >> miscodings on current implementation. Unifying the API can enforce >> developers >> to avoid such miscodings. >> >> Thank you, >> > > I tried this problem in another way, I'd like to share it here. > > First: create two uprobes: > > # ./perf probe --exec /home/wangnan/perf dso__new%return %ax > Added new event: > probe_perf:dso__new (on dso__new%return in /home/wangnan/perf with > %ax) > > You can now use it in all perf tools, such as: > > perf record -e probe_perf:dso__new -aR sleep 1 > > # ./perf probe --exec /home/wangnan/perf dso__delete dso > Added new event: > probe_perf:dso__delete (on dso__delete in /home/wangnan/perf with dso) > > You can now use it in all perf tools, such as: > > perf record -e probe_perf:dso__delete -aR sleep 1 > > > Then start test: > > # ./perf record -g -e probe_perf:dso__new -e probe_perf:dso__delete > ./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 > > [ perf record: Woken up 1 times to write data ] > [ perf record: Captured and wrote 0.048 MB perf.data (178 samples) ] > > > From the perf report result I know two dso objects are leak: > > 90 probe_perf:dso__new ` > 88 probe_perf:dso__delete > The above result is gotten from yesterday's perf/core. I also tried today's perf/core and get: 90 probe_perf:dso__new ` 90 probe_perf:dso__delete So we fix these leak. Thank you. -- 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]
| From | 平松雅巳 / HIRAMATU，MASAMI <masami.hiramatsu.pt@hitachi.com> |
|---|---|
| Date | 2015-12-11 05:00 +0100 |
| Message-ID | <qEn2a-7bp-3@gated-at.bofh.it> |
| In reply to | #1289071 |
RnJvbTogV2FuZ25hbiAoRikgW21haWx0bzp3YW5nbmFuMEBodWF3ZWkuY29tXQ0KPg0KPg0KPk9u IDIwMTUvMTIvMTEgMTA6MTUsIOW5s+advumbheW3syAvIEhJUkFNQVRV77yMTUFTQU1JIHdyb3Rl Og0KPj4gRnJvbTogJ0FybmFsZG8gQ2FydmFsaG8gZGUgTWVsbycgW21haWx0bzphY21lQGtlcm5l bC5vcmddDQo+Pj4gQnV0IHRoaXMgcmVxdWlyZXMgaGF2aW5nIHRoZXNlIHNwZWNpYWwgcmVmY250 X18gcm91dGluZXMsIHRoYXQgd2lsbCBtYWtlDQo+Pj4gdG9vbHMvcGVyZi8gY29kZSBwYXR0ZXJu cyBmb3IgcmVmZXJlbmNlIGNvdW50cyBsb29rIGRpZmZlcmVudCB0aGF0IHRoZQ0KPj4+IHJlZmNv dW50IHBhdHRlcm5zIGluIHRoZSBrZXJuZWwgOi1cDQo+PiBCVFcsIEkgdGhpbmsgZXZlbiB3aXRo b3V0IHRoZSByZWZjbnQgZGVidWdnZXIsIHdlJ2QgYmV0dGVyIGludHJvZHVjZSB0aGlzDQo+PiBr aW5kIEFQSSB0byB1bmlmeSB0aGUgcmVmY250IG9wZXJhdGlvbiBpbiBwZXJmIGNvZGUuIEFzIEkg c2FpZCwgd2UgaGF2ZSBtYW55DQo+PiBtaXNjb2RpbmdzIG9uIGN1cnJlbnQgaW1wbGVtZW50YXRp b24uIFVuaWZ5aW5nIHRoZSBBUEkgY2FuIGVuZm9yY2UgZGV2ZWxvcGVycw0KPj4gdG8gYXZvaWQg c3VjaCBtaXNjb2RpbmdzLg0KPj4NCj4+IFRoYW5rIHlvdSwNCj4+DQo+DQo+SSB0cmllZCB0aGlz IHByb2JsZW0gaW4gYW5vdGhlciB3YXksIEknZCBsaWtlIHRvIHNoYXJlIGl0IGhlcmUuDQoNCk5v LCB3aGF0IEkgc2FpZCBoZXJlIGlzIHRoZSBpc3N1ZSBvbiB0aGUgY29kaW5nIHBvbGljeS4gSWYg d2UgaGF2ZSBubyBzcGVjaWFsDQpyZWFzb24gdGhhdCBlYWNoIG9iamVjdCAoY2xhc3MpIGhhcyBp dHMgb3duIHJlZmVyZW5jZSBjb3VudGVyIGltcGxlbWVudGF0aW9uLA0Kd2UnZCBiZXR0ZXIgdW5p ZnkgaXQgZm9yIGJpbmRpbmcgdGhlbSB0byBvbmUgcG9saWN5Lg0KDQoNCj4NCj5UaGVuIGZyb20g cGVyZiBzY3JpcHQncyByZXN1bHQsIHNlYXJjaCBmb3IgdGhlIHR3byBhZGRyZXNzOg0KPg0KPnBl cmYgMjMyMDMgWzAwNF0gNjY1MjQ0LjE3MDM4NzogcHJvYmVfcGVyZjpkc29fX25ldzogKDRhYWVl MCA8LSA1MGE1ZWIpDQo+YXJnMT0weDM0Y2IzNTANCj4gICAgICAgICAgICAgICAgICAgMTBhNWVi IGRzb19fbG9hZF9zeW0gKC9ob21lL3cwMDIyOTc1Ny9wZXJmKQ0KPiAgICAgICAgICAgICAgICAg ICAgYWY0MmQgZHNvX19sb2FkX3ZtbGludXggKC9ob21lL3cwMDIyOTc1Ny9wZXJmKQ0KPiAgICAg ICAgICAgICAgICAgICAgYWY1OGMgZHNvX19sb2FkX3ZtbGludXhfcGF0aCAoL2hvbWUvdzAwMjI5 NzU3L3BlcmYpDQo+ICAgICAgICAgICAgICAgICAgIDEwYzQwYSBvcGVuX2RlYnVnaW5mbyAoL2hv bWUvdzAwMjI5NzU3L3BlcmYpDQo+ICAgICAgICAgICAgICAgICAgIDExMWQzOSBjb252ZXJ0X3Bl cmZfcHJvYmVfZXZlbnRzICgvaG9tZS93MDAyMjk3NTcvcGVyZikNCj4gICAgICAgICAgICAgICAg ICAgIDYwM2E3IF9fY21kX3Byb2JlLmlzcmEuMyAoL2hvbWUvdzAwMjI5NzU3L3BlcmYpDQo+ICAg ICAgICAgICAgICAgICAgICA2MGE0NCBjbWRfcHJvYmUgKC9ob21lL3cwMDIyOTc1Ny9wZXJmKQ0K PiAgICAgICAgICAgICAgICAgICAgN2Y2ZDEgcnVuX2J1aWx0aW4gKC9ob21lL3cwMDIyOTc1Ny9w ZXJmKQ0KPiAgICAgICAgICAgICAgICAgICAgMzMwNTYgbWFpbiAoL2hvbWUvdzAwMjI5NzU3L3Bl cmYpDQo+ICAgICAgICAgICAgICAgICAgICAyMWJkNSBfX2xpYmNfc3RhcnRfbWFpbg0KPigvdG1w L294eWdlbl9yb290LXcwMDIyOTc1Ny9saWI2NC9saWJjLTIuMTguc28pDQo+DQo+cGVyZiAyMzIw MyBbMDA0XSA2NjUyNDQuMTcwNjc5OiBwcm9iZV9wZXJmOmRzb19fbmV3OiAoNGFhZWUwIDwtIDUw YTVlYikNCj5hcmcxPTB4MzRkNDY0MA0KPiAgICAgICAgICAgICAgICAgICAxMGE1ZWIgZHNvX19s b2FkX3N5bSAoL2hvbWUvdzAwMjI5NzU3L3BlcmYpDQo+ICAgICAgICAgICAgICAgICAgICBhZjQy ZCBkc29fX2xvYWRfdm1saW51eCAoL2hvbWUvdzAwMjI5NzU3L3BlcmYpDQo+ICAgICAgICAgICAg ICAgICAgICBhZjU4YyBkc29fX2xvYWRfdm1saW51eF9wYXRoICgvaG9tZS93MDAyMjk3NTcvcGVy ZikNCj4gICAgICAgICAgICAgICAgICAgMTBjNDBhIG9wZW5fZGVidWdpbmZvICgvaG9tZS93MDAy Mjk3NTcvcGVyZikNCj4gICAgICAgICAgICAgICAgICAgMTExZDM5IGNvbnZlcnRfcGVyZl9wcm9i ZV9ldmVudHMgKC9ob21lL3cwMDIyOTc1Ny9wZXJmKQ0KPiAgICAgICAgICAgICAgICAgICAgNjAz YTcgX19jbWRfcHJvYmUuaXNyYS4zICgvaG9tZS93MDAyMjk3NTcvcGVyZikNCj4gICAgICAgICAg ICAgICAgICAgIDYwYTQ0IGNtZF9wcm9iZSAoL2hvbWUvdzAwMjI5NzU3L3BlcmYpDQo+ICAgICAg ICAgICAgICAgICAgICA3ZjZkMSBydW5fYnVpbHRpbiAoL2hvbWUvdzAwMjI5NzU3L3BlcmYpDQo+ ICAgICAgICAgICAgICAgICAgICAzMzA1NiBtYWluICgvaG9tZS93MDAyMjk3NTcvcGVyZikNCj4g ICAgICAgICAgICAgICAgICAgIDIxYmQ1IF9fbGliY19zdGFydF9tYWluDQo+KC90bXAvb3h5Z2Vu X3Jvb3QtdzAwMjI5NzU3L2xpYjY0L2xpYmMtMi4xOC5zbykNCg0KQlRXLCBhY3R1YWxseSB0aGUg YW5hbHlzaXMgb2YgdGhpcyBsZXZlbCAob2JqZWN0IG1lbW9yeSBsZWFrcykgY2FuIGJlIGZvdW5k DQogKGFuZCBhbmFseXplZCkgYnkgdmFsZ3JpbmQgKHBsZWFzZSB0cnkgaXQpLg0KVGhlIHByb2Js ZW0gaXMgbm90IGhhcHBlbiBvbmx5IHdoZW4gdGhlIGNyZWF0aW5nIHNpdGUsIGJ1dCB0aGUgcmVm Y250IGNhbg0KbGVhayB3aGVuIHByb2Nlc3NpbmcgdGhlIG9iamVjdCBhZnRlcndhcmRzLiBUaGF0 IGlzIHRoZSByZWFzb24gd2h5IEkgbWFkZQ0KcmVmY250IGRlYnVnZ2VyIHRvIHN1cHBvcnQgcHJl Y2lzZSBiYWNrdHJhY2luZyBmb3IgZWFjaCBvcGVyYXRpb24uDQoNClRoYW5rcywNCg0K -- 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]
| From | Arnaldo Carvalho de Melo <acme@kernel.org> |
|---|---|
| Date | 2015-12-11 23:30 +0100 |
| Message-ID | <qEEmm-21X-29@gated-at.bofh.it> |
| In reply to | #1287507 |
Em Wed, Dec 09, 2015 at 10:41:38AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Wed, Dec 09, 2015 at 11:10:48AM +0900, Masami Hiramatsu escreveu:
> > 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.
So, I think I fixed the thread->refcnt case, please take a look at my
perf/core branch, more specifically this patch:
From a9a8442c64a86599600ecb2ae0f296b5c93e2a98 Mon Sep 17 00:00:00 2001
From: Arnaldo Carvalho de Melo <acme@redhat.com>
Date: Fri, 11 Dec 2015 19:11:23 -0300
Subject: [PATCH 1/1] perf thread: Fix reference count initial state
We should always return from thread__new(), the constructor, with the
object with a reference count of one, so that:
struct thread *thread = thread__new();
thread__put(thread);
Will call thread__delete().
If any reference is made to that 'thread' variable, it better use
thread__get(thread) to hold a reference.
We were returning with thread->refcnt set to zero, fix it and some cases
where thread__delete() was being called, which were not a problem
because just one reference was being used, now that we set it to 1, use
thread__put() instead.
Reported-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Wang Nan <wangnan0@huawei.com>
Link: http://lkml.kernel.org/n/tip-4b9mkuk66to4ecckpmpvqx6s@git.kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/util/intel-pt.c | 4 ++--
tools/perf/util/machine.c | 19 ++++++++++++-------
tools/perf/util/thread.c | 10 ++++++++--
3 files changed, 22 insertions(+), 11 deletions(-)
diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
index 97f963a3dcb9..81a2eb77ba7f 100644
--- a/tools/perf/util/intel-pt.c
+++ b/tools/perf/util/intel-pt.c
@@ -1744,7 +1744,7 @@ static void intel_pt_free(struct perf_session *session)
auxtrace_heap__free(&pt->heap);
intel_pt_free_events(session);
session->auxtrace = NULL;
- thread__delete(pt->unknown_thread);
+ thread__put(pt->unknown_thread);
free(pt);
}
@@ -2153,7 +2153,7 @@ int intel_pt_process_auxtrace_info(union perf_event *event,
return 0;
err_delete_thread:
- thread__delete(pt->unknown_thread);
+ thread__zput(pt->unknown_thread);
err_free_queues:
intel_pt_log_disable();
auxtrace_queues__free(&pt->queues);
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 1407d5107480..ad79297c76c8 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -352,13 +352,18 @@ static void machine__update_thread_pid(struct machine *machine,
}
th->mg = map_groups__get(leader->mg);
-
+out_put:
+ thread__put(leader);
return;
-
out_err:
pr_err("Failed to join map groups for %d:%d\n", th->pid_, th->tid);
+ goto out_put;
}
+/*
+ * Caller must eventually drop thread->refcnt returned with a successfull
+ * lookup/new thread inserted.
+ */
static struct thread *____machine__findnew_thread(struct machine *machine,
pid_t pid, pid_t tid,
bool create)
@@ -376,7 +381,7 @@ static struct thread *____machine__findnew_thread(struct machine *machine,
if (th != NULL) {
if (th->tid == tid) {
machine__update_thread_pid(machine, th, pid);
- return th;
+ return thread__get(th);
}
machine->last_match = NULL;
@@ -389,7 +394,7 @@ static struct thread *____machine__findnew_thread(struct machine *machine,
if (th->tid == tid) {
machine->last_match = th;
machine__update_thread_pid(machine, th, pid);
- return th;
+ return thread__get(th);
}
if (tid < th->tid)
@@ -417,7 +422,7 @@ static struct thread *____machine__findnew_thread(struct machine *machine,
if (thread__init_map_groups(th, machine)) {
rb_erase_init(&th->rb_node, &machine->threads);
RB_CLEAR_NODE(&th->rb_node);
- thread__delete(th);
+ thread__put(th);
return NULL;
}
/*
@@ -441,7 +446,7 @@ struct thread *machine__findnew_thread(struct machine *machine, pid_t pid,
struct thread *th;
pthread_rwlock_wrlock(&machine->threads_lock);
- th = thread__get(__machine__findnew_thread(machine, pid, tid));
+ th = __machine__findnew_thread(machine, pid, tid);
pthread_rwlock_unlock(&machine->threads_lock);
return th;
}
@@ -451,7 +456,7 @@ struct thread *machine__find_thread(struct machine *machine, pid_t pid,
{
struct thread *th;
pthread_rwlock_rdlock(&machine->threads_lock);
- th = thread__get(____machine__findnew_thread(machine, pid, tid, false));
+ th = ____machine__findnew_thread(machine, pid, tid, false);
pthread_rwlock_unlock(&machine->threads_lock);
return th;
}
diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
index 0a9ae8014729..dfd00c6dad6e 100644
--- a/tools/perf/util/thread.c
+++ b/tools/perf/util/thread.c
@@ -19,8 +19,10 @@ int thread__init_map_groups(struct thread *thread, struct machine *machine)
thread->mg = map_groups__new(machine);
} else {
leader = __machine__findnew_thread(machine, pid, pid);
- if (leader)
+ if (leader) {
thread->mg = map_groups__get(leader->mg);
+ thread__put(leader);
+ }
}
return thread->mg ? 0 : -1;
@@ -53,7 +55,7 @@ struct thread *thread__new(pid_t pid, pid_t tid)
goto err_thread;
list_add(&comm->list, &thread->comm_list);
- atomic_set(&thread->refcnt, 0);
+ atomic_set(&thread->refcnt, 1);
RB_CLEAR_NODE(&thread->rb_node);
}
@@ -95,6 +97,10 @@ struct thread *thread__get(struct thread *thread)
void thread__put(struct thread *thread)
{
if (thread && atomic_dec_and_test(&thread->refcnt)) {
+ /*
+ * Remove it from the dead_threads list, as last reference
+ * is gone.
+ */
list_del_init(&thread->node);
thread__delete(thread);
}
--
2.1.0
--
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] | [standalone]
Page 2 of 2 — ← Prev page 1 [2]
Back to top | Article view | linux.kernel
csiph-web