| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20160219150821.GD16141@kernel.org>
Date: Fri, 19 Feb 2016 12:08:21 -0300
From: Arnaldo Carvalho de Melo <acme@...nel.org>
To: Wang Nan <wangnan0@...wei.com>
Cc: Alexei Starovoitov <ast@...nel.org>,
Brendan Gregg <brendan.d.gregg@...il.com>,
Adrian Hunter <adrian.hunter@...el.com>,
Cody P Schafer <dev@...yps.com>,
"David S. Miller" <davem@...emloft.net>,
He Kuang <hekuang@...wei.com>,
Jérémie Galarneau
<jeremie.galarneau@...icios.com>, Jiri Olsa <jolsa@...nel.org>,
Kirill Smelkov <kirr@...edi.com>,
Li Zefan <lizefan@...wei.com>,
Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
Namhyung Kim <namhyung@...nel.org>,
Peter Zijlstra <peterz@...radead.org>, pi3orama@....com,
linux-kernel@...r.kernel.org
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@...wei.com>
> Cc: He Kuang <hekuang@...wei.com>
> Cc: Jiri Olsa <jolsa@...nel.org>
> Cc: Alexei Starovoitov <ast@...nel.org>
> Cc: Arnaldo Carvalho de Melo <acme@...hat.com>
> Cc: Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
> Cc: Namhyung Kim <namhyung@...nel.org>
> Cc: Zefan Li <lizefan@...wei.com>
> Cc: pi3orama@....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
Powered by blists - more mailing lists