Re: [PATCH] perf pmu: Fix alias matching

[John Garry]

> > Fixes: c47a5599eda3 ("perf tools: Fix pattern matching for same 
> > substring in different PMU type")
> > Signed-off-by: John Garry <john.garry@xxxxxxxxxx>
> > ---
> > @Jin Yao, please test for your scenarios
>
> For x86, the form uncore_pmu_{digits} or the uncore_pmu itself are 
> supported. We don't have more complex case such as the name in the form 
> aaa_bbbX_cccY. So my test didn't cover that complex form.

My next thing to do is to add support for these more complex scenarios 
in the PMU events self tests

> For my test, your patch works, thanks! :)

Good

> > Note:
> > About any effect in perf_pmu__match() -> perf_pmu__valid_suffix()
> > callchain, this seems to be called for wildcard in PMU names in metric
> > expressions. We don't have any metrics for arm64 which use feature.
> > However, I hacked an existing metric to use a wildcard and it looks ok.
> > Also the "DRAM_BW_Use" metric on my broadwell uses this feature, and it
> > looks ok.
> >
> > diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> > index a1bd7007a8b4..fc683bc41715 100644
> > --- a/tools/perf/util/pmu.c
> > +++ b/tools/perf/util/pmu.c
> > @@ -742,9 +742,13 @@ struct pmu_events_map *__weak 
> > pmu_events_map__find(void)
> >       return perf_pmu__find_map(NULL);
> >   }
> > -static bool perf_pmu__valid_suffix(char *pmu_name, char *tok)
> > +/*
> > + * Suffix must be in form tok_{digits}, or tok{digits}, or same as 
> > pmu_name
> > + * to be valid.
> > + */
> > +static bool perf_pmu__valid_suffix(const char *pmu_name, char *tok)
> >   {
> > -    char *p;
> > +    const char *p;
> >       if (strncmp(pmu_name, tok, strlen(tok)))
> >           return false;
> > @@ -753,12 +757,16 @@ static bool perf_pmu__valid_suffix(char 
> > *pmu_name, char *tok)
> >       if (*p == 0)
> >           return true;
> > -    if (*p != '_')
> > -        return false;
> > +    if (*p == '_')
> > +        ++p;
> > -    ++p;
> > -    if (*p == 0 || !isdigit(*p))
> > -        return false;
> > +    /* Ensure we end in a number */
> > +    while (1) {
> > +        if (!isdigit(*p))
> > +            return false;
> > +        if (*(++p) == 0)
> > +            break;
> > +    }
>
> Do we check *p before first isdigit? For example,
>
> if (*p == 0)
>      return false;
>
> While (*p) {
>      if (!isdigit(*p)
>          return false;
>      ++p;
> }
>
> But maybe isdigit can handle the null string well. I'm just feeling a 
> bit unsure.

isdigit() can safely handle 0 and returns 0 for that case, so what I 
added should be ok

> >       return true;
> >   }
> > @@ -789,12 +797,19 @@ bool pmu_uncore_alias_match(const char 
> > *pmu_name, const char *name)
> >        *        match "socket" in "socketX_pmunameY" and then 
> > "pmuname" in
> >        *        "pmunameY".
> >        */
> > -    for (; tok; name += strlen(tok), tok = strtok_r(NULL, ",", &tmp)) {
> > +    while (1) {
> > +        char *next_tok = strtok_r(NULL, ",", &tmp);
> > +
> >           name = strstr(name, tok);
> > -        if (!name || !perf_pmu__valid_suffix((char *)name, tok)) {
> > +        if (!name ||
> > +            (!next_tok && !perf_pmu__valid_suffix(name, tok))) {
> >               res = false;
> >               goto out;
> >           }
> > +        if (!next_tok)
> > +            break;
> > +        tok = next_tok;
> > +        name += strlen(tok);
> >       }
> >       res = true;
>
> My test didn't cover the tokens which were delimited by ','. I assume 
> you have tested that on arm64 system. :)

Right
Thanks,
John