[PATCH 1/2] perf tools: Allow r0x<HEX> event syntax

[PATCH 1/2] perf tools: Allow r0x<HEX> event syntax

[Jiri Olsa]

Adding support to specify raw event with 'r0<HEX>' syntax within
pmu term syntax like:

  -e cpu/r0xdead/

It will be used to specify raw events in cases where they conflict
with real pmu terms, like 'read', which is valid raw event syntax,
but also a possible pmu term name as reported by Jin Yao.

Reported-by: Jin Yao <yao.jin@linux.intel.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/Documentation/perf-list.txt | 1 +
 tools/perf/tests/parse-events.c        | 5 +++++
 tools/perf/util/parse-events.l         | 1 +
 3 files changed, 7 insertions(+)

diff --git a/tools/perf/Documentation/perf-list.txt b/tools/perf/Documentation/perf-list.txt
index 376a50b3452d..10ed539a8859 100644
--- a/tools/perf/Documentation/perf-list.txt
+++ b/tools/perf/Documentation/perf-list.txt
@@ -119,6 +119,7 @@ It's also possible to use pmu syntax:
 
  perf record -e r1a8 -a sleep 1
  perf record -e cpu/r1a8/ ...
+ perf record -e cpu/r0x1a8/ ...
 
 You should refer to the processor specific documentation for getting these
 details. Some of them are referenced in the SEE ALSO section below.
diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
index 895188b63f96..5aaddcb0058a 100644
--- a/tools/perf/tests/parse-events.c
+++ b/tools/perf/tests/parse-events.c
@@ -1766,6 +1766,11 @@ static struct evlist_test test__events_pmu[] = {
 		.check = test__checkevent_raw_pmu,
 		.id    = 4,
 	},
+	{
+		.name  = "software/r0x1a/",
+		.check = test__checkevent_raw_pmu,
+		.id    = 4,
+	},
 };
 
 struct terms_test {
diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
index 56912c9641f5..44c85fea5d00 100644
--- a/tools/perf/util/parse-events.l
+++ b/tools/perf/util/parse-events.l
@@ -293,6 +293,7 @@ percore			{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_PERCORE); }
 aux-output		{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_AUX_OUTPUT); }
 aux-sample-size		{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_AUX_SAMPLE_SIZE); }
 r{num_raw_hex}		{ return raw(yyscanner); }
+r0x{num_raw_hex}	{ return raw(yyscanner); }
 ,			{ return ','; }
 "/"			{ BEGIN(INITIAL); return '/'; }
 {name_minus}		{ return str(yyscanner, PE_NAME); }
-- 
2.25.4

[PATCH 2/2] perf tools: Fix term parsing for raw syntax

[Jiri Olsa]

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;
+}
+
 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

Re: [PATCH 1/2] perf tools: Allow r0x<HEX> event syntax

[Ian Rogers]

On Sat, Jul 25, 2020 at 5:20 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding support to specify raw event with 'r0<HEX>' syntax within
> pmu term syntax like:
>
>   -e cpu/r0xdead/
>
> It will be used to specify raw events in cases where they conflict
> with real pmu terms, like 'read', which is valid raw event syntax,
> but also a possible pmu term name as reported by Jin Yao.
>
> Reported-by: Jin Yao <yao.jin@linux.intel.com>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Acked-by: Ian Rogers <irogers@google.com>

> ---
>  tools/perf/Documentation/perf-list.txt | 1 +
>  tools/perf/tests/parse-events.c        | 5 +++++
>  tools/perf/util/parse-events.l         | 1 +
>  3 files changed, 7 insertions(+)
>
> diff --git a/tools/perf/Documentation/perf-list.txt b/tools/perf/Documentation/perf-list.txt
> index 376a50b3452d..10ed539a8859 100644
> --- a/tools/perf/Documentation/perf-list.txt
> +++ b/tools/perf/Documentation/perf-list.txt
> @@ -119,6 +119,7 @@ It's also possible to use pmu syntax:
>
>   perf record -e r1a8 -a sleep 1
>   perf record -e cpu/r1a8/ ...
> + perf record -e cpu/r0x1a8/ ...
>
>  You should refer to the processor specific documentation for getting these
>  details. Some of them are referenced in the SEE ALSO section below.
> diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
> index 895188b63f96..5aaddcb0058a 100644
> --- a/tools/perf/tests/parse-events.c
> +++ b/tools/perf/tests/parse-events.c
> @@ -1766,6 +1766,11 @@ static struct evlist_test test__events_pmu[] = {
>                 .check = test__checkevent_raw_pmu,
>                 .id    = 4,
>         },
> +       {
> +               .name  = "software/r0x1a/",
> +               .check = test__checkevent_raw_pmu,
> +               .id    = 4,
> +       },
>  };
>
>  struct terms_test {
> diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
> index 56912c9641f5..44c85fea5d00 100644
> --- a/tools/perf/util/parse-events.l
> +++ b/tools/perf/util/parse-events.l
> @@ -293,6 +293,7 @@ percore                     { return term(yyscanner, PARSE_EVENTS__TERM_TYPE_PERCORE); }
>  aux-output             { return term(yyscanner, PARSE_EVENTS__TERM_TYPE_AUX_OUTPUT); }
>  aux-sample-size                { return term(yyscanner, PARSE_EVENTS__TERM_TYPE_AUX_SAMPLE_SIZE); }
>  r{num_raw_hex}         { return raw(yyscanner); }
> +r0x{num_raw_hex}       { return raw(yyscanner); }
>  ,                      { return ','; }
>  "/"                    { BEGIN(INITIAL); return '/'; }
>  {name_minus}           { return str(yyscanner, PE_NAME); }
> --
> 2.25.4
>

Re: [PATCH 2/2] perf tools: Fix term parsing for raw syntax

[Ian Rogers]

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
>

Re: [PATCH 2/2] perf tools: Fix term parsing for raw syntax

[Jiri Olsa]

On Sat, Jul 25, 2020 at 09:58:13AM -0700, Ian Rogers wrote:

SNIP

> >         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.

both perf_pmu_events_list/cnt are static and I'd like to keep it that
way, so the function needs to be in here.. I'll add comment explaining
this

jirka

Re: [PATCH 1/2] perf tools: Allow r0x<HEX> event syntax

[Arnaldo Carvalho de Melo]

Em Sat, Jul 25, 2020 at 09:40:55AM -0700, Ian Rogers escreveu:
> On Sat, Jul 25, 2020 at 5:20 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding support to specify raw event with 'r0<HEX>' syntax within
> > pmu term syntax like:
> >
> >   -e cpu/r0xdead/
> >
> > It will be used to specify raw events in cases where they conflict
> > with real pmu terms, like 'read', which is valid raw event syntax,
> > but also a possible pmu term name as reported by Jin Yao.
> >
> > Reported-by: Jin Yao <yao.jin@linux.intel.com>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> 
> Acked-by: Ian Rogers <irogers@google.com>

Thanks, applied.

- Arnaldo