From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1949213AbcBSPIa (ORCPT ); Fri, 19 Feb 2016 10:08:30 -0500 Received: from mail.kernel.org ([198.145.29.136]:58159 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1947730AbcBSPI2 (ORCPT ); Fri, 19 Feb 2016 10:08:28 -0500 Date: Fri, 19 Feb 2016 12:08:21 -0300 From: Arnaldo Carvalho de Melo To: Wang Nan Cc: Alexei Starovoitov , Brendan Gregg , Adrian Hunter , Cody P Schafer , "David S. Miller" , He Kuang , =?iso-8859-1?Q?J=E9r=E9mie?= Galarneau , Jiri Olsa , Kirill Smelkov , Li Zefan , Masami Hiramatsu , Namhyung Kim , Peter Zijlstra , pi3orama@163.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH 10/55] perf stat: Forbid user passing improper config terms Message-ID: <20160219150821.GD16141@kernel.org> References: <1455882283-79592-1-git-send-email-wangnan0@huawei.com> <1455882283-79592-11-git-send-email-wangnan0@huawei.com> <20160219140333.GB16141@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160219140333.GB16141@kernel.org> X-Url: http://acmel.wordpress.com User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Em Fri, Feb 19, 2016 at 11:43:58AM +0000, Wang Nan escreveu: > 'perf stat' accepts some config terms but doesn't apply them. For > example: > > # perf stat -e 'instructions/no-inherit/' -e 'instructions/inherit/' bash > # ls > # exit This seems to do the trick, but I wonder if we couldn't instead pass a callback or instead allow tools to provide a list of supported terms. I'll apply the patch as it improves the situation, i.e. before people were left wondering why the modifiers were not producing any effect, now they get a nice message, but I'd like to hear from Jiri what are his thoughts, when he's back. - Arnaldo > Performance counter stats for 'bash': > > 266258061 instructions/no-inherit/ > 266258061 instructions/inherit/ > > 1.402183915 seconds time elapsed > > The result is confusing, because user may expect the first > 'instructions' event exclude the 'ls' command. > > This patch forbid most of these config terms for 'perf stat'. > > Result: > > # ./perf stat -e 'instructions/no-inherit/' -e 'instructions/inherit/' bash > event syntax error: 'instructions/no-inherit/' > \___ 'no-inherit' is not usable in 'perf stat' > ... > > We can add blocked config terms back when 'perf stat' really supports them. > > This patch also removes unavailable config term from error message: > > # ./perf stat -e 'instructions/badterm/' ls > event syntax error: 'instructions/badterm/' > \___ unknown term > > valid terms: config,config1,config2,name > > # ./perf stat -e 'cpu/badterm/' ls > event syntax error: 'cpu/badterm/' > \___ unknown term > > valid terms: pc,any,inv,edge,cmask,event,in_tx,ldlat,umask,in_tx_cp,offcore_rsp,config,config1,config2,name > > Signed-off-by: Wang Nan > Cc: He Kuang > Cc: Jiri Olsa > Cc: Alexei Starovoitov > Cc: Arnaldo Carvalho de Melo > Cc: Masami Hiramatsu > Cc: Namhyung Kim > Cc: Zefan Li > Cc: pi3orama@163.com > --- > tools/perf/builtin-stat.c | 1 + > tools/perf/util/parse-events.c | 48 ++++++++++++++++++++++++++++++++++++++++++ > tools/perf/util/parse-events.h | 1 + > 3 files changed, 50 insertions(+) > > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c > index 86289df..8c0bc0f 100644 > --- a/tools/perf/builtin-stat.c > +++ b/tools/perf/builtin-stat.c > @@ -1831,6 +1831,7 @@ int cmd_stat(int argc, const char **argv, const char *prefix __maybe_unused) > if (evsel_list == NULL) > return -ENOMEM; > > + parse_events__shrink_config_terms(); > argc = parse_options_subcommand(argc, argv, stat_options, stat_subcommands, > (const char **) stat_usage, > PARSE_OPT_STOP_AT_NON_OPTION); > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c > index 3fe0d11..5b8a13b 100644 > --- a/tools/perf/util/parse-events.c > +++ b/tools/perf/util/parse-events.c > @@ -816,6 +816,41 @@ static const char *config_term_names[__PARSE_EVENTS__TERM_TYPE_NR] = { > [PARSE_EVENTS__TERM_TYPE_INHERIT] = "inherit", > }; > > +static bool config_term_shrinked; > + > +static bool > +config_term_avail(int term_type, struct parse_events_error *err) > +{ > + if (term_type < 0 || term_type >= __PARSE_EVENTS__TERM_TYPE_NR) { > + err->str = strdup("Invalid term_type"); > + return false; > + } > + if (!config_term_shrinked) > + return true; > + > + switch (term_type) { > + case PARSE_EVENTS__TERM_TYPE_CONFIG: > + case PARSE_EVENTS__TERM_TYPE_CONFIG1: > + case PARSE_EVENTS__TERM_TYPE_CONFIG2: > + case PARSE_EVENTS__TERM_TYPE_NAME: > + return true; > + default: > + if (!err) > + return false; > + > + /* term_type is validated so indexing is safe */ > + if (asprintf(&err->str, "'%s' is not usable in 'perf stat'", > + config_term_names[term_type]) < 0) > + err->str = NULL; > + return false; > + } > +} > + > +void parse_events__shrink_config_terms(void) > +{ > + config_term_shrinked = true; > +} > + > typedef int config_term_func_t(struct perf_event_attr *attr, > struct parse_events_term *term, > struct parse_events_error *err); > @@ -885,6 +920,17 @@ do { \ > return -EINVAL; > } > > + /* > + * Check term availbility after basic checking so > + * PARSE_EVENTS__TERM_TYPE_USER can be found and filtered. > + * > + * If check availbility at the entry of this function, > + * user will see "'' is not usable in 'perf stat'" > + * if an invalid config term is provided for legacy events > + * (for example, instructions/badterm/...), which is confusing. > + */ > + if (!config_term_avail(term->type_term, err)) > + return -EINVAL; > return 0; > #undef CHECK_TYPE_VAL > } > @@ -2177,6 +2223,8 @@ static void config_terms_list(char *buf, size_t buf_sz) > for (i = 0; i < __PARSE_EVENTS__TERM_TYPE_NR; i++) { > const char *name = config_term_names[i]; > > + if (!config_term_avail(i, NULL)) > + continue; > if (!name) > continue; > if (name[0] == '<') > diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h > index 2196db9..b6afc57 100644 > --- a/tools/perf/util/parse-events.h > +++ b/tools/perf/util/parse-events.h > @@ -106,6 +106,7 @@ struct parse_events_terms { > struct list_head *terms; > }; > > +void parse_events__shrink_config_terms(void); > int parse_events__is_hardcoded_term(struct parse_events_term *term); > int parse_events_term__num(struct parse_events_term **term, > int type_term, char *config, u64 num, > -- > 1.8.3.4