On Fri, Mar 05, 2021 at 11:06:58AM +0000, John Garry wrote:
>
> 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.
right, so perf_pmu__find_map does not take perf_pmu as argument
anymore, so the prefix does not fit, how about pmu_events_map__find ?
thanks,
jirka
>
> 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!
>
