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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YEPZNssS200w3Axy@krava>
Date:   Sat, 6 Mar 2021 20:34:14 +0100
From:   Jiri Olsa <jolsa@...hat.com>
To:     John Garry <john.garry@...wei.com>
Cc:     will@...nel.org, mathieu.poirier@...aro.org, leo.yan@...aro.org,
        peterz@...radead.org, mingo@...hat.com, acme@...nel.org,
        mark.rutland@....com, alexander.shishkin@...ux.intel.com,
        namhyung@...nel.org, irogers@...gle.com,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linuxarm@...wei.com, zhangshaokun@...ilicon.com,
        qiangqing.zhang@....com, kjain@...ux.ibm.com
Subject: Re: [PATCH 1/5] perf metricgroup: Support printing metrics for arm64

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!
>