[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ac57e637-7281-59b8-5658-3195e6d335f5@oracle.com>
Date: Fri, 11 Aug 2023 14:59:57 +0100
From: John Garry <john.g.garry@...cle.com>
To: Ian Rogers <irogers@...gle.com>,
Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...hat.com>,
Arnaldo Carvalho de Melo <acme@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Jiri Olsa <jolsa@...nel.org>,
Namhyung Kim <namhyung@...nel.org>,
Adrian Hunter <adrian.hunter@...el.com>,
Kan Liang <kan.liang@...ux.intel.com>,
Ravi Bangoria <ravi.bangoria@....com>,
Kajol Jain <kjain@...ux.ibm.com>,
linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/3] perf pmus: Sort pmus by name then suffix
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.
> , 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
> For example, "breakpoint" comes
> before "cpu", "uncore_imc_free_running_0" comes before
> "uncore_imc_free_running_1".
>
> Signed-off-by: Ian Rogers <irogers@...gle.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?
> + 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?
> +
> + 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.
> +}
> +
> /* 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)
Powered by blists - more mailing lists