Re: [PATCH v8 1/8] perf pmu: "Compat" supports matching multiple identifiers
From: Jing Zhang
Date: Tue Sep 12 2023 - 23:12:39 EST
在 2023/9/12 上午1:32, Ian Rogers 写道:
> On Sun, Sep 10, 2023 at 7:32 PM Jing Zhang <renyu.zj@xxxxxxxxxxxxxxxxx> wrote:
>>
>>
>>
>> 在 2023/9/9 上午5:33, Ian Rogers 写道:
>>> On Thu, Sep 7, 2023 at 4:58 AM Jing Zhang <renyu.zj@xxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxx>
>>>> Reviewed-by: John Garry <john.g.garry@xxxxxxxxxx>
>>>> ---
>>>> 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
>>>>