* [Question] perf tools: lex parsing issue
@ 2021-08-23 11:27 John Garry
2021-08-25 0:13 ` Ian Rogers
0 siblings, 1 reply; 5+ messages in thread
From: John Garry @ 2021-08-23 11:27 UTC (permalink / raw)
To: Jiri Olsa, Arnaldo Carvalho de Melo
Cc: linux-kernel, linux-perf-users, Peter Zijlstra, irogers,
Ingo Molnar, Mark Rutland, Alexander Shishkin, Namhyung Kim
Hi jirka,
If you remember from some time ago we discussed how the lex parsing
creates strange aliases:
https://lore.kernel.org/lkml/20200320093006.GA1343171@krava/
I am no expert on l+y, but it seems that we simply don't set the term
config field for known term types. Well, not for
PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD type anyway.
This super hack resolves that issue:
--->8----
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -765,7 +765,12 @@ event_config ',' event_term
struct list_head *head = $1;
struct parse_events_term *term = $3
+ if (term->type_term == PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD) {
+ term->config = strdup("period");
+ }
+
if (!head) {
parse_events_term__delete(term);
YYABORT;
--
----8-----
So we get "umask=0x80,period=0x30d40,event=0x6" now, rather than
"umask=0x80,(null)=0x30d40,event=0x6", for the perf_pmu_alias.str, as an
example.
Did you ever get a chance to look into this issue? Do you know how could
or should this field be set properly?
Some more background:
The reason I was looking at this is because I think it causes a problem
for pmu-events (JSONs) aliasing for some PMUs. Specifically it's PMU
which use "config=xxx" in sysfs files in
/sys/bus/event_source/devices/PMUx/events/, rather than "event=xxx". The
actual problem is that I trigger this warn in pmu.c:
static void perf_pmu_assign_str(char *name, const char *field, char
**old_str,
char **new_str)
{
if (*new_str) { /* Have new string, check with old */
if (strcasecmp(*old_str, *new_str))
pr_debug("alias %s differs i ... <---
As I get "config=event=0xXXX" vs "config=(null)=0xXXX"
As I am not sure how to solve that yet, but, since we have
config=(null), I thought it best to solve the first issue first.
Thanks,
John
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Question] perf tools: lex parsing issue 2021-08-23 11:27 [Question] perf tools: lex parsing issue John Garry @ 2021-08-25 0:13 ` Ian Rogers 2021-08-25 14:36 ` John Garry 0 siblings, 1 reply; 5+ messages in thread From: Ian Rogers @ 2021-08-25 0:13 UTC (permalink / raw) To: John Garry Cc: Jiri Olsa, Arnaldo Carvalho de Melo, linux-kernel, linux-perf-users, Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin, Namhyung Kim On Mon, Aug 23, 2021 at 4:23 AM John Garry <john.garry@huawei.com> wrote: > > Hi jirka, > > If you remember from some time ago we discussed how the lex parsing > creates strange aliases: > > https://lore.kernel.org/lkml/20200320093006.GA1343171@krava/ > > I am no expert on l+y, but it seems that we simply don't set the term > config field for known term types. Well, not for > PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD type anyway. > > This super hack resolves that issue: > > --->8---- > > --- a/tools/perf/util/parse-events.y > +++ b/tools/perf/util/parse-events.y > @@ -765,7 +765,12 @@ event_config ',' event_term > struct list_head *head = $1; > struct parse_events_term *term = $3 > > + if (term->type_term == PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD) { > + term->config = strdup("period"); > + } > + > if (!head) { > parse_events_term__delete(term); > YYABORT; > -- > > ----8----- Agreed this is hacky, I think it'd be better to fix this up in the output. For example: diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c index 6cdbee8a12e7..c77c42275efa 100644 --- a/tools/perf/util/pmu.c +++ b/tools/perf/util/pmu.c @@ -365,15 +365,21 @@ static int __perf_pmu__new_alias(struct list_head *list, char *dir, char *name, memset(newval, 0, sizeof(newval)); ret = 0; list_for_each_entry(term, &alias->terms, list) { + const char * config = term->config; + if (ret) ret += scnprintf(newval + ret, sizeof(newval) - ret, ","); + if (!config) { + /* Note: config_term_names in parse_events.c isn't accessible */ + config = config_term_names[term->type_term]; + } if (term->type_val == PARSE_EVENTS__TERM_TYPE_NUM) ret += scnprintf(newval + ret, sizeof(newval) - ret, - "%s=%#x", term->config, term->val.num); + "%s=%#x", config, term->val.num); else if (term->type_val == PARSE_EVENTS__TERM_TYPE_STR) ret += scnprintf(newval + ret, sizeof(newval) - ret, - "%s=%s", term->config, term->val.str); + "%s=%s", config, term->val.str); } alias->name = strdup(name); It looks like a similar fix is needed in format_alias. Thanks, Ian > So we get "umask=0x80,period=0x30d40,event=0x6" now, rather than > "umask=0x80,(null)=0x30d40,event=0x6", for the perf_pmu_alias.str, as an > example. > > Did you ever get a chance to look into this issue? Do you know how could > or should this field be set properly? > > Some more background: > The reason I was looking at this is because I think it causes a problem > for pmu-events (JSONs) aliasing for some PMUs. Specifically it's PMU > which use "config=xxx" in sysfs files in > /sys/bus/event_source/devices/PMUx/events/, rather than "event=xxx". The > actual problem is that I trigger this warn in pmu.c: > > static void perf_pmu_assign_str(char *name, const char *field, char > **old_str, > char **new_str) > { > > if (*new_str) { /* Have new string, check with old */ > if (strcasecmp(*old_str, *new_str)) > pr_debug("alias %s differs i ... <--- > > As I get "config=event=0xXXX" vs "config=(null)=0xXXX" > > As I am not sure how to solve that yet, but, since we have > config=(null), I thought it best to solve the first issue first. > > Thanks, > John ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Question] perf tools: lex parsing issue 2021-08-25 0:13 ` Ian Rogers @ 2021-08-25 14:36 ` John Garry 2021-08-25 16:47 ` Ian Rogers 0 siblings, 1 reply; 5+ messages in thread From: John Garry @ 2021-08-25 14:36 UTC (permalink / raw) To: Ian Rogers Cc: Jiri Olsa, Arnaldo Carvalho de Melo, linux-kernel, linux-perf-users, Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin, Namhyung Kim On 25/08/2021 01:13, Ian Rogers wrote: Thanks for getting back to me >> + if (term->type_term == PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD) { >> + term->config = strdup("period"); >> + } >> + >> if (!head) { >> parse_events_term__delete(term); >> YYABORT; >> -- >> >> ----8----- > Agreed this is hacky, This might be a *bit* better: diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c index e5eae23cfceb..e597beaaa179 100644 --- a/tools/perf/util/parse-events.c +++ b/tools/perf/util/parse-events.c @@ -1058,6 +1058,13 @@ static const char *config_term_names[__PARSE_EVENTS__TERM_TYPE_NR] = { [PARSE_EVENTS__TERM_TYPE_AUX_SAMPLE_SIZE] = "aux-sample-size", }; +const char * get_config_term(int term_type) +{ + if (term_type >= __PARSE_EVENTS__TERM_TYPE_NR) + return ""; + return config_term_names[term_type]; +} + static bool config_term_shrinked; static bool diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h index bf6e41aa9b6a..0f658732535f 100644 --- a/tools/perf/util/parse-events.h +++ b/tools/perf/util/parse-events.h @@ -90,6 +90,8 @@ enum { __PARSE_EVENTS__TERM_TYPE_NR, }; +const char *get_config_term(int term_type); + struct parse_events_array { size_t nr_ranges; struct { diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y index 9321bd0e2f76..8d6d3fae226d 100644 --- a/tools/perf/util/parse-events.y +++ b/tools/perf/util/parse-events.y @@ -832,8 +832,9 @@ PE_TERM '=' PE_NAME PE_TERM '=' PE_VALUE { struct parse_events_term *term; + char *config = strdup(get_config_term($1)); - ABORT_ON(parse_events_term__num(&term, (int)$1, NULL, $3, false, &@1, &@3)); + ABORT_ON(parse_events_term__num(&term, (int)$1, config, $3, false, &@1, &@3)); $$ = term; } > I think it'd be better to fix this up in the > output. For example: > > diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c > index 6cdbee8a12e7..c77c42275efa 100644 > --- a/tools/perf/util/pmu.c > +++ b/tools/perf/util/pmu.c > @@ -365,15 +365,21 @@ static int __perf_pmu__new_alias(struct > list_head *list, char *dir, char *name, > memset(newval, 0, sizeof(newval)); > ret = 0; > list_for_each_entry(term, &alias->terms, list) { > + const char * config = term->config; > + > if (ret) > ret += scnprintf(newval + ret, sizeof(newval) - ret, > ","); > + if (!config) { > + /* Note: config_term_names in parse_events.c > isn't accessible */ > + config = config_term_names[term->type_term]; We could just expose some parse-events.c API, like above. > + } > if (term->type_val == PARSE_EVENTS__TERM_TYPE_NUM) > ret += scnprintf(newval + ret, sizeof(newval) - ret, > - "%s=%#x", term->config, term->val.num); > + "%s=%#x", config, term->val.num); > else if (term->type_val == PARSE_EVENTS__TERM_TYPE_STR) > ret += scnprintf(newval + ret, sizeof(newval) - ret, > - "%s=%s", term->config, term->val.str); > + "%s=%s", config, term->val.str); > } > > alias->name = strdup(name); But how about this alternative simple one: diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c index e5eae23cfceb..9e5df934d22d 100644 --- a/tools/perf/util/parse-events.c +++ b/tools/perf/util/parse-events.c @@ -3084,7 +3084,7 @@ int parse_events_term__num(struct parse_events_term **term, struct parse_events_term temp = { .type_val = PARSE_EVENTS__TERM_TYPE_NUM, .type_term = type_term, - .config = config, + .config = config ? : strdup(config_term_names[type_term]), .no_value = no_value, .err_term = loc_term ? loc_term->first_column : 0, .err_val = loc_val ? loc_val->first_column : 0, -- > > It looks like a similar fix is needed in format_alias. And the change in parse_events_term__num(), above, seems to resolve that. Previously we would have something like these in format_alias(): buf=i915/bcs0-busy, pmu name=i915, alias->name=bcs0-busy ->str=(null)=0x1000 And now: buf=i915/bcs0-busy, pmu name=i915, alias->name=bcs0-busy ->str=config=0x1000 Thoughts? Thanks, John ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Question] perf tools: lex parsing issue 2021-08-25 14:36 ` John Garry @ 2021-08-25 16:47 ` Ian Rogers 2021-08-25 16:58 ` John Garry 0 siblings, 1 reply; 5+ messages in thread From: Ian Rogers @ 2021-08-25 16:47 UTC (permalink / raw) To: John Garry Cc: Jiri Olsa, Arnaldo Carvalho de Melo, linux-kernel, linux-perf-users, Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin, Namhyung Kim On Wed, Aug 25, 2021 at 7:32 AM John Garry <john.garry@huawei.com> wrote: > > On 25/08/2021 01:13, Ian Rogers wrote: > > Thanks for getting back to me > > > >> + if (term->type_term == PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD) { > >> + term->config = strdup("period"); > >> + } > >> + > >> if (!head) { > >> parse_events_term__delete(term); > >> YYABORT; > >> -- > >> > >> ----8----- > > Agreed this is hacky, > > This might be a *bit* better: > > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c > index e5eae23cfceb..e597beaaa179 100644 > --- a/tools/perf/util/parse-events.c > +++ b/tools/perf/util/parse-events.c > @@ -1058,6 +1058,13 @@ static const char > *config_term_names[__PARSE_EVENTS__TERM_TYPE_NR] = { > [PARSE_EVENTS__TERM_TYPE_AUX_SAMPLE_SIZE] = "aux-sample-size", > }; > > +const char * get_config_term(int term_type) > +{ > + if (term_type >= __PARSE_EVENTS__TERM_TYPE_NR) > + return ""; > + return config_term_names[term_type]; > +} > + > static bool config_term_shrinked; > > static bool > diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h > index bf6e41aa9b6a..0f658732535f 100644 > --- a/tools/perf/util/parse-events.h > +++ b/tools/perf/util/parse-events.h > @@ -90,6 +90,8 @@ enum { > __PARSE_EVENTS__TERM_TYPE_NR, > }; > > +const char *get_config_term(int term_type); > + > struct parse_events_array { > size_t nr_ranges; > struct { > diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y > index 9321bd0e2f76..8d6d3fae226d 100644 > --- a/tools/perf/util/parse-events.y > +++ b/tools/perf/util/parse-events.y > @@ -832,8 +832,9 @@ PE_TERM '=' PE_NAME > PE_TERM '=' PE_VALUE > { > struct parse_events_term *term; > + char *config = strdup(get_config_term($1)); > > - ABORT_ON(parse_events_term__num(&term, (int)$1, NULL, $3, false, &@1, > &@3)); > + ABORT_ON(parse_events_term__num(&term, (int)$1, config, $3, false, > &@1, &@3)); > $$ = term; > } > > > > I think it'd be better to fix this up in the > > output. For example: > > > > diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c > > index 6cdbee8a12e7..c77c42275efa 100644 > > --- a/tools/perf/util/pmu.c > > +++ b/tools/perf/util/pmu.c > > @@ -365,15 +365,21 @@ static int __perf_pmu__new_alias(struct > > list_head *list, char *dir, char *name, > > memset(newval, 0, sizeof(newval)); > > ret = 0; > > list_for_each_entry(term, &alias->terms, list) { > > + const char * config = term->config; > > + > > if (ret) > > ret += scnprintf(newval + ret, sizeof(newval) - ret, > > ","); > > + if (!config) { > > + /* Note: config_term_names in parse_events.c > > isn't accessible */ > > + config = config_term_names[term->type_term]; > > We could just expose some parse-events.c API, like above. > > > + } > > if (term->type_val == PARSE_EVENTS__TERM_TYPE_NUM) > > ret += scnprintf(newval + ret, sizeof(newval) - ret, > > - "%s=%#x", term->config, term->val.num); > > + "%s=%#x", config, term->val.num); > > else if (term->type_val == PARSE_EVENTS__TERM_TYPE_STR) > > ret += scnprintf(newval + ret, sizeof(newval) - ret, > > - "%s=%s", term->config, term->val.str); > > + "%s=%s", config, term->val.str); > > } > > > > alias->name = strdup(name); > > But how about this alternative simple one: > > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c > index e5eae23cfceb..9e5df934d22d 100644 > --- a/tools/perf/util/parse-events.c > +++ b/tools/perf/util/parse-events.c > @@ -3084,7 +3084,7 @@ int parse_events_term__num(struct > parse_events_term **term, > struct parse_events_term temp = { > .type_val = PARSE_EVENTS__TERM_TYPE_NUM, > .type_term = type_term, > - .config = config, > + .config = config ? : strdup(config_term_names[type_term]), > .no_value = no_value, > .err_term = loc_term ? loc_term->first_column : 0, > .err_val = loc_val ? loc_val->first_column : 0, > -- If you have this change isn't the change to parse_events_term__num() unnecessary? This 1 liner fix looks good to me. Thanks, Ian > > > > It looks like a similar fix is needed in format_alias. > > And the change in parse_events_term__num(), above, seems to resolve that. > > Previously we would have something like these in format_alias(): > > buf=i915/bcs0-busy, pmu name=i915, alias->name=bcs0-busy ->str=(null)=0x1000 > > And now: > > buf=i915/bcs0-busy, pmu name=i915, alias->name=bcs0-busy ->str=config=0x1000 > > > Thoughts? > > Thanks, > John > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Question] perf tools: lex parsing issue 2021-08-25 16:47 ` Ian Rogers @ 2021-08-25 16:58 ` John Garry 0 siblings, 0 replies; 5+ messages in thread From: John Garry @ 2021-08-25 16:58 UTC (permalink / raw) To: Ian Rogers Cc: Jiri Olsa, Arnaldo Carvalho de Melo, linux-kernel, linux-perf-users, Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin, Namhyung Kim On 25/08/2021 17:47, Ian Rogers wrote: > On Wed, Aug 25, 2021 at 7:32 AM John Garry<john.garry@huawei.com> wrote: >> On 25/08/2021 01:13, Ian Rogers wrote: >> >> Thanks for getting back to me >> >> >>>> + if (term->type_term == PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD) { >>>> + term->config = strdup("period"); >>>> + } >>>> + >>>> if (!head) { >>>> parse_events_term__delete(term); >>>> YYABORT; >>>> -- >>>> >>>> ----8----- >>> Agreed this is hacky, >> This might be a*bit* better: >> >> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c >> index e5eae23cfceb..e597beaaa179 100644 >> --- a/tools/perf/util/parse-events.c >> +++ b/tools/perf/util/parse-events.c >> @@ -1058,6 +1058,13 @@ static const char >> *config_term_names[__PARSE_EVENTS__TERM_TYPE_NR] = { >> [PARSE_EVENTS__TERM_TYPE_AUX_SAMPLE_SIZE] = "aux-sample-size", >> }; >> >> +const char * get_config_term(int term_type) >> +{ >> + if (term_type >= __PARSE_EVENTS__TERM_TYPE_NR) >> + return ""; >> + return config_term_names[term_type]; >> +} >> + >> static bool config_term_shrinked; >> >> static bool >> diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h >> index bf6e41aa9b6a..0f658732535f 100644 >> --- a/tools/perf/util/parse-events.h >> +++ b/tools/perf/util/parse-events.h >> @@ -90,6 +90,8 @@ enum { >> __PARSE_EVENTS__TERM_TYPE_NR, >> }; >> >> +const char *get_config_term(int term_type); >> + >> struct parse_events_array { >> size_t nr_ranges; >> struct { >> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y >> index 9321bd0e2f76..8d6d3fae226d 100644 >> --- a/tools/perf/util/parse-events.y >> +++ b/tools/perf/util/parse-events.y >> @@ -832,8 +832,9 @@ PE_TERM '=' PE_NAME >> PE_TERM '=' PE_VALUE >> { >> struct parse_events_term *term; >> + char *config = strdup(get_config_term($1)); >> >> - ABORT_ON(parse_events_term__num(&term, (int)$1, NULL, $3, false, &@1, >> &@3)); >> + ABORT_ON(parse_events_term__num(&term, (int)$1, config, $3, false, >> &@1, &@3)); >> $$ = term; >> } >> >> >>> I think it'd be better to fix this up in the >>> output. For example: >>> >>> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c >>> index 6cdbee8a12e7..c77c42275efa 100644 >>> --- a/tools/perf/util/pmu.c >>> +++ b/tools/perf/util/pmu.c >>> @@ -365,15 +365,21 @@ static int __perf_pmu__new_alias(struct >>> list_head *list, char *dir, char *name, >>> memset(newval, 0, sizeof(newval)); >>> ret = 0; >>> list_for_each_entry(term, &alias->terms, list) { >>> + const char * config = term->config; >>> + >>> if (ret) >>> ret += scnprintf(newval + ret, sizeof(newval) - ret, >>> ","); >>> + if (!config) { >>> + /* Note: config_term_names in parse_events.c >>> isn't accessible */ >>> + config = config_term_names[term->type_term]; >> We could just expose some parse-events.c API, like above. >> >>> + } >>> if (term->type_val == PARSE_EVENTS__TERM_TYPE_NUM) >>> ret += scnprintf(newval + ret, sizeof(newval) - ret, >>> - "%s=%#x", term->config, term->val.num); >>> + "%s=%#x", config, term->val.num); >>> else if (term->type_val == PARSE_EVENTS__TERM_TYPE_STR) >>> ret += scnprintf(newval + ret, sizeof(newval) - ret, >>> - "%s=%s", term->config, term->val.str); >>> + "%s=%s", config, term->val.str); >>> } >>> >>> alias->name = strdup(name); >> But how about this alternative simple one: >> >> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c >> index e5eae23cfceb..9e5df934d22d 100644 >> --- a/tools/perf/util/parse-events.c >> +++ b/tools/perf/util/parse-events.c >> @@ -3084,7 +3084,7 @@ int parse_events_term__num(struct >> parse_events_term **term, >> struct parse_events_term temp = { >> .type_val = PARSE_EVENTS__TERM_TYPE_NUM, >> .type_term = type_term, >> - .config = config, >> + .config = config ? : strdup(config_term_names[type_term]), >> .no_value = no_value, >> .err_term = loc_term ? loc_term->first_column : 0, >> .err_val = loc_val ? loc_val->first_column : 0, >> -- > If you have this change isn't the change to parse_events_term__num() > unnecessary? eh, this is parse_events_term__num(). Anyway, yes, this 1-line change in parse_events_term__num() only does look to fix it. More testing required, though. This 1 liner fix looks good to me. Cheers, John ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-08-25 16:54 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-08-23 11:27 [Question] perf tools: lex parsing issue John Garry 2021-08-25 0:13 ` Ian Rogers 2021-08-25 14:36 ` John Garry 2021-08-25 16:47 ` Ian Rogers 2021-08-25 16:58 ` John Garry
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).