Re: [RFC PATCH 2/4] perf-probe: Add offline output directory option

[Arnaldo Carvalho de Melo]

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:

  [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
  #

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

:-)

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.

Also please fix the OPT_STRING string, it should start with a capital
letter:

        --max-probes <n>  Set how many probe points can be found for a probe.
        --no-inlines      Don't search inlined functions
        --outdir <directory>
                          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.

- Arnaldo
 
> Signed-off-by: Masami Hiramatsu <mhiramat@xxxxxxxxxx>
> ---
>  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: