Messages in this thread Patch in this message |  | | Date | Fri, 19 Feb 2016 12:08:21 -0300 | From | Arnaldo Carvalho de Melo <> | Subject | Re: [PATCH 10/55] perf stat: Forbid user passing improper config terms |
| |
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 <wangnan0@huawei.com> > Cc: He Kuang <hekuang@huawei.com> > Cc: Jiri Olsa <jolsa@kernel.org> > Cc: Alexei Starovoitov <ast@kernel.org> > Cc: Arnaldo Carvalho de Melo <acme@redhat.com> > Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> > Cc: Namhyung Kim <namhyung@kernel.org> > Cc: Zefan Li <lizefan@huawei.com> > 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 "'<sysfs term>' 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
|  |