Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754806AbbKMPqg (ORCPT ); Fri, 13 Nov 2015 10:46:36 -0500 Received: from mail.kernel.org ([198.145.29.136]:52209 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754295AbbKMPqe (ORCPT ); Fri, 13 Nov 2015 10:46:34 -0500 Date: Fri, 13 Nov 2015 12:46:29 -0300 From: Arnaldo Carvalho de Melo To: Wang Nan Cc: masami.hiramatsu.pt@hitachi.com, ast@kernel.org, lizefan@huawei.com, pi3orama@163.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH 05/12] perf tools: Allow BPF program config probing options Message-ID: <20151113154629.GL7160@kernel.org> References: <1447417761-156094-1-git-send-email-wangnan0@huawei.com> <1447417761-156094-6-git-send-email-wangnan0@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1447417761-156094-6-git-send-email-wangnan0@huawei.com> X-Url: http://acmel.wordpress.com User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6476 Lines: 237 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 > Cc: Alexei Starovoitov > Cc: Arnaldo Carvalho de Melo > Cc: Masami Hiramatsu > Cc: Zefan Li > 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= ", > "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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/