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.
Add a set of PMUs tests for pmu_name_len_no_suffix and pmu_name_cmp.
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/tests/Build | 1 +
tools/perf/tests/builtin-test.c | 1 +
tools/perf/tests/pmus.c | 108 ++++++++++++++++++++++++++++++++
tools/perf/tests/tests.h | 2 +
tools/perf/util/pmu.c | 2 +-
tools/perf/util/pmus.c | 51 ++++++++-------
tools/perf/util/pmus.h | 7 ++-
7 files changed, 146 insertions(+), 26 deletions(-)
create mode 100644 tools/perf/tests/pmus.c
diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build
index c7f9d9676095..a7bab6e9300f 100644
--- a/tools/perf/tests/Build
+++ b/tools/perf/tests/Build
@@ -14,6 +14,7 @@ perf-y += perf-record.o
perf-y += evsel-roundtrip-name.o
perf-$(CONFIG_LIBTRACEEVENT) += evsel-tp-sched.o
perf-y += fdarray.o
+perf-y += pmus.o
perf-y += pmu.o
perf-y += pmu-events.o
perf-y += hists_common.o
diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index d13ee7683d9d..c90f270a469a 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -68,6 +68,7 @@ static struct test_suite *generic_tests[] = {
&suite__parse_events,
&suite__expr,
&suite__PERF_RECORD,
+ &suite__pmus,
&suite__pmu,
&suite__pmu_events,
&suite__dso_data,
diff --git a/tools/perf/tests/pmus.c b/tools/perf/tests/pmus.c
new file mode 100644
index 000000000000..6279c925e689
--- /dev/null
+++ b/tools/perf/tests/pmus.c
@@ -0,0 +1,108 @@
+// SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause)
+#include "pmus.h"
+#include "tests.h"
+#include <string.h>
+#include <linux/kernel.h>
+
+static const char * const uncore_chas[] = {
+ "uncore_cha_0",
+ "uncore_cha_1",
+ "uncore_cha_2",
+ "uncore_cha_3",
+ "uncore_cha_4",
+ "uncore_cha_5",
+ "uncore_cha_6",
+ "uncore_cha_7",
+ "uncore_cha_8",
+ "uncore_cha_9",
+ "uncore_cha_10",
+ "uncore_cha_11",
+ "uncore_cha_12",
+ "uncore_cha_13",
+ "uncore_cha_14",
+ "uncore_cha_15",
+ "uncore_cha_16",
+ "uncore_cha_17",
+ "uncore_cha_18",
+ "uncore_cha_19",
+ "uncore_cha_20",
+ "uncore_cha_21",
+ "uncore_cha_22",
+ "uncore_cha_23",
+ "uncore_cha_24",
+ "uncore_cha_25",
+ "uncore_cha_26",
+ "uncore_cha_27",
+ "uncore_cha_28",
+ "uncore_cha_29",
+ "uncore_cha_30",
+ "uncore_cha_31",
+};
+
+static const char * const mrvl_ddrs[] = {
+ "mrvl_ddr_pmu_87e1b0000000",
+ "mrvl_ddr_pmu_87e1b1000000",
+ "mrvl_ddr_pmu_87e1b2000000",
+ "mrvl_ddr_pmu_87e1b3000000",
+ "mrvl_ddr_pmu_87e1b4000000",
+ "mrvl_ddr_pmu_87e1b5000000",
+ "mrvl_ddr_pmu_87e1b6000000",
+ "mrvl_ddr_pmu_87e1b7000000",
+ "mrvl_ddr_pmu_87e1b8000000",
+ "mrvl_ddr_pmu_87e1b9000000",
+ "mrvl_ddr_pmu_87e1ba000000",
+ "mrvl_ddr_pmu_87e1bb000000",
+ "mrvl_ddr_pmu_87e1bc000000",
+ "mrvl_ddr_pmu_87e1bd000000",
+ "mrvl_ddr_pmu_87e1be000000",
+ "mrvl_ddr_pmu_87e1bf000000",
+};
+
+static int test__name_len(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
+{
+ TEST_ASSERT_EQUAL("cpu", pmu_name_len_no_suffix("cpu"), (int)strlen("cpu"));
+ TEST_ASSERT_EQUAL("i915", pmu_name_len_no_suffix("i915"), (int)strlen("i915"));
+ for (size_t i = 0; i < ARRAY_SIZE(uncore_chas); i++) {
+ TEST_ASSERT_EQUAL("Strips uncore_cha suffix",
+ pmu_name_len_no_suffix(uncore_chas[i]),
+ (int)strlen("uncore_cha"));
+ }
+ for (size_t i = 0; i < ARRAY_SIZE(mrvl_ddrs); i++) {
+ TEST_ASSERT_EQUAL("Strips mrvl_ddr_pmu suffix",
+ pmu_name_len_no_suffix(mrvl_ddrs[i]),
+ (int)strlen("mrvl_ddr_pmu"));
+ }
+ return TEST_OK;
+}
+
+static int test__name_cmp(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
+{
+ TEST_ASSERT_EQUAL("cpu", pmu_name_cmp("cpu", "cpu"), 0);
+ TEST_ASSERT_EQUAL("i915", pmu_name_cmp("i915", "i915"), 0);
+ TEST_ASSERT_VAL("i915", pmu_name_cmp("cpu", "i915") < 0);
+ TEST_ASSERT_VAL("i915", pmu_name_cmp("i915", "cpu") > 0);
+ for (size_t i = 1; i < ARRAY_SIZE(uncore_chas); i++) {
+ TEST_ASSERT_VAL("uncore_cha suffixes ordered lt",
+ pmu_name_cmp(uncore_chas[i-1], uncore_chas[i]) < 0);
+ TEST_ASSERT_VAL("uncore_cha suffixes ordered gt",
+ pmu_name_cmp(uncore_chas[i], uncore_chas[i-1]) > 0);
+ }
+ for (size_t i = 1; i < ARRAY_SIZE(mrvl_ddrs); i++) {
+ TEST_ASSERT_VAL("mrvl_ddr_pmu suffixes ordered lt",
+ pmu_name_cmp(mrvl_ddrs[i-1], mrvl_ddrs[i]) < 0);
+ TEST_ASSERT_VAL("mrvl_ddr_pmu suffixes ordered gt",
+ pmu_name_cmp(mrvl_ddrs[i], mrvl_ddrs[i-1]) > 0);
+ }
+ return TEST_OK;
+}
+
+static struct test_case tests__pmus[] = {
+ TEST_CASE("PMU name combining", name_len),
+ TEST_CASE("PMU name comparison", name_cmp),
+ { .name = NULL, }
+};
+
+struct test_suite suite__pmus = {
+ .desc = "PMUs test",
+ .test_cases = tests__pmus,
+};
diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
index 3aa7701ee0e9..03278f0f7698 100644
--- a/tools/perf/tests/tests.h
+++ b/tools/perf/tests/tests.h
@@ -3,6 +3,7 @@
#define TESTS_H
#include <stdbool.h>
+#include <debug.h>
enum {
TEST_OK = 0,
@@ -81,6 +82,7 @@ DECLARE_SUITE(PERF_RECORD);
DECLARE_SUITE(perf_evsel__roundtrip_name_test);
DECLARE_SUITE(perf_evsel__tp_sched_test);
DECLARE_SUITE(syscall_openat_tp_fields);
+DECLARE_SUITE(pmus);
DECLARE_SUITE(pmu);
DECLARE_SUITE(pmu_events);
DECLARE_SUITE(attr);
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
On Thu, Mar 28, 2024 at 11:48 PM Ian Rogers <[email protected]> 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.
>
> Add a set of PMUs tests for pmu_name_len_no_suffix and pmu_name_cmp.
>
> Signed-off-by: Ian Rogers <[email protected]>
Ping. Here is a link to how marvell encode their PMU names that this
patch is hoping to fix:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/drivers/perf/marvell_cn10k_ddr_pmu.c?h=perf-tools-next#n675
Thanks,
Ian
> ---
> tools/perf/tests/Build | 1 +
> tools/perf/tests/builtin-test.c | 1 +
> tools/perf/tests/pmus.c | 108 ++++++++++++++++++++++++++++++++
> tools/perf/tests/tests.h | 2 +
> tools/perf/util/pmu.c | 2 +-
> tools/perf/util/pmus.c | 51 ++++++++-------
> tools/perf/util/pmus.h | 7 ++-
> 7 files changed, 146 insertions(+), 26 deletions(-)
> create mode 100644 tools/perf/tests/pmus.c
>
> diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build
> index c7f9d9676095..a7bab6e9300f 100644
> --- a/tools/perf/tests/Build
> +++ b/tools/perf/tests/Build
> @@ -14,6 +14,7 @@ perf-y += perf-record.o
> perf-y += evsel-roundtrip-name.o
> perf-$(CONFIG_LIBTRACEEVENT) += evsel-tp-sched.o
> perf-y += fdarray.o
> +perf-y += pmus.o
> perf-y += pmu.o
> perf-y += pmu-events.o
> perf-y += hists_common.o
> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> index d13ee7683d9d..c90f270a469a 100644
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c
> @@ -68,6 +68,7 @@ static struct test_suite *generic_tests[] = {
> &suite__parse_events,
> &suite__expr,
> &suite__PERF_RECORD,
> + &suite__pmus,
> &suite__pmu,
> &suite__pmu_events,
> &suite__dso_data,
> diff --git a/tools/perf/tests/pmus.c b/tools/perf/tests/pmus.c
> new file mode 100644
> index 000000000000..6279c925e689
> --- /dev/null
> +++ b/tools/perf/tests/pmus.c
> @@ -0,0 +1,108 @@
> +// SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause)
> +#include "pmus.h"
> +#include "tests.h"
> +#include <string.h>
> +#include <linux/kernel.h>
> +
> +static const char * const uncore_chas[] = {
> + "uncore_cha_0",
> + "uncore_cha_1",
> + "uncore_cha_2",
> + "uncore_cha_3",
> + "uncore_cha_4",
> + "uncore_cha_5",
> + "uncore_cha_6",
> + "uncore_cha_7",
> + "uncore_cha_8",
> + "uncore_cha_9",
> + "uncore_cha_10",
> + "uncore_cha_11",
> + "uncore_cha_12",
> + "uncore_cha_13",
> + "uncore_cha_14",
> + "uncore_cha_15",
> + "uncore_cha_16",
> + "uncore_cha_17",
> + "uncore_cha_18",
> + "uncore_cha_19",
> + "uncore_cha_20",
> + "uncore_cha_21",
> + "uncore_cha_22",
> + "uncore_cha_23",
> + "uncore_cha_24",
> + "uncore_cha_25",
> + "uncore_cha_26",
> + "uncore_cha_27",
> + "uncore_cha_28",
> + "uncore_cha_29",
> + "uncore_cha_30",
> + "uncore_cha_31",
> +};
> +
> +static const char * const mrvl_ddrs[] = {
> + "mrvl_ddr_pmu_87e1b0000000",
> + "mrvl_ddr_pmu_87e1b1000000",
> + "mrvl_ddr_pmu_87e1b2000000",
> + "mrvl_ddr_pmu_87e1b3000000",
> + "mrvl_ddr_pmu_87e1b4000000",
> + "mrvl_ddr_pmu_87e1b5000000",
> + "mrvl_ddr_pmu_87e1b6000000",
> + "mrvl_ddr_pmu_87e1b7000000",
> + "mrvl_ddr_pmu_87e1b8000000",
> + "mrvl_ddr_pmu_87e1b9000000",
> + "mrvl_ddr_pmu_87e1ba000000",
> + "mrvl_ddr_pmu_87e1bb000000",
> + "mrvl_ddr_pmu_87e1bc000000",
> + "mrvl_ddr_pmu_87e1bd000000",
> + "mrvl_ddr_pmu_87e1be000000",
> + "mrvl_ddr_pmu_87e1bf000000",
> +};
> +
> +static int test__name_len(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
> +{
> + TEST_ASSERT_EQUAL("cpu", pmu_name_len_no_suffix("cpu"), (int)strlen("cpu"));
> + TEST_ASSERT_EQUAL("i915", pmu_name_len_no_suffix("i915"), (int)strlen("i915"));
> + for (size_t i = 0; i < ARRAY_SIZE(uncore_chas); i++) {
> + TEST_ASSERT_EQUAL("Strips uncore_cha suffix",
> + pmu_name_len_no_suffix(uncore_chas[i]),
> + (int)strlen("uncore_cha"));
> + }
> + for (size_t i = 0; i < ARRAY_SIZE(mrvl_ddrs); i++) {
> + TEST_ASSERT_EQUAL("Strips mrvl_ddr_pmu suffix",
> + pmu_name_len_no_suffix(mrvl_ddrs[i]),
> + (int)strlen("mrvl_ddr_pmu"));
> + }
> + return TEST_OK;
> +}
> +
> +static int test__name_cmp(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
> +{
> + TEST_ASSERT_EQUAL("cpu", pmu_name_cmp("cpu", "cpu"), 0);
> + TEST_ASSERT_EQUAL("i915", pmu_name_cmp("i915", "i915"), 0);
> + TEST_ASSERT_VAL("i915", pmu_name_cmp("cpu", "i915") < 0);
> + TEST_ASSERT_VAL("i915", pmu_name_cmp("i915", "cpu") > 0);
> + for (size_t i = 1; i < ARRAY_SIZE(uncore_chas); i++) {
> + TEST_ASSERT_VAL("uncore_cha suffixes ordered lt",
> + pmu_name_cmp(uncore_chas[i-1], uncore_chas[i]) < 0);
> + TEST_ASSERT_VAL("uncore_cha suffixes ordered gt",
> + pmu_name_cmp(uncore_chas[i], uncore_chas[i-1]) > 0);
> + }
> + for (size_t i = 1; i < ARRAY_SIZE(mrvl_ddrs); i++) {
> + TEST_ASSERT_VAL("mrvl_ddr_pmu suffixes ordered lt",
> + pmu_name_cmp(mrvl_ddrs[i-1], mrvl_ddrs[i]) < 0);
> + TEST_ASSERT_VAL("mrvl_ddr_pmu suffixes ordered gt",
> + pmu_name_cmp(mrvl_ddrs[i], mrvl_ddrs[i-1]) > 0);
> + }
> + return TEST_OK;
> +}
> +
> +static struct test_case tests__pmus[] = {
> + TEST_CASE("PMU name combining", name_len),
> + TEST_CASE("PMU name comparison", name_cmp),
> + { .name = NULL, }
> +};
> +
> +struct test_suite suite__pmus = {
> + .desc = "PMUs test",
> + .test_cases = tests__pmus,
> +};
> diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
> index 3aa7701ee0e9..03278f0f7698 100644
> --- a/tools/perf/tests/tests.h
> +++ b/tools/perf/tests/tests.h
> @@ -3,6 +3,7 @@
> #define TESTS_H
>
> #include <stdbool.h>
> +#include <debug.h>
>
> enum {
> TEST_OK = 0,
> @@ -81,6 +82,7 @@ DECLARE_SUITE(PERF_RECORD);
> DECLARE_SUITE(perf_evsel__roundtrip_name_test);
> DECLARE_SUITE(perf_evsel__tp_sched_test);
> DECLARE_SUITE(syscall_openat_tp_fields);
> +DECLARE_SUITE(pmus);
> DECLARE_SUITE(pmu);
> DECLARE_SUITE(pmu_events);
> DECLARE_SUITE(attr);
> 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
>
On 2024-03-29 2:48 a.m., Ian Rogers 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.
>
> Add a set of PMUs tests for pmu_name_len_no_suffix and pmu_name_cmp.
>
> Signed-off-by: Ian Rogers <[email protected]>
> ---
> tools/perf/tests/Build | 1 +
> tools/perf/tests/builtin-test.c | 1 +
> tools/perf/tests/pmus.c | 108 ++++++++++++++++++++++++++++++++
> tools/perf/tests/tests.h | 2 +
> tools/perf/util/pmu.c | 2 +-
> tools/perf/util/pmus.c | 51 ++++++++-------
> tools/perf/util/pmus.h | 7 ++-
> 7 files changed, 146 insertions(+), 26 deletions(-)
> create mode 100644 tools/perf/tests/pmus.c
I think it's better to split the patch into two patches. One is to
update the interface. The other is to add a new test.
>
> diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build
> index c7f9d9676095..a7bab6e9300f 100644
> --- a/tools/perf/tests/Build
> +++ b/tools/perf/tests/Build
> @@ -14,6 +14,7 @@ perf-y += perf-record.o
> perf-y += evsel-roundtrip-name.o
> perf-$(CONFIG_LIBTRACEEVENT) += evsel-tp-sched.o
> perf-y += fdarray.o
> +perf-y += pmus.o
> perf-y += pmu.o
> perf-y += pmu-events.o
> perf-y += hists_common.o
> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> index d13ee7683d9d..c90f270a469a 100644
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c
> @@ -68,6 +68,7 @@ static struct test_suite *generic_tests[] = {
> &suite__parse_events,
> &suite__expr,
> &suite__PERF_RECORD,
> + &suite__pmus,
> &suite__pmu,
> &suite__pmu_events,
> &suite__dso_data,
> diff --git a/tools/perf/tests/pmus.c b/tools/perf/tests/pmus.c
> new file mode 100644
> index 000000000000..6279c925e689
> --- /dev/null
> +++ b/tools/perf/tests/pmus.c
> @@ -0,0 +1,108 @@
> +// SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause)
> +#include "pmus.h"
> +#include "tests.h"
> +#include <string.h>
> +#include <linux/kernel.h>
> +
> +static const char * const uncore_chas[] = {
> + "uncore_cha_0",
> + "uncore_cha_1",
> + "uncore_cha_2",
> + "uncore_cha_3",
> + "uncore_cha_4",
> + "uncore_cha_5",
> + "uncore_cha_6",
> + "uncore_cha_7",
> + "uncore_cha_8",
> + "uncore_cha_9",
> + "uncore_cha_10",
> + "uncore_cha_11",
> + "uncore_cha_12",
> + "uncore_cha_13",
> + "uncore_cha_14",
> + "uncore_cha_15",
> + "uncore_cha_16",
> + "uncore_cha_17",
> + "uncore_cha_18",
> + "uncore_cha_19",
> + "uncore_cha_20",
> + "uncore_cha_21",
> + "uncore_cha_22",
> + "uncore_cha_23",
> + "uncore_cha_24",
> + "uncore_cha_25",
> + "uncore_cha_26",
> + "uncore_cha_27",
> + "uncore_cha_28",
> + "uncore_cha_29",
> + "uncore_cha_30",
> + "uncore_cha_31",
> +};
> +
> +static const char * const mrvl_ddrs[] = {
> + "mrvl_ddr_pmu_87e1b0000000",
> + "mrvl_ddr_pmu_87e1b1000000",
> + "mrvl_ddr_pmu_87e1b2000000",
> + "mrvl_ddr_pmu_87e1b3000000",
> + "mrvl_ddr_pmu_87e1b4000000",
> + "mrvl_ddr_pmu_87e1b5000000",
> + "mrvl_ddr_pmu_87e1b6000000",
> + "mrvl_ddr_pmu_87e1b7000000",
> + "mrvl_ddr_pmu_87e1b8000000",
> + "mrvl_ddr_pmu_87e1b9000000",
> + "mrvl_ddr_pmu_87e1ba000000",
> + "mrvl_ddr_pmu_87e1bb000000",
> + "mrvl_ddr_pmu_87e1bc000000",
> + "mrvl_ddr_pmu_87e1bd000000",
> + "mrvl_ddr_pmu_87e1be000000",
> + "mrvl_ddr_pmu_87e1bf000000",
> +};
> +
> +static int test__name_len(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
> +{
> + TEST_ASSERT_EQUAL("cpu", pmu_name_len_no_suffix("cpu"), (int)strlen("cpu"));
> + TEST_ASSERT_EQUAL("i915", pmu_name_len_no_suffix("i915"), (int)strlen("i915"));
> + for (size_t i = 0; i < ARRAY_SIZE(uncore_chas); i++) {
> + TEST_ASSERT_EQUAL("Strips uncore_cha suffix",
> + pmu_name_len_no_suffix(uncore_chas[i]),
> + (int)strlen("uncore_cha"));
> + }
> + for (size_t i = 0; i < ARRAY_SIZE(mrvl_ddrs); i++) {
> + TEST_ASSERT_EQUAL("Strips mrvl_ddr_pmu suffix",
> + pmu_name_len_no_suffix(mrvl_ddrs[i]),
> + (int)strlen("mrvl_ddr_pmu"));
> + }
> + return TEST_OK;
> +}
> +
> +static int test__name_cmp(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
> +{
> + TEST_ASSERT_EQUAL("cpu", pmu_name_cmp("cpu", "cpu"), 0);
> + TEST_ASSERT_EQUAL("i915", pmu_name_cmp("i915", "i915"), 0);
> + TEST_ASSERT_VAL("i915", pmu_name_cmp("cpu", "i915") < 0);
> + TEST_ASSERT_VAL("i915", pmu_name_cmp("i915", "cpu") > 0);
> + for (size_t i = 1; i < ARRAY_SIZE(uncore_chas); i++) {
> + TEST_ASSERT_VAL("uncore_cha suffixes ordered lt",
> + pmu_name_cmp(uncore_chas[i-1], uncore_chas[i]) < 0);
> + TEST_ASSERT_VAL("uncore_cha suffixes ordered gt",
> + pmu_name_cmp(uncore_chas[i], uncore_chas[i-1]) > 0);
> + }
> + for (size_t i = 1; i < ARRAY_SIZE(mrvl_ddrs); i++) {
> + TEST_ASSERT_VAL("mrvl_ddr_pmu suffixes ordered lt",
> + pmu_name_cmp(mrvl_ddrs[i-1], mrvl_ddrs[i]) < 0);
> + TEST_ASSERT_VAL("mrvl_ddr_pmu suffixes ordered gt",
> + pmu_name_cmp(mrvl_ddrs[i], mrvl_ddrs[i-1]) > 0);
> + }
> + return TEST_OK;
> +}
> +
> +static struct test_case tests__pmus[] = {
> + TEST_CASE("PMU name combining", name_len),
> + TEST_CASE("PMU name comparison", name_cmp),
> + { .name = NULL, }
> +};
> +
> +struct test_suite suite__pmus = {
> + .desc = "PMUs test",
> + .test_cases = tests__pmus,
> +};
> diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
> index 3aa7701ee0e9..03278f0f7698 100644
> --- a/tools/perf/tests/tests.h
> +++ b/tools/perf/tests/tests.h
> @@ -3,6 +3,7 @@
> #define TESTS_H
>
> #include <stdbool.h>
> +#include <debug.h>
>
> enum {
> TEST_OK = 0,
> @@ -81,6 +82,7 @@ DECLARE_SUITE(PERF_RECORD);
> DECLARE_SUITE(perf_evsel__roundtrip_name_test);
> DECLARE_SUITE(perf_evsel__tp_sched_test);
> DECLARE_SUITE(syscall_openat_tp_fields);
> +DECLARE_SUITE(pmus);
> DECLARE_SUITE(pmu);
> DECLARE_SUITE(pmu_events);
> DECLARE_SUITE(attr);
> 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--;
The letter in the hex may be part of a PMU name.
Searched the kernel code and got this PMU name of s390.
rc = perf_pmu_register(&cpumf_pmu, "cpum_cf", -1);
Such kinds of PMUs may have issues.
Thanks,
Kan
>
> - 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);
>
On Tue, Apr 9, 2024 at 8:17 AM Liang, Kan <[email protected]> wrote:
>
>
>
> On 2024-03-29 2:48 a.m., Ian Rogers 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.
> >
> > Add a set of PMUs tests for pmu_name_len_no_suffix and pmu_name_cmp.
> >
> > Signed-off-by: Ian Rogers <[email protected]>
> > ---
> > tools/perf/tests/Build | 1 +
> > tools/perf/tests/builtin-test.c | 1 +
> > tools/perf/tests/pmus.c | 108 ++++++++++++++++++++++++++++++++
> > tools/perf/tests/tests.h | 2 +
> > tools/perf/util/pmu.c | 2 +-
> > tools/perf/util/pmus.c | 51 ++++++++-------
> > tools/perf/util/pmus.h | 7 ++-
> > 7 files changed, 146 insertions(+), 26 deletions(-)
> > create mode 100644 tools/perf/tests/pmus.c
>
> I think it's better to split the patch into two patches. One is to
> update the interface. The other is to add a new test.
Sgtm, will fix for v2.
> >
> > diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build
> > index c7f9d9676095..a7bab6e9300f 100644
> > --- a/tools/perf/tests/Build
> > +++ b/tools/perf/tests/Build
> > @@ -14,6 +14,7 @@ perf-y += perf-record.o
> > perf-y += evsel-roundtrip-name.o
> > perf-$(CONFIG_LIBTRACEEVENT) += evsel-tp-sched.o
> > perf-y += fdarray.o
> > +perf-y += pmus.o
> > perf-y += pmu.o
> > perf-y += pmu-events.o
> > perf-y += hists_common.o
> > diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> > index d13ee7683d9d..c90f270a469a 100644
> > --- a/tools/perf/tests/builtin-test.c
> > +++ b/tools/perf/tests/builtin-test.c
> > @@ -68,6 +68,7 @@ static struct test_suite *generic_tests[] = {
> > &suite__parse_events,
> > &suite__expr,
> > &suite__PERF_RECORD,
> > + &suite__pmus,
> > &suite__pmu,
> > &suite__pmu_events,
> > &suite__dso_data,
> > diff --git a/tools/perf/tests/pmus.c b/tools/perf/tests/pmus.c
> > new file mode 100644
> > index 000000000000..6279c925e689
> > --- /dev/null
> > +++ b/tools/perf/tests/pmus.c
> > @@ -0,0 +1,108 @@
> > +// SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause)
> > +#include "pmus.h"
> > +#include "tests.h"
> > +#include <string.h>
> > +#include <linux/kernel.h>
> > +
> > +static const char * const uncore_chas[] = {
> > + "uncore_cha_0",
> > + "uncore_cha_1",
> > + "uncore_cha_2",
> > + "uncore_cha_3",
> > + "uncore_cha_4",
> > + "uncore_cha_5",
> > + "uncore_cha_6",
> > + "uncore_cha_7",
> > + "uncore_cha_8",
> > + "uncore_cha_9",
> > + "uncore_cha_10",
> > + "uncore_cha_11",
> > + "uncore_cha_12",
> > + "uncore_cha_13",
> > + "uncore_cha_14",
> > + "uncore_cha_15",
> > + "uncore_cha_16",
> > + "uncore_cha_17",
> > + "uncore_cha_18",
> > + "uncore_cha_19",
> > + "uncore_cha_20",
> > + "uncore_cha_21",
> > + "uncore_cha_22",
> > + "uncore_cha_23",
> > + "uncore_cha_24",
> > + "uncore_cha_25",
> > + "uncore_cha_26",
> > + "uncore_cha_27",
> > + "uncore_cha_28",
> > + "uncore_cha_29",
> > + "uncore_cha_30",
> > + "uncore_cha_31",
> > +};
> > +
> > +static const char * const mrvl_ddrs[] = {
> > + "mrvl_ddr_pmu_87e1b0000000",
> > + "mrvl_ddr_pmu_87e1b1000000",
> > + "mrvl_ddr_pmu_87e1b2000000",
> > + "mrvl_ddr_pmu_87e1b3000000",
> > + "mrvl_ddr_pmu_87e1b4000000",
> > + "mrvl_ddr_pmu_87e1b5000000",
> > + "mrvl_ddr_pmu_87e1b6000000",
> > + "mrvl_ddr_pmu_87e1b7000000",
> > + "mrvl_ddr_pmu_87e1b8000000",
> > + "mrvl_ddr_pmu_87e1b9000000",
> > + "mrvl_ddr_pmu_87e1ba000000",
> > + "mrvl_ddr_pmu_87e1bb000000",
> > + "mrvl_ddr_pmu_87e1bc000000",
> > + "mrvl_ddr_pmu_87e1bd000000",
> > + "mrvl_ddr_pmu_87e1be000000",
> > + "mrvl_ddr_pmu_87e1bf000000",
> > +};
> > +
> > +static int test__name_len(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
> > +{
> > + TEST_ASSERT_EQUAL("cpu", pmu_name_len_no_suffix("cpu"), (int)strlen("cpu"));
> > + TEST_ASSERT_EQUAL("i915", pmu_name_len_no_suffix("i915"), (int)strlen("i915"));
> > + for (size_t i = 0; i < ARRAY_SIZE(uncore_chas); i++) {
> > + TEST_ASSERT_EQUAL("Strips uncore_cha suffix",
> > + pmu_name_len_no_suffix(uncore_chas[i]),
> > + (int)strlen("uncore_cha"));
> > + }
> > + for (size_t i = 0; i < ARRAY_SIZE(mrvl_ddrs); i++) {
> > + TEST_ASSERT_EQUAL("Strips mrvl_ddr_pmu suffix",
> > + pmu_name_len_no_suffix(mrvl_ddrs[i]),
> > + (int)strlen("mrvl_ddr_pmu"));
> > + }
> > + return TEST_OK;
> > +}
> > +
> > +static int test__name_cmp(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
> > +{
> > + TEST_ASSERT_EQUAL("cpu", pmu_name_cmp("cpu", "cpu"), 0);
> > + TEST_ASSERT_EQUAL("i915", pmu_name_cmp("i915", "i915"), 0);
> > + TEST_ASSERT_VAL("i915", pmu_name_cmp("cpu", "i915") < 0);
> > + TEST_ASSERT_VAL("i915", pmu_name_cmp("i915", "cpu") > 0);
> > + for (size_t i = 1; i < ARRAY_SIZE(uncore_chas); i++) {
> > + TEST_ASSERT_VAL("uncore_cha suffixes ordered lt",
> > + pmu_name_cmp(uncore_chas[i-1], uncore_chas[i]) < 0);
> > + TEST_ASSERT_VAL("uncore_cha suffixes ordered gt",
> > + pmu_name_cmp(uncore_chas[i], uncore_chas[i-1]) > 0);
> > + }
> > + for (size_t i = 1; i < ARRAY_SIZE(mrvl_ddrs); i++) {
> > + TEST_ASSERT_VAL("mrvl_ddr_pmu suffixes ordered lt",
> > + pmu_name_cmp(mrvl_ddrs[i-1], mrvl_ddrs[i]) < 0);
> > + TEST_ASSERT_VAL("mrvl_ddr_pmu suffixes ordered gt",
> > + pmu_name_cmp(mrvl_ddrs[i], mrvl_ddrs[i-1]) > 0);
> > + }
> > + return TEST_OK;
> > +}
> > +
> > +static struct test_case tests__pmus[] = {
> > + TEST_CASE("PMU name combining", name_len),
> > + TEST_CASE("PMU name comparison", name_cmp),
> > + { .name = NULL, }
> > +};
> > +
> > +struct test_suite suite__pmus = {
> > + .desc = "PMUs test",
> > + .test_cases = tests__pmus,
> > +};
> > diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
> > index 3aa7701ee0e9..03278f0f7698 100644
> > --- a/tools/perf/tests/tests.h
> > +++ b/tools/perf/tests/tests.h
> > @@ -3,6 +3,7 @@
> > #define TESTS_H
> >
> > #include <stdbool.h>
> > +#include <debug.h>
> >
> > enum {
> > TEST_OK = 0,
> > @@ -81,6 +82,7 @@ DECLARE_SUITE(PERF_RECORD);
> > DECLARE_SUITE(perf_evsel__roundtrip_name_test);
> > DECLARE_SUITE(perf_evsel__tp_sched_test);
> > DECLARE_SUITE(syscall_openat_tp_fields);
> > +DECLARE_SUITE(pmus);
> > DECLARE_SUITE(pmu);
> > DECLARE_SUITE(pmu_events);
> > DECLARE_SUITE(attr);
> > 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--;
>
> The letter in the hex may be part of a PMU name.
>
> Searched the kernel code and got this PMU name of s390.
>
> rc = perf_pmu_register(&cpumf_pmu, "cpum_cf", -1);
>
> Such kinds of PMUs may have issues.
Ugh, good catch and why do we always do this? :-) I'm reminded that
raw events are "r[0-9a-f]+" which of course matches the name "read",
had we made raw events "r0x[0-9a-f]+" then a bunch of complexity in
the event parsing could have been avoided. I think in this case the
"m" in "cpum" should save things, but if Thomas could check on a real
s390 it'd be awesome :-) We have similar issues on Intel, for example
the PMU name "i" will match "i915", which doesn't exactly feel working
as intended. The other option is to rename the Marvell PMUs? Maybe
Bharat and/or Bhaskra who did that work could comment?
Thanks,
Ian
> Thanks,
> Kan
> >
> > - 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);
> >