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

2014-08-13 Thread 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 +, 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...@hitachi.com


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2014-08-13 Thread 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 +, 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2014-08-13 Thread Masami Hiramatsu 
(2014/08/13 14:22), Namhyung Kim wrote:
> Hi Masami,
> 
> On Wed, 13 Aug 2014 00:50:55 +, 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...@hitachi.com


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2014-08-12 Thread Namhyung Kim 
Hi Masami,

On Wed, 13 Aug 2014 00:50:55 +, 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;
>  }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2014-08-12 Thread 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 
Reported-by: Naohiro Aota 
Cc: Peter Zijlstra 
Cc: Paul Mackerras 
Cc: Ingo Molnar 
Cc: Arnaldo Carvalho de Melo 
---
 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