在 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 >>>>