Re: [PATCH v8 1/8] perf pmu: "Compat" supports matching multiple identifiers

[Jing Zhang <renyu.zj@linux.alibaba.com>]

在 2023/9/12 上午1:32, Ian Rogers 写道:
> On Sun, Sep 10, 2023 at 7:32 PM Jing Zhang <renyu.zj@linux.alibaba.com> wrote:
>>
>>
>>
>> 在 2023/9/9 上午5:33, Ian Rogers 写道:
>>> On Thu, Sep 7, 2023 at 4:58 AM Jing Zhang <renyu.zj@linux.alibaba.com> wrote:
>>>>
>>>> The jevent "Compat" is used for uncore PMU alias or metric definitions.
>>>>
>>>> The same PMU driver has different PMU identifiers due to different
>>>> hardware versions and types, but they may have some common PMU event.
>>>> Since a Compat value can only match one identifier, when adding the
>>>> same event alias to PMUs with different identifiers, each identifier
>>>> needs to be defined once, which is not streamlined enough.
>>>>
>>>> So let "Compat" supports matching multiple identifiers for uncore PMU
>>>> alias. For example, the Compat value {43401;436*} can match the PMU
>>>> identifier "43401", that is, CMN600_r0p0, and the PMU identifier with
>>>> the prefix "436", that is, all CMN650, where "*" is a wildcard.
>>>> Tokens in Unit field are delimited by ';' with no spaces.
>>>>
>>>> Signed-off-by: Jing Zhang <renyu.zj@linux.alibaba.com>
>>>> Reviewed-by: John Garry <john.g.garry@oracle.com>
>>>> ---
>>>>  tools/perf/util/pmu.c | 28 ++++++++++++++++++++++++++--
>>>>  tools/perf/util/pmu.h |  1 +
>>>>  2 files changed, 27 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
>>>> index e215985..c3c3818 100644
>>>> --- a/tools/perf/util/pmu.c
>>>> +++ b/tools/perf/util/pmu.c
>>>> @@ -875,6 +875,30 @@ static bool pmu_uncore_alias_match(const char *pmu_name, const char *name)
>>>>         return res;
>>>>  }
>>>>
>>>> +bool pmu_uncore_identifier_match(const char *id, const char *compat)
>>>> +{
>>>> +       char *tmp = NULL, *tok, *str;
>>>> +       bool res = false;
>>>> +
>>>> +       /*
>>>> +        * The strdup() call is necessary here because "compat" is a const str*
>>>> +        * type and cannot be used as an argument to strtok_r().
>>>> +        */
>>>> +       str = strdup(compat);
>>>> +       if (!str)
>>>> +               return false;
>>>> +
>>>> +       tok = strtok_r(str, ";", &tmp);
>>>
>>> Did the comma vs semicolon difference get explained? It seems to add
>>> inconsistency to use a semicolon.
>>>
>>
>> Hi Ian,
>>
>> Yes, I explained the reason for using semicolons instead of commas in v7.
>>
>> I use a semicolon instead of a comma because I want to distinguish it from the function
>> of the comma in "Unit" and avoid confusion between the use of commas in "Unit" and "Compat".
>> Because in Unit, commas act as wildcards, and in “Compat”, the semicolon means “or”. So
>> I think semicolons are more appropriate.
>>
>> John also raised this issue earlier, and we finally agreed to use semicolons.
>> What do you think?
> 
> I'm okay with it, but thanks for capturing the why of this. I'd like
> at some point to make the wildcarding of things less ad hoc. For
> example, on x86 we use regular expressions to match cpuid:
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/pmu-events/arch/x86/mapfile.csv?h=perf-tools-next

Thank you for the example. I was not aware that regular expressions were
already being used for matching in tools/perf.

> but file name style matching for pmus:
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/pmu.c?h=perf-tools-next#n1974
> Given that we're okay with regular expressions then I don't see why
> everything shouldn't be a regular expression. This could, for example,
> make matching PMUs more specific than just adding a star and doing a
> file name match. For an example of why this is weird, on my laptop:
> ```
> $ perf stat -e i/actual-frequency/ true
> 
> Performance counter stats for 'system wide':
> 
>                 0      i/actual-frequency/
> 
>       0.001168195 seconds time elapsed
> ```
> The PMU I used here as 'i' is /sys/devices/i915 as we allow arbitrary
> numbers after a PMU name for cases of multiple uncore PMUs.
> 
> My feeling longer term is that the matching distinction of Unit and
> Compat, comma and semi-colon, would be better captured with regular
> expressions as I think they show the intent in the matching more
> clearly.
> 

Yes, using regular expressions is indeed a better choice for consistency and clarity,
and I will try using regular expressions for Compat matching in the next version.

Thanks,
Jing

> Thanks,
> Ian
> 
> 
>> Thanks,
>> Jing
>>
>>> Thanks,
>>> Ian
>>>
>>>> +       for (; tok; tok = strtok_r(NULL, ";", &tmp)) {
>>>> +               if (!fnmatch(tok, id, FNM_CASEFOLD)) {
>>>> +                       res = true;
>>>> +                       break;
>>>> +               }
>>>> +       }
>>>> +       free(str);
>>>> +       return res;
>>>> +}
>>>> +
>>>>  static int pmu_add_cpu_aliases_map_callback(const struct pmu_event *pe,
>>>>                                         const struct pmu_events_table *table __maybe_unused,
>>>>                                         void *vdata)
>>>> @@ -915,8 +939,8 @@ static int pmu_add_sys_aliases_iter_fn(const struct pmu_event *pe,
>>>>         if (!pe->compat || !pe->pmu)
>>>>                 return 0;
>>>>
>>>> -       if (!strcmp(pmu->id, pe->compat) &&
>>>> -           pmu_uncore_alias_match(pe->pmu, pmu->name)) {
>>>> +       if (pmu_uncore_alias_match(pe->pmu, pmu->name) &&
>>>> +                       pmu_uncore_identifier_match(pmu->id, pe->compat)) {
>>>>                 perf_pmu__new_alias(pmu,
>>>>                                 pe->name,
>>>>                                 pe->desc,
>>>> diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
>>>> index bd5d804..1bf5cf1 100644
>>>> --- a/tools/perf/util/pmu.h
>>>> +++ b/tools/perf/util/pmu.h
>>>> @@ -240,6 +240,7 @@ void pmu_add_cpu_aliases_table(struct perf_pmu *pmu,
>>>>  char *perf_pmu__getcpuid(struct perf_pmu *pmu);
>>>>  const struct pmu_events_table *pmu_events_table__find(void);
>>>>  const struct pmu_metrics_table *pmu_metrics_table__find(void);
>>>> +bool pmu_uncore_identifier_match(const char *id, const char *compat);
>>>>
>>>>  int perf_pmu__convert_scale(const char *scale, char **end, double *sval);
>>>>
>>>> --
>>>> 1.8.3.1
>>>>