[PATCH 1/4] perf tools: Allow use of an exclusive option more than once

[PATCH 1/4] perf tools: Allow use of an exclusive option more than once

[Namhyung Kim]

The exclusive options are to prohibit use of conflicting options at the
same time.  But it had a side effect that it also limits a such option
can be used at most once.  Currently the only user of the flag is perf
probe and it allows to use such options more than once, but when one
tries to use it, perf will fail like below:

  $ sudo perf probe -x /lib/libc-2.20.so --add malloc --add free
    Error: option `add' cannot be used with add
  ...

Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/parse-options.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/parse-options.c b/tools/perf/util/parse-options.c
index f62dee7bd924..4a015f77e2b5 100644
--- a/tools/perf/util/parse-options.c
+++ b/tools/perf/util/parse-options.c
@@ -46,7 +46,7 @@ static int get_value(struct parse_opt_ctx_t *p,
 		return opterror(opt, "is not usable", flags);
 
 	if (opt->flags & PARSE_OPT_EXCLUSIVE) {
-		if (p->excl_opt) {
+		if (p->excl_opt && p->excl_opt != opt) {
 			char msg[128];
 
 			if (((flags & OPT_SHORT) && p->excl_opt->short_name) ||
-- 
2.2.1

[PATCH 2/4] perf probe: Do not rely on map__load() filter to find symbols

[Namhyung Kim]

The find_probe_trace_events_from_map() searches matching symbol from a
map (so from a backing dso).  For uprobes, it'll create a new map (and
dso) and loads it using a filter.  It's a little bit inefficient in that
it'll read out the symbol table everytime but works well anyway.

For kprobes however, it'll reuse existing kernel map which might be
loaded before.  In this case map__load() just returns with no result.
It makes kprobes always failed to find symbol even if it exists in the
map (dso).

To fix it, use map__find_symbol_by_name() instead.  It'll load a map
with full symbols and sorts them by name.  It needs to search sibing
nodes since there can be multiple (local) symbols with same name.  Now
resulting symbol references are saved in the funcs list.

Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/probe-event.c | 101 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 87 insertions(+), 14 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 7f9b8632e433..e5af16988791 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -2191,20 +2191,86 @@ static int __add_probe_trace_events(struct perf_probe_event *pev,
 	return ret;
 }
 
-static char *looking_function_name;
-static int num_matched_functions;
+struct symbol_entry {
+	struct list_head node;
+	struct symbol *sym;
+};
 
-static int probe_function_filter(struct map *map __maybe_unused,
-				      struct symbol *sym)
+/* returns 1 if symbol was added, 0 if symbol was skipped, -1 if error */
+static int add_symbol_entry(struct symbol *sym, struct list_head *head)
 {
-	if ((sym->binding == STB_GLOBAL || sym->binding == STB_LOCAL) &&
-	    strcmp(looking_function_name, sym->name) == 0) {
-		num_matched_functions++;
+	struct symbol_entry *ent;
+
+	if (sym->binding != STB_GLOBAL && sym->binding != STB_LOCAL)
 		return 0;
-	}
+
+	ent = malloc(sizeof(*ent));
+	if (ent == NULL)
+		return -1;
+
+	ent->sym = sym;
+	list_add(&ent->node, head);
 	return 1;
 }
 
+static int find_probe_functions(struct map *map, char *name, struct list_head *head)
+{
+	struct symbol *sym, *orig_sym;
+	struct symbol_entry *ent;
+	struct rb_node *node;
+	int found = 0;
+	int ret;
+
+	sym = map__find_symbol_by_name(map, name, NULL);
+	if (sym == NULL)
+		return 0;
+
+	ret = add_symbol_entry(sym, head);
+	if (ret < 0)
+		goto err;
+
+	found += ret;
+
+	/* search back and forth to find symbols that have same name */
+	orig_sym = sym;
+
+	while ((node = rb_prev(&sym->rb_node)) != NULL) {
+		sym = rb_entry(node, struct symbol, rb_node);
+		if (strcmp(name, sym->name))
+			break;
+
+		ret = add_symbol_entry(sym, head);
+		if (ret < 0)
+			goto err;
+
+		found += ret;
+	}
+
+	sym = orig_sym;
+
+	while ((node = rb_next(&sym->rb_node)) != NULL) {
+		sym = rb_entry(node, struct symbol, rb_node);
+		if (strcmp(name, sym->name))
+			break;
+
+		ret = add_symbol_entry(sym, head);
+		if (ret < 0)
+			goto err;
+
+		found += ret;
+	}
+
+	return found;
+
+err:
+	while (!list_empty(head)) {
+		ent = list_first_entry(head, struct symbol_entry, node);
+		list_del(&ent->node);
+		free(ent);
+	}
+	return 0;
+}
+
 #define strdup_or_goto(str, label)	\
 	({ char *__p = strdup(str); if (!__p) goto label; __p; })
 
@@ -2220,10 +2286,12 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
 	struct kmap *kmap = NULL;
 	struct ref_reloc_sym *reloc_sym = NULL;
 	struct symbol *sym;
-	struct rb_node *nd;
 	struct probe_trace_event *tev;
 	struct perf_probe_point *pp = &pev->point;
 	struct probe_trace_point *tp;
+	LIST_HEAD(funcs);
+	struct symbol_entry *ent;
+	int num_matched_functions;
 	int ret, i;
 
 	/* Init maps of given executable or kernel */
@@ -2240,10 +2308,8 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
 	 * Load matched symbols: Since the different local symbols may have
 	 * same name but different addresses, this lists all the symbols.
 	 */
-	num_matched_functions = 0;
-	looking_function_name = pp->function;
-	ret = map__load(map, probe_function_filter);
-	if (ret || num_matched_functions == 0) {
+	num_matched_functions = find_probe_functions(map, pp->function, &funcs);
+	if (num_matched_functions == 0) {
 		pr_err("Failed to find symbol %s in %s\n", pp->function,
 			target ? : "kernel");
 		ret = -ENOENT;
@@ -2273,7 +2339,7 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
 	}
 
 	ret = 0;
-	map__for_each_symbol(map, sym, nd) {
+	list_for_each_entry(ent, &funcs, node) {
 		tev = (*tevs) + ret;
 		tp = &tev->point;
 		if (ret == num_matched_functions) {
@@ -2282,6 +2348,7 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
 		}
 		ret++;
 
+		sym = ent->sym;
 		if (pp->offset > sym->end - sym->start) {
 			pr_warning("Offset %ld is bigger than the size of %s\n",
 				   pp->offset, sym->name);
@@ -2324,6 +2391,12 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
 	}
 
 out:
+	while (!list_empty(&funcs)) {
+		ent = list_first_entry(&funcs, struct symbol_entry, node);
+		list_del(&ent->node);
+		free(ent);
+	}
+
 	if (map && pev->uprobes) {
 		/* Only when using uprobe(exec) map needs to be released */
 		dso__delete(map->dso);
-- 
2.2.1

[PATCH 3/4] perf probe: Fix probing kretprobes

[Namhyung Kim]

The commit dfef99cd0b2c ("perf probe: Use ref_reloc_sym based address
instead of the symbol name") converts kprobes to use ref_reloc_sym
(i.e. _stext) and offset instead of using symbol's name directly.  So
on my system, adding do_fork ends up with like below:

  $ sudo perf probe -v --add do_fork%return
  probe-definition(0): do_fork%return
  symbol:do_fork file:(null) line:0 offset:0 return:1 lazy:(null)
  0 arguments
  Looking at the vmlinux_path (7 entries long)
  Using /lib/modules/3.17.6-1-ARCH/build/vmlinux for symbols
  Could not open debuginfo. Try to use symbols.
  Opening /sys/kernel/debug/tracing/kprobe_events write=1
  Added new event:
  Writing event: r:probe/do_fork _stext+456136
  Failed to write event: Invalid argument
  Error: Failed to add events. Reason: Operation not permitted (Code: -1)

As you can see, the do_fork was translated to _stext+456136.  This was
because to support (local) symbols that have same name.  But the
problem is that kretprobe requires to be inserted at function start
point so it simply checks whether it's called with offset 0.  And if
not, it'll return with -EINVAL.  You can see it with dmesg.

  $ dmesg | tail -1
    [125621.764103] Return probe must be used without offset.

So we need to use the symbol name instead of ref_reloc_sym in case of
return probes.

Reported-by: Jiri Olsa <jolsa@redhat.com>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/probe-event.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index e5af16988791..6fe5aa357efc 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -2321,7 +2321,7 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
 		goto out;
 	}
 
-	if (!pev->uprobes) {
+	if (!pev->uprobes && !pp->retprobe) {
 		kmap = map__kmap(map);
 		reloc_sym = kmap->ref_reloc_sym;
 		if (!reloc_sym) {
-- 
2.2.1

[PATCH 4/4] perf probe: Propagate error code when write(2) failed

[Namhyung Kim]

When it failed to write probe commands to the probe_event file in
debugfs, it needs to propagate the error code properly.  Current code
blindly uses the return value of the write(2) so it always uses
-1 (-EPERM) and it might confuse users.

Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/probe-event.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 6fe5aa357efc..ddc295885af0 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -2052,9 +2052,11 @@ static int write_probe_trace_event(int fd, struct probe_trace_event *tev)
 	pr_debug("Writing event: %s\n", buf);
 	if (!probe_event_dry_run) {
 		ret = write(fd, buf, strlen(buf));
-		if (ret <= 0)
+		if (ret <= 0) {
+			ret = -errno;
 			pr_warning("Failed to write event: %s\n",
 				   strerror_r(errno, sbuf, sizeof(sbuf)));
+		}
 	}
 	free(buf);
 	return ret;
-- 
2.2.1

Re: [PATCH 4/4] perf probe: Propagate error code when write(2) failed

[Masami Hiramatsu]

(2015/01/10 19:33), Namhyung Kim wrote:
> When it failed to write probe commands to the probe_event file in
> debugfs, it needs to propagate the error code properly.  Current code
> blindly uses the return value of the write(2) so it always uses
> -1 (-EPERM) and it might confuse users.
> 

Good catch! :)

Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>

> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/probe-event.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 6fe5aa357efc..ddc295885af0 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -2052,9 +2052,11 @@ static int write_probe_trace_event(int fd, struct probe_trace_event *tev)
>  	pr_debug("Writing event: %s\n", buf);
>  	if (!probe_event_dry_run) {
>  		ret = write(fd, buf, strlen(buf));
> -		if (ret <= 0)
> +		if (ret <= 0) {
> +			ret = -errno;
>  			pr_warning("Failed to write event: %s\n",
>  				   strerror_r(errno, sbuf, sizeof(sbuf)));
> +		}
>  	}
>  	free(buf);
>  	return ret;
> 


-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH 3/4] perf probe: Fix probing kretprobes

[Jiri Olsa]

On Sat, Jan 10, 2015 at 07:33:47PM +0900, Namhyung Kim wrote:
> The commit dfef99cd0b2c ("perf probe: Use ref_reloc_sym based address
> instead of the symbol name") converts kprobes to use ref_reloc_sym
> (i.e. _stext) and offset instead of using symbol's name directly.  So
> on my system, adding do_fork ends up with like below:
> 
>   $ sudo perf probe -v --add do_fork%return
>   probe-definition(0): do_fork%return
>   symbol:do_fork file:(null) line:0 offset:0 return:1 lazy:(null)
>   0 arguments
>   Looking at the vmlinux_path (7 entries long)
>   Using /lib/modules/3.17.6-1-ARCH/build/vmlinux for symbols
>   Could not open debuginfo. Try to use symbols.
>   Opening /sys/kernel/debug/tracing/kprobe_events write=1
>   Added new event:
>   Writing event: r:probe/do_fork _stext+456136
>   Failed to write event: Invalid argument
>   Error: Failed to add events. Reason: Operation not permitted (Code: -1)
> 
> As you can see, the do_fork was translated to _stext+456136.  This was
> because to support (local) symbols that have same name.  But the
> problem is that kretprobe requires to be inserted at function start
> point so it simply checks whether it's called with offset 0.  And if
> not, it'll return with -EINVAL.  You can see it with dmesg.
> 
>   $ dmesg | tail -1
>     [125621.764103] Return probe must be used without offset.
> 
> So we need to use the symbol name instead of ref_reloc_sym in case of
> return probes.

with your patchset on top of current Arnaldo's perf/core,
I'm still getting the same error:


[root@krava perf]# ./perf probe -a fork_exit=do_fork%return
Added new event:
Failed to write event: Invalid argument
  Error: Failed to add events.
[root@krava perf]# ./perf probe -v --add do_fork%return
probe-definition(0): do_fork%return
symbol:do_fork file:(null) line:0 offset:0 return:1 lazy:(null)
0 arguments
Using /root/.debug/.build-id/60/4c5fd8a6c9202acac3a9e82ecd34b22b8caef6 for symbols
Open Debuginfo file: /root/.debug/.build-id/60/4c5fd8a6c9202acac3a9e82ecd34b22b8caef6
Try to find probe point from debuginfo.
Probe point found: do_fork+0
Found 1 probe_trace_events.
Opening /sys/kernel/debug/tracing/kprobe_events write=1
Added new event:
Writing event: r:probe/do_fork _text+597840
Failed to write event: Invalid argument
  Error: Failed to add events. Reason: Invalid argument (Code: -22)
[root@krava perf]# dmesg | tail -1
[113205.738833] Return probe must be used without offset.


thanks,
jirka

Re: [PATCH 1/4] perf tools: Allow use of an exclusive option more than once

[Masami Hiramatsu]

(2015/01/10 19:33), Namhyung Kim wrote:
> The exclusive options are to prohibit use of conflicting options at the
> same time.  But it had a side effect that it also limits a such option
> can be used at most once.  Currently the only user of the flag is perf
> probe and it allows to use such options more than once, but when one
> tries to use it, perf will fail like below:
> 
>   $ sudo perf probe -x /lib/libc-2.20.so --add malloc --add free
>     Error: option `add' cannot be used with add
>   ...
> 
> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>

Reviewed-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>

Thanks!

> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/parse-options.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/parse-options.c b/tools/perf/util/parse-options.c
> index f62dee7bd924..4a015f77e2b5 100644
> --- a/tools/perf/util/parse-options.c
> +++ b/tools/perf/util/parse-options.c
> @@ -46,7 +46,7 @@ static int get_value(struct parse_opt_ctx_t *p,
>  		return opterror(opt, "is not usable", flags);
>  
>  	if (opt->flags & PARSE_OPT_EXCLUSIVE) {
> -		if (p->excl_opt) {
> +		if (p->excl_opt && p->excl_opt != opt) {
>  			char msg[128];
>  
>  			if (((flags & OPT_SHORT) && p->excl_opt->short_name) ||
> 


-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: Re: [PATCH 3/4] perf probe: Fix probing kretprobes

[Masami Hiramatsu]

(2015/01/12 20:26), Jiri Olsa wrote:
> On Sat, Jan 10, 2015 at 07:33:47PM +0900, Namhyung Kim wrote:
>> The commit dfef99cd0b2c ("perf probe: Use ref_reloc_sym based address
>> instead of the symbol name") converts kprobes to use ref_reloc_sym
>> (i.e. _stext) and offset instead of using symbol's name directly.  So
>> on my system, adding do_fork ends up with like below:
>>
>>   $ sudo perf probe -v --add do_fork%return
>>   probe-definition(0): do_fork%return
>>   symbol:do_fork file:(null) line:0 offset:0 return:1 lazy:(null)
>>   0 arguments
>>   Looking at the vmlinux_path (7 entries long)
>>   Using /lib/modules/3.17.6-1-ARCH/build/vmlinux for symbols
>>   Could not open debuginfo. Try to use symbols.
>>   Opening /sys/kernel/debug/tracing/kprobe_events write=1
>>   Added new event:
>>   Writing event: r:probe/do_fork _stext+456136
>>   Failed to write event: Invalid argument
>>   Error: Failed to add events. Reason: Operation not permitted (Code: -1)
>>
>> As you can see, the do_fork was translated to _stext+456136.  This was
>> because to support (local) symbols that have same name.  But the
>> problem is that kretprobe requires to be inserted at function start
>> point so it simply checks whether it's called with offset 0.  And if
>> not, it'll return with -EINVAL.  You can see it with dmesg.
>>
>>   $ dmesg | tail -1
>>     [125621.764103] Return probe must be used without offset.
>>
>> So we need to use the symbol name instead of ref_reloc_sym in case of
>> return probes.
> 
> with your patchset on top of current Arnaldo's perf/core,
> I'm still getting the same error:
> 
> 
> [root@krava perf]# ./perf probe -a fork_exit=do_fork%return
> Added new event:
> Failed to write event: Invalid argument
>   Error: Failed to add events.
> [root@krava perf]# ./perf probe -v --add do_fork%return
> probe-definition(0): do_fork%return
> symbol:do_fork file:(null) line:0 offset:0 return:1 lazy:(null)
> 0 arguments
> Using /root/.debug/.build-id/60/4c5fd8a6c9202acac3a9e82ecd34b22b8caef6 for symbols
> Open Debuginfo file: /root/.debug/.build-id/60/4c5fd8a6c9202acac3a9e82ecd34b22b8caef6
> Try to find probe point from debuginfo.
> Probe point found: do_fork+0
> Found 1 probe_trace_events.
> Opening /sys/kernel/debug/tracing/kprobe_events write=1
> Added new event:
> Writing event: r:probe/do_fork _text+597840
> Failed to write event: Invalid argument
>   Error: Failed to add events. Reason: Invalid argument (Code: -22)
> [root@krava perf]# dmesg | tail -1
> [113205.738833] Return probe must be used without offset.

Yes, it seems that Namhyung's patch fixes a half of the code path.
We should fix post_process_probe_trace_events() to skip symbol
conversion or we shouldn't use debuginfo when %return is given.
Even if we find inlined function for retprobe, we can't use it
for retprobe...

Thank you,

-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH 3/4] perf probe: Fix probing kretprobes

[Masami Hiramatsu]

(2015/01/10 19:33), Namhyung Kim wrote:
> The commit dfef99cd0b2c ("perf probe: Use ref_reloc_sym based address
> instead of the symbol name") converts kprobes to use ref_reloc_sym
> (i.e. _stext) and offset instead of using symbol's name directly.  So
> on my system, adding do_fork ends up with like below:
> 
>   $ sudo perf probe -v --add do_fork%return
>   probe-definition(0): do_fork%return
>   symbol:do_fork file:(null) line:0 offset:0 return:1 lazy:(null)
>   0 arguments
>   Looking at the vmlinux_path (7 entries long)
>   Using /lib/modules/3.17.6-1-ARCH/build/vmlinux for symbols
>   Could not open debuginfo. Try to use symbols.
>   Opening /sys/kernel/debug/tracing/kprobe_events write=1
>   Added new event:
>   Writing event: r:probe/do_fork _stext+456136
>   Failed to write event: Invalid argument
>   Error: Failed to add events. Reason: Operation not permitted (Code: -1)
> 
> As you can see, the do_fork was translated to _stext+456136.  This was
> because to support (local) symbols that have same name.  But the
> problem is that kretprobe requires to be inserted at function start
> point so it simply checks whether it's called with offset 0.  And if
> not, it'll return with -EINVAL.  You can see it with dmesg.
> 
>   $ dmesg | tail -1
>     [125621.764103] Return probe must be used without offset.
> 
> So we need to use the symbol name instead of ref_reloc_sym in case of
> return probes.

Thanks, but as I pointed in previous mail, this patch fixes just one
code path which doesn't use debuginfo. With debuginfo, perf-probe
executes try_to_find_probe_trace_events() to analyze debuginfo.
We can just skip calling it for retprobes, but I prefer to keep
it works. So just skipping conversion in post_process_probe_trace_events()
is better.

Thank you,

> 
> Reported-by: Jiri Olsa <jolsa@redhat.com>
> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/probe-event.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index e5af16988791..6fe5aa357efc 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -2321,7 +2321,7 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
>  		goto out;
>  	}
>  
> -	if (!pev->uprobes) {
> +	if (!pev->uprobes && !pp->retprobe) {
>  		kmap = map__kmap(map);
>  		reloc_sym = kmap->ref_reloc_sym;
>  		if (!reloc_sym) {
> 


-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH 2/4] perf probe: Do not rely on map__load() filter to find symbols

[Masami Hiramatsu]

(2015/01/10 19:33), Namhyung Kim wrote:
> The find_probe_trace_events_from_map() searches matching symbol from a
> map (so from a backing dso).  For uprobes, it'll create a new map (and
> dso) and loads it using a filter.  It's a little bit inefficient in that
> it'll read out the symbol table everytime but works well anyway.
> 
> For kprobes however, it'll reuse existing kernel map which might be
> loaded before.  In this case map__load() just returns with no result.
> It makes kprobes always failed to find symbol even if it exists in the
> map (dso).
> 
> To fix it, use map__find_symbol_by_name() instead.  It'll load a map
> with full symbols and sorts them by name.  It needs to search sibing
> nodes since there can be multiple (local) symbols with same name.  Now
> resulting symbol references are saved in the funcs list.
> 
> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/probe-event.c | 101 ++++++++++++++++++++++++++++++++++++------
>  1 file changed, 87 insertions(+), 14 deletions(-)
> 
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 7f9b8632e433..e5af16988791 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -2191,20 +2191,86 @@ static int __add_probe_trace_events(struct perf_probe_event *pev,
>  	return ret;
>  }
>  
> -static char *looking_function_name;
> -static int num_matched_functions;
> +struct symbol_entry {
> +	struct list_head node;
> +	struct symbol *sym;
> +};
>  
> -static int probe_function_filter(struct map *map __maybe_unused,
> -				      struct symbol *sym)
> +/* returns 1 if symbol was added, 0 if symbol was skipped, -1 if error */
> +static int add_symbol_entry(struct symbol *sym, struct list_head *head)
>  {
> -	if ((sym->binding == STB_GLOBAL || sym->binding == STB_LOCAL) &&
> -	    strcmp(looking_function_name, sym->name) == 0) {
> -		num_matched_functions++;
> +	struct symbol_entry *ent;
> +
> +	if (sym->binding != STB_GLOBAL && sym->binding != STB_LOCAL)
>  		return 0;
> -	}
> +
> +	ent = malloc(sizeof(*ent));
> +	if (ent == NULL)
> +		return -1;

return -ENOMEM; ?

> +
> +	ent->sym = sym;
> +	list_add(&ent->node, head);
>  	return 1;
>  }
>  
> +static int find_probe_functions(struct map *map, char *name, struct list_head *head)
> +{
> +	struct symbol *sym, *orig_sym;
> +	struct symbol_entry *ent;
> +	struct rb_node *node;
> +	int found = 0;
> +	int ret;
> +
> +	sym = map__find_symbol_by_name(map, name, NULL);
> +	if (sym == NULL)
> +		return 0;
> +
> +	ret = add_symbol_entry(sym, head);
> +	if (ret < 0)
> +		goto err;
> +
> +	found += ret;

If ret always be 1 in successful case, we'd better do "found++" here.
And it also means we can do it shorter as below.

if (add_symbol_entry(sym, head) < 0)
	goto err;
found++;

> +
> +	/* search back and forth to find symbols that have same name */

Hmm, I see. but this code looks no-good sign... Can we have any generic
synonym handling routine?

Other parts looks good to me:)

Thank you,



-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH 1/4] perf tools: Allow use of an exclusive option more than once

[Arnaldo Carvalho de Melo]

Em Mon, Jan 12, 2015 at 08:44:23PM +0900, Masami Hiramatsu escreveu:
> (2015/01/10 19:33), Namhyung Kim wrote:
> > The exclusive options are to prohibit use of conflicting options at the
> > same time.  But it had a side effect that it also limits a such option
> > can be used at most once.  Currently the only user of the flag is perf
> > probe and it allows to use such options more than once, but when one
> > tries to use it, perf will fail like below:
> > 
> >   $ sudo perf probe -x /lib/libc-2.20.so --add malloc --add free
> >     Error: option `add' cannot be used with add
> >   ...
> > 
> > Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> 
> Reviewed-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>

Thanks, applied to perf/urgent
 
> Thanks!
> 
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/util/parse-options.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tools/perf/util/parse-options.c b/tools/perf/util/parse-options.c
> > index f62dee7bd924..4a015f77e2b5 100644
> > --- a/tools/perf/util/parse-options.c
> > +++ b/tools/perf/util/parse-options.c
> > @@ -46,7 +46,7 @@ static int get_value(struct parse_opt_ctx_t *p,
> >  		return opterror(opt, "is not usable", flags);
> >  
> >  	if (opt->flags & PARSE_OPT_EXCLUSIVE) {
> > -		if (p->excl_opt) {
> > +		if (p->excl_opt && p->excl_opt != opt) {
> >  			char msg[128];
> >  
> >  			if (((flags & OPT_SHORT) && p->excl_opt->short_name) ||
> > 
> 
> 
> -- 
> Masami HIRAMATSU
> Software Platform Research Dept. Linux Technology Research Center
> Hitachi, Ltd., Yokohama Research Laboratory
> E-mail: masami.hiramatsu.pt@hitachi.com
> 

Re: [PATCH 4/4] perf probe: Propagate error code when write(2) failed

[Arnaldo Carvalho de Melo]

Em Mon, Jan 12, 2015 at 08:17:52PM +0900, Masami Hiramatsu escreveu:
> (2015/01/10 19:33), Namhyung Kim wrote:
> > When it failed to write probe commands to the probe_event file in
> > debugfs, it needs to propagate the error code properly.  Current code
> > blindly uses the return value of the write(2) so it always uses
> > -1 (-EPERM) and it might confuse users.
> > 
> 
> Good catch! :)
> 
> Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>

Thanks, applied to perf/urgent
 
> > Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/util/probe-event.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> > index 6fe5aa357efc..ddc295885af0 100644
> > --- a/tools/perf/util/probe-event.c
> > +++ b/tools/perf/util/probe-event.c
> > @@ -2052,9 +2052,11 @@ static int write_probe_trace_event(int fd, struct probe_trace_event *tev)
> >  	pr_debug("Writing event: %s\n", buf);
> >  	if (!probe_event_dry_run) {
> >  		ret = write(fd, buf, strlen(buf));
> > -		if (ret <= 0)
> > +		if (ret <= 0) {
> > +			ret = -errno;
> >  			pr_warning("Failed to write event: %s\n",
> >  				   strerror_r(errno, sbuf, sizeof(sbuf)));
> > +		}
> >  	}
> >  	free(buf);
> >  	return ret;
> > 
> 
> 
> -- 
> Masami HIRAMATSU
> Software Platform Research Dept. Linux Technology Research Center
> Hitachi, Ltd., Yokohama Research Laboratory
> E-mail: masami.hiramatsu.pt@hitachi.com
> 

Re: [PATCH 2/4] perf probe: Do not rely on map__load() filter to find symbols

[Namhyung Kim]

On Mon, Jan 12, 2015 at 09:31:30PM +0900, Masami Hiramatsu wrote:
> (2015/01/10 19:33), Namhyung Kim wrote:
> > The find_probe_trace_events_from_map() searches matching symbol from a
> > map (so from a backing dso).  For uprobes, it'll create a new map (and
> > dso) and loads it using a filter.  It's a little bit inefficient in that
> > it'll read out the symbol table everytime but works well anyway.
> > 
> > For kprobes however, it'll reuse existing kernel map which might be
> > loaded before.  In this case map__load() just returns with no result.
> > It makes kprobes always failed to find symbol even if it exists in the
> > map (dso).
> > 
> > To fix it, use map__find_symbol_by_name() instead.  It'll load a map
> > with full symbols and sorts them by name.  It needs to search sibing
> > nodes since there can be multiple (local) symbols with same name.  Now
> > resulting symbol references are saved in the funcs list.
> > 
> > Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/util/probe-event.c | 101 ++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 87 insertions(+), 14 deletions(-)
> > 
> > diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> > index 7f9b8632e433..e5af16988791 100644
> > --- a/tools/perf/util/probe-event.c
> > +++ b/tools/perf/util/probe-event.c
> > @@ -2191,20 +2191,86 @@ static int __add_probe_trace_events(struct perf_probe_event *pev,
> >  	return ret;
> >  }
> >  
> > -static char *looking_function_name;
> > -static int num_matched_functions;
> > +struct symbol_entry {
> > +	struct list_head node;
> > +	struct symbol *sym;
> > +};
> >  
> > -static int probe_function_filter(struct map *map __maybe_unused,
> > -				      struct symbol *sym)
> > +/* returns 1 if symbol was added, 0 if symbol was skipped, -1 if error */
> > +static int add_symbol_entry(struct symbol *sym, struct list_head *head)
> >  {
> > -	if ((sym->binding == STB_GLOBAL || sym->binding == STB_LOCAL) &&
> > -	    strcmp(looking_function_name, sym->name) == 0) {
> > -		num_matched_functions++;
> > +	struct symbol_entry *ent;
> > +
> > +	if (sym->binding != STB_GLOBAL && sym->binding != STB_LOCAL)
> >  		return 0;
> > -	}
> > +
> > +	ent = malloc(sizeof(*ent));
> > +	if (ent == NULL)
> > +		return -1;
> 
> return -ENOMEM; ?

Okay, will change.


> 
> > +
> > +	ent->sym = sym;
> > +	list_add(&ent->node, head);
> >  	return 1;
> >  }
> >  
> > +static int find_probe_functions(struct map *map, char *name, struct list_head *head)
> > +{
> > +	struct symbol *sym, *orig_sym;
> > +	struct symbol_entry *ent;
> > +	struct rb_node *node;
> > +	int found = 0;
> > +	int ret;
> > +
> > +	sym = map__find_symbol_by_name(map, name, NULL);
> > +	if (sym == NULL)
> > +		return 0;
> > +
> > +	ret = add_symbol_entry(sym, head);
> > +	if (ret < 0)
> > +		goto err;
> > +
> > +	found += ret;
> 
> If ret always be 1 in successful case, we'd better do "found++" here.
> And it also means we can do it shorter as below.
> 
> if (add_symbol_entry(sym, head) < 0)
> 	goto err;
> found++;

But it can return 0 in successful case like STB_WEAK..  I'm not sure
how we can handle the weak functions properly, but anyway the original
code already ignored the weak functions.


> 
> > +
> > +	/* search back and forth to find symbols that have same name */
> 
> Hmm, I see. but this code looks no-good sign... Can we have any generic
> synonym handling routine?

Like what?  I guess we can change map__find_symbol_by_name() to return
a list of symbols or add a new function to do it.  Is it okay to you?


> 
> Other parts looks good to me:)

Thanks for your review!
Namhyung

Re: [PATCH 3/4] perf probe: Fix probing kretprobes

[Namhyung Kim]

On Mon, Jan 12, 2015 at 09:22:13PM +0900, Masami Hiramatsu wrote:
> (2015/01/10 19:33), Namhyung Kim wrote:
> > The commit dfef99cd0b2c ("perf probe: Use ref_reloc_sym based address
> > instead of the symbol name") converts kprobes to use ref_reloc_sym
> > (i.e. _stext) and offset instead of using symbol's name directly.  So
> > on my system, adding do_fork ends up with like below:
> > 
> >   $ sudo perf probe -v --add do_fork%return
> >   probe-definition(0): do_fork%return
> >   symbol:do_fork file:(null) line:0 offset:0 return:1 lazy:(null)
> >   0 arguments
> >   Looking at the vmlinux_path (7 entries long)
> >   Using /lib/modules/3.17.6-1-ARCH/build/vmlinux for symbols
> >   Could not open debuginfo. Try to use symbols.
> >   Opening /sys/kernel/debug/tracing/kprobe_events write=1
> >   Added new event:
> >   Writing event: r:probe/do_fork _stext+456136
> >   Failed to write event: Invalid argument
> >   Error: Failed to add events. Reason: Operation not permitted (Code: -1)
> > 
> > As you can see, the do_fork was translated to _stext+456136.  This was
> > because to support (local) symbols that have same name.  But the
> > problem is that kretprobe requires to be inserted at function start
> > point so it simply checks whether it's called with offset 0.  And if
> > not, it'll return with -EINVAL.  You can see it with dmesg.
> > 
> >   $ dmesg | tail -1
> >     [125621.764103] Return probe must be used without offset.
> > 
> > So we need to use the symbol name instead of ref_reloc_sym in case of
> > return probes.
> 
> Thanks, but as I pointed in previous mail, this patch fixes just one
> code path which doesn't use debuginfo. With debuginfo, perf-probe
> executes try_to_find_probe_trace_events() to analyze debuginfo.
> We can just skip calling it for retprobes, but I prefer to keep
> it works. So just skipping conversion in post_process_probe_trace_events()
> is better.

Ah, missed that.  I only tested on my laptop which has no debuginfo.
I will add the check into the post_process_probe_trace_events().

Thanks,
Namhyung

Re: [PATCH 2/4] perf probe: Do not rely on map__load() filter to find symbols

[Masami Hiramatsu]

(2015/01/14 10:45), Namhyung Kim wrote:
> On Mon, Jan 12, 2015 at 09:31:30PM +0900, Masami Hiramatsu wrote:
>> (2015/01/10 19:33), Namhyung Kim wrote:
>>> The find_probe_trace_events_from_map() searches matching symbol from a
>>> map (so from a backing dso).  For uprobes, it'll create a new map (and
>>> dso) and loads it using a filter.  It's a little bit inefficient in that
>>> it'll read out the symbol table everytime but works well anyway.
>>>
>>> For kprobes however, it'll reuse existing kernel map which might be
>>> loaded before.  In this case map__load() just returns with no result.
>>> It makes kprobes always failed to find symbol even if it exists in the
>>> map (dso).
>>>
>>> To fix it, use map__find_symbol_by_name() instead.  It'll load a map
>>> with full symbols and sorts them by name.  It needs to search sibing
>>> nodes since there can be multiple (local) symbols with same name.  Now
>>> resulting symbol references are saved in the funcs list.
>>>
>>> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>>> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>>> ---
>>>  tools/perf/util/probe-event.c | 101 ++++++++++++++++++++++++++++++++++++------
>>>  1 file changed, 87 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
>>> index 7f9b8632e433..e5af16988791 100644
>>> --- a/tools/perf/util/probe-event.c
>>> +++ b/tools/perf/util/probe-event.c
>>> @@ -2191,20 +2191,86 @@ static int __add_probe_trace_events(struct perf_probe_event *pev,
>>>  	return ret;
>>>  }
>>>  
>>> -static char *looking_function_name;
>>> -static int num_matched_functions;
>>> +struct symbol_entry {
>>> +	struct list_head node;
>>> +	struct symbol *sym;
>>> +};
>>>  
>>> -static int probe_function_filter(struct map *map __maybe_unused,
>>> -				      struct symbol *sym)
>>> +/* returns 1 if symbol was added, 0 if symbol was skipped, -1 if error */
>>> +static int add_symbol_entry(struct symbol *sym, struct list_head *head)
>>>  {
>>> -	if ((sym->binding == STB_GLOBAL || sym->binding == STB_LOCAL) &&
>>> -	    strcmp(looking_function_name, sym->name) == 0) {
>>> -		num_matched_functions++;
>>> +	struct symbol_entry *ent;
>>> +
>>> +	if (sym->binding != STB_GLOBAL && sym->binding != STB_LOCAL)
>>>  		return 0;
>>> -	}
>>> +
>>> +	ent = malloc(sizeof(*ent));
>>> +	if (ent == NULL)
>>> +		return -1;
>>
>> return -ENOMEM; ?
> 
> Okay, will change.
> 
> 
>>
>>> +
>>> +	ent->sym = sym;
>>> +	list_add(&ent->node, head);
>>>  	return 1;
>>>  }
>>>  
>>> +static int find_probe_functions(struct map *map, char *name, struct list_head *head)
>>> +{
>>> +	struct symbol *sym, *orig_sym;
>>> +	struct symbol_entry *ent;
>>> +	struct rb_node *node;
>>> +	int found = 0;
>>> +	int ret;
>>> +
>>> +	sym = map__find_symbol_by_name(map, name, NULL);
>>> +	if (sym == NULL)
>>> +		return 0;
>>> +
>>> +	ret = add_symbol_entry(sym, head);
>>> +	if (ret < 0)
>>> +		goto err;
>>> +
>>> +	found += ret;
>>
>> If ret always be 1 in successful case, we'd better do "found++" here.
>> And it also means we can do it shorter as below.
>>
>> if (add_symbol_entry(sym, head) < 0)
>> 	goto err;
>> found++;
> 
> But it can return 0 in successful case like STB_WEAK..  I'm not sure
> how we can handle the weak functions properly, but anyway the original
> code already ignored the weak functions.

Ah, I see... OK, it should not be changed.

>>> +
>>> +	/* search back and forth to find symbols that have same name */
>>
>> Hmm, I see. but this code looks no-good sign... Can we have any generic
>> synonym handling routine?
> 
> Like what?  I guess we can change map__find_symbol_by_name() to return
> a list of symbols or add a new function to do it.  Is it okay to you?

Yeah, one possible solution is introducing map__find_all_symbols_by_name()
for looking up all the symbols who have same name. Or, map__find_symbol_by_name()
does forward search on rbtree to find the first symbol and returns it, so that
caller just do backward search.

Thank you,

-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

[tip:perf/urgent] perf probe: Propagate error code when write(2) failed

[tip-bot for Namhyung Kim]

Commit-ID:  7949ba1fa249caa7e611abb8d92f40b0a46c8617
Gitweb:     http://git.kernel.org/tip/7949ba1fa249caa7e611abb8d92f40b0a46c8617
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Sat, 10 Jan 2015 19:33:48 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Fri, 16 Jan 2015 17:49:28 -0300

perf probe: Propagate error code when write(2) failed

When it failed to write probe commands to the probe_event file in
debugfs, it needs to propagate the error code properly.  Current code
blindly uses the return value of the write(2) so it always uses
-1 (-EPERM) and it might confuse users.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1420886028-15135-4-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/probe-event.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 7f9b863..94a717b 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -2052,9 +2052,11 @@ static int write_probe_trace_event(int fd, struct probe_trace_event *tev)
 	pr_debug("Writing event: %s\n", buf);
 	if (!probe_event_dry_run) {
 		ret = write(fd, buf, strlen(buf));
-		if (ret <= 0)
+		if (ret <= 0) {
+			ret = -errno;
 			pr_warning("Failed to write event: %s\n",
 				   strerror_r(errno, sbuf, sizeof(sbuf)));
+		}
 	}
 	free(buf);
 	return ret;

[tip:perf/core] perf tools: Allow use of an exclusive option more than once

[tip-bot for Namhyung Kim]

Commit-ID:  5594b557aacaafbba7ad8e5ed29005df883bfe3a
Gitweb:     http://git.kernel.org/tip/5594b557aacaafbba7ad8e5ed29005df883bfe3a
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Sat, 10 Jan 2015 19:33:45 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 21 Jan 2015 13:24:33 -0300

perf tools: Allow use of an exclusive option more than once

The exclusive options are to prohibit use of conflicting options at the
same time.  But it had a side effect that it also limits a such option
can be used at most once.  Currently the only user of the flag is perf
probe and it allows to use such options more than once, but when one
tries to use it, perf will fail like below:

  $ sudo perf probe -x /lib/libc-2.20.so --add malloc --add free
    Error: option `add' cannot be used with add
  ...

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Reviewed-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1420886028-15135-1-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/parse-options.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/parse-options.c b/tools/perf/util/parse-options.c
index f62dee7..4a015f7 100644
--- a/tools/perf/util/parse-options.c
+++ b/tools/perf/util/parse-options.c
@@ -46,7 +46,7 @@ static int get_value(struct parse_opt_ctx_t *p,
 		return opterror(opt, "is not usable", flags);
 
 	if (opt->flags & PARSE_OPT_EXCLUSIVE) {
-		if (p->excl_opt) {
+		if (p->excl_opt && p->excl_opt != opt) {
 			char msg[128];
 
 			if (((flags & OPT_SHORT) && p->excl_opt->short_name) ||