Messages in this thread | | | From | Ian Rogers <> | Date | Fri, 11 Aug 2023 08:15:30 -0700 | Subject | Re: [PATCH v2 1/3] perf pmus: Sort pmus by name then suffix |
| |
On Fri, Aug 11, 2023 at 7:00 AM John Garry <john.g.garry@oracle.com> wrote: > > On 10/08/2023 22:49, Ian Rogers wrote: > > Sort PMUs by name. If two PMUs have the same name but differ by > > suffix > > I think that the wording here can be improved. If they have the same > name, then they cannot differ. I think that you mean that two PMUs have > the same name apart from a difference in suffix.
Sure.
> > , sort the suffixes numerically. > > I don't know how this will affect some hisi pmus which have special > naming formats, like hisi_l3c_sscl1_4
Anything not starting with uncore_ is assumed not to have a suffix.
> > For example, "breakpoint" comes > > before "cpu", "uncore_imc_free_running_0" comes before > > "uncore_imc_free_running_1". > > > > Signed-off-by: Ian Rogers <irogers@google.com> > > --- > > tools/perf/util/pmus.c | 48 ++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 48 insertions(+) > > > > diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c > > index c58ba9fb6a36..3581710667b0 100644 > > --- a/tools/perf/util/pmus.c > > +++ b/tools/perf/util/pmus.c > > @@ -1,8 +1,10 @@ > > // SPDX-License-Identifier: GPL-2.0 > > #include <linux/list.h> > > +#include <linux/list_sort.h> > > #include <linux/zalloc.h> > > #include <subcmd/pager.h> > > #include <sys/types.h> > > +#include <ctype.h> > > #include <dirent.h> > > #include <pthread.h> > > #include <string.h> > > @@ -33,6 +35,31 @@ static LIST_HEAD(other_pmus); > > static bool read_sysfs_core_pmus; > > static bool read_sysfs_all_pmus; > > > > +static int pmu_name_len_no_suffix(const char *str, unsigned long *num) > > +{ > > + int orig_len, len; > > + > > + orig_len = len = strlen(str); > > + > > + /* Non-uncore PMUs have their full length, for example, i915. */ > > + if (strncmp(str, "uncore_", 7)) > > + return len; > > + > > + /* > > + * Count trailing digits and '_', if '_{num}' suffix isn't present use > > + * the full length. > > + */ > > + while (len > 0 && isdigit(str[len - 1])) > > + len--; > > + > > + if (len > 0 && len != orig_len && str[len - 1] == '_') { > > + if (num) > > + *num = strtoul(&str[len], NULL, 10); > > + return len - 1; > > + } > > + return orig_len; > > +} > > + > > void perf_pmus__destroy(void) > > { > > struct perf_pmu *pmu, *tmp; > > @@ -122,6 +149,25 @@ static struct perf_pmu *perf_pmu__find2(int dirfd, const char *name) > > return perf_pmu__lookup(core_pmu ? &core_pmus : &other_pmus, dirfd, name); > > } > > > > +static int pmus_cmp(void *priv __maybe_unused, > > + const struct list_head *lhs, const struct list_head *rhs) > > +{ > > + unsigned long lhs_num, rhs_num; > > + 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); > > > This is a bit of a monster... at least it should have a comment on what > it is doing. Do you consider your own version of strncmp which can > handle numbers in the suffix as another solution?
Sure, the intention is to be intention revealing getting a left hand and right hand pmu name, the length of the name part and the suffix number. I'm not sure a comment can do more than restate what the code is doing.
> > + 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); > > Could min(lhs_pmu_name_len, rhs_pmu_name_len) be used here?
Right, there is a suitable definition in linux/kernel.h
> > + > > + 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); > > double ternary operator on same line ain't great - can this be changed > into multiple return statements and also commented.
The alternative is: return lhs_num - rhs_num; which removes the compares and has a bug around minimum integer everybody ignores.
Thanks, Ian
> > +} > > + > > /* Add all pmus in sysfs to pmu list: */ > > static void pmu_read_sysfs(bool core_only) > > { > > @@ -156,6 +202,8 @@ static void pmu_read_sysfs(bool core_only) > > if (!perf_pmu__create_placeholder_core_pmu(&core_pmus)) > > pr_err("Failure to set up any core PMUs\n"); > > } > > + list_sort(NULL, &core_pmus, pmus_cmp); > > + list_sort(NULL, &other_pmus, pmus_cmp); > > if (!list_empty(&core_pmus)) { > > read_sysfs_core_pmus = true; > > if (!core_only) >
| |