Messages in this thread | ![/](/images/icornerl.gif) | | From | Ian Rogers <> | Date | Tue, 23 Apr 2024 15:48:48 -0700 | Subject | Re: [PATCH v2 1/2] perf pmus: Sort/merge/aggregate PMUs like mrvl_ddr_pmu |
| |
On Tue, Apr 9, 2024 at 6:13 PM Ian Rogers <irogers@google.com> wrote: > > The mrvl_ddr_pmu is uncore and has a hexadecimal address suffix while > the previous PMU sorting/merging code assumes uncore PMU names start > with uncore_ and have a decimal suffix. Because of the previous > assumption it isn't possible to wildcard the mrvl_ddr_pmu. > > Modify pmu_name_len_no_suffix but also remove the suffix number out > argument, this is because we don't know if a suffix number of say 10 > is in hexadecimal or decimal. As the only use of the suffix number is > in comparisons, it is safe there to compare the values as hexadecimal.
+Thomas Richter
Thomas could you help validate that this doesn't cause issues on s390 with the cpum_cf PMU (note how cf potentially looks like a hex suffix).
Thanks, Ian
> Signed-off-by: Ian Rogers <irogers@google.com> > --- > tools/perf/util/pmu.c | 2 +- > tools/perf/util/pmus.c | 51 ++++++++++++++++++++++-------------------- > tools/perf/util/pmus.h | 7 +++++- > 3 files changed, 34 insertions(+), 26 deletions(-) > > diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c > index f39cbbc1a7ec..b0cca5841f90 100644 > --- a/tools/perf/util/pmu.c > +++ b/tools/perf/util/pmu.c > @@ -1657,7 +1657,7 @@ static char *format_alias(char *buf, int len, const struct perf_pmu *pmu, > { > struct parse_events_term *term; > int pmu_name_len = skip_duplicate_pmus > - ? pmu_name_len_no_suffix(pmu->name, /*num=*/NULL) > + ? pmu_name_len_no_suffix(pmu->name) > : (int)strlen(pmu->name); > int used = snprintf(buf, len, "%.*s/%s", pmu_name_len, pmu->name, alias->name); > > diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c > index 16505071d362..b4ddcd0ade26 100644 > --- a/tools/perf/util/pmus.c > +++ b/tools/perf/util/pmus.c > @@ -39,31 +39,44 @@ static bool read_sysfs_all_pmus; > > static void pmu_read_sysfs(bool core_only); > > -int pmu_name_len_no_suffix(const char *str, unsigned long *num) > +int pmu_name_len_no_suffix(const char *str) > { > int orig_len, len; > > orig_len = len = strlen(str); > > - /* Non-uncore PMUs have their full length, for example, i915. */ > - if (!strstarts(str, "uncore_")) > - return len; > - > /* > * Count trailing digits and '_', if '_{num}' suffix isn't present use > * the full length. > */ > - while (len > 0 && isdigit(str[len - 1])) > + while (len > 0 && isxdigit(str[len - 1])) > len--; > > - if (len > 0 && len != orig_len && str[len - 1] == '_') { > - if (num) > - *num = strtoul(&str[len], NULL, 10); > + if (len > 0 && len != orig_len && str[len - 1] == '_') > return len - 1; > - } > + > return orig_len; > } > > +int pmu_name_cmp(const char *lhs_pmu_name, const char *rhs_pmu_name) > +{ > + unsigned long lhs_num = 0, rhs_num = 0; > + int lhs_pmu_name_len = pmu_name_len_no_suffix(lhs_pmu_name); > + int rhs_pmu_name_len = pmu_name_len_no_suffix(rhs_pmu_name); > + int ret = strncmp(lhs_pmu_name, rhs_pmu_name, > + lhs_pmu_name_len < rhs_pmu_name_len ? lhs_pmu_name_len : rhs_pmu_name_len); > + > + if (lhs_pmu_name_len != rhs_pmu_name_len || ret != 0 || lhs_pmu_name_len == 0) > + return ret; > + > + if (lhs_pmu_name_len + 1 < (int)strlen(lhs_pmu_name)) > + lhs_num = strtoul(&lhs_pmu_name[lhs_pmu_name_len + 1], NULL, 16); > + if (rhs_pmu_name_len + 1 < (int)strlen(rhs_pmu_name)) > + rhs_num = strtoul(&rhs_pmu_name[rhs_pmu_name_len + 1], NULL, 16); > + > + return lhs_num < rhs_num ? -1 : (lhs_num > rhs_num ? 1 : 0); > +} > + > void perf_pmus__destroy(void) > { > struct perf_pmu *pmu, *tmp; > @@ -164,20 +177,10 @@ static struct perf_pmu *perf_pmu__find2(int dirfd, const char *name) > static int pmus_cmp(void *priv __maybe_unused, > const struct list_head *lhs, const struct list_head *rhs) > { > - unsigned long lhs_num = 0, rhs_num = 0; > struct perf_pmu *lhs_pmu = container_of(lhs, struct perf_pmu, list); > struct perf_pmu *rhs_pmu = container_of(rhs, struct perf_pmu, list); > - const char *lhs_pmu_name = lhs_pmu->name ?: ""; > - const char *rhs_pmu_name = rhs_pmu->name ?: ""; > - int lhs_pmu_name_len = pmu_name_len_no_suffix(lhs_pmu_name, &lhs_num); > - int rhs_pmu_name_len = pmu_name_len_no_suffix(rhs_pmu_name, &rhs_num); > - int ret = strncmp(lhs_pmu_name, rhs_pmu_name, > - lhs_pmu_name_len < rhs_pmu_name_len ? lhs_pmu_name_len : rhs_pmu_name_len); > > - if (lhs_pmu_name_len != rhs_pmu_name_len || ret != 0 || lhs_pmu_name_len == 0) > - return ret; > - > - return lhs_num < rhs_num ? -1 : (lhs_num > rhs_num ? 1 : 0); > + return pmu_name_cmp(lhs_pmu->name ?: "", rhs_pmu->name ?: ""); > } > > /* Add all pmus in sysfs to pmu list: */ > @@ -297,11 +300,11 @@ static struct perf_pmu *perf_pmus__scan_skip_duplicates(struct perf_pmu *pmu) > pmu_read_sysfs(/*core_only=*/false); > pmu = list_prepare_entry(pmu, &core_pmus, list); > } else > - last_pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "", NULL); > + last_pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: ""); > > if (use_core_pmus) { > list_for_each_entry_continue(pmu, &core_pmus, list) { > - int pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "", /*num=*/NULL); > + int pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: ""); > > if (last_pmu_name_len == pmu_name_len && > !strncmp(last_pmu_name, pmu->name ?: "", pmu_name_len)) > @@ -313,7 +316,7 @@ static struct perf_pmu *perf_pmus__scan_skip_duplicates(struct perf_pmu *pmu) > pmu = list_prepare_entry(pmu, &other_pmus, list); > } > list_for_each_entry_continue(pmu, &other_pmus, list) { > - int pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "", /*num=*/NULL); > + int pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: ""); > > if (last_pmu_name_len == pmu_name_len && > !strncmp(last_pmu_name, pmu->name ?: "", pmu_name_len)) > diff --git a/tools/perf/util/pmus.h b/tools/perf/util/pmus.h > index 94d2a08d894b..624c2d53fc30 100644 > --- a/tools/perf/util/pmus.h > +++ b/tools/perf/util/pmus.h > @@ -2,10 +2,15 @@ > #ifndef __PMUS_H > #define __PMUS_H > > +#include <stdbool.h> > +#include <linux/list.h> > + > struct perf_pmu; > struct print_callbacks; > > -int pmu_name_len_no_suffix(const char *str, unsigned long *num); > +int pmu_name_len_no_suffix(const char *str); > +/* Exposed for testing only. */ > +int pmu_name_cmp(const char *lhs_pmu_name, const char *rhs_pmu_name); > > void perf_pmus__destroy(void); > > -- > 2.44.0.478.gd926399ef9-goog >
| ![\](/images/icornerr.gif) |