Re: [Question] perf tools: lex parsing issue
From: Ian Rogers <irogers@google.com>
Date: 2021-08-25 00:14:08
Also in:
lkml
Subsystem:
performance events subsystem, the rest · Maintainers:
Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Namhyung Kim, Linus Torvalds
On Mon, Aug 23, 2021 at 4:23 AM John Garry [off-list ref] wrote:
quoted hunk ↗ jump to hunk
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/ (local) 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_termstruct 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(structlist_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