Date:   Wed, 25 Aug 2021 09:47:32 -0700
From:   Ian Rogers <irogers@...gle.com>
To:     John Garry <john.garry@...wei.com>
Cc:     Jiri Olsa <jolsa@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-perf-users@...r.kernel.org" <linux-perf-users@...r.kernel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Namhyung Kim <namhyung@...nel.org>
Subject: Re: [Question] perf tools: lex parsing issue

On Wed, Aug 25, 2021 at 7:32 AM John Garry <john.garry@...wei.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
>

Powered by blists - more mailing lists