The mrvl_ddr_pmu is uncore and has a hexadecimal address
suffix. Current PMU sorting/merging code assumes uncore PMU names
start with uncore_ and have a decimal suffix. Add support for
hexadecimal suffixes and add tests.
v3. Rebase and move tests from pmus.c to the existing pmu.c.
Ian Rogers (2):
perf pmus: Sort/merge/aggregate PMUs like mrvl_ddr_pmu
perf tests: Add some pmu core functionality tests
tools/perf/tests/pmu.c | 95 ++++++++++++++++++++++++++++++++++++++++++
tools/perf/util/pmu.c | 4 +-
tools/perf/util/pmus.c | 53 ++++++++++++-----------
tools/perf/util/pmus.h | 7 +++-
4 files changed, 131 insertions(+), 28 deletions(-)
--
2.45.0.rc1.225.g2a3ae87e7f-goog
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.
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/util/pmu.c | 4 ++--
tools/perf/util/pmus.c | 53 ++++++++++++++++++++++--------------------
tools/perf/util/pmus.h | 7 +++++-
3 files changed, 36 insertions(+), 28 deletions(-)
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index b3b072feef02..32e5a73273d7 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -1761,7 +1761,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);
@@ -1805,7 +1805,7 @@ int perf_pmu__for_each_event(struct perf_pmu *pmu, bool skip_duplicate_pmus,
info.pmu_name = event->pmu_name ?: pmu->name;
pmu_name_len = skip_duplicate_pmus
- ? pmu_name_len_no_suffix(info.pmu_name, /*num=*/NULL)
+ ? pmu_name_len_no_suffix(info.pmu_name)
: (int)strlen(info.pmu_name);
info.alias = NULL;
if (event->desc) {
diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
index b9b4c5eb5002..28cfa7adbfd7 100644
--- a/tools/perf/util/pmus.c
+++ b/tools/perf/util/pmus.c
@@ -40,31 +40,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;
@@ -167,20 +180,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: */
@@ -300,11 +303,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))
@@ -316,7 +319,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))
@@ -566,7 +569,7 @@ void perf_pmus__print_raw_pmu_events(const struct print_callbacks *print_cb, voi
.long_string = STRBUF_INIT,
.num_formats = 0,
};
- int len = pmu_name_len_no_suffix(pmu->name, /*num=*/NULL);
+ int len = pmu_name_len_no_suffix(pmu->name);
const char *desc = "(see 'man perf-list' or 'man perf-record' on how to encode it)";
if (!pmu->is_core)
diff --git a/tools/perf/util/pmus.h b/tools/perf/util/pmus.h
index 9d4ded80b8e9..69d64fbd4259 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.45.0.rc1.225.g2a3ae87e7f-goog
Test behavior of PMU names and comparisons wrt suffixes using Intel
uncore_cha and marvell mrvl_ddr_pmu as examples.
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/tests/pmu.c | 95 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 95 insertions(+)
diff --git a/tools/perf/tests/pmu.c b/tools/perf/tests/pmu.c
index 06cc0e46cb28..9dc77f29f723 100644
--- a/tools/perf/tests/pmu.c
+++ b/tools/perf/tests/pmu.c
@@ -3,6 +3,7 @@
#include "evsel.h"
#include "parse-events.h"
#include "pmu.h"
+#include "pmus.h"
#include "tests.h"
#include "debug.h"
#include "fncache.h"
@@ -340,10 +341,104 @@ static int test__pmu_event_names(struct test_suite *test __maybe_unused,
return ret;
}
+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__pmu[] = {
TEST_CASE("Parsing with PMU format directory", pmu_format),
TEST_CASE("Parsing with PMU event", pmu_events),
TEST_CASE("PMU event names", pmu_event_names),
+ TEST_CASE("PMU name combining", name_len),
+ TEST_CASE("PMU name comparison", name_cmp),
{ .name = NULL, }
};
--
2.45.0.rc1.225.g2a3ae87e7f-goog
On Sat, May 4, 2024 at 8:16 PM Ian Rogers <[email protected]> wrote:
>
> The mrvl_ddr_pmu is uncore and has a hexadecimal address
> suffix. Current PMU sorting/merging code assumes uncore PMU names
> start with uncore_ and have a decimal suffix. Add support for
> hexadecimal suffixes and add tests.
>
> v3. Rebase and move tests from pmus.c to the existing pmu.c.
>
> Ian Rogers (2):
> perf pmus: Sort/merge/aggregate PMUs like mrvl_ddr_pmu
> perf tests: Add some pmu core functionality tests
Hi, these patches have been hanging around since March [1], it would
be nice to either be landing them or getting feedback on what to fix.
Thanks,
Ian
[1] https://lore.kernel.org/lkml/[email protected]/
> tools/perf/tests/pmu.c | 95 ++++++++++++++++++++++++++++++++++++++++++
> tools/perf/util/pmu.c | 4 +-
> tools/perf/util/pmus.c | 53 ++++++++++++-----------
> tools/perf/util/pmus.h | 7 +++-
> 4 files changed, 131 insertions(+), 28 deletions(-)
>
> --
> 2.45.0.rc1.225.g2a3ae87e7f-goog
>
On Fri, May 10, 2024 at 11:13 AM Ian Rogers <[email protected]> wrote:
>
> On Sat, May 4, 2024 at 8:16 PM Ian Rogers <[email protected]> wrote:
> >
> > The mrvl_ddr_pmu is uncore and has a hexadecimal address
> > suffix. Current PMU sorting/merging code assumes uncore PMU names
> > start with uncore_ and have a decimal suffix. Add support for
> > hexadecimal suffixes and add tests.
> >
> > v3. Rebase and move tests from pmus.c to the existing pmu.c.
> >
> > Ian Rogers (2):
> > perf pmus: Sort/merge/aggregate PMUs like mrvl_ddr_pmu
> > perf tests: Add some pmu core functionality tests
>
> Hi, these patches have been hanging around since March [1], it would
> be nice to either be landing them or getting feedback on what to fix.
>
> Thanks,
> Ian
>
> [1] https://lore.kernel.org/lkml/[email protected]/
+Tuan Phan, Robin Murphy
Here is another PMU with the same suffix convention/issue:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/drivers/perf/arm_dmc620_pmu.c?h=perf-tools-next#n706
Thanks,
Ian
> > tools/perf/tests/pmu.c | 95 ++++++++++++++++++++++++++++++++++++++++++
> > tools/perf/util/pmu.c | 4 +-
> > tools/perf/util/pmus.c | 53 ++++++++++++-----------
> > tools/perf/util/pmus.h | 7 +++-
> > 4 files changed, 131 insertions(+), 28 deletions(-)
> >
> > --
> > 2.45.0.rc1.225.g2a3ae87e7f-goog
> >
On Fri, May 10, 2024 at 11:13:36AM -0700, Ian Rogers wrote:
> On Sat, May 4, 2024 at 8:16 PM Ian Rogers <[email protected]> wrote:
> >
> > The mrvl_ddr_pmu is uncore and has a hexadecimal address
> > suffix. Current PMU sorting/merging code assumes uncore PMU names
> > start with uncore_ and have a decimal suffix. Add support for
> > hexadecimal suffixes and add tests.
> >
> > v3. Rebase and move tests from pmus.c to the existing pmu.c.
> >
> > Ian Rogers (2):
> > perf pmus: Sort/merge/aggregate PMUs like mrvl_ddr_pmu
> > perf tests: Add some pmu core functionality tests
>
> Hi, these patches have been hanging around since March [1], it would
> be nice to either be landing them or getting feedback on what to fix.
Thanks, applied to perf-tools-next,
- Arnaldo
> Thanks,
> Ian
>
> [1] https://lore.kernel.org/lkml/[email protected]/
>
> > tools/perf/tests/pmu.c | 95 ++++++++++++++++++++++++++++++++++++++++++
> > tools/perf/util/pmu.c | 4 +-
> > tools/perf/util/pmus.c | 53 ++++++++++++-----------
> > tools/perf/util/pmus.h | 7 +++-
> > 4 files changed, 131 insertions(+), 28 deletions(-)
> >
> > --
> > 2.45.0.rc1.225.g2a3ae87e7f-goog
> >
On Sat, May 04, 2024 at 08:16:23PM -0700, 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.
Removed these patches due to:
util/pmus.c: In function ‘pmu_name_cmp’:
util/pmus.c:67:19: error: ‘strncmp’ specified bound [18446744071562067968, 18446744073709551615] exceeds maximum object size 9223372036854775807 [-Werror=stringop-overread]
67 | int ret = strncmp(lhs_pmu_name, rhs_pmu_name,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
68 | lhs_pmu_name_len < rhs_pmu_name_len ? lhs_pmu_name_len : rhs_pmu_name_len);
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make[4]: *** [/tmp/perf-6.9.0-rc5/tools/build/Makefile.build:105: util/pmus.o] Error 1
Noticed while doing a 'make -C tools/perf build-test', the tarbz test.
acme@x1:~/git/perf-tools-next$ head /etc/os-release
NAME="Fedora Linux"
VERSION="39 (Workstation Edition)"
ID=fedora
VERSION_ID=39
VERSION_CODENAME=""
PLATFORM_ID="platform:f39"
PRETTY_NAME="Fedora Linux 39 (Workstation Edition)"
ANSI_COLOR="0;38;2;60;110;180"
LOGO=fedora-logo-icon
CPE_NAME="cpe:/o:fedoraproject:fedora:39"
acme@x1:~/git/perf-tools-next$
- Arnaldo
> Signed-off-by: Ian Rogers <[email protected]>
> ---
> tools/perf/util/pmu.c | 4 ++--
> tools/perf/util/pmus.c | 53 ++++++++++++++++++++++--------------------
> tools/perf/util/pmus.h | 7 +++++-
> 3 files changed, 36 insertions(+), 28 deletions(-)
>
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index b3b072feef02..32e5a73273d7 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -1761,7 +1761,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);
>
> @@ -1805,7 +1805,7 @@ int perf_pmu__for_each_event(struct perf_pmu *pmu, bool skip_duplicate_pmus,
>
> info.pmu_name = event->pmu_name ?: pmu->name;
> pmu_name_len = skip_duplicate_pmus
> - ? pmu_name_len_no_suffix(info.pmu_name, /*num=*/NULL)
> + ? pmu_name_len_no_suffix(info.pmu_name)
> : (int)strlen(info.pmu_name);
> info.alias = NULL;
> if (event->desc) {
> diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
> index b9b4c5eb5002..28cfa7adbfd7 100644
> --- a/tools/perf/util/pmus.c
> +++ b/tools/perf/util/pmus.c
> @@ -40,31 +40,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;
> @@ -167,20 +180,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: */
> @@ -300,11 +303,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))
> @@ -316,7 +319,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))
> @@ -566,7 +569,7 @@ void perf_pmus__print_raw_pmu_events(const struct print_callbacks *print_cb, voi
> .long_string = STRBUF_INIT,
> .num_formats = 0,
> };
> - int len = pmu_name_len_no_suffix(pmu->name, /*num=*/NULL);
> + int len = pmu_name_len_no_suffix(pmu->name);
> const char *desc = "(see 'man perf-list' or 'man perf-record' on how to encode it)";
>
> if (!pmu->is_core)
> diff --git a/tools/perf/util/pmus.h b/tools/perf/util/pmus.h
> index 9d4ded80b8e9..69d64fbd4259 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.45.0.rc1.225.g2a3ae87e7f-goog
On 2024-05-10 8:15 pm, Ian Rogers wrote:
> On Fri, May 10, 2024 at 11:13 AM Ian Rogers <[email protected]> wrote:
>>
>> On Sat, May 4, 2024 at 8:16 PM Ian Rogers <[email protected]> wrote:
>>>
>>> The mrvl_ddr_pmu is uncore and has a hexadecimal address
>>> suffix. Current PMU sorting/merging code assumes uncore PMU names
>>> start with uncore_ and have a decimal suffix. Add support for
>>> hexadecimal suffixes and add tests.
>>>
>>> v3. Rebase and move tests from pmus.c to the existing pmu.c.
>>>
>>> Ian Rogers (2):
>>> perf pmus: Sort/merge/aggregate PMUs like mrvl_ddr_pmu
>>> perf tests: Add some pmu core functionality tests
>>
>> Hi, these patches have been hanging around since March [1], it would
>> be nice to either be landing them or getting feedback on what to fix.
>>
>> Thanks,
>> Ian
>>
>> [1] https://lore.kernel.org/lkml/[email protected]/
>
> +Tuan Phan, Robin Murphy
>
> Here is another PMU with the same suffix convention/issue:
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/drivers/perf/arm_dmc620_pmu.c?h=perf-tools-next#n706
There are at least one or two more as well - certainly arm_smmuv3_pmu
which I think may have been where this pattern first started. Now that
we've finally done the right thing with the parent mechanism to provide
a user-visible relationship of PMU instances to their corresponding
Devicetree/ACPI devices, hopefully we can discourage any further use of
this rather clunky trick of using the MMIO address as an identifier in
the PMU name. However there's then also stuff like dwc_pcie_pmu which
encodes a PCI ID as a hex suffix, so understanding hex suffixes in
general might still be a reasonable idea for the tool, if the
alternative would be maintaining a list of specific prefixes (even if
that would be hoped to remain fairly small).
Thanks,
Robin.
On Mon, May 13, 2024 at 7:35 AM Robin Murphy <[email protected]> wrote:
>
> On 2024-05-10 8:15 pm, Ian Rogers wrote:
> > On Fri, May 10, 2024 at 11:13 AM Ian Rogers <[email protected]> wrote:
> >>
> >> On Sat, May 4, 2024 at 8:16 PM Ian Rogers <[email protected]> wrote:
> >>>
> >>> The mrvl_ddr_pmu is uncore and has a hexadecimal address
> >>> suffix. Current PMU sorting/merging code assumes uncore PMU names
> >>> start with uncore_ and have a decimal suffix. Add support for
> >>> hexadecimal suffixes and add tests.
> >>>
> >>> v3. Rebase and move tests from pmus.c to the existing pmu.c.
> >>>
> >>> Ian Rogers (2):
> >>> perf pmus: Sort/merge/aggregate PMUs like mrvl_ddr_pmu
> >>> perf tests: Add some pmu core functionality tests
> >>
> >> Hi, these patches have been hanging around since March [1], it would
> >> be nice to either be landing them or getting feedback on what to fix.
> >>
> >> Thanks,
> >> Ian
> >>
> >> [1] https://lore.kernel.org/lkml/[email protected]/
> >
> > +Tuan Phan, Robin Murphy
> >
> > Here is another PMU with the same suffix convention/issue:
> > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/drivers/perf/arm_dmc620_pmu.c?h=perf-tools-next#n706
>
> There are at least one or two more as well - certainly arm_smmuv3_pmu
> which I think may have been where this pattern first started. Now that
> we've finally done the right thing with the parent mechanism to provide
> a user-visible relationship of PMU instances to their corresponding
> Devicetree/ACPI devices, hopefully we can discourage any further use of
> this rather clunky trick of using the MMIO address as an identifier in
> the PMU name. However there's then also stuff like dwc_pcie_pmu which
> encodes a PCI ID as a hex suffix, so understanding hex suffixes in
> general might still be a reasonable idea for the tool, if the
> alternative would be maintaining a list of specific prefixes (even if
> that would be hoped to remain fairly small).
Thanks Robin, the problem that Kan identified was that IBM s390 have a
PMU called cpum_cf. So the _cf is a valid hex suffix. Maybe there
needs to be a minimum hex suffix length to deal with this. We made the
0x optional in perf "raw" events like `perf stat -e 'r12abcd'...` and
now we have the problem that the word `read` is both quite a desirable
perf event name and also a valid hex encoding. We prefer the event
name if it exists, but we have to make the code more complex to deal
with this. It would be nice if we were capturing the conventions in:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/ABI/testing
Like with the sysfs-bus-event_source-* files. I'll do a v4 of these
changes and see if I can do something to document the suffixes, would
be great to get an Acked-by/Tested-by.
Thanks,
Ian
> Thanks,
> Robin.