Re: [PATCHv2 2/2] perf tools: Fix term parsing for raw syntax
Em Mon, Jul 27, 2020 at 08:26:27AM +0800, Jin, Yao escreveu:
>
>
> On 7/27/2020 8:21 AM, Jin, Yao wrote:
> >
> >
> > On 7/26/2020 3:52 PM, Jiri Olsa 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 r 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
> > > Signed-off-by: Jiri Olsa
> > > ---
> > > v2 changes:
> > > - added comment to perf_pmu__test_parse_init
> >
> > Acked-by: Jin Yao
> >
> > Thanks
> > Jin Yao
>
> Also added with:
> Fixes: 3a6c51e4d66c ("perf parser: Add support to specify rXXX event with
> pmu")?
Thanks, applied.
- Arnaldo
Re: [PATCHv2 2/2] perf tools: Fix term parsing for raw syntax
On 7/27/2020 8:21 AM, Jin, Yao wrote:
On 7/26/2020 3:52 PM, Jiri Olsa 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 r 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
Signed-off-by: Jiri Olsa
---
v2 changes:
- added comment to perf_pmu__test_parse_init
Acked-by: Jin Yao
Thanks
Jin Yao
Also added with:
Fixes: 3a6c51e4d66c ("perf parser: Add support to specify rXXX event with pmu")?
Thanks
Jin Yao
Re: [PATCHv2 2/2] perf tools: Fix term parsing for raw syntax
On 7/26/2020 3:52 PM, Jiri Olsa 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 r 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 Signed-off-by: Jiri Olsa --- v2 changes: - added comment to perf_pmu__test_parse_init Acked-by: Jin Yao Thanks Jin Yao
Re: [PATCHv2 2/2] perf tools: Fix term parsing for raw syntax
On Sun, Jul 26, 2020 at 12:53 AM Jiri Olsa 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 r 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
> Signed-off-by: Jiri Olsa
> ---
> v2 changes:
> - added comment to perf_pmu__test_parse_init
Acked-by: Ian Rogers
Thanks,
Ian
> tools/perf/tests/parse-events.c | 37 -
> tools/perf/util/parse-events.c | 28 +
> tools/perf/util/parse-events.h | 2 ++
> tools/perf/util/parse-events.l | 19 ++---
> 4 files changed, 77 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();
>
> + /*
> +* 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(, 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..9f7260e69113 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -2019,6 +2019,32 @@ static void perf_pmu__parse_init(void)
> perf_pmu__parse_cleanup();
> }
>
> +/*
> + * This function injects special term in
> + * perf_pmu_events_list so the test code
> + * can check on this functionality.
> + */
> +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 +2106,8 @@ int parse_events_terms(struct list_head *terms, const
> char *str)
> int ret;
>
> ret =
[PATCHv2 2/2] perf tools: Fix term parsing for raw syntax
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 r 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
Signed-off-by: Jiri Olsa
---
v2 changes:
- added comment to perf_pmu__test_parse_init
tools/perf/tests/parse-events.c | 37 -
tools/perf/util/parse-events.c | 28 +
tools/perf/util/parse-events.h | 2 ++
tools/perf/util/parse-events.l | 19 ++---
4 files changed, 77 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();
+ /*
+* 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(, 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..9f7260e69113 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -2019,6 +2019,32 @@ static void perf_pmu__parse_init(void)
perf_pmu__parse_cleanup();
}
+/*
+ * This function injects special term in
+ * perf_pmu_events_list so the test code
+ * can check on this functionality.
+ */
+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 +2106,8 @@ int parse_events_terms(struct list_head *terms, const
char *str)
int ret;
ret = parse_events__scanner(str, _state);
+ perf_pmu__parse_cleanup();
+
if (!ret) {
list_splice(parse_state.terms, terms);
zfree(_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
+++
