Re: [PATCH 05/12] perf tools: Allow BPF program config probing options

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Fri, Nov 13, 2015 at 12:29:14PM +0000, Wang Nan escreveu:
> By extending the syntax of BPF object section names, this patch allows
> user to config probing options like what they can do in 'perf probe'.
> 
> Test result:
> 
> For following BPF file bpf.c:
> 
>  SEC("inlines=no\n"
>      "func=SyS_dup?")
>  int func(void *ctx)
>  {
>  	return 1;
>  }
> 
>  Cmdline:
> 
>  # ./perf record  -e ./test_probe_glob.c ls /
>  ...
>  [ perf record: Woken up 1 times to write data ]
>  [ perf record: Captured and wrote 0.013 MB perf.data ]
>  # ./perf evlist
>  perf_bpf_probe:func_1
>  perf_bpf_probe:func
> 
> Change "inlines=no" to "inlines=yes":
> 
> Cmdline:
> 
>  # ./perf record  -e ./test_probe_glob.c ls /
>  ...
>  [ perf record: Woken up 2 times to write data ]
>  [ perf record: Captured and wrote 0.013 MB perf.data ]
>  # ./perf evlist
>  perf_bpf_probe:func_3
>  perf_bpf_probe:func_2
>  perf_bpf_probe:func_1
>  perf_bpf_probe:func
> 
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Cc: Zefan Li <lizefan@huawei.com>
> Cc: pi3orama@163.com
> ---
>  tools/perf/util/bpf-loader.c | 50 +++++++++++++++++++++++++++++++++++++++++++-
>  tools/perf/util/config.c     |  9 ++++----
>  tools/perf/util/util.c       | 18 ++++++++++++++++
>  tools/perf/util/util.h       |  2 ++
>  4 files changed, 74 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/perf/util/bpf-loader.c b/tools/perf/util/bpf-loader.c
> index 8d78785..a368ead 100644
> --- a/tools/perf/util/bpf-loader.c
> +++ b/tools/perf/util/bpf-loader.c
> @@ -125,6 +125,38 @@ config__module(const char *value, struct perf_probe_event *pev)
>  	return 0;
>  }
>  
> +static int
> +config__bool(const char *value,
> +	     bool *pbool, bool invert)
> +{
> +	int err;
> +	bool bool_value;
> +
> +	if (!pbool)
> +		return -EINVAL;
> +
> +	err = convert_str_to_bool(value, &bool_value);
> +	if (err)
> +		return err;
> +
> +	*pbool = invert ? !bool_value : bool_value;
> +	return 0;


	if (!err)
		*pbool = invert ? !bool_value : bool_value;

	return err;

But again, no strict requirement, just a more compact form :)

> +}
> +
> +static int
> +config__inlines(const char *value,
> +		struct perf_probe_event *pev __maybe_unused)
> +{
> +	return config__bool(value, &probe_conf.no_inlines, true);
> +}
> +
> +static int
> +config__force(const char *value,
> +	      struct perf_probe_event *pev __maybe_unused)
> +{
> +	return config__bool(value, &probe_conf.force_add, false);
> +}
> +
>  static struct {
>  	const char *key;
>  	const char *usage;
> @@ -142,7 +174,19 @@ static struct {
>  		"module=<module name>    ",
>  		"Set kprobe module",
>  		config__module,
> -	}
> +	},
> +	{
> +		"inlines",
> +		"inlines=[yes|no]        ",
> +		"Probe at inline symbol",
> +		config__inlines,
> +	},
> +	{
> +		"force",
> +		"force=[yes|no]          ",
> +		"Forcibly add events with existing name",
> +		config__force,
> +	},

Named initializers for both, please

>  };
>  
>  static int
> @@ -240,6 +284,10 @@ config_bpf_program(struct bpf_program *prog)
>  	const char *config_str;
>  	int err;
>  
> +	/* Initialize per-program probing setting */
> +	probe_conf.no_inlines = false;
> +	probe_conf.force_add = false;
> +
>  	config_str = bpf_program__title(prog, false);
>  	if (IS_ERR(config_str)) {
>  		pr_debug("bpf: unable to get title for program\n");
> diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
> index 2e452ac..8219798 100644
> --- a/tools/perf/util/config.c
> +++ b/tools/perf/util/config.c
> @@ -351,15 +351,16 @@ int perf_config_int(const char *name, const char *value)
>  
>  static int perf_config_bool_or_int(const char *name, const char *value, int *is_bool)
>  {
> +	bool str_bool;
> +
>  	*is_bool = 1;
>  	if (!value)
>  		return 1;
>  	if (!*value)
>  		return 0;
> -	if (!strcasecmp(value, "true") || !strcasecmp(value, "yes") || !strcasecmp(value, "on"))
> -		return 1;
> -	if (!strcasecmp(value, "false") || !strcasecmp(value, "no") || !strcasecmp(value, "off"))
> -		return 0;
> +
> +	if (convert_str_to_bool(value, &str_bool) == 0)
> +		return str_bool ? 1 : 0;
>  	*is_bool = 0;
>  	return perf_config_int(name, value);
>  }
> diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
> index 47b1e36..55785d5 100644
> --- a/tools/perf/util/util.c
> +++ b/tools/perf/util/util.c
> @@ -695,3 +695,21 @@ fetch_kernel_version(unsigned int *puint, char *str,
>  		*puint = (version << 16) + (patchlevel << 8) + sublevel;
>  	return 0;
>  }
> +
> +int convert_str_to_bool(const char *str, bool *result)

strtobool() should be more compact and convey the same idea....

Hey, I googled for that name and guess what, the kernel has exactly this
function:

lib/string.c

/**
 * strtobool - convert common user inputs into boolean values
 * @s: input string
 * @res: result
 *
 * This routine returns 0 iff the first character is one of 'Yy1Nn0'.
 * Otherwise it will return -EINVAL.  Value pointed to by res is
 * updated upon finding a match.
 */

include/linux/string.h

So, please add it to tools/include/linux/string.h and
tools/lib/util/string.c, this way we use the same code as the kernel,
with the same function signature, etc.

Also this looks like a good thing to have on a separate patch, one that
introduces strtobool(), then the rest of this patch.

- Arnaldo

> +{
> +	if (!result || !str)
> +		return -EINVAL;
> +
> +	if (!strcasecmp(str, "true") || !strcasecmp(str, "yes") || !strcasecmp(str, "on")) {
> +		*result = true;
> +		return 0;
> +	}
> +
> +	if (!strcasecmp(str, "false") || !strcasecmp(str, "no") || !strcasecmp(str, "off")) {
> +		*result = false;
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
> index dcc6590..be90932 100644
> --- a/tools/perf/util/util.h
> +++ b/tools/perf/util/util.h
> @@ -358,4 +358,6 @@ int fetch_kernel_version(unsigned int *puint,
>  #define KVER_FMT	"%d.%d.%d"
>  #define KVER_PARAM(x)	KVER_VERSION(x), KVER_PATCHLEVEL(x), KVER_SUBLEVEL(x)
>  
> +int convert_str_to_bool(const char *str, bool *result);
> +
>  #endif /* GIT_COMPAT_UTIL_H */
> -- 
> 1.8.3.4