[PATCH v3 0/3] perf: strcmp_cpuid_str() expression fixups

[PATCH v3 0/3] perf: strcmp_cpuid_str() expression fixups

[James Clark]

Set of fixes related to the comments here [1]. Mainly cleanups,
additional tests and refactoring since adding the new strcmp_cpuid_str()
metric expression.

I added the string replace function to the perf utils
rather than tools/lib/string.c because it didn't seem
easy to add tests for tools/lib.

[1]: https://lore.kernel.org/linux-arm-kernel/CAP-5=fVnUx0BnJC7X1rrm42OD7Bk=ZsHWNwAZMBYyB7yWhBfhQ@mail.gmail.com/
[2]: https://lore.kernel.org/linux-perf-users/ZQC7da2AM9ih8RMz@kernel.org/

---
Changes since v2:
  * Drop patches that were already applied (which makes the cover letter
    mostly redundant)
  * Avoid generating the compiler warning reported here [2]

Changes since v1:
  * s -> haystack
  * find -> needle

James Clark (3):
  perf pmu: Move pmu__find_core_pmu() to pmus.c
  perf pmus: Simplify perf_pmus__find_core_pmu()
  perf pmu: Remove unused function

 tools/perf/arch/arm64/util/pmu.c | 20 ++++++++------------
 tools/perf/tests/expr.c          |  2 +-
 tools/perf/util/expr.c           |  2 +-
 tools/perf/util/pmu.c            | 22 ----------------------
 tools/perf/util/pmu.h            |  3 +--
 tools/perf/util/pmus.c           |  8 +++++++-
 6 files changed, 18 insertions(+), 39 deletions(-)

-- 
2.34.1

[PATCH v3 1/3] perf pmu: Move pmu__find_core_pmu() to pmus.c

[James Clark]

pmu__find_core_pmu() more logically belongs in pmus.c because it
iterates over all PMUs, so move it to pmus.c

At the same time rename it to perf_pmus__find_core_pmu() to match the
naming convention in this file.

list_prepare_entry() can't be used in perf_pmus__scan_core() anymore now
that it's called from the same compilation unit. This is with -O2
(specifically -O1 -ftree-vrp -finline-functions
-finline-small-functions) which allow the bounds of the array
access to be determined at compile time. list_prepare_entry() subtracts
the offset of the 'list' member in struct perf_pmu from &core_pmus,
which isn't a struct perf_pmu. The compiler sees that pmu results in
&core_pmus - 8 and refuses to compile. At runtime this works because
list_for_each_entry_continue() always adds the offset back again before
dereferencing ->next, but it's technically undefined behavior. With
-fsanitize=undefined an additional warning is generated.

Using list_first_entry_or_null() to get the first entry here avoids
doing &core_pmus - 8 but has the same result and fixes both the compile
warning and the undefined behavior warning. There are other uses of
list_prepare_entry() in pmus.c, but the compiler doesn't seem to be
able to see that they can also be called with &core_pmus, so I won't
change any at this time.

Signed-off-by: James Clark <james.clark@arm.com>
---
 tools/perf/arch/arm64/util/pmu.c |  6 +++---
 tools/perf/tests/expr.c          |  2 +-
 tools/perf/util/expr.c           |  2 +-
 tools/perf/util/pmu.c            | 17 -----------------
 tools/perf/util/pmu.h            |  2 +-
 tools/perf/util/pmus.c           | 20 +++++++++++++++++++-
 6 files changed, 25 insertions(+), 24 deletions(-)

diff --git a/tools/perf/arch/arm64/util/pmu.c b/tools/perf/arch/arm64/util/pmu.c
index 615084eb88d8..3d9330feebd2 100644
--- a/tools/perf/arch/arm64/util/pmu.c
+++ b/tools/perf/arch/arm64/util/pmu.c
@@ -10,7 +10,7 @@
 
 const struct pmu_metrics_table *pmu_metrics_table__find(void)
 {
-	struct perf_pmu *pmu = pmu__find_core_pmu();
+	struct perf_pmu *pmu = perf_pmus__find_core_pmu();
 
 	if (pmu)
 		return perf_pmu__find_metrics_table(pmu);
@@ -20,7 +20,7 @@ const struct pmu_metrics_table *pmu_metrics_table__find(void)
 
 const struct pmu_events_table *pmu_events_table__find(void)
 {
-	struct perf_pmu *pmu = pmu__find_core_pmu();
+	struct perf_pmu *pmu = perf_pmus__find_core_pmu();
 
 	if (pmu)
 		return perf_pmu__find_events_table(pmu);
@@ -32,7 +32,7 @@ double perf_pmu__cpu_slots_per_cycle(void)
 {
 	char path[PATH_MAX];
 	unsigned long long slots = 0;
-	struct perf_pmu *pmu = pmu__find_core_pmu();
+	struct perf_pmu *pmu = perf_pmus__find_core_pmu();
 
 	if (pmu) {
 		perf_pmu__pathname_scnprintf(path, sizeof(path),
diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
index 78544092ef0c..e3aa9d4fcf3a 100644
--- a/tools/perf/tests/expr.c
+++ b/tools/perf/tests/expr.c
@@ -76,7 +76,7 @@ static int test__expr(struct test_suite *t __maybe_unused, int subtest __maybe_u
 	struct expr_parse_ctx *ctx;
 	bool is_intel = false;
 	char strcmp_cpuid_buf[256];
-	struct perf_pmu *pmu = pmu__find_core_pmu();
+	struct perf_pmu *pmu = perf_pmus__find_core_pmu();
 	char *cpuid = perf_pmu__getcpuid(pmu);
 	char *escaped_cpuid1, *escaped_cpuid2;
 
diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
index 4488f306de78..7be23b3ac082 100644
--- a/tools/perf/util/expr.c
+++ b/tools/perf/util/expr.c
@@ -509,7 +509,7 @@ double expr__strcmp_cpuid_str(const struct expr_parse_ctx *ctx __maybe_unused,
 		       bool compute_ids __maybe_unused, const char *test_id)
 {
 	double ret;
-	struct perf_pmu *pmu = pmu__find_core_pmu();
+	struct perf_pmu *pmu = perf_pmus__find_core_pmu();
 	char *cpuid = perf_pmu__getcpuid(pmu);
 
 	if (!cpuid)
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index e2159854ab26..f50a5636633f 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -2050,20 +2050,3 @@ void perf_pmu__delete(struct perf_pmu *pmu)
 	zfree(&pmu->id);
 	free(pmu);
 }
-
-struct perf_pmu *pmu__find_core_pmu(void)
-{
-	struct perf_pmu *pmu = NULL;
-
-	while ((pmu = perf_pmus__scan_core(pmu))) {
-		/*
-		 * The cpumap should cover all CPUs. Otherwise, some CPUs may
-		 * not support some events or have different event IDs.
-		 */
-		if (RC_CHK_ACCESS(pmu->cpus)->nr != cpu__max_cpu().cpu)
-			return NULL;
-
-		return pmu;
-	}
-	return NULL;
-}
diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
index bd5d804a6736..d7b46085642d 100644
--- a/tools/perf/util/pmu.h
+++ b/tools/perf/util/pmu.h
@@ -264,6 +264,6 @@ int perf_pmu__pathname_fd(int dirfd, const char *pmu_name, const char *filename,
 struct perf_pmu *perf_pmu__lookup(struct list_head *pmus, int dirfd, const char *lookup_name);
 struct perf_pmu *perf_pmu__create_placeholder_core_pmu(struct list_head *core_pmus);
 void perf_pmu__delete(struct perf_pmu *pmu);
-struct perf_pmu *pmu__find_core_pmu(void);
+struct perf_pmu *perf_pmus__find_core_pmu(void);
 
 #endif /* __PMU_H */
diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
index 6631367c756f..cec869cbe163 100644
--- a/tools/perf/util/pmus.c
+++ b/tools/perf/util/pmus.c
@@ -10,6 +10,7 @@
 #include <pthread.h>
 #include <string.h>
 #include <unistd.h>
+#include "cpumap.h"
 #include "debug.h"
 #include "evsel.h"
 #include "pmus.h"
@@ -268,7 +269,7 @@ struct perf_pmu *perf_pmus__scan_core(struct perf_pmu *pmu)
 {
 	if (!pmu) {
 		pmu_read_sysfs(/*core_only=*/true);
-		pmu = list_prepare_entry(pmu, &core_pmus, list);
+		return list_first_entry_or_null(&core_pmus, typeof(*pmu), list);
 	}
 	list_for_each_entry_continue(pmu, &core_pmus, list)
 		return pmu;
@@ -592,3 +593,20 @@ struct perf_pmu *evsel__find_pmu(const struct evsel *evsel)
 	}
 	return pmu;
 }
+
+struct perf_pmu *perf_pmus__find_core_pmu(void)
+{
+	struct perf_pmu *pmu = NULL;
+
+	while ((pmu = perf_pmus__scan_core(pmu))) {
+		/*
+		 * The cpumap should cover all CPUs. Otherwise, some CPUs may
+		 * not support some events or have different event IDs.
+		 */
+		if (RC_CHK_ACCESS(pmu->cpus)->nr != cpu__max_cpu().cpu)
+			return NULL;
+
+		return pmu;
+	}
+	return NULL;
+}
-- 
2.34.1

Re: [PATCH v3 1/3] perf pmu: Move pmu__find_core_pmu() to pmus.c

[Ian Rogers]

On Wed, Sep 13, 2023 at 8:34 AM James Clark <james.clark@arm.com> wrote:
>
> pmu__find_core_pmu() more logically belongs in pmus.c because it
> iterates over all PMUs, so move it to pmus.c
>
> At the same time rename it to perf_pmus__find_core_pmu() to match the
> naming convention in this file.
>
> list_prepare_entry() can't be used in perf_pmus__scan_core() anymore now
> that it's called from the same compilation unit. This is with -O2
> (specifically -O1 -ftree-vrp -finline-functions
> -finline-small-functions) which allow the bounds of the array
> access to be determined at compile time. list_prepare_entry() subtracts
> the offset of the 'list' member in struct perf_pmu from &core_pmus,
> which isn't a struct perf_pmu. The compiler sees that pmu results in
> &core_pmus - 8 and refuses to compile. At runtime this works because
> list_for_each_entry_continue() always adds the offset back again before
> dereferencing ->next, but it's technically undefined behavior. With
> -fsanitize=undefined an additional warning is generated.
>
> Using list_first_entry_or_null() to get the first entry here avoids
> doing &core_pmus - 8 but has the same result and fixes both the compile
> warning and the undefined behavior warning. There are other uses of
> list_prepare_entry() in pmus.c, but the compiler doesn't seem to be
> able to see that they can also be called with &core_pmus, so I won't
> change any at this time.
>
> Signed-off-by: James Clark <james.clark@arm.com>

Reviewed-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/perf/arch/arm64/util/pmu.c |  6 +++---
>  tools/perf/tests/expr.c          |  2 +-
>  tools/perf/util/expr.c           |  2 +-
>  tools/perf/util/pmu.c            | 17 -----------------
>  tools/perf/util/pmu.h            |  2 +-
>  tools/perf/util/pmus.c           | 20 +++++++++++++++++++-
>  6 files changed, 25 insertions(+), 24 deletions(-)
>
> diff --git a/tools/perf/arch/arm64/util/pmu.c b/tools/perf/arch/arm64/util/pmu.c
> index 615084eb88d8..3d9330feebd2 100644
> --- a/tools/perf/arch/arm64/util/pmu.c
> +++ b/tools/perf/arch/arm64/util/pmu.c
> @@ -10,7 +10,7 @@
>
>  const struct pmu_metrics_table *pmu_metrics_table__find(void)
>  {
> -       struct perf_pmu *pmu = pmu__find_core_pmu();
> +       struct perf_pmu *pmu = perf_pmus__find_core_pmu();
>
>         if (pmu)
>                 return perf_pmu__find_metrics_table(pmu);
> @@ -20,7 +20,7 @@ const struct pmu_metrics_table *pmu_metrics_table__find(void)
>
>  const struct pmu_events_table *pmu_events_table__find(void)
>  {
> -       struct perf_pmu *pmu = pmu__find_core_pmu();
> +       struct perf_pmu *pmu = perf_pmus__find_core_pmu();
>
>         if (pmu)
>                 return perf_pmu__find_events_table(pmu);
> @@ -32,7 +32,7 @@ double perf_pmu__cpu_slots_per_cycle(void)
>  {
>         char path[PATH_MAX];
>         unsigned long long slots = 0;
> -       struct perf_pmu *pmu = pmu__find_core_pmu();
> +       struct perf_pmu *pmu = perf_pmus__find_core_pmu();
>
>         if (pmu) {
>                 perf_pmu__pathname_scnprintf(path, sizeof(path),
> diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
> index 78544092ef0c..e3aa9d4fcf3a 100644
> --- a/tools/perf/tests/expr.c
> +++ b/tools/perf/tests/expr.c
> @@ -76,7 +76,7 @@ static int test__expr(struct test_suite *t __maybe_unused, int subtest __maybe_u
>         struct expr_parse_ctx *ctx;
>         bool is_intel = false;
>         char strcmp_cpuid_buf[256];
> -       struct perf_pmu *pmu = pmu__find_core_pmu();
> +       struct perf_pmu *pmu = perf_pmus__find_core_pmu();
>         char *cpuid = perf_pmu__getcpuid(pmu);
>         char *escaped_cpuid1, *escaped_cpuid2;
>
> diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
> index 4488f306de78..7be23b3ac082 100644
> --- a/tools/perf/util/expr.c
> +++ b/tools/perf/util/expr.c
> @@ -509,7 +509,7 @@ double expr__strcmp_cpuid_str(const struct expr_parse_ctx *ctx __maybe_unused,
>                        bool compute_ids __maybe_unused, const char *test_id)
>  {
>         double ret;
> -       struct perf_pmu *pmu = pmu__find_core_pmu();
> +       struct perf_pmu *pmu = perf_pmus__find_core_pmu();
>         char *cpuid = perf_pmu__getcpuid(pmu);
>
>         if (!cpuid)
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index e2159854ab26..f50a5636633f 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -2050,20 +2050,3 @@ void perf_pmu__delete(struct perf_pmu *pmu)
>         zfree(&pmu->id);
>         free(pmu);
>  }
> -
> -struct perf_pmu *pmu__find_core_pmu(void)
> -{
> -       struct perf_pmu *pmu = NULL;
> -
> -       while ((pmu = perf_pmus__scan_core(pmu))) {
> -               /*
> -                * The cpumap should cover all CPUs. Otherwise, some CPUs may
> -                * not support some events or have different event IDs.
> -                */
> -               if (RC_CHK_ACCESS(pmu->cpus)->nr != cpu__max_cpu().cpu)
> -                       return NULL;
> -
> -               return pmu;
> -       }
> -       return NULL;
> -}
> diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
> index bd5d804a6736..d7b46085642d 100644
> --- a/tools/perf/util/pmu.h
> +++ b/tools/perf/util/pmu.h
> @@ -264,6 +264,6 @@ int perf_pmu__pathname_fd(int dirfd, const char *pmu_name, const char *filename,
>  struct perf_pmu *perf_pmu__lookup(struct list_head *pmus, int dirfd, const char *lookup_name);
>  struct perf_pmu *perf_pmu__create_placeholder_core_pmu(struct list_head *core_pmus);
>  void perf_pmu__delete(struct perf_pmu *pmu);
> -struct perf_pmu *pmu__find_core_pmu(void);
> +struct perf_pmu *perf_pmus__find_core_pmu(void);
>
>  #endif /* __PMU_H */
> diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
> index 6631367c756f..cec869cbe163 100644
> --- a/tools/perf/util/pmus.c
> +++ b/tools/perf/util/pmus.c
> @@ -10,6 +10,7 @@
>  #include <pthread.h>
>  #include <string.h>
>  #include <unistd.h>
> +#include "cpumap.h"
>  #include "debug.h"
>  #include "evsel.h"
>  #include "pmus.h"
> @@ -268,7 +269,7 @@ struct perf_pmu *perf_pmus__scan_core(struct perf_pmu *pmu)
>  {
>         if (!pmu) {
>                 pmu_read_sysfs(/*core_only=*/true);
> -               pmu = list_prepare_entry(pmu, &core_pmus, list);
> +               return list_first_entry_or_null(&core_pmus, typeof(*pmu), list);
>         }
>         list_for_each_entry_continue(pmu, &core_pmus, list)
>                 return pmu;
> @@ -592,3 +593,20 @@ struct perf_pmu *evsel__find_pmu(const struct evsel *evsel)
>         }
>         return pmu;
>  }
> +
> +struct perf_pmu *perf_pmus__find_core_pmu(void)
> +{
> +       struct perf_pmu *pmu = NULL;
> +
> +       while ((pmu = perf_pmus__scan_core(pmu))) {
> +               /*
> +                * The cpumap should cover all CPUs. Otherwise, some CPUs may
> +                * not support some events or have different event IDs.
> +                */
> +               if (RC_CHK_ACCESS(pmu->cpus)->nr != cpu__max_cpu().cpu)
> +                       return NULL;
> +
> +               return pmu;
> +       }
> +       return NULL;
> +}
> --
> 2.34.1
>

Re: [PATCH v3 1/3] perf pmu: Move pmu__find_core_pmu() to pmus.c

[John Garry]

On 13/09/2023 16:33, James Clark wrote:
> pmu__find_core_pmu() more logically belongs in pmus.c because it
> iterates over all PMUs, so move it to pmus.c
> 
> At the same time rename it to perf_pmus__find_core_pmu() to match the
> naming convention in this file.
> 
> list_prepare_entry() can't be used in perf_pmus__scan_core() anymore now
> that it's called from the same compilation unit. This is with -O2
> (specifically -O1 -ftree-vrp -finline-functions
> -finline-small-functions) which allow the bounds of the array
> access to be determined at compile time. list_prepare_entry() subtracts
> the offset of the 'list' member in struct perf_pmu from &core_pmus,
> which isn't a struct perf_pmu. The compiler sees that pmu results in
> &core_pmus - 8 and refuses to compile. At runtime this works because
> list_for_each_entry_continue() always adds the offset back again before
> dereferencing ->next, but it's technically undefined behavior. With
> -fsanitize=undefined an additional warning is generated.
> 
> Using list_first_entry_or_null() to get the first entry here avoids
> doing &core_pmus - 8 but has the same result and fixes both the compile
> warning and the undefined behavior warning. There are other uses of
> list_prepare_entry() in pmus.c, but the compiler doesn't seem to be
> able to see that they can also be called with &core_pmus, so I won't
> change any at this time.
> 
> Signed-off-by: James Clark<james.clark@arm.com>
> ---


Reviewed-by: John Garry <john.g.garry@oracle.com>

[PATCH v3 2/3] perf pmus: Simplify perf_pmus__find_core_pmu()

[James Clark]

Currently the while loop always either exits on the first iteration with
a core PMU, or exits with NULL on heterogeneous systems or when not all
CPUs are online.

Both of the latter behaviors are undesirable for platforms other than
Arm so simplify it to always return the first core PMU, or NULL if none
exist.

This behavior was depended on by the Arm version of
pmu_metrics_table__find(), so the logic has been moved there instead.

Suggested-by: Ian Rogers <irogers@google.com>
Reviewed-by: Ian Rogers <irogers@google.com>
Signed-off-by: James Clark <james.clark@arm.com>
---
 tools/perf/arch/arm64/util/pmu.c |  8 +++++++-
 tools/perf/util/pmus.c           | 14 +-------------
 2 files changed, 8 insertions(+), 14 deletions(-)

diff --git a/tools/perf/arch/arm64/util/pmu.c b/tools/perf/arch/arm64/util/pmu.c
index 3d9330feebd2..3099f5f448ba 100644
--- a/tools/perf/arch/arm64/util/pmu.c
+++ b/tools/perf/arch/arm64/util/pmu.c
@@ -10,8 +10,14 @@
 
 const struct pmu_metrics_table *pmu_metrics_table__find(void)
 {
-	struct perf_pmu *pmu = perf_pmus__find_core_pmu();
+	struct perf_pmu *pmu;
+
+	/* Metrics aren't currently supported on heterogeneous Arm systems */
+	if (perf_pmus__num_core_pmus() > 1)
+		return NULL;
 
+	/* Doesn't matter which one here because they'll all be the same */
+	pmu = perf_pmus__find_core_pmu();
 	if (pmu)
 		return perf_pmu__find_metrics_table(pmu);
 
diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
index cec869cbe163..64e798e68a2d 100644
--- a/tools/perf/util/pmus.c
+++ b/tools/perf/util/pmus.c
@@ -596,17 +596,5 @@ struct perf_pmu *evsel__find_pmu(const struct evsel *evsel)
 
 struct perf_pmu *perf_pmus__find_core_pmu(void)
 {
-	struct perf_pmu *pmu = NULL;
-
-	while ((pmu = perf_pmus__scan_core(pmu))) {
-		/*
-		 * The cpumap should cover all CPUs. Otherwise, some CPUs may
-		 * not support some events or have different event IDs.
-		 */
-		if (RC_CHK_ACCESS(pmu->cpus)->nr != cpu__max_cpu().cpu)
-			return NULL;
-
-		return pmu;
-	}
-	return NULL;
+	return perf_pmus__scan_core(NULL);
 }
-- 
2.34.1

Re: [PATCH v3 2/3] perf pmus: Simplify perf_pmus__find_core_pmu()

[John Garry]

On 13/09/2023 16:33, James Clark wrote:
> Currently the while loop always either exits on the first iteration with
> a core PMU, or exits with NULL on heterogeneous systems or when not all
> CPUs are online.
> 
> Both of the latter behaviors are undesirable for platforms other than
> Arm so simplify it to always return the first core PMU, or NULL if none
> exist.
> 
> This behavior was depended on by the Arm version of
> pmu_metrics_table__find(), so the logic has been moved there instead.
> 
> Suggested-by: Ian Rogers <irogers@google.com>
> Reviewed-by: Ian Rogers <irogers@google.com>
> Signed-off-by: James Clark <james.clark@arm.com>

Reviewed-by: John Garry <john.g.garry@oracle.com>

Re: [PATCH v3 2/3] perf pmus: Simplify perf_pmus__find_core_pmu()

[James Clark]

On 13/09/2023 16:33, James Clark wrote:
> Currently the while loop always either exits on the first iteration with
> a core PMU, or exits with NULL on heterogeneous systems or when not all
> CPUs are online.
> 
> Both of the latter behaviors are undesirable for platforms other than
> Arm so simplify it to always return the first core PMU, or NULL if none
> exist.
> 
> This behavior was depended on by the Arm version of
> pmu_metrics_table__find(), so the logic has been moved there instead.
> 
> Suggested-by: Ian Rogers <irogers@google.com>
> Reviewed-by: Ian Rogers <irogers@google.com>
> Signed-off-by: James Clark <james.clark@arm.com>

Turns out the "Simple expression parser" test is failing on
heterogeneous arm systems without this patch. I didn't realise there was
a dependency and should have put the commits the other way round. I will
leave the error message here in case someone bumps into it, but no fix
is required apart from applying the remaining patches in this set:

 $ perf test expr -v
  4: Simple expression parser                                        :
 --- start ---
 test child forked, pid 4902
 Using CPUID 0x00000000410fd070
 FAILED tests/expr.c:83 get_cpuid
 test child finished with -1
 ---- end ----
 Simple expression parser: FAILED!


> ---
>  tools/perf/arch/arm64/util/pmu.c |  8 +++++++-
>  tools/perf/util/pmus.c           | 14 +-------------
>  2 files changed, 8 insertions(+), 14 deletions(-)
> 
> diff --git a/tools/perf/arch/arm64/util/pmu.c b/tools/perf/arch/arm64/util/pmu.c
> index 3d9330feebd2..3099f5f448ba 100644
> --- a/tools/perf/arch/arm64/util/pmu.c
> +++ b/tools/perf/arch/arm64/util/pmu.c
> @@ -10,8 +10,14 @@
>  
>  const struct pmu_metrics_table *pmu_metrics_table__find(void)
>  {
> -	struct perf_pmu *pmu = perf_pmus__find_core_pmu();
> +	struct perf_pmu *pmu;
> +
> +	/* Metrics aren't currently supported on heterogeneous Arm systems */
> +	if (perf_pmus__num_core_pmus() > 1)
> +		return NULL;
>  
> +	/* Doesn't matter which one here because they'll all be the same */
> +	pmu = perf_pmus__find_core_pmu();
>  	if (pmu)
>  		return perf_pmu__find_metrics_table(pmu);
>  
> diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
> index cec869cbe163..64e798e68a2d 100644
> --- a/tools/perf/util/pmus.c
> +++ b/tools/perf/util/pmus.c
> @@ -596,17 +596,5 @@ struct perf_pmu *evsel__find_pmu(const struct evsel *evsel)
>  
>  struct perf_pmu *perf_pmus__find_core_pmu(void)
>  {
> -	struct perf_pmu *pmu = NULL;
> -
> -	while ((pmu = perf_pmus__scan_core(pmu))) {
> -		/*
> -		 * The cpumap should cover all CPUs. Otherwise, some CPUs may
> -		 * not support some events or have different event IDs.
> -		 */
> -		if (RC_CHK_ACCESS(pmu->cpus)->nr != cpu__max_cpu().cpu)
> -			return NULL;
> -
> -		return pmu;
> -	}
> -	return NULL;
> +	return perf_pmus__scan_core(NULL);
>  }

[PATCH v3 3/3] perf pmu: Remove unused function

[James Clark]

pmu_events_table__find() is no longer used so remove it and its Arm
specific version.

Reviewed-by: Ian Rogers <irogers@google.com>
Signed-off-by: James Clark <james.clark@arm.com>
---
 tools/perf/arch/arm64/util/pmu.c | 10 ----------
 tools/perf/util/pmu.c            |  5 -----
 tools/perf/util/pmu.h            |  1 -
 3 files changed, 16 deletions(-)

diff --git a/tools/perf/arch/arm64/util/pmu.c b/tools/perf/arch/arm64/util/pmu.c
index 3099f5f448ba..2a4eab2d160e 100644
--- a/tools/perf/arch/arm64/util/pmu.c
+++ b/tools/perf/arch/arm64/util/pmu.c
@@ -24,16 +24,6 @@ const struct pmu_metrics_table *pmu_metrics_table__find(void)
 	return NULL;
 }
 
-const struct pmu_events_table *pmu_events_table__find(void)
-{
-	struct perf_pmu *pmu = perf_pmus__find_core_pmu();
-
-	if (pmu)
-		return perf_pmu__find_events_table(pmu);
-
-	return NULL;
-}
-
 double perf_pmu__cpu_slots_per_cycle(void)
 {
 	char path[PATH_MAX];
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index f50a5636633f..0d81c059c91c 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -776,11 +776,6 @@ char *perf_pmu__getcpuid(struct perf_pmu *pmu)
 	return cpuid;
 }
 
-__weak const struct pmu_events_table *pmu_events_table__find(void)
-{
-	return perf_pmu__find_events_table(NULL);
-}
-
 __weak const struct pmu_metrics_table *pmu_metrics_table__find(void)
 {
 	return perf_pmu__find_metrics_table(NULL);
diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
index d7b46085642d..04b317b17d66 100644
--- a/tools/perf/util/pmu.h
+++ b/tools/perf/util/pmu.h
@@ -238,7 +238,6 @@ void pmu_add_cpu_aliases_table(struct perf_pmu *pmu,
 			       const struct pmu_events_table *table);
 
 char *perf_pmu__getcpuid(struct perf_pmu *pmu);
-const struct pmu_events_table *pmu_events_table__find(void);
 const struct pmu_metrics_table *pmu_metrics_table__find(void);
 
 int perf_pmu__convert_scale(const char *scale, char **end, double *sval);
-- 
2.34.1

Re: [PATCH v3 3/3] perf pmu: Remove unused function

[John Garry]

On 13/09/2023 16:33, James Clark wrote:
> pmu_events_table__find() is no longer used so remove it and its Arm
> specific version.
> 
> Reviewed-by: Ian Rogers <irogers@google.com>
> Signed-off-by: James Clark <james.clark@arm.com>
> ---

You really should put the name of the function in the subject. Apart 
from that:

Reviewed-by: John Garry <john.g.garry@oracle.com>

>   tools/perf/arch/arm64/util/pmu.c | 10 ----------
>   tools/perf/util/pmu.c            |  5 -----
>   tools/perf/util/pmu.h            |  1 -
>   3 files changed, 16 deletions(-)
> 
> diff --git a/tools/perf/arch/arm64/util/pmu.c b/tools/perf/arch/arm64/util/pmu.c
> index 3099f5f448ba..2a4eab2d160e 100644
> --- a/tools/perf/arch/arm64/util/pmu.c
> +++ b/tools/perf/arch/arm64/util/pmu.c
> @@ -24,16 +24,6 @@ const struct pmu_metrics_table *pmu_metrics_table__find(void)
>   	return NULL;
>   }
>   
> -const struct pmu_events_table *pmu_events_table__find(void)
> -{
> -	struct perf_pmu *pmu = perf_pmus__find_core_pmu();
> -
> -	if (pmu)
> -		return perf_pmu__find_events_table(pmu);
> -
> -	return NULL;
> -}
> -
>   double perf_pmu__cpu_slots_per_cycle(void)
>   {
>   	char path[PATH_MAX];
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index f50a5636633f..0d81c059c91c 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -776,11 +776,6 @@ char *perf_pmu__getcpuid(struct perf_pmu *pmu)
>   	return cpuid;
>   }
>   
> -__weak const struct pmu_events_table *pmu_events_table__find(void)
> -{
> -	return perf_pmu__find_events_table(NULL);
> -}
> -
>   __weak const struct pmu_metrics_table *pmu_metrics_table__find(void)
>   {
>   	return perf_pmu__find_metrics_table(NULL);
> diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
> index d7b46085642d..04b317b17d66 100644
> --- a/tools/perf/util/pmu.h
> +++ b/tools/perf/util/pmu.h
> @@ -238,7 +238,6 @@ void pmu_add_cpu_aliases_table(struct perf_pmu *pmu,
>   			       const struct pmu_events_table *table);
>   
>   char *perf_pmu__getcpuid(struct perf_pmu *pmu);
> -const struct pmu_events_table *pmu_events_table__find(void);
>   const struct pmu_metrics_table *pmu_metrics_table__find(void);
>   
>   int perf_pmu__convert_scale(const char *scale, char **end, double *sval);

Re: [PATCH v3 3/3] perf pmu: Remove unused function

[James Clark]

On 14/09/2023 07:41, John Garry wrote:
> On 13/09/2023 16:33, James Clark wrote:
>> pmu_events_table__find() is no longer used so remove it and its Arm
>> specific version.
>>
>> Reviewed-by: Ian Rogers <irogers@google.com>
>> Signed-off-by: James Clark <james.clark@arm.com>
>> ---
> 
> You really should put the name of the function in the subject. Apart
> from that:
> 
> Reviewed-by: John Garry <john.g.garry@oracle.com>
> 

Noted, will do next time. Thanks for the reviews.

>>   tools/perf/arch/arm64/util/pmu.c | 10 ----------
>>   tools/perf/util/pmu.c            |  5 -----
>>   tools/perf/util/pmu.h            |  1 -
>>   3 files changed, 16 deletions(-)
>>
>> diff --git a/tools/perf/arch/arm64/util/pmu.c
>> b/tools/perf/arch/arm64/util/pmu.c
>> index 3099f5f448ba..2a4eab2d160e 100644
>> --- a/tools/perf/arch/arm64/util/pmu.c
>> +++ b/tools/perf/arch/arm64/util/pmu.c
>> @@ -24,16 +24,6 @@ const struct pmu_metrics_table
>> *pmu_metrics_table__find(void)
>>       return NULL;
>>   }
>>   -const struct pmu_events_table *pmu_events_table__find(void)
>> -{
>> -    struct perf_pmu *pmu = perf_pmus__find_core_pmu();
>> -
>> -    if (pmu)
>> -        return perf_pmu__find_events_table(pmu);
>> -
>> -    return NULL;
>> -}
>> -
>>   double perf_pmu__cpu_slots_per_cycle(void)
>>   {
>>       char path[PATH_MAX];
>> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
>> index f50a5636633f..0d81c059c91c 100644
>> --- a/tools/perf/util/pmu.c
>> +++ b/tools/perf/util/pmu.c
>> @@ -776,11 +776,6 @@ char *perf_pmu__getcpuid(struct perf_pmu *pmu)
>>       return cpuid;
>>   }
>>   -__weak const struct pmu_events_table *pmu_events_table__find(void)
>> -{
>> -    return perf_pmu__find_events_table(NULL);
>> -}
>> -
>>   __weak const struct pmu_metrics_table *pmu_metrics_table__find(void)
>>   {
>>       return perf_pmu__find_metrics_table(NULL);
>> diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
>> index d7b46085642d..04b317b17d66 100644
>> --- a/tools/perf/util/pmu.h
>> +++ b/tools/perf/util/pmu.h
>> @@ -238,7 +238,6 @@ void pmu_add_cpu_aliases_table(struct perf_pmu *pmu,
>>                      const struct pmu_events_table *table);
>>     char *perf_pmu__getcpuid(struct perf_pmu *pmu);
>> -const struct pmu_events_table *pmu_events_table__find(void);
>>   const struct pmu_metrics_table *pmu_metrics_table__find(void);
>>     int perf_pmu__convert_scale(const char *scale, char **end, double
>> *sval);
> 

Re: [PATCH v3 0/3] perf: strcmp_cpuid_str() expression fixups

[Namhyung Kim]

Hello,

On Wed, Sep 13, 2023 at 8:34 AM James Clark <james.clark@arm.com> wrote:
>
> Set of fixes related to the comments here [1]. Mainly cleanups,
> additional tests and refactoring since adding the new strcmp_cpuid_str()
> metric expression.
>
> I added the string replace function to the perf utils
> rather than tools/lib/string.c because it didn't seem
> easy to add tests for tools/lib.
>
> [1]: https://lore.kernel.org/linux-arm-kernel/CAP-5=fVnUx0BnJC7X1rrm42OD7Bk=ZsHWNwAZMBYyB7yWhBfhQ@mail.gmail.com/
> [2]: https://lore.kernel.org/linux-perf-users/ZQC7da2AM9ih8RMz@kernel.org/
>
> ---
> Changes since v2:
>   * Drop patches that were already applied (which makes the cover letter
>     mostly redundant)
>   * Avoid generating the compiler warning reported here [2]
>
> Changes since v1:
>   * s -> haystack
>   * find -> needle
>
> James Clark (3):
>   perf pmu: Move pmu__find_core_pmu() to pmus.c
>   perf pmus: Simplify perf_pmus__find_core_pmu()
>   perf pmu: Remove unused function

Applied to perf-tools-next, thanks!