[RFC] perf probe: -x option position issue

[RFC] perf probe: -x option position issue

[Jiri Olsa]

hi,
Martin found out following issue.. having following ex binary:

---
int main(void)
{
        return 0;
}
---

following will create uprobe on main:

  [root@dell-per510-01 perf]# gcc -g -o ex ex.c
  [root@dell-per510-01 perf]# ./perf probe -x ./ex -a main
  Added new event:
    probe_ex:main        (on main in /root/linux/tools/perf/ex)

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

          perf record -e probe_ex:main -aR sleep 1

  [root@dell-per510-01 perf]# cat /sys/kernel/debug/tracing/uprobe_events 
  p:probe_ex/main /root/linux/tools/perf/ex:0x00000000000004f6


while following will create (?) kprobe with complain in dmesg:

  [root@dell-per510-01 perf]# gcc -g -o ex ex.c
  [root@dell-per510-01 perf]# ./perf probe -a main -x ./ex
  Added new event:
    probe:main           (on main in ex)

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

          perf record -e probe:main -aR sleep 1

  [root@dell-per510-01 perf]# dmesg | tail -2
  [16986.182159] Could not insert probe at ex:main+0: -2
  [16986.187030] This probe might be able to register aftertarget module is loaded. Continue.


that does not seem as an expected behaviour, or am I missing something?

thanks,
jirka

Re: [RFC] perf probe: -x option position issue

[Arnaldo Carvalho de Melo]

Em Mon, Mar 30, 2015 at 07:46:55PM +0200, Jiri Olsa escreveu:
> hi,
> Martin found out following issue.. having following ex binary:
> 
> ---
> int main(void)
> {
>         return 0;
> }
> ---
> 
> following will create uprobe on main:
> 
>   [root@dell-per510-01 perf]# gcc -g -o ex ex.c
>   [root@dell-per510-01 perf]# ./perf probe -x ./ex -a main
>   Added new event:
>     probe_ex:main        (on main in /root/linux/tools/perf/ex)
> 
>   You can now use it in all perf tools, such as:
> 
>           perf record -e probe_ex:main -aR sleep 1
> 
>   [root@dell-per510-01 perf]# cat /sys/kernel/debug/tracing/uprobe_events 
>   p:probe_ex/main /root/linux/tools/perf/ex:0x00000000000004f6
> 
> 
> while following will create (?) kprobe with complain in dmesg:

Right, it looks like it will create the probe on the currently selected
DSO, which, if you have none, will be the kernel, thus the kprobe
(probe:main), while if you do a '-x ./ex -a main' you are selecting the
'ex' DSO and then asking for the probe to be added to a function named
'main', on that DSO, that 'perf probe' realizes is a userspace binary,
thus creates a uprobe: probe_%DSONAME:%FUNCTIONAME.

I wonder if I can do:

[root@ssdandy acme]# perf probe -a icmp_rcv -x ./ex -a main
Probe point 'icmp_rcv' not found.
  Error: Failed to add events.


No, I can't, I'd say we should support that, i.e. inserting multiple
probes per command line, for different DSOs, etc. I.e. the above would
be equivalent to these two calls:

[root@ssdandy acme]# perf probe -a icmp_rcv
Added new event:
  probe:icmp_rcv       (on icmp_rcv)

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

	perf record -e probe:icmp_rcv -aR sleep 1

[root@ssdandy acme]# perf probe -x ./ex -a main
Added new event:
  probe_ex:main        (on main in /home/acme/ex)

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

	perf record -e probe_ex:main -aR sleep 1

[root@ssdandy acme]#


But it isn't like that, so, yes, what you report is a bug, both for your
expectation (that I think is that it should put a uprobes with both your
examples) and for mine (that it would add the first to the kernel, and
the second to the selected DSO via -x).

- Arnaldo
 
>   [root@dell-per510-01 perf]# gcc -g -o ex ex.c
>   [root@dell-per510-01 perf]# ./perf probe -a main -x ./ex
>   Added new event:
>     probe:main           (on main in ex)
> 
>   You can now use it in all perf tools, such as:
> 
>           perf record -e probe:main -aR sleep 1
> 
>   [root@dell-per510-01 perf]# dmesg | tail -2
>   [16986.182159] Could not insert probe at ex:main+0: -2
>   [16986.187030] This probe might be able to register aftertarget module is loaded. Continue.
> 
> 
> that does not seem as an expected behaviour, or am I missing something?
> 
> thanks,
> jirka

Re: [RFC] perf probe: -x option position issue

[Masami Hiramatsu]

(2015/03/31 4:48), Arnaldo Carvalho de Melo wrote:
> Em Mon, Mar 30, 2015 at 07:46:55PM +0200, Jiri Olsa escreveu:
>> hi,
>> Martin found out following issue.. having following ex binary:
>>
>> ---
>> int main(void)
>> {
>>         return 0;
>> }
>> ---
>>
>> following will create uprobe on main:
>>
>>   [root@dell-per510-01 perf]# gcc -g -o ex ex.c
>>   [root@dell-per510-01 perf]# ./perf probe -x ./ex -a main
>>   Added new event:
>>     probe_ex:main        (on main in /root/linux/tools/perf/ex)
>>
>>   You can now use it in all perf tools, such as:
>>
>>           perf record -e probe_ex:main -aR sleep 1
>>
>>   [root@dell-per510-01 perf]# cat /sys/kernel/debug/tracing/uprobe_events 
>>   p:probe_ex/main /root/linux/tools/perf/ex:0x00000000000004f6
>>
>>
>> while following will create (?) kprobe with complain in dmesg:
> 
> Right, it looks like it will create the probe on the currently selected
> DSO, which, if you have none, will be the kernel, thus the kprobe
> (probe:main), while if you do a '-x ./ex -a main' you are selecting the
> 'ex' DSO and then asking for the probe to be added to a function named
> 'main', on that DSO, that 'perf probe' realizes is a userspace binary,
> thus creates a uprobe: probe_%DSONAME:%FUNCTIONAME.

Right, but this looks strange and not easy to expect.

> 
> I wonder if I can do:
> 
> [root@ssdandy acme]# perf probe -a icmp_rcv -x ./ex -a main
> Probe point 'icmp_rcv' not found.
>   Error: Failed to add events.

Ah, this should be fixed. Even with multiple -x, it fails.

# ./perf probe -x /usr/lib64/libc-2.17.so -a malloc -x ./perf -a main

 usage: perf probe [<options>] 'PROBEDEF' ['PROBEDEF' ...]
    or: perf probe [<options>] --add 'PROBEDEF' [--add 'PROBEDEF' ...]
    or: perf probe [<options>] --del '[GROUP:]EVENT' ...
    or: perf probe --list
    or: perf probe [<options>] --line 'LINEDESC'
    or: perf probe [<options>] --vars 'PROBEPOINT'

    -x, --exec <executable|path>
                          target executable name or path

...

I'd like to start with setting up the event only on single binary,
and showing appropriate error message.

> No, I can't, I'd say we should support that, i.e. inserting multiple
> probes per command line, for different DSOs, etc. I.e. the above would
> be equivalent to these two calls:
> 
> [root@ssdandy acme]# perf probe -a icmp_rcv
> Added new event:
>   probe:icmp_rcv       (on icmp_rcv)
> 
> You can now use it in all perf tools, such as:
> 
> 	perf record -e probe:icmp_rcv -aR sleep 1
> 
> [root@ssdandy acme]# perf probe -x ./ex -a main
> Added new event:
>   probe_ex:main        (on main in /home/acme/ex)
> 
> You can now use it in all perf tools, such as:
> 
> 	perf record -e probe_ex:main -aR sleep 1
> 
> [root@ssdandy acme]#

OK, finally we should support that.

> But it isn't like that, so, yes, what you report is a bug, both for your
> expectation (that I think is that it should put a uprobes with both your
> examples) and for mine (that it would add the first to the kernel, and
> the second to the selected DSO via -x).

Yes, both are bugs. I'll fix that.

BTW, let me check that the below behaviors are OK for you.

perf probe -x BIN -a XXX
	 -> setup XXX on BIN
perf probe -a XXX -x BIN
	 -> setup XXX on BIN
perf probe -a XXX -x BIN -a YYY
	 -> setup XXX on kernel and YYY on BIN
perf probe -x BIN -a XXX -x BIN2 -a YYY
	 -> setup XXX on BIN and YYY on BIN2

Thank you,

> 
> - Arnaldo
>  
>>   [root@dell-per510-01 perf]# gcc -g -o ex ex.c
>>   [root@dell-per510-01 perf]# ./perf probe -a main -x ./ex
>>   Added new event:
>>     probe:main           (on main in ex)
>>
>>   You can now use it in all perf tools, such as:
>>
>>           perf record -e probe:main -aR sleep 1
>>
>>   [root@dell-per510-01 perf]# dmesg | tail -2
>>   [16986.182159] Could not insert probe at ex:main+0: -2
>>   [16986.187030] This probe might be able to register aftertarget module is loaded. Continue.
>>
>>
>> that does not seem as an expected behaviour, or am I missing something?
>>
>> thanks,
>> jirka
> 


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

Re: [RFC] perf probe: -x option position issue

[Arnaldo Carvalho de Melo]

Em Tue, Mar 31, 2015 at 05:04:18PM +0900, Masami Hiramatsu escreveu:
> (2015/03/31 4:48), Arnaldo Carvalho de Melo wrote:
> > No, I can't, I'd say we should support that, i.e. inserting multiple
> > probes per command line, for different DSOs, etc. I.e. the above would
> > be equivalent to these two calls:

> > [root@ssdandy acme]# perf probe -a icmp_rcv
> > Added new event:
> >   probe:icmp_rcv       (on icmp_rcv)

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

> > 	perf record -e probe:icmp_rcv -aR sleep 1

> > [root@ssdandy acme]# perf probe -x ./ex -a main
> > Added new event:
> >   probe_ex:main        (on main in /home/acme/ex)

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

> > 	perf record -e probe_ex:main -aR sleep 1

> > [root@ssdandy acme]#

> OK, finally we should support that.

> > But it isn't like that, so, yes, what you report is a bug, both for your
> > expectation (that I think is that it should put a uprobes with both your
> > examples) and for mine (that it would add the first to the kernel, and
> > the second to the selected DSO via -x).

> Yes, both are bugs. I'll fix that.

> BTW, let me check that the below behaviors are OK for you.

> perf probe -x BIN -a XXX
> 	 -> setup XXX on BIN

Ok

> perf probe -a XXX -x BIN
> 	 -> setup XXX on BIN
> perf probe -a XXX -x BIN -a YYY
> 	 -> setup XXX on kernel and YYY on BIN

The two above are inconsistent, I think, first one, for me, doesn't make
sense, i.e. it says: Add XXX to the selected DSO, which, as none was
specified at that point, should be the kernel, right?

I.e. if we do:

> perf probe -a XXX

Without that extra -x that is coming _after_ the command to add a probe
to XXX (-a XXX), what is that the tool should do (does from day 1, when
'probe' was first introduced in tools/perf/):

Add a probe to XXX _in the kernel_, i.e. not specifying a DSO means: its
for the kernel.

So, for me:

> perf probe -a XXX -x BIN
> 	 -> setup XXX on BIN

Is invalid (or inocuous if what one wants is to add a probe for XXX on
the BIN dso), because it doesn't make sense _if you want to support
adding multiple probes for different DSOs on the same command line_,
because it would mean:

  Add a probe to XXX _in the kernel_, then select BIN as the DSO for
which probes will be then specified, but in this example, none are
specified after that "-x BIN", so, I think that:

 perf probe -a XXX -x BIN

and:

 perf probe -a XXX

Mean the same thing, i.e. add a probe for XXX in the kernel.

> perf probe -x BIN -a XXX -x BIN2 -a YYY
> 	 -> setup XXX on BIN and YYY on BIN2
 
Ok.

Also, more generically, I think that:

perf probe -a AAA -a BBB -a CCC -a DDD -x /lib64/libc-2.17.so -a malloc -a free \
           -x /usr/lib64/libthread_db-1.0.so -a td_lookup -a td_thr_event_enable

Should add kprobes for AAA, BBB, CCC and DDD in the kernel, uprobes for
malloc and free on libc and uprobes for td_lookup and
td_thr_event_enable on libthread_db.

Making it even more compact would be a bonus:

perf probe -a AAA,BBB,CCC,DDD -x /lib64/libc-2.17.so -a malloc,free \
           -x /usr/lib64/libthread_db-1.0.so -a td_lookup,td_thr_event_enable

:-)

- Arnaldo

Re: Re: [RFC] perf probe: -x option position issue

[Masami Hiramatsu]

(2015/03/31 22:33), Arnaldo Carvalho de Melo wrote:
> Em Tue, Mar 31, 2015 at 05:04:18PM +0900, Masami Hiramatsu escreveu:
>> (2015/03/31 4:48), Arnaldo Carvalho de Melo wrote:
>>> No, I can't, I'd say we should support that, i.e. inserting multiple
>>> probes per command line, for different DSOs, etc. I.e. the above would
>>> be equivalent to these two calls:
> 
>>> [root@ssdandy acme]# perf probe -a icmp_rcv
>>> Added new event:
>>>   probe:icmp_rcv       (on icmp_rcv)
> 
>>> You can now use it in all perf tools, such as:
> 
>>> 	perf record -e probe:icmp_rcv -aR sleep 1
> 
>>> [root@ssdandy acme]# perf probe -x ./ex -a main
>>> Added new event:
>>>   probe_ex:main        (on main in /home/acme/ex)
> 
>>> You can now use it in all perf tools, such as:
> 
>>> 	perf record -e probe_ex:main -aR sleep 1
> 
>>> [root@ssdandy acme]#
> 
>> OK, finally we should support that.
> 
>>> But it isn't like that, so, yes, what you report is a bug, both for your
>>> expectation (that I think is that it should put a uprobes with both your
>>> examples) and for mine (that it would add the first to the kernel, and
>>> the second to the selected DSO via -x).
> 
>> Yes, both are bugs. I'll fix that.
> 
>> BTW, let me check that the below behaviors are OK for you.
> 
>> perf probe -x BIN -a XXX
>> 	 -> setup XXX on BIN
> 
> Ok
> 
>> perf probe -a XXX -x BIN
>> 	 -> setup XXX on BIN
>> perf probe -a XXX -x BIN -a YYY
>> 	 -> setup XXX on kernel and YYY on BIN
> 
> The two above are inconsistent, I think, first one, for me, doesn't make
> sense, i.e. it says: Add XXX to the selected DSO, which, as none was
> specified at that point, should be the kernel, right?

OK.

> 
> I.e. if we do:
> 
>> perf probe -a XXX
> 
> Without that extra -x that is coming _after_ the command to add a probe
> to XXX (-a XXX), what is that the tool should do (does from day 1, when
> 'probe' was first introduced in tools/perf/):
> 
> Add a probe to XXX _in the kernel_, i.e. not specifying a DSO means: its
> for the kernel.
> 
> So, for me:
> 
>> perf probe -a XXX -x BIN
>> 	 -> setup XXX on BIN
> 
> Is invalid (or inocuous if what one wants is to add a probe for XXX on
> the BIN dso), because it doesn't make sense _if you want to support
> adding multiple probes for different DSOs on the same command line_,
> because it would mean:
> 
>   Add a probe to XXX _in the kernel_, then select BIN as the DSO for
> which probes will be then specified, but in this example, none are
> specified after that "-x BIN", so, I think that:
> 
>  perf probe -a XXX -x BIN
> 
> and:
> 
>  perf probe -a XXX
> 
> Mean the same thing, i.e. add a probe for XXX in the kernel.

OK, so if we have -x BIN after -a, it should have an error, since
that may be not what the user intend.

>> perf probe -x BIN -a XXX -x BIN2 -a YYY
>> 	 -> setup XXX on BIN and YYY on BIN2
>  
> Ok.
> 
> Also, more generically, I think that:
> 
> perf probe -a AAA -a BBB -a CCC -a DDD -x /lib64/libc-2.17.so -a malloc -a free \
>            -x /usr/lib64/libthread_db-1.0.so -a td_lookup -a td_thr_event_enable
> 
> Should add kprobes for AAA, BBB, CCC and DDD in the kernel, uprobes for
> malloc and free on libc and uprobes for td_lookup and
> td_thr_event_enable on libthread_db.

Yes, that is what I'll do on perf probe. All the probes after -x XXX are
defined on XXX binary (or module).

> 
> Making it even more compact would be a bonus:
> 
> perf probe -a AAA,BBB,CCC,DDD -x /lib64/libc-2.17.so -a malloc,free \
>            -x /usr/lib64/libthread_db-1.0.so -a td_lookup,td_thr_event_enable
> 
> :-)

Sorry, this does not fit to current syntax of probe definition, since
we may have some arguments on each event...

Thank you,

-- 
Masami HIRAMATSU
Linux Technology Research Center, System Productivity Research Dept.
Center for Technology Innovation - Systems Engineering
Hitachi, Ltd., Research & Development Group
E-mail: masami.hiramatsu.pt@hitachi.com

[PATCH perf/core 1/2] perf-probe: Support multiple probes on different binaries

[Masami Hiramatsu]

Support multiple probes on different binaries with just
one command.

In the result, this example sets up the probes on icmp_rcv in
kernel, on main and set_target in perf, and on pcspkr_event
in pcspker.ko driver.
  -----
  # perf probe -a icmp_rcv -x ./perf -a main -a set_target \
   -m /lib/modules/4.0.0-rc5+/kernel/drivers/input/misc/pcspkr.ko \
   -a pcspkr_event
  Added new event:
    probe:icmp_rcv       (on icmp_rcv)

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

          perf record -e probe:icmp_rcv -aR sleep 1

  Added new event:
    probe_perf:main      (on main in /home/mhiramat/ksrc/linux-3/tools/perf/perf)

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

          perf record -e probe_perf:main -aR sleep 1

  Added new event:
    probe_perf:set_target (on set_target in /home/mhiramat/ksrc/linux-3/tools/perf/perf)

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

          perf record -e probe_perf:set_target -aR sleep 1

  Added new event:
    probe:pcspkr_event   (on pcspkr_event in pcspkr)

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

          perf record -e probe:pcspkr_event -aR sleep 1
  -----

Reported-by: Arnaldo Carvalho de Melo <acme@infradead.org>
Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
---
 tools/perf/builtin-probe.c    |    9 +++++++--
 tools/perf/util/probe-event.c |    5 +++--
 tools/perf/util/probe-event.h |    6 +++---
 3 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
index 921bb69..2df23e1 100644
--- a/tools/perf/builtin-probe.c
+++ b/tools/perf/builtin-probe.c
@@ -78,6 +78,11 @@ static int parse_probe_event(const char *str)
 	}
 
 	pev->uprobes = params.uprobes;
+	if (params.target) {
+		pev->target = strdup(params.target);
+		if (!pev->target)
+			return -ENOMEM;
+	}
 
 	/* Parse a perf-probe command into event */
 	ret = parse_perf_probe_command(str, pev);
@@ -178,7 +183,7 @@ static int opt_set_target(const struct option *opt, const char *str,
 	int ret = -ENOENT;
 	char *tmp;
 
-	if  (str && !params.target) {
+	if  (str) {
 		if (!strcmp(opt->long_name, "exec"))
 			params.uprobes = true;
 #ifdef HAVE_DWARF_SUPPORT
@@ -200,6 +205,7 @@ static int opt_set_target(const struct option *opt, const char *str,
 			if (!tmp)
 				return -ENOMEM;
 		}
+		free(params.target);
 		params.target = tmp;
 		ret = 0;
 	}
@@ -487,7 +493,6 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
 	if (params.nevents) {
 		ret = add_perf_probe_events(params.events, params.nevents,
 					    params.max_probe_points,
-					    params.target,
 					    params.force_add);
 		if (ret < 0) {
 			pr_err_with_code("  Error: Failed to add events.", ret);
diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 8feac07..9b02db8 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -1903,6 +1903,7 @@ void clear_perf_probe_event(struct perf_probe_event *pev)
 
 	free(pev->event);
 	free(pev->group);
+	free(pev->target);
 	clear_perf_probe_point(&pev->point);
 
 	for (i = 0; i < pev->nargs; i++) {
@@ -2651,7 +2652,7 @@ struct __event_package {
 };
 
 int add_perf_probe_events(struct perf_probe_event *pevs, int npevs,
-			  int max_tevs, const char *target, bool force_add)
+			  int max_tevs, bool force_add)
 {
 	int i, j, ret;
 	struct __event_package *pkgs;
@@ -2675,7 +2676,7 @@ int add_perf_probe_events(struct perf_probe_event *pevs, int npevs,
 		ret  = convert_to_probe_trace_events(pkgs[i].pev,
 						     &pkgs[i].tevs,
 						     max_tevs,
-						     target);
+						     pkgs[i].pev->target);
 		if (ret < 0)
 			goto end;
 		pkgs[i].ntevs = ret;
diff --git a/tools/perf/util/probe-event.h b/tools/perf/util/probe-event.h
index e01e994..d6b7834 100644
--- a/tools/perf/util/probe-event.h
+++ b/tools/perf/util/probe-event.h
@@ -73,7 +73,8 @@ struct perf_probe_event {
 	char			*group;	/* Group name */
 	struct perf_probe_point	point;	/* Probe point */
 	int			nargs;	/* Number of arguments */
-	bool			uprobes;
+	bool			uprobes;	/* Uprobe event flag */
+	char			*target;	/* Target binary */
 	struct perf_probe_arg	*args;	/* Arguments */
 };
 
@@ -124,8 +125,7 @@ extern int line_range__init(struct line_range *lr);
 extern const char *kernel_get_module_path(const char *module);
 
 extern int add_perf_probe_events(struct perf_probe_event *pevs, int npevs,
-				 int max_probe_points, const char *module,
-				 bool force_add);
+				 int max_probe_points, bool force_add);
 extern int del_perf_probe_events(struct strlist *dellist);
 extern int show_perf_probe_events(void);
 extern int show_line_range(struct line_range *lr, const char *module,

[PATCH perf/core 2/2] perf-probe: Check the orphaned -x option

[Masami Hiramatsu]

To avoid probing in unintended binary, the orphaned -x option
must be checked and warned.

Without this patch, following command sets up the probe in
the kernel.
  -----
  # perf probe -a strcpy -x ./perf
  Added new event:
    probe:strcpy         (on strcpy)

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

          perf record -e probe:strcpy -aR sleep 1
  -----

But in this case, it seems that the user may want to probe
in the perf binary. With this patch, perf-probe correctly
handles the orphaned -x.
  -----
  # perf probe -a strcpy -x ./perf
    Error: -x/-m must follow the probe definitions.
  ...
  -----

Reported-by: Jiri Olsa <jolsa@redhat.com>
Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
---
 tools/perf/builtin-probe.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
index 2df23e1..f7b1af6 100644
--- a/tools/perf/builtin-probe.c
+++ b/tools/perf/builtin-probe.c
@@ -56,6 +56,7 @@ static struct {
 	bool mod_events;
 	bool uprobes;
 	bool quiet;
+	bool target_used;
 	int nevents;
 	struct perf_probe_event events[MAX_PROBES];
 	struct strlist *dellist;
@@ -82,6 +83,7 @@ static int parse_probe_event(const char *str)
 		pev->target = strdup(params.target);
 		if (!pev->target)
 			return -ENOMEM;
+		params.target_used = true;
 	}
 
 	/* Parse a perf-probe command into event */
@@ -107,6 +109,7 @@ static int set_target(const char *ptr)
 		params.target = strdup(ptr);
 		if (!params.target)
 			return -ENOMEM;
+		params.target_used = false;
 
 		found = 1;
 		buf = ptr + (strlen(ptr) - 3);
@@ -207,6 +210,7 @@ static int opt_set_target(const struct option *opt, const char *str,
 		}
 		free(params.target);
 		params.target = tmp;
+		params.target_used = false;
 		ret = 0;
 	}
 
@@ -491,6 +495,12 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
 	}
 
 	if (params.nevents) {
+		/* Ensure the last given target is used */
+		if (params.target && !params.target_used) {
+			pr_warning("  Error: -x/-m must follow the probe definitions.\n");
+			usage_with_options(probe_usage, options);
+		}
+
 		ret = add_perf_probe_events(params.events, params.nevents,
 					    params.max_probe_points,
 					    params.force_add);

Re: [PATCH perf/core 2/2] perf-probe: Check the orphaned -x option

[Jiri Olsa]

On Wed, Apr 01, 2015 at 07:25:42PM +0900, Masami Hiramatsu wrote:
> To avoid probing in unintended binary, the orphaned -x option
> must be checked and warned.
> 
> Without this patch, following command sets up the probe in
> the kernel.
>   -----
>   # perf probe -a strcpy -x ./perf
>   Added new event:
>     probe:strcpy         (on strcpy)
> 
>   You can now use it in all perf tools, such as:
> 
>           perf record -e probe:strcpy -aR sleep 1
>   -----
> 
> But in this case, it seems that the user may want to probe
> in the perf binary. With this patch, perf-probe correctly
> handles the orphaned -x.
>   -----
>   # perf probe -a strcpy -x ./perf
>     Error: -x/-m must follow the probe definitions.
>   ...
>   -----
> 
> Reported-by: Jiri Olsa <jolsa@redhat.com>

works nicely, thanks a lot!

Tested-by: Jiri Olsa <jolsa@kernel.org>

jirka

Re: [PATCH perf/core 2/2] perf-probe: Check the orphaned -x option

[Masami Hiramatsu]

Ping?

(2015/04/01 19:25), Masami Hiramatsu wrote:
> To avoid probing in unintended binary, the orphaned -x option
> must be checked and warned.
> 
> Without this patch, following command sets up the probe in
> the kernel.
>   -----
>   # perf probe -a strcpy -x ./perf
>   Added new event:
>     probe:strcpy         (on strcpy)
> 
>   You can now use it in all perf tools, such as:
> 
>           perf record -e probe:strcpy -aR sleep 1
>   -----
> 
> But in this case, it seems that the user may want to probe
> in the perf binary. With this patch, perf-probe correctly
> handles the orphaned -x.
>   -----
>   # perf probe -a strcpy -x ./perf
>     Error: -x/-m must follow the probe definitions.
>   ...
>   -----
> 
> Reported-by: Jiri Olsa <jolsa@redhat.com>
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> ---
>  tools/perf/builtin-probe.c |   10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
> index 2df23e1..f7b1af6 100644
> --- a/tools/perf/builtin-probe.c
> +++ b/tools/perf/builtin-probe.c
> @@ -56,6 +56,7 @@ static struct {
>  	bool mod_events;
>  	bool uprobes;
>  	bool quiet;
> +	bool target_used;
>  	int nevents;
>  	struct perf_probe_event events[MAX_PROBES];
>  	struct strlist *dellist;
> @@ -82,6 +83,7 @@ static int parse_probe_event(const char *str)
>  		pev->target = strdup(params.target);
>  		if (!pev->target)
>  			return -ENOMEM;
> +		params.target_used = true;
>  	}
>  
>  	/* Parse a perf-probe command into event */
> @@ -107,6 +109,7 @@ static int set_target(const char *ptr)
>  		params.target = strdup(ptr);
>  		if (!params.target)
>  			return -ENOMEM;
> +		params.target_used = false;
>  
>  		found = 1;
>  		buf = ptr + (strlen(ptr) - 3);
> @@ -207,6 +210,7 @@ static int opt_set_target(const struct option *opt, const char *str,
>  		}
>  		free(params.target);
>  		params.target = tmp;
> +		params.target_used = false;
>  		ret = 0;
>  	}
>  
> @@ -491,6 +495,12 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
>  	}
>  
>  	if (params.nevents) {
> +		/* Ensure the last given target is used */
> +		if (params.target && !params.target_used) {
> +			pr_warning("  Error: -x/-m must follow the probe definitions.\n");
> +			usage_with_options(probe_usage, options);
> +		}
> +
>  		ret = add_perf_probe_events(params.events, params.nevents,
>  					    params.max_probe_points,
>  					    params.force_add);
> 
> 


-- 
Masami HIRAMATSU
Linux Technology Research Center, System Productivity Research Dept.
Center for Technology Innovation - Systems Engineering
Hitachi, Ltd., Research & Development Group
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH perf/core 2/2] perf-probe: Check the orphaned -x option

[Arnaldo Carvalho de Melo]

Em Fri, Apr 10, 2015 at 03:51:57PM +0900, Masami Hiramatsu escreveu:
> Ping?

Sorry for the delay, thought I had processed those already, done now,
will be in my next pull req.

Thanks for working on it, I tested both and all seems fine,

- Arnaldo
 
> (2015/04/01 19:25), Masami Hiramatsu wrote:
> > To avoid probing in unintended binary, the orphaned -x option
> > must be checked and warned.
> > 
> > Without this patch, following command sets up the probe in
> > the kernel.
> >   -----
> >   # perf probe -a strcpy -x ./perf
> >   Added new event:
> >     probe:strcpy         (on strcpy)
> > 
> >   You can now use it in all perf tools, such as:
> > 
> >           perf record -e probe:strcpy -aR sleep 1
> >   -----
> > 
> > But in this case, it seems that the user may want to probe
> > in the perf binary. With this patch, perf-probe correctly
> > handles the orphaned -x.
> >   -----
> >   # perf probe -a strcpy -x ./perf
> >     Error: -x/-m must follow the probe definitions.
> >   ...
> >   -----
> > 
> > Reported-by: Jiri Olsa <jolsa@redhat.com>
> > Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> > ---
> >  tools/perf/builtin-probe.c |   10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
> > index 2df23e1..f7b1af6 100644
> > --- a/tools/perf/builtin-probe.c
> > +++ b/tools/perf/builtin-probe.c
> > @@ -56,6 +56,7 @@ static struct {
> >  	bool mod_events;
> >  	bool uprobes;
> >  	bool quiet;
> > +	bool target_used;
> >  	int nevents;
> >  	struct perf_probe_event events[MAX_PROBES];
> >  	struct strlist *dellist;
> > @@ -82,6 +83,7 @@ static int parse_probe_event(const char *str)
> >  		pev->target = strdup(params.target);
> >  		if (!pev->target)
> >  			return -ENOMEM;
> > +		params.target_used = true;
> >  	}
> >  
> >  	/* Parse a perf-probe command into event */
> > @@ -107,6 +109,7 @@ static int set_target(const char *ptr)
> >  		params.target = strdup(ptr);
> >  		if (!params.target)
> >  			return -ENOMEM;
> > +		params.target_used = false;
> >  
> >  		found = 1;
> >  		buf = ptr + (strlen(ptr) - 3);
> > @@ -207,6 +210,7 @@ static int opt_set_target(const struct option *opt, const char *str,
> >  		}
> >  		free(params.target);
> >  		params.target = tmp;
> > +		params.target_used = false;
> >  		ret = 0;
> >  	}
> >  
> > @@ -491,6 +495,12 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
> >  	}
> >  
> >  	if (params.nevents) {
> > +		/* Ensure the last given target is used */
> > +		if (params.target && !params.target_used) {
> > +			pr_warning("  Error: -x/-m must follow the probe definitions.\n");
> > +			usage_with_options(probe_usage, options);
> > +		}
> > +
> >  		ret = add_perf_probe_events(params.events, params.nevents,
> >  					    params.max_probe_points,
> >  					    params.force_add);
> > 
> > 
> 
> 
> -- 
> Masami HIRAMATSU
> Linux Technology Research Center, System Productivity Research Dept.
> Center for Technology Innovation - Systems Engineering
> Hitachi, Ltd., Research & Development Group
> E-mail: masami.hiramatsu.pt@hitachi.com
> 

[tip:perf/core] perf probe: Support multiple probes on different binaries

[tip-bot for Masami Hiramatsu]

Commit-ID:  7afb3fab390871b1d20b1dbb94e03b8a3861cb0d
Gitweb:     http://git.kernel.org/tip/7afb3fab390871b1d20b1dbb94e03b8a3861cb0d
Author:     Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
AuthorDate: Wed, 1 Apr 2015 19:25:39 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Fri, 10 Apr 2015 10:19:53 -0300

perf probe: Support multiple probes on different binaries

Support multiple probes on different binaries with just
one command.

In the result, this example sets up the probes on icmp_rcv in
kernel, on main and set_target in perf, and on pcspkr_event
in pcspker.ko driver.
  -----
  # perf probe -a icmp_rcv -x ./perf -a main -a set_target \
   -m /lib/modules/4.0.0-rc5+/kernel/drivers/input/misc/pcspkr.ko \
   -a pcspkr_event
  Added new event:
    probe:icmp_rcv       (on icmp_rcv)

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

          perf record -e probe:icmp_rcv -aR sleep 1

  Added new event:
    probe_perf:main      (on main in /home/mhiramat/ksrc/linux-3/tools/perf/perf)

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

          perf record -e probe_perf:main -aR sleep 1

  Added new event:
    probe_perf:set_target (on set_target in /home/mhiramat/ksrc/linux-3/tools/perf/perf)

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

          perf record -e probe_perf:set_target -aR sleep 1

  Added new event:
    probe:pcspkr_event   (on pcspkr_event in pcspkr)

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

          perf record -e probe:pcspkr_event -aR sleep 1
  -----

Reported-by: Arnaldo Carvalho de Melo <acme@infradead.org>
Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/20150401102539.17137.46454.stgit@localhost.localdomain
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-probe.c    | 9 +++++++--
 tools/perf/util/probe-event.c | 5 +++--
 tools/perf/util/probe-event.h | 6 +++---
 3 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
index 921bb69..2df23e1 100644
--- a/tools/perf/builtin-probe.c
+++ b/tools/perf/builtin-probe.c
@@ -78,6 +78,11 @@ static int parse_probe_event(const char *str)
 	}
 
 	pev->uprobes = params.uprobes;
+	if (params.target) {
+		pev->target = strdup(params.target);
+		if (!pev->target)
+			return -ENOMEM;
+	}
 
 	/* Parse a perf-probe command into event */
 	ret = parse_perf_probe_command(str, pev);
@@ -178,7 +183,7 @@ static int opt_set_target(const struct option *opt, const char *str,
 	int ret = -ENOENT;
 	char *tmp;
 
-	if  (str && !params.target) {
+	if  (str) {
 		if (!strcmp(opt->long_name, "exec"))
 			params.uprobes = true;
 #ifdef HAVE_DWARF_SUPPORT
@@ -200,6 +205,7 @@ static int opt_set_target(const struct option *opt, const char *str,
 			if (!tmp)
 				return -ENOMEM;
 		}
+		free(params.target);
 		params.target = tmp;
 		ret = 0;
 	}
@@ -487,7 +493,6 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
 	if (params.nevents) {
 		ret = add_perf_probe_events(params.events, params.nevents,
 					    params.max_probe_points,
-					    params.target,
 					    params.force_add);
 		if (ret < 0) {
 			pr_err_with_code("  Error: Failed to add events.", ret);
diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index b788517..30545ce 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -1906,6 +1906,7 @@ void clear_perf_probe_event(struct perf_probe_event *pev)
 
 	free(pev->event);
 	free(pev->group);
+	free(pev->target);
 	clear_perf_probe_point(&pev->point);
 
 	for (i = 0; i < pev->nargs; i++) {
@@ -2654,7 +2655,7 @@ struct __event_package {
 };
 
 int add_perf_probe_events(struct perf_probe_event *pevs, int npevs,
-			  int max_tevs, const char *target, bool force_add)
+			  int max_tevs, bool force_add)
 {
 	int i, j, ret;
 	struct __event_package *pkgs;
@@ -2678,7 +2679,7 @@ int add_perf_probe_events(struct perf_probe_event *pevs, int npevs,
 		ret  = convert_to_probe_trace_events(pkgs[i].pev,
 						     &pkgs[i].tevs,
 						     max_tevs,
-						     target);
+						     pkgs[i].pev->target);
 		if (ret < 0)
 			goto end;
 		pkgs[i].ntevs = ret;
diff --git a/tools/perf/util/probe-event.h b/tools/perf/util/probe-event.h
index e01e994..d6b7834 100644
--- a/tools/perf/util/probe-event.h
+++ b/tools/perf/util/probe-event.h
@@ -73,7 +73,8 @@ struct perf_probe_event {
 	char			*group;	/* Group name */
 	struct perf_probe_point	point;	/* Probe point */
 	int			nargs;	/* Number of arguments */
-	bool			uprobes;
+	bool			uprobes;	/* Uprobe event flag */
+	char			*target;	/* Target binary */
 	struct perf_probe_arg	*args;	/* Arguments */
 };
 
@@ -124,8 +125,7 @@ extern int line_range__init(struct line_range *lr);
 extern const char *kernel_get_module_path(const char *module);
 
 extern int add_perf_probe_events(struct perf_probe_event *pevs, int npevs,
-				 int max_probe_points, const char *module,
-				 bool force_add);
+				 int max_probe_points, bool force_add);
 extern int del_perf_probe_events(struct strlist *dellist);
 extern int show_perf_probe_events(void);
 extern int show_line_range(struct line_range *lr, const char *module,

[tip:perf/core] perf probe: Check the orphaned -x option

[tip-bot for Masami Hiramatsu]

Commit-ID:  8cb0aa4c2db395b143cd5165586dc17677960002
Gitweb:     http://git.kernel.org/tip/8cb0aa4c2db395b143cd5165586dc17677960002
Author:     Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
AuthorDate: Wed, 1 Apr 2015 19:25:42 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Fri, 10 Apr 2015 10:21:30 -0300

perf probe: Check the orphaned -x option

To avoid probing in unintended binary, the orphaned -x option must be
checked and warned.

Without this patch, following command sets up the probe in the kernel.

  -----
  # perf probe -a strcpy -x ./perf
  Added new event:
    probe:strcpy         (on strcpy)

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

          perf record -e probe:strcpy -aR sleep 1
  -----

But in this case, it seems that the user may want to probe in the perf
binary. With this patch, perf-probe correctly handles the orphaned -x.

  -----
  # perf probe -a strcpy -x ./perf
    Error: -x/-m must follow the probe definitions.
  ...
  -----

Reported-by: Jiri Olsa <jolsa@redhat.com>
Acked-by: Jiri Olsa <jolsa@redhat.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/20150401102541.17137.75477.stgit@localhost.localdomain
Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-probe.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
index 2df23e1..f7b1af6 100644
--- a/tools/perf/builtin-probe.c
+++ b/tools/perf/builtin-probe.c
@@ -56,6 +56,7 @@ static struct {
 	bool mod_events;
 	bool uprobes;
 	bool quiet;
+	bool target_used;
 	int nevents;
 	struct perf_probe_event events[MAX_PROBES];
 	struct strlist *dellist;
@@ -82,6 +83,7 @@ static int parse_probe_event(const char *str)
 		pev->target = strdup(params.target);
 		if (!pev->target)
 			return -ENOMEM;
+		params.target_used = true;
 	}
 
 	/* Parse a perf-probe command into event */
@@ -107,6 +109,7 @@ static int set_target(const char *ptr)
 		params.target = strdup(ptr);
 		if (!params.target)
 			return -ENOMEM;
+		params.target_used = false;
 
 		found = 1;
 		buf = ptr + (strlen(ptr) - 3);
@@ -207,6 +210,7 @@ static int opt_set_target(const struct option *opt, const char *str,
 		}
 		free(params.target);
 		params.target = tmp;
+		params.target_used = false;
 		ret = 0;
 	}
 
@@ -491,6 +495,12 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
 	}
 
 	if (params.nevents) {
+		/* Ensure the last given target is used */
+		if (params.target && !params.target_used) {
+			pr_warning("  Error: -x/-m must follow the probe definitions.\n");
+			usage_with_options(probe_usage, options);
+		}
+
 		ret = add_perf_probe_events(params.events, params.nevents,
 					    params.max_probe_points,
 					    params.force_add);