Messages in this thread | | | From | Ian Rogers <> | Date | Mon, 8 Apr 2024 10:43:04 -0700 | Subject | Re: [PATCH v1] perf pmus: Sort/merge/aggregate PMUs like mrvl_ddr_pmu |
| |
On Thu, Mar 28, 2024 at 11:48 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. > > Add a set of PMUs tests for pmu_name_len_no_suffix and pmu_name_cmp. > > Signed-off-by: Ian Rogers <irogers@google.com>
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 >
| |