Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200725121959.1181869-1-jolsa@kernel.org> <20200725121959.1181869-2-jolsa@kernel.org>
In-Reply-To: <20200725121959.1181869-2-jolsa@kernel.org>
From:   Ian Rogers <irogers@google.com>
Date:   Sat, 25 Jul 2020 09:58:13 -0700
Message-ID: <CAP-5=fUmJSZfCrP=gdYOwd2gpAgXsiSTBDwtLp=_e7a434weTA@mail.gmail.com>
Subject: Re: [PATCH 2/2] perf tools: Fix term parsing for raw syntax
To:     Jiri Olsa <jolsa@kernel.org>
Cc:     Arnaldo Carvalho de Melo <acme@kernel.org>,
        Jin Yao <yao.jin@linux.intel.com>,
        lkml <linux-kernel@vger.kernel.org>,
        Ingo Molnar <mingo@kernel.org>,
        Namhyung Kim <namhyung@kernel.org>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Peter Zijlstra <a.p.zijlstra@chello.nl>,
        Michael Petlan <mpetlan@redhat.com>,
        Stephane Eranian <eranian@google.com>,
        Andi Kleen <ak@linux.intel.com>,
        Thomas Richter <tmricht@linux.ibm.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sat, Jul 25, 2020 at 5:20 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Jin Yao reported issue with possible conflict between raw
> events and term values in pmu event syntax.
>
> Currently following syntax is resolved as raw event with
> 0xead value:
>   uncore_imc_free_running/read/
>
> instead of using 'read' term from uncore_imc_free_running pmu,
> because 'read' is correct raw event syntax with 0xead value.
>
> To solve this issue we do following:
>   - check existing terms during rXXXX syntax processing
>     and make them priority in case of conflict
>   - allow pmu/r0x1234/ syntax to be able to specify conflicting
>     raw event (implemented in previous patch)
>
> Also adding automated tests for this and perf_pmu__parse_cleanup
> call to parse_events_terms, so the test gets properly cleaned up.
>
> Reported-by: Jin Yao <yao.jin@linux.intel.com>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/perf/tests/parse-events.c | 37 ++++++++++++++++++++++++++++++++-
>  tools/perf/util/parse-events.c  | 23 ++++++++++++++++++++
>  tools/perf/util/parse-events.h  |  2 ++
>  tools/perf/util/parse-events.l  | 19 ++++++++++-------
>  4 files changed, 72 insertions(+), 9 deletions(-)
>
> diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
> index 5aaddcb0058a..7f9f87a470c3 100644
> --- a/tools/perf/tests/parse-events.c
> +++ b/tools/perf/tests/parse-events.c
> @@ -631,6 +631,34 @@ static int test__checkterms_simple(struct list_head *terms)
>         TEST_ASSERT_VAL("wrong val", term->val.num == 1);
>         TEST_ASSERT_VAL("wrong config", !strcmp(term->config, "umask"));
>
> +       /*
> +        * read
> +        *
> +        * The perf_pmu__test_parse_init injects 'read' term into
> +        * perf_pmu_events_list, so 'read' is evaluated as read term
> +        * and not as raw event with 'ead' hex value.
> +        */
> +       term = list_entry(term->list.next, struct parse_events_term, list);
> +       TEST_ASSERT_VAL("wrong type term",
> +                       term->type_term == PARSE_EVENTS__TERM_TYPE_USER);
> +       TEST_ASSERT_VAL("wrong type val",
> +                       term->type_val == PARSE_EVENTS__TERM_TYPE_NUM);
> +       TEST_ASSERT_VAL("wrong val", term->val.num == 1);
> +       TEST_ASSERT_VAL("wrong config", !strcmp(term->config, "read"));
> +
> +       /*
> +        * r0xead
> +        *
> +        * To be still able to pass 'ead' value with 'r' syntax,
> +        * we added support to parse 'r0xHEX' event.
> +        */
> +       term = list_entry(term->list.next, struct parse_events_term, list);
> +       TEST_ASSERT_VAL("wrong type term",
> +                       term->type_term == PARSE_EVENTS__TERM_TYPE_CONFIG);
> +       TEST_ASSERT_VAL("wrong type val",
> +                       term->type_val == PARSE_EVENTS__TERM_TYPE_NUM);
> +       TEST_ASSERT_VAL("wrong val", term->val.num == 0xead);
> +       TEST_ASSERT_VAL("wrong config", !term->config);
>         return 0;
>  }
>
> @@ -1781,7 +1809,7 @@ struct terms_test {
>
>  static struct terms_test test__terms[] = {
>         [0] = {
> -               .str   = "config=10,config1,config2=3,umask=1",
> +               .str   = "config=10,config1,config2=3,umask=1,read,r0xead",
>                 .check = test__checkterms_simple,
>         },
>  };
> @@ -1841,6 +1869,13 @@ static int test_term(struct terms_test *t)
>
>         INIT_LIST_HEAD(&terms);
>
> +       /*
> +        * The perf_pmu__test_parse_init prepares perf_pmu_events_list
> +        * which gets freed in parse_events_terms.
> +        */
> +       if (perf_pmu__test_parse_init())
> +               return -1;
> +
>         ret = parse_events_terms(&terms, t->str);
>         if (ret) {
>                 pr_debug("failed to parse terms '%s', err %d\n",
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index e88e4c7a2a9a..254f02a7fb0d 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -2019,6 +2019,27 @@ static void perf_pmu__parse_init(void)
>         perf_pmu__parse_cleanup();
>  }
>
> +int perf_pmu__test_parse_init(void)
> +{
> +       struct perf_pmu_event_symbol *list;
> +
> +       list = malloc(sizeof(*list) * 1);
> +       if (!list)
> +               return -ENOMEM;
> +
> +       list->type   = PMU_EVENT_SYMBOL;
> +       list->symbol = strdup("read");
> +
> +       if (!list->symbol) {
> +               free(list);
> +               return -ENOMEM;
> +       }
> +
> +       perf_pmu_events_list = list;
> +       perf_pmu_events_list_num = 1;
> +       return 0;
> +}

nit: It's easy to see in the test code why this is necessary, could
the function be moved there? If not perhaps add a function comment?
The test in the function name is quite load bearing.

> +
>  enum perf_pmu_event_symbol_type
>  perf_pmu__parse_check(const char *name)
>  {
> @@ -2080,6 +2101,8 @@ int parse_events_terms(struct list_head *terms, const char *str)
>         int ret;
>
>         ret = parse_events__scanner(str, &parse_state);
> +       perf_pmu__parse_cleanup();
> +
>         if (!ret) {
>                 list_splice(parse_state.terms, terms);
>                 zfree(&parse_state.terms);
> diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> index f0010095fc8c..00cde7d2e30c 100644
> --- a/tools/perf/util/parse-events.h
> +++ b/tools/perf/util/parse-events.h
> @@ -261,4 +261,6 @@ static inline bool is_sdt_event(char *str __maybe_unused)
>  }
>  #endif /* HAVE_LIBELF_SUPPORT */
>
> +int perf_pmu__test_parse_init(void);
> +
>  #endif /* __PERF_PARSE_EVENTS_H */
> diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
> index 44c85fea5d00..3ca5fd2829ca 100644
> --- a/tools/perf/util/parse-events.l
> +++ b/tools/perf/util/parse-events.l
> @@ -41,14 +41,6 @@ static int value(yyscan_t scanner, int base)
>         return __value(yylval, text, base, PE_VALUE);
>  }
>
> -static int raw(yyscan_t scanner)
> -{
> -       YYSTYPE *yylval = parse_events_get_lval(scanner);
> -       char *text = parse_events_get_text(scanner);
> -
> -       return __value(yylval, text + 1, 16, PE_RAW);
> -}
> -
>  static int str(yyscan_t scanner, int token)
>  {
>         YYSTYPE *yylval = parse_events_get_lval(scanner);
> @@ -72,6 +64,17 @@ static int str(yyscan_t scanner, int token)
>         return token;
>  }
>
> +static int raw(yyscan_t scanner)
> +{
> +       YYSTYPE *yylval = parse_events_get_lval(scanner);
> +       char *text = parse_events_get_text(scanner);
> +
> +       if (perf_pmu__parse_check(text) == PMU_EVENT_SYMBOL)
> +               return str(scanner, PE_NAME);
> +
> +       return __value(yylval, text + 1, 16, PE_RAW);
> +}
> +
>  static bool isbpf_suffix(char *text)
>  {
>         int len = strlen(text);
> --
> 2.25.4
>