| Message ID | 1694087913-46144-2-git-send-email-renyu.zj@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add metrics for Arm CMN | expand |
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. 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 >
在 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? 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 >>
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 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. 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 > >>
在 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 >>>>
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); + 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);
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(-)