perf probe: -x option position issue

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

Em Mon, Mar 30, 2015 at 07:46:55PM +0200, Jiri Olsa escreveu:

Quoted 25 lines

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

Quoted 1 line

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

Quoted 18 lines

[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

Quoted 16 lines · Arnaldo Carvalho de Melo

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)

Quoted 5 lines

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.

Quoted 4 lines

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 -a XXX

perf probe -a XXX -x BIN

	 -> setup XXX on BIN

perf probe -x BIN -a XXX -x BIN2 -a YYY

	 -> setup XXX on BIN and YYY on BIN2

Em Tue, Mar 31, 2015 at 05:04:18PM +0900, Masami Hiramatsu escreveu:

Quoted 30 lines

Quoted 16 lines · Arnaldo Carvalho de Melo

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)

Quoted 5 lines

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.

Quoted 4 lines

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).

Quoted 6 lines

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

Quoted 4 lines

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:

Quoted 1 line

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:

Quoted 2 lines

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.

Quoted 2 lines

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

:-)

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 <[email protected]>

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 <[email protected]>

Signed-off-by Masami Hiramatsu <[email protected]>

---
 tools/perf/builtin-probe.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

tools/perf/builtin-probe.c +10 -0

index 2df23e1..f7b1af6 100644
@@ -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);

Ping?

Quoted 81 lines · 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 <[email protected]>

Signed-off-by Masami Hiramatsu <[email protected]>

---
 tools/perf/builtin-probe.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

tools/perf/builtin-probe.c +10 -0

index 2df23e1..f7b1af6 100644
@@ -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);

Signature · 5 lines

Masami HIRAMATSU
Linux Technology Research Center, System Productivity Research Dept.
Center for Technology Innovation - Systems Engineering
Hitachi, Ltd., Research & Development Group
E-mail: [email protected]