[PATCH 1/2] [BUGFIX] perf probe: Fix --list option to show events only with uprobe events

[PATCH 1/2] [BUGFIX] perf probe: Fix --list option to show events only with uprobe events

[Masami Hiramatsu]

Current perf probe --list doesn't work if only CONFIG_UPROBE_EVENTS=y
because it aborts when it fails to open kprobe_events file before
checking uprobe_events file.

This fixes --list option to show dynamic events if it can open
either kprobe_events or uprobe_events. Only if it failed to open
both of them, it shows an error message and aborts.

Without this patch, if we run perf probe -l on the kernel configured
with CONFIG_KPROBE_EVENTS=n and CONFIG_UPROBE_EVENTS=y,

  # perf probe -l
  /sys/kernel/debug/tracing/kprobe_events file does not exist - please rebuild ker
    Error: Failed to show event list.

With this patch,

  # perf probe -l
    probe_perf:alloc_event (on alloc_event@lib/traceevent/event-parse.c in /home/fedora/ksrc/linux-3/tools/perf/perf)


Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Reported-by: Naohiro Aota <naota@elisp.net>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
---
 tools/perf/util/probe-event.c |   84 ++++++++++++++++++++++++++---------------
 1 file changed, 54 insertions(+), 30 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 9a0a183..b4f6555 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -1780,10 +1780,10 @@ static void clear_probe_trace_event(struct probe_trace_event *tev)
 	memset(tev, 0, sizeof(*tev));
 }
 
-static void print_warn_msg(const char *file, bool is_kprobe)
+static void print_open_warning(int err, bool is_kprobe)
 {
 
-	if (errno == ENOENT) {
+	if (err == -ENOENT) {
 		const char *config;
 
 		if (!is_kprobe)
@@ -1791,25 +1791,25 @@ static void print_warn_msg(const char *file, bool is_kprobe)
 		else
 			config = "CONFIG_KPROBE_EVENTS";
 
-		pr_warning("%s file does not exist - please rebuild kernel"
-				" with %s.\n", file, config);
-	} else
-		pr_warning("Failed to open %s file: %s\n", file,
-				strerror(errno));
+		pr_warning("%cprobe_events file does not exist"
+			   " - please rebuild kernel with %s.\n",
+			   is_kprobe ? 'k' : 'u', config);
+	} else if (err == -ENOTSUP)
+		pr_warning("Debugfs is not mounted.\n");
+	else
+		pr_warning("Failed to open %cprobe_events: %s\n",
+			   is_kprobe ? 'k' : 'u', strerror(-err));
 }
 
-static int open_probe_events(const char *trace_file, bool readwrite,
-				bool is_kprobe)
+static int open_probe_events(const char *trace_file, bool readwrite)
 {
 	char buf[PATH_MAX];
 	const char *__debugfs;
 	int ret;
 
 	__debugfs = debugfs_find_mountpoint();
-	if (__debugfs == NULL) {
-		pr_warning("Debugfs is not mounted.\n");
-		return -ENOENT;
-	}
+	if (__debugfs == NULL)
+		return -ENOTSUP;
 
 	ret = e_snprintf(buf, PATH_MAX, "%s/%s", __debugfs, trace_file);
 	if (ret >= 0) {
@@ -1820,19 +1820,19 @@ static int open_probe_events(const char *trace_file, bool readwrite,
 			ret = open(buf, O_RDONLY, 0);
 
 		if (ret < 0)
-			print_warn_msg(buf, is_kprobe);
+			ret = -errno;
 	}
 	return ret;
 }
 
 static int open_kprobe_events(bool readwrite)
 {
-	return open_probe_events("tracing/kprobe_events", readwrite, true);
+	return open_probe_events("tracing/kprobe_events", readwrite);
 }
 
 static int open_uprobe_events(bool readwrite)
 {
-	return open_probe_events("tracing/uprobe_events", readwrite, false);
+	return open_probe_events("tracing/uprobe_events", readwrite);
 }
 
 /* Get raw string list of current kprobe_events  or uprobe_events */
@@ -1940,27 +1940,44 @@ static int __show_perf_probe_events(int fd, bool is_kprobe)
 /* List up current perf-probe events */
 int show_perf_probe_events(void)
 {
-	int fd, ret;
+	int kp_fd, up_fd, ret;
 
 	setup_pager();
-	fd = open_kprobe_events(false);
-
-	if (fd < 0)
-		return fd;
 
 	ret = init_symbol_maps(false);
 	if (ret < 0)
 		return ret;
 
-	ret = __show_perf_probe_events(fd, true);
-	close(fd);
+	kp_fd = open_kprobe_events(false);
+	if (kp_fd >= 0) {
+		ret = __show_perf_probe_events(kp_fd, true);
+		close(kp_fd);
+		if (ret < 0)
+			goto out;
+	}
 
-	fd = open_uprobe_events(false);
-	if (fd >= 0) {
-		ret = __show_perf_probe_events(fd, false);
-		close(fd);
+	up_fd = open_uprobe_events(false);
+	if (kp_fd < 0 && up_fd < 0) {
+		/* Both kprobes and uprobes are disabled, warn it. */
+		if (kp_fd == -ENOTSUP && up_fd == -ENOTSUP)
+			pr_warning("Debugfs is not mounted.\n");
+		else if (kp_fd == -ENOENT && up_fd == -ENOENT)
+			pr_warning("Please rebuild kernel with "
+				   "CONFIG_KPROBE_EVENTS or/and "
+				   "CONFIG_UPROBE_EVENTS.\n");
+		else
+			pr_warning("Failed to open kprobe events: %s.\n" \
+				   "Failed to open uprobe events: %s.\n",
+				   strerror(-kp_fd), strerror(-up_fd));
+		ret = kp_fd;
+		goto out;
 	}
 
+	if (up_fd >= 0) {
+		ret = __show_perf_probe_events(up_fd, false);
+		close(up_fd);
+	}
+out:
 	exit_symbol_maps();
 	return ret;
 }
@@ -2075,8 +2092,11 @@ static int __add_probe_trace_events(struct perf_probe_event *pev,
 	else
 		fd = open_kprobe_events(true);
 
-	if (fd < 0)
+	if (fd < 0) {
+		print_open_warning(fd, !pev->uprobes);
 		return fd;
+	}
+
 	/* Get current event names */
 	namelist = get_probe_trace_event_names(fd, false);
 	if (!namelist) {
@@ -2449,13 +2469,17 @@ int del_perf_probe_events(struct strlist *dellist)
 
 	/* Get current event names */
 	kfd = open_kprobe_events(true);
-	if (kfd < 0)
+	if (kfd < 0) {
+		print_open_warning(kfd, true);
 		return kfd;
+	}
 
 	namelist = get_probe_trace_event_names(kfd, true);
 	ufd = open_uprobe_events(true);
 
-	if (ufd >= 0)
+	if (ufd < 0)
+		print_open_warning(ufd, false);
+	else
 		unamelist = get_probe_trace_event_names(ufd, true);
 
 	if (namelist == NULL && unamelist == NULL)

[PATCH 2/2] [BUGFIX] perf probe: Fix --del option to delete events only with uprobe events

[Masami Hiramatsu]

Current perf probe --del doesn't work if only CONFIG_UPROBE_EVENTS=y
because it aborts when it fails to open kprobe_events file before
checking uprobe_events file.

This fixes --del option to delete dynamic events if it can open
either kprobe_events or uprobe_events. Only if it failed to open
both of them, it shows an error message and aborts.

Without this patch, if we run perf probe -d on the kernel configured
with CONFIG_KPROBE_EVENTS=n and CONFIG_UPROBE_EVENTS=y,

  # perf probe -d \*
  kprobe_events file does not exist - please rebuild kernel with CONFIG_KPROBE_EVENTS.
    Error: Failed to delete events.

With this patch,

  # perf probe -d \*
  Removed event: probe_perf:alloc_event

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
---
 tools/perf/util/probe-event.c |   43 ++++++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 20 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index b4f6555..03108f6 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -1801,6 +1801,20 @@ static void print_open_warning(int err, bool is_kprobe)
 			   is_kprobe ? 'k' : 'u', strerror(-err));
 }
 
+static void print_both_open_warning(int kerr, int uerr)
+{
+	/* Both kprobes and uprobes are disabled, warn it. */
+	if (kerr == -ENOTSUP && uerr == -ENOTSUP)
+		pr_warning("Debugfs is not mounted.\n");
+	else if (kerr == -ENOENT && uerr == -ENOENT)
+		pr_warning("Please rebuild kernel with CONFIG_KPROBE_EVENTS "
+			   "or/and CONFIG_UPROBE_EVENTS.\n");
+	else
+		pr_warning("Failed to open kprobe events: %s.\n" \
+			   "Failed to open uprobe events: %s.\n",
+			   strerror(-kerr), strerror(-uerr));
+}
+
 static int open_probe_events(const char *trace_file, bool readwrite)
 {
 	char buf[PATH_MAX];
@@ -1958,17 +1972,7 @@ int show_perf_probe_events(void)
 
 	up_fd = open_uprobe_events(false);
 	if (kp_fd < 0 && up_fd < 0) {
-		/* Both kprobes and uprobes are disabled, warn it. */
-		if (kp_fd == -ENOTSUP && up_fd == -ENOTSUP)
-			pr_warning("Debugfs is not mounted.\n");
-		else if (kp_fd == -ENOENT && up_fd == -ENOENT)
-			pr_warning("Please rebuild kernel with "
-				   "CONFIG_KPROBE_EVENTS or/and "
-				   "CONFIG_UPROBE_EVENTS.\n");
-		else
-			pr_warning("Failed to open kprobe events: %s.\n" \
-				   "Failed to open uprobe events: %s.\n",
-				   strerror(-kp_fd), strerror(-up_fd));
+		print_both_open_warning(kp_fd, up_fd);
 		ret = kp_fd;
 		goto out;
 	}
@@ -2469,19 +2473,18 @@ int del_perf_probe_events(struct strlist *dellist)
 
 	/* Get current event names */
 	kfd = open_kprobe_events(true);
-	if (kfd < 0) {
-		print_open_warning(kfd, true);
-		return kfd;
-	}
+	if (kfd >= 0)
+		namelist = get_probe_trace_event_names(kfd, true);
 
-	namelist = get_probe_trace_event_names(kfd, true);
 	ufd = open_uprobe_events(true);
-
-	if (ufd < 0)
-		print_open_warning(ufd, false);
-	else
+	if (ufd >= 0)
 		unamelist = get_probe_trace_event_names(ufd, true);
 
+	if (kfd < 0 && ufd < 0) {
+		print_both_open_warning(kfd, ufd);
+		goto error;
+	}
+
 	if (namelist == NULL && unamelist == NULL)
 		goto error;
 

Re: [PATCH 1/2] [BUGFIX] perf probe: Fix --list option to show events only with uprobe events

[Namhyung Kim]

Hi Masami,

On Wed, 13 Aug 2014 00:50:55 +0000, Masami Hiramatsu wrote:
> +	if (kp_fd < 0 && up_fd < 0) {
> +		/* Both kprobes and uprobes are disabled, warn it. */
> +		if (kp_fd == -ENOTSUP && up_fd == -ENOTSUP)
> +			pr_warning("Debugfs is not mounted.\n");
> +		else if (kp_fd == -ENOENT && up_fd == -ENOENT)
> +			pr_warning("Please rebuild kernel with "
> +				   "CONFIG_KPROBE_EVENTS or/and "
> +				   "CONFIG_UPROBE_EVENTS.\n");
> +		else
> +			pr_warning("Failed to open kprobe events: %s.\n" \
> +				   "Failed to open uprobe events: %s.\n",
> +				   strerror(-kp_fd), strerror(-up_fd));

It seems the second strerror() might overwrite the message of the
first.  You'd better using strerror_r() IMHO.

Thanks,
Namhyung


> +		ret = kp_fd;
> +		goto out;
>  	}
>  
> +	if (up_fd >= 0) {
> +		ret = __show_perf_probe_events(up_fd, false);
> +		close(up_fd);
> +	}
> +out:
>  	exit_symbol_maps();
>  	return ret;
>  }

Re: [PATCH 1/2] [BUGFIX] perf probe: Fix --list option to show events only with uprobe events

[Masami Hiramatsu]

(2014/08/13 14:22), Namhyung Kim wrote:
> Hi Masami,
> 
> On Wed, 13 Aug 2014 00:50:55 +0000, Masami Hiramatsu wrote:
>> +	if (kp_fd < 0 && up_fd < 0) {
>> +		/* Both kprobes and uprobes are disabled, warn it. */
>> +		if (kp_fd == -ENOTSUP && up_fd == -ENOTSUP)
>> +			pr_warning("Debugfs is not mounted.\n");
>> +		else if (kp_fd == -ENOENT && up_fd == -ENOENT)
>> +			pr_warning("Please rebuild kernel with "
>> +				   "CONFIG_KPROBE_EVENTS or/and "
>> +				   "CONFIG_UPROBE_EVENTS.\n");
>> +		else
>> +			pr_warning("Failed to open kprobe events: %s.\n" \
>> +				   "Failed to open uprobe events: %s.\n",
>> +				   strerror(-kp_fd), strerror(-up_fd));
> 
> It seems the second strerror() might overwrite the message of the
> first.  You'd better using strerror_r() IMHO.

Oops, right, it must use the same buffer...
But instead of using strerror_r, we can call pr_warning twice. Or should we
better replace all strerror to strerror_r in perf? (it should be another series)

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/2] [BUGFIX] perf probe: Fix --list option to show events only with uprobe events

[Arnaldo Carvalho de Melo]

Em Wed, Aug 13, 2014 at 11:21:34PM +0900, Masami Hiramatsu escreveu:
> (2014/08/13 14:22), Namhyung Kim wrote:
> > On Wed, 13 Aug 2014 00:50:55 +0000, Masami Hiramatsu wrote:
> >> +	if (kp_fd < 0 && up_fd < 0) {
> >> +		/* Both kprobes and uprobes are disabled, warn it. */
> >> +		if (kp_fd == -ENOTSUP && up_fd == -ENOTSUP)
> >> +			pr_warning("Debugfs is not mounted.\n");
> >> +		else if (kp_fd == -ENOENT && up_fd == -ENOENT)
> >> +			pr_warning("Please rebuild kernel with "
> >> +				   "CONFIG_KPROBE_EVENTS or/and "
> >> +				   "CONFIG_UPROBE_EVENTS.\n");
> >> +		else
> >> +			pr_warning("Failed to open kprobe events: %s.\n" \
> >> +				   "Failed to open uprobe events: %s.\n",
> >> +				   strerror(-kp_fd), strerror(-up_fd));
> > 
> > It seems the second strerror() might overwrite the message of the
> > first.  You'd better using strerror_r() IMHO.

Well spotted!
 
> Oops, right, it must use the same buffer...
> But instead of using strerror_r, we can call pr_warning twice. Or should we
> better replace all strerror to strerror_r in perf? (it should be another series)

Well, don't introduce new strerror() uses, we have threads in perf
already and if both try to use strerror() for different reasons, say the
UI to print something to the user and some logging/debugging thread do
it to the disk, we may race.

So, in this case, please use strerror_r() and if you feel like
contributing the changes to any other place where strerror() is still
used, you are welcome to do so at a later patch :)

- Arnaldo

Re: [PATCH 1/2] [BUGFIX] perf probe: Fix --list option to show events only with uprobe events

[Masami Hiramatsu]

(2014/08/13 23:48), Arnaldo Carvalho de Melo wrote:
> Em Wed, Aug 13, 2014 at 11:21:34PM +0900, Masami Hiramatsu escreveu:
>> (2014/08/13 14:22), Namhyung Kim wrote:
>>> On Wed, 13 Aug 2014 00:50:55 +0000, Masami Hiramatsu wrote:
>>>> +	if (kp_fd < 0 && up_fd < 0) {
>>>> +		/* Both kprobes and uprobes are disabled, warn it. */
>>>> +		if (kp_fd == -ENOTSUP && up_fd == -ENOTSUP)
>>>> +			pr_warning("Debugfs is not mounted.\n");
>>>> +		else if (kp_fd == -ENOENT && up_fd == -ENOENT)
>>>> +			pr_warning("Please rebuild kernel with "
>>>> +				   "CONFIG_KPROBE_EVENTS or/and "
>>>> +				   "CONFIG_UPROBE_EVENTS.\n");
>>>> +		else
>>>> +			pr_warning("Failed to open kprobe events: %s.\n" \
>>>> +				   "Failed to open uprobe events: %s.\n",
>>>> +				   strerror(-kp_fd), strerror(-up_fd));
>>>
>>> It seems the second strerror() might overwrite the message of the
>>> first.  You'd better using strerror_r() IMHO.
> 
> Well spotted!
>  
>> Oops, right, it must use the same buffer...
>> But instead of using strerror_r, we can call pr_warning twice. Or should we
>> better replace all strerror to strerror_r in perf? (it should be another series)
> 
> Well, don't introduce new strerror() uses, we have threads in perf
> already and if both try to use strerror() for different reasons, say the
> UI to print something to the user and some logging/debugging thread do
> it to the disk, we may race.

OK, I'll use strerror_r() in next version.

> So, in this case, please use strerror_r() and if you feel like
> contributing the changes to any other place where strerror() is still
> used, you are welcome to do so at a later patch :)

Yeah, I'll do that. :)

Thank you!

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