From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757958AbcHYGiI (ORCPT ); Thu, 25 Aug 2016 02:38:08 -0400 Received: from mail.kernel.org ([198.145.29.136]:48182 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757076AbcHYGiH (ORCPT ); Thu, 25 Aug 2016 02:38:07 -0400 Date: Thu, 25 Aug 2016 15:37:52 +0900 From: Masami Hiramatsu To: Arnaldo Carvalho de Melo Cc: linux-kernel@vger.kernel.org, Peter Zijlstra , Ingo Molnar , Jiri Olsa Subject: Re: [RFC PATCH 2/4] perf-probe: Add offline output directory option Message-Id: <20160825153752.7cf214ae4d81360e32b8a37a@kernel.org> In-Reply-To: <20160824125845.GB10063@kernel.org> References: <147201825017.5713.10732844032749364745.stgit@devbox> <147201829193.5713.7794866764620845084.stgit@devbox> <20160824125845.GB10063@kernel.org> X-Mailer: Sylpheed 3.5.0 (GTK+ 2.24.30; x86_64-pc-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 24 Aug 2016 09:58:45 -0300 Arnaldo Carvalho de Melo wrote: > Em Wed, Aug 24, 2016 at 02:58:12PM +0900, Masami Hiramatsu escreveu: > > Add offline output direcrtory option. This allows user to > > store probe event definition in offline output directory. > > In such cases you should show it in action, like I am doing now to test > this patch: OK, I'll add it. > > [root@jouet ~]# perf probe --outdir=my_probes do_open > kprobe_events file does not exist - please rebuild kernel with CONFIG_KPROBE_EVENTS. > Error: Failed to add events. > [root@jouet ~]# > > Oops, misleading error message, probably I need to first do a mkdir: > > [root@jouet ~]# mkdir my_probes > [root@jouet ~]# perf probe --outdir=my_probes do_open > Added new event: > probe:do_open (on do_open) > > You can now use it in all perf tools, such as: > > perf record -e probe:do_open -aR sleep 1 > > [root@jouet ~]# ls -la my_probes > total 12 > drwxr-xr-x. 2 root root 4096 Aug 24 09:47 . > dr-xr-x---. 31 root root 4096 Aug 24 09:47 .. > -rw-r--r--. 1 root root 30 Aug 24 09:47 kprobe_events > [root@jouet ~]# cat my_probes/kprobe_events > p:probe/do_open _text+3481584 > [root@jouet ~]# > > Wouldn't it be better to call it some other name and imply --dry-run? "outdir" > seems too vague. Perhaps --definition and instead have it run like: > > # perf probe --definition do_open > p:probe/do_open _text+3481584 > # Yeah, it looks good especially we don't have to think about --del or --list with that... > > Then one could even use it to redirect it to a file, tee + file and even test it as: > > # perf probe --definition do_open > /sys/kernel/debug/tracing/kprobe_events > # > > For debugging: > > # perf probe --definition do_open | tee /sys/kernel/debug/tracing/kprobe_events > > :-) > Yeah, it also makes use perf-probe in shell script easier. > I picked "--kprobes" first, but that would left out e.g. uprobes, then I > thought about --event, but also sounds vague, read > Documentation/trace/kprobetrace.txt and saw this bit: > > ------- > Usage examples > -------------- > To add a probe as a new event, write a new definition to kprobe_events > as below. > > echo 'p:myprobe do_sys_open dfd=%ax filename=%dx flags=%cx mode=+4($stack)' > /sys/kernel/debug/tracing/kprobe_events > ------- > > So you used "definition" for that string, might as well be consistent and use > it here as well. OK, I couldn't get better name. --definition or -D will be good. > Also please fix the OPT_STRING string, it should start with a capital > letter: Ah, I see :) > > --max-probes Set how many probe points can be found for a probe. > --no-inlines Don't search inlined functions > --outdir > path to offline output directory > --range Show variables location range in scope (with --vars only) > > > See how it stands out? All the others start with a capital letter. > > I applied the first patch, totally trivial. Thanks! > > - Arnaldo > > > Signed-off-by: Masami Hiramatsu > > --- > > tools/perf/builtin-probe.c | 2 ++ > > tools/perf/util/probe-event.h | 1 + > > tools/perf/util/probe-file.c | 19 ++++++++++++++++--- > > 3 files changed, 19 insertions(+), 3 deletions(-) > > > > diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c > > index ee5b421..5b45ec8 100644 > > --- a/tools/perf/builtin-probe.c > > +++ b/tools/perf/builtin-probe.c > > @@ -517,6 +517,8 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused) > > "Show variables location range in scope (with --vars only)"), > > OPT_STRING('k', "vmlinux", &symbol_conf.vmlinux_name, > > "file", "vmlinux pathname"), > > + OPT_STRING(0, "outdir", &probe_conf.output_dir, > > + "directory", "path to offline output directory"), > > OPT_STRING('s', "source", &symbol_conf.source_prefix, > > "directory", "path to kernel source"), > > OPT_BOOLEAN('\0', "no-inlines", &probe_conf.no_inlines, > > diff --git a/tools/perf/util/probe-event.h b/tools/perf/util/probe-event.h > > index f4f45db..75b572a 100644 > > --- a/tools/perf/util/probe-event.h > > +++ b/tools/perf/util/probe-event.h > > @@ -14,6 +14,7 @@ struct probe_conf { > > bool no_inlines; > > bool cache; > > int max_probes; > > + const char *output_dir; > > }; > > extern struct probe_conf probe_conf; > > extern bool probe_event_dry_run; > > diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c > > index 6f931e4..41db430 100644 > > --- a/tools/perf/util/probe-file.c > > +++ b/tools/perf/util/probe-file.c > > @@ -73,16 +73,25 @@ static void print_both_open_warning(int kerr, int uerr) > > static int open_probe_events(const char *trace_file, bool readwrite) > > { > > char buf[PATH_MAX]; > > + const char *tracing_dir = tracing_path; > > + int oflag = 0; > > + mode_t mode = 0; > > int ret; > > > > + if (probe_conf.output_dir) { > > + tracing_dir = probe_conf.output_dir; > > + oflag = O_CREAT; > > + mode = 0644; > > + } > > + > > ret = e_snprintf(buf, PATH_MAX, "%s/%s", > > - tracing_path, trace_file); > > + tracing_dir, trace_file); > > if (ret >= 0) { > > pr_debug("Opening %s write=%d\n", buf, readwrite); > > if (readwrite && !probe_event_dry_run) > > - ret = open(buf, O_RDWR | O_APPEND, 0); > > + ret = open(buf, O_RDWR | O_APPEND | oflag, mode); > > else > > - ret = open(buf, O_RDONLY, 0); > > + ret = open(buf, O_RDONLY | oflag, mode); > > > > if (ret < 0) > > ret = -errno; > > @@ -242,6 +251,8 @@ int probe_file__add_event(int fd, struct probe_trace_event *tev) > > pr_warning("Failed to write event: %s\n", > > str_error_r(errno, sbuf, sizeof(sbuf))); > > } > > + if (probe_conf.output_dir) > > + ret = write(fd, "\n", 1) == 1 ? 0 : -errno; > > } > > free(buf); > > > > @@ -274,6 +285,8 @@ static int __del_trace_probe_event(int fd, struct str_node *ent) > > ret = -errno; > > goto error; > > } > > + if (probe_conf.output_dir) > > + ret = write(fd, "\n", 1) == 1 ? 0 : -errno; > > > > return 0; > > error: -- Masami Hiramatsu