Re: [Question] perf tools: lex parsing issue

[Ian Rogers]

On Mon, Aug 23, 2021 at 4:23 AM John Garry <john.garry@xxxxxxxxxx> 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