Re: [PATCH 1/5] perf metricgroup: Support printing metrics for arm64

[John Garry]

Hi Jirka,

> > -	struct pmu_events_map *map = perf_pmu__find_map(NULL);
> > +	struct pmu_events_map *map = find_cpumap();
> so this is just for arm at the moment right?

Yes - but to be more accurate, arm64.

At the moment, from the archs which use pmu-events, only arm64 and nds32 
have versions of get_cpuid_str() which require a non-NULL pmu argument.

But then apparently nds32 only supports a single CPU, so this issue of 
heterogeneous CPUs should not be a concern there :)

> could we rather make this arch specific code, so we don't need
> to do the scanning on archs where this is not needed?
>
> like marking perf_pmu__find_map as __weak and add arm specific
> version?

Well I was thinking that this code should not be in metricgroup.c anyway.

So there is code which is common in current perf_pmu__find_map() for all 
archs.

I could factor that out into a common function, below. Just a bit 
worried about perf_pmu__find_map() and perf_pmu__find_pmu_map() being 
confused.

Here's how that would look:

+++ b/tools/perf/arch/arm64/util/pmu.c

#include "../../util/cpumap.h"
#include "../../util/pmu.h"

struct pmu_events_map *perf_pmu__find_map(void)
{
	struct perf_pmu *pmu = perf_pmu__find("armv8_pmuv3_0");

	if (!pmu || !pmu->cpus || pmu->cpus->nr != cpu__max_cpu())
		return NULL;

	return perf_pmu__find_pmu_map(pmu);
}

And:

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 26c990e32378..312164ce9299 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -618,7 +618,7 @@ static int metricgroup__print_sys_event_iter(struct 
pmu_event *pe, void *data)
 void metricgroup__print(bool metrics, bool metricgroups, char *filter,
 			bool raw, bool details)
 {
-	struct pmu_events_map *map = perf_pmu__find_map(NULL);
+	struct pmu_events_map *map = perf_pmu__find_map();
 	struct pmu_event *pe;
 	int i;
 	struct rblist groups;
@@ -1253,8 +1253,7 @@ int metricgroup__parse_groups(const struct option 
*opt,
 			      struct rblist *metric_events)
 {
 	struct evlist *perf_evlist = *(struct evlist **)opt->value;
-	struct pmu_events_map *map = perf_pmu__find_map(NULL);
-
+	struct pmu_events_map *map = perf_pmu__find_map();

 	return parse_groups(perf_evlist, str, metric_no_group,
 			    metric_no_merge, NULL, metric_events, map);
@@ -1273,7 +1272,7 @@ int metricgroup__parse_groups_test(struct evlist 
*evlist,

 bool metricgroup__has_metric(const char *metric)
 {
-	struct pmu_events_map *map = perf_pmu__find_map(NULL);
+	struct pmu_events_map *map = perf_pmu__find_map();
 	struct pmu_event *pe;
 	int i;

diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 44ef28302fc7..d49bf20b6058 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -690,7 +690,7 @@ static char *perf_pmu__getcpuid(struct perf_pmu *pmu)
 	return cpuid;
 }

-struct pmu_events_map *perf_pmu__find_map(struct perf_pmu *pmu)
+struct pmu_events_map *perf_pmu__find_pmu_map(struct perf_pmu *pmu)
 {
 	struct pmu_events_map *map;
 	char *cpuid = perf_pmu__getcpuid(pmu);
@@ -717,6 +717,11 @@ struct pmu_events_map *perf_pmu__find_map(struct 
perf_pmu *pmu)
 	return map;
 }

+struct pmu_events_map *__weak perf_pmu__find_map(void)
+{
+	return perf_pmu__find_pmu_map(NULL);
+}
+
 bool pmu_uncore_alias_match(const char *pmu_name, const char *name)
 {
 	char *tmp = NULL, *tok, *str;
@@ -805,7 +810,7 @@ static void pmu_add_cpu_aliases(struct list_head 
*head, struct perf_pmu *pmu)
 {
 	struct pmu_events_map *map;

-	map = perf_pmu__find_map(pmu);
+	map = perf_pmu__find_pmu_map(pmu);
 	if (!map)
 		return;


Thoughts?

Thanks!