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: <95205463-4c80-4e8a-a7c0-c2a4e4553838@huawei.com>
Date:   Fri, 5 Mar 2021 11:06:58 +0000
From:   John Garry <john.garry@...wei.com>
To:     Jiri Olsa <jolsa@...hat.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


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!