[PATCH v4 0/4] Add support for ThunderX2 pmu events using json files

[PATCH v4 0/4] Add support for ThunderX2 pmu events using json files

[Ganapatrao Kulkarni]

Extending json/jevent framework for parsing arm64 event files.
Adding jevents for ThunderX2 implementation defined PMU events.

v4:
   - Rebased to 4.13-rc1

v3:
   - Addressed comments from Will Deacon and Jayachandran C.
   - Rebased to 4.12-rc1

v2:
   - Updated as per Mark Rutland's suggestions.
   - Added provision for get_cpuid_str to get cpu id string
     from associated cpus of pmu core device.

v1: Initial patchset.

Ganapatrao Kulkarni (4):
  perf utils: passing pmu as a parameter to function get_cpuid_str
  perf tools arm64: Add support for get_cpuid_str function.
  perf utils: Add helper function is_pmu_core to detect PMU CORE devices
  perf vendor events arm64: Add ThunderX2 implementation defined pmu
    core events

 tools/perf/arch/arm64/util/Build                   |  1 +
 tools/perf/arch/arm64/util/header.c                | 59 ++++++++++++++++++++
 tools/perf/arch/powerpc/util/header.c              |  2 +-
 tools/perf/arch/x86/util/header.c                  |  2 +-
 tools/perf/pmu-events/arch/arm64/mapfile.csv       | 15 ++++++
 .../arm64/thunderx2/implementation-defined.json    | 62 ++++++++++++++++++++++
 tools/perf/util/header.h                           |  3 +-
 tools/perf/util/pmu.c                              | 53 +++++++++++++++---
 8 files changed, 186 insertions(+), 11 deletions(-)
 create mode 100644 tools/perf/arch/arm64/util/header.c
 create mode 100644 tools/perf/pmu-events/arch/arm64/mapfile.csv
 create mode 100644 tools/perf/pmu-events/arch/arm64/thunderx2/implementation-defined.json

-- 
2.9.4

[PATCH v4 1/4] perf utils: passing pmu as a parameter to function get_cpuid_str

[Ganapatrao Kulkarni]

cpuid string will not be same on all CPUs on heterogeneous
platforms like ARM's big.LITTLE, adding provision(using pmu->cpus)
to find cpuid string from associated CPUs of PMU CORE device.

Signed-off-by: Ganapatrao Kulkarni <ganapatrao.kulkarni@cavium.com>
---
 tools/perf/arch/powerpc/util/header.c | 2 +-
 tools/perf/arch/x86/util/header.c     | 2 +-
 tools/perf/util/header.h              | 3 ++-
 tools/perf/util/pmu.c                 | 9 +++++----
 4 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/tools/perf/arch/powerpc/util/header.c b/tools/perf/arch/powerpc/util/header.c
index 9aaa6f5..2953681 100644
--- a/tools/perf/arch/powerpc/util/header.c
+++ b/tools/perf/arch/powerpc/util/header.c
@@ -34,7 +34,7 @@ get_cpuid(char *buffer, size_t sz)
 }
 
 char *
-get_cpuid_str(void)
+get_cpuid_str(struct perf_pmu *pmu __maybe_unused)
 {
 	char *bufp;
 
diff --git a/tools/perf/arch/x86/util/header.c b/tools/perf/arch/x86/util/header.c
index a74a48d..d52bc27 100644
--- a/tools/perf/arch/x86/util/header.c
+++ b/tools/perf/arch/x86/util/header.c
@@ -65,7 +65,7 @@ get_cpuid(char *buffer, size_t sz)
 }
 
 char *
-get_cpuid_str(void)
+get_cpuid_str(struct perf_pmu *pmu __maybe_unused)
 {
 	char *buf = malloc(128);
 
diff --git a/tools/perf/util/header.h b/tools/perf/util/header.h
index d30109b..05e5758 100644
--- a/tools/perf/util/header.h
+++ b/tools/perf/util/header.h
@@ -8,6 +8,7 @@
 #include <linux/types.h>
 #include "event.h"
 #include "env.h"
+#include "pmu.h"
 
 enum {
 	HEADER_RESERVED		= 0,	/* always cleared */
@@ -151,5 +152,5 @@ int write_padded(int fd, const void *bf, size_t count, size_t count_aligned);
  */
 int get_cpuid(char *buffer, size_t sz);
 
-char *get_cpuid_str(void);
+char *get_cpuid_str(struct perf_pmu *pmu __maybe_unused);
 #endif /* __PERF_HEADER_H */
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index ac16a9d..aefdbd1 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -511,7 +511,7 @@ static struct cpu_map *pmu_cpumask(const char *name)
  * Each architecture should provide a more precise id string that
  * can be use to match the architecture's "mapfile".
  */
-char * __weak get_cpuid_str(void)
+char * __weak get_cpuid_str(struct perf_pmu *pmu __maybe_unused)
 {
 	return NULL;
 }
@@ -521,7 +521,8 @@ char * __weak get_cpuid_str(void)
  * to the current running CPU. Then, add all PMU events from that table
  * as aliases.
  */
-static void pmu_add_cpu_aliases(struct list_head *head, const char *name)
+static void pmu_add_cpu_aliases(struct list_head *head, const char *name,
+		struct perf_pmu *pmu)
 {
 	int i;
 	struct pmu_events_map *map;
@@ -533,7 +534,7 @@ static void pmu_add_cpu_aliases(struct list_head *head, const char *name)
 	if (cpuid)
 		cpuid = strdup(cpuid);
 	if (!cpuid)
-		cpuid = get_cpuid_str();
+		cpuid = get_cpuid_str(pmu);
 	if (!cpuid)
 		return;
 
@@ -610,12 +611,12 @@ static struct perf_pmu *pmu_lookup(const char *name)
 	if (pmu_aliases(name, &aliases))
 		return NULL;
 
-	pmu_add_cpu_aliases(&aliases, name);
 	pmu = zalloc(sizeof(*pmu));
 	if (!pmu)
 		return NULL;
 
 	pmu->cpus = pmu_cpumask(name);
+	pmu_add_cpu_aliases(&aliases, name, pmu);
 
 	INIT_LIST_HEAD(&pmu->format);
 	INIT_LIST_HEAD(&pmu->aliases);
-- 
2.9.4

[PATCH v4 2/4] perf tools arm64: Add support for get_cpuid_str function.

[Ganapatrao Kulkarni]

function get_cpuid_str returns MIDR string of the first online
cpu from the range of cpus associated with the pmu core device.

Signed-off-by: Ganapatrao Kulkarni <ganapatrao.kulkarni@cavium.com>
---
 tools/perf/arch/arm64/util/Build    |  1 +
 tools/perf/arch/arm64/util/header.c | 59 +++++++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+)
 create mode 100644 tools/perf/arch/arm64/util/header.c

diff --git a/tools/perf/arch/arm64/util/Build b/tools/perf/arch/arm64/util/Build
index cef6fb3..b1ab72d 100644
--- a/tools/perf/arch/arm64/util/Build
+++ b/tools/perf/arch/arm64/util/Build
@@ -1,3 +1,4 @@
+libperf-y += header.o
 libperf-$(CONFIG_DWARF)     += dwarf-regs.o
 libperf-$(CONFIG_LOCAL_LIBUNWIND) += unwind-libunwind.o
 
diff --git a/tools/perf/arch/arm64/util/header.c b/tools/perf/arch/arm64/util/header.c
new file mode 100644
index 0000000..4e25498
--- /dev/null
+++ b/tools/perf/arch/arm64/util/header.c
@@ -0,0 +1,59 @@
+#include <stdio.h>
+#include <stdlib.h>
+#include <api/fs/fs.h>
+#include "header.h"
+
+#define MIDR "/regs/identification/midr_el1"
+#define MIDR_SIZE 128
+
+char *get_cpuid_str(struct perf_pmu *pmu)
+{
+	char *buf = malloc(MIDR_SIZE);
+	char *temp = NULL;
+	char path[PATH_MAX];
+	const char *sysfs = sysfs__mountpoint();
+	int cpu, ret;
+	unsigned long long midr;
+	struct cpu_map *cpus;
+	FILE *file;
+
+	if (!pmu->cpus)
+		return NULL;
+
+	if (!sysfs)
+		return NULL;
+
+	/* read midr from list of cpus mapped to this pmu */
+	cpus = cpu_map__get(pmu->cpus);
+	for (cpu = 0; cpu < cpus->nr; cpu++) {
+		ret = snprintf(path, PATH_MAX,
+				"%s/devices/system/cpu/cpu%d"MIDR,
+				sysfs, cpus->map[cpu]);
+		if (ret == PATH_MAX) {
+			pr_err("sysfs path crossed PATH_MAX(%d) size\n", PATH_MAX);
+			goto err;
+		}
+
+		file = fopen(path, "r");
+		if (!file)
+			continue;
+
+		temp = fgets(buf, MIDR_SIZE, file);
+		fclose(file);
+		if (!temp)
+			continue;
+
+		/* Ignore/clear Variant[23:20] and
+		 * Revision[3:0] of MIDR
+		 */
+		midr = strtoll(buf, NULL, 16);
+		midr &= (~(0xf << 20 | 0xf));
+		snprintf(buf, MIDR_SIZE, "0x%016llx", midr);
+		cpu_map__put(cpus);
+		return buf;
+	}
+
+err:	cpu_map__put(cpus);
+	free(buf);
+	return NULL;
+}
-- 
2.9.4

[PATCH v4 3/4] perf utils: Add helper function is_pmu_core to detect PMU CORE devices

[Ganapatrao Kulkarni]

On some platforms, PMU core devices sysfs name is not cpu.
Adding function is_pmu_core to detect as core device using
core device specific hints in sysfs.

For arm64 platforms, all core devices have file "cpus" in sysfs.

Signed-off-by: Ganapatrao Kulkarni <ganapatrao.kulkarni@cavium.com>
---
 tools/perf/util/pmu.c | 44 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 40 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index aefdbd1..0057d1c 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -506,6 +506,39 @@ static struct cpu_map *pmu_cpumask(const char *name)
 }
 
 /*
+ *  PMU CORE devices have different name other than cpu in sysfs on some
+ *  platforms. looking for possible sysfs files to identify as core device.
+ */
+static int is_pmu_core(const char *name)
+{
+	struct stat st;
+	char path[PATH_MAX];
+	const char *sysfs = sysfs__mountpoint();
+	const char **template;
+	const char *templates[] = {
+		 "%s/bus/event_source/devices/%s/cpus",
+		 NULL
+	};
+
+	if (!sysfs)
+		return 0;
+
+	/* Look for cpu sysfs */
+	snprintf(path, PATH_MAX, "%s/bus/event_source/devices/cpu", sysfs);
+	if ((stat(path, &st) == 0) &&
+			(strncmp(name, "cpu", strlen("cpu")) == 0))
+		return 1;
+
+	for (template = templates; *template; template++) {
+		snprintf(path, PATH_MAX, *template, sysfs, name);
+		if (stat(path, &st) == 0)
+			return 1;
+	}
+
+	return 0;
+}
+
+/*
  * Return the CPU id as a raw string.
  *
  * Each architecture should provide a more precise id string that
@@ -558,15 +591,18 @@ static void pmu_add_cpu_aliases(struct list_head *head, const char *name,
 	 */
 	i = 0;
 	while (1) {
-		const char *pname;
 
 		pe = &map->table[i++];
 		if (!pe->name)
 			break;
 
-		pname = pe->pmu ? pe->pmu : "cpu";
-		if (strncmp(pname, name, strlen(pname)))
-			continue;
+		if (!is_pmu_core(name)) {
+			/* check for uncore devices */
+			if (pe->pmu == NULL)
+				continue;
+			if (strncmp(pe->pmu, name, strlen(pe->pmu)))
+				continue;
+		}
 
 		/* need type casts to override 'const' */
 		__perf_pmu__new_alias(head, NULL, (char *)pe->name,
-- 
2.9.4

[PATCH v4 4/4] perf vendor events arm64: Add ThunderX2 implementation defined pmu core events

[Ganapatrao Kulkarni]

This is not a full event list, but a short list of useful events.

Signed-off-by: Ganapatrao Kulkarni <ganapatrao.kulkarni@cavium.com>
---
 tools/perf/pmu-events/arch/arm64/mapfile.csv       | 15 ++++++
 .../arm64/thunderx2/implementation-defined.json    | 62 ++++++++++++++++++++++
 2 files changed, 77 insertions(+)
 create mode 100644 tools/perf/pmu-events/arch/arm64/mapfile.csv
 create mode 100644 tools/perf/pmu-events/arch/arm64/thunderx2/implementation-defined.json

diff --git a/tools/perf/pmu-events/arch/arm64/mapfile.csv b/tools/perf/pmu-events/arch/arm64/mapfile.csv
new file mode 100644
index 0000000..7167086
--- /dev/null
+++ b/tools/perf/pmu-events/arch/arm64/mapfile.csv
@@ -0,0 +1,15 @@
+# Format:
+#	MIDR,Version,JSON/file/pathname,Type
+#
+# where
+#	MIDR	Processor version
+#		Variant[23:20] and Revision [3:0] should be zero.
+#	Version could be used to track version of of JSON file
+#		but currently unused.
+#	JSON/file/pathname is the path to JSON file, relative
+#		to tools/perf/pmu-events/arch/arm64/.
+#	Type is core, uncore etc
+#
+#
+#Family-model,Version,Filename,EventType
+0x00000000420f5160,v1,thunderx2,core
diff --git a/tools/perf/pmu-events/arch/arm64/thunderx2/implementation-defined.json b/tools/perf/pmu-events/arch/arm64/thunderx2/implementation-defined.json
new file mode 100644
index 0000000..2db45c4
--- /dev/null
+++ b/tools/perf/pmu-events/arch/arm64/thunderx2/implementation-defined.json
@@ -0,0 +1,62 @@
+[
+    {
+        "PublicDescription": "Attributable Level 1 data cache access, read",
+        "EventCode": "0x40",
+        "EventName": "l1d_cache_rd",
+        "BriefDescription": "L1D cache read",
+    },
+    {
+        "PublicDescription": "Attributable Level 1 data cache access, write ",
+        "EventCode": "0x41",
+        "EventName": "l1d_cache_wr",
+        "BriefDescription": "L1D cache write",
+    },
+    {
+        "PublicDescription": "Attributable Level 1 data cache refill, read",
+        "EventCode": "0x42",
+        "EventName": "l1d_cache_refill_rd",
+        "BriefDescription": "L1D cache refill read",
+    },
+    {
+        "PublicDescription": "Attributable Level 1 data cache refill, write",
+        "EventCode": "0x43",
+        "EventName": "l1d_cache_refill_wr",
+        "BriefDescription": "L1D refill write",
+    },
+    {
+        "PublicDescription": "Attributable Level 1 data TLB refill, read",
+        "EventCode": "0x4C",
+        "EventName": "l1d_tlb_refill_rd",
+        "BriefDescription": "L1D tlb refill read",
+    },
+    {
+        "PublicDescription": "Attributable Level 1 data TLB refill, write",
+        "EventCode": "0x4D",
+        "EventName": "l1d_tlb_refill_wr",
+        "BriefDescription": "L1D tlb refill write",
+    },
+    {
+        "PublicDescription": "Attributable Level 1 data or unified TLB access, read",
+        "EventCode": "0x4E",
+        "EventName": "l1d_tlb_rd",
+        "BriefDescription": "L1D tlb read",
+    },
+    {
+        "PublicDescription": "Attributable Level 1 data or unified TLB access, write",
+        "EventCode": "0x4F",
+        "EventName": "l1d_tlb_wr",
+        "BriefDescription": "L1D tlb write",
+    },
+    {
+        "PublicDescription": "Bus access read",
+        "EventCode": "0x60",
+        "EventName": "bus_access_rd",
+        "BriefDescription": "Bus access read",
+   },
+   {
+        "PublicDescription": "Bus access write",
+        "EventCode": "0x61",
+        "EventName": "bus_access_wr",
+        "BriefDescription": "Bus access write",
+   }
+]
-- 
2.9.4

Re: [PATCH v4 0/4] Add support for ThunderX2 pmu events using json files

[Zhangshaokun]

Hi Ganapat

I have tested patch-v4 on Hisilicon's hip08 board for implementation defined PMU events,
and it works, So
Tested-by: Shaokun Zhang <zhangshaokun@hisilicon.com>

Thanks.
Shaokun

On 2017/7/20 13:56, Ganapatrao Kulkarni wrote:
> Extending json/jevent framework for parsing arm64 event files.
> Adding jevents for ThunderX2 implementation defined PMU events.
> 
> v4:
>    - Rebased to 4.13-rc1
> 
> v3:
>    - Addressed comments from Will Deacon and Jayachandran C.
>    - Rebased to 4.12-rc1
> 
> v2:
>    - Updated as per Mark Rutland's suggestions.
>    - Added provision for get_cpuid_str to get cpu id string
>      from associated cpus of pmu core device.
> 
> v1: Initial patchset.
> 
> Ganapatrao Kulkarni (4):
>   perf utils: passing pmu as a parameter to function get_cpuid_str
>   perf tools arm64: Add support for get_cpuid_str function.
>   perf utils: Add helper function is_pmu_core to detect PMU CORE devices
>   perf vendor events arm64: Add ThunderX2 implementation defined pmu
>     core events
> 
>  tools/perf/arch/arm64/util/Build                   |  1 +
>  tools/perf/arch/arm64/util/header.c                | 59 ++++++++++++++++++++
>  tools/perf/arch/powerpc/util/header.c              |  2 +-
>  tools/perf/arch/x86/util/header.c                  |  2 +-
>  tools/perf/pmu-events/arch/arm64/mapfile.csv       | 15 ++++++
>  .../arm64/thunderx2/implementation-defined.json    | 62 ++++++++++++++++++++++
>  tools/perf/util/header.h                           |  3 +-
>  tools/perf/util/pmu.c                              | 53 +++++++++++++++---
>  8 files changed, 186 insertions(+), 11 deletions(-)
>  create mode 100644 tools/perf/arch/arm64/util/header.c
>  create mode 100644 tools/perf/pmu-events/arch/arm64/mapfile.csv
>  create mode 100644 tools/perf/pmu-events/arch/arm64/thunderx2/implementation-defined.json
> 

Re: [PATCH v4 0/4] Add support for ThunderX2 pmu events using json files

[Ganapatrao Kulkarni]

can this be merged/queued to next, if no further comments please?

On Wed, Jul 26, 2017 at 8:19 AM, Zhangshaokun
<zhangshaokun@hisilicon.com> wrote:
>
> Hi Ganapat
>
> I have tested patch-v4 on Hisilicon's hip08 board for implementation defined PMU events,
> and it works, So
> Tested-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
>
> Thanks.
> Shaokun
>
> On 2017/7/20 13:56, Ganapatrao Kulkarni wrote:
>> Extending json/jevent framework for parsing arm64 event files.
>> Adding jevents for ThunderX2 implementation defined PMU events.
>>
>> v4:
>>    - Rebased to 4.13-rc1
>>
>> v3:
>>    - Addressed comments from Will Deacon and Jayachandran C.
>>    - Rebased to 4.12-rc1
>>
>> v2:
>>    - Updated as per Mark Rutland's suggestions.
>>    - Added provision for get_cpuid_str to get cpu id string
>>      from associated cpus of pmu core device.
>>
>> v1: Initial patchset.
>>
>> Ganapatrao Kulkarni (4):
>>   perf utils: passing pmu as a parameter to function get_cpuid_str
>>   perf tools arm64: Add support for get_cpuid_str function.
>>   perf utils: Add helper function is_pmu_core to detect PMU CORE devices
>>   perf vendor events arm64: Add ThunderX2 implementation defined pmu
>>     core events
>>
>>  tools/perf/arch/arm64/util/Build                   |  1 +
>>  tools/perf/arch/arm64/util/header.c                | 59 ++++++++++++++++++++
>>  tools/perf/arch/powerpc/util/header.c              |  2 +-
>>  tools/perf/arch/x86/util/header.c                  |  2 +-
>>  tools/perf/pmu-events/arch/arm64/mapfile.csv       | 15 ++++++
>>  .../arm64/thunderx2/implementation-defined.json    | 62 ++++++++++++++++++++++
>>  tools/perf/util/header.h                           |  3 +-
>>  tools/perf/util/pmu.c                              | 53 +++++++++++++++---
>>  8 files changed, 186 insertions(+), 11 deletions(-)
>>  create mode 100644 tools/perf/arch/arm64/util/header.c
>>  create mode 100644 tools/perf/pmu-events/arch/arm64/mapfile.csv
>>  create mode 100644 tools/perf/pmu-events/arch/arm64/thunderx2/implementation-defined.json
>>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel


thanks
Ganapat

Re: [PATCH v4 2/4] perf tools arm64: Add support for get_cpuid_str function.

[Will Deacon]

On Thu, Jul 20, 2017 at 11:26:06AM +0530, Ganapatrao Kulkarni wrote:
> function get_cpuid_str returns MIDR string of the first online
> cpu from the range of cpus associated with the pmu core device.
> 
> Signed-off-by: Ganapatrao Kulkarni <ganapatrao.kulkarni@cavium.com>
> ---
>  tools/perf/arch/arm64/util/Build    |  1 +
>  tools/perf/arch/arm64/util/header.c | 59 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 60 insertions(+)
>  create mode 100644 tools/perf/arch/arm64/util/header.c
> 
> diff --git a/tools/perf/arch/arm64/util/Build b/tools/perf/arch/arm64/util/Build
> index cef6fb3..b1ab72d 100644
> --- a/tools/perf/arch/arm64/util/Build
> +++ b/tools/perf/arch/arm64/util/Build
> @@ -1,3 +1,4 @@
> +libperf-y += header.o
>  libperf-$(CONFIG_DWARF)     += dwarf-regs.o
>  libperf-$(CONFIG_LOCAL_LIBUNWIND) += unwind-libunwind.o
>  
> diff --git a/tools/perf/arch/arm64/util/header.c b/tools/perf/arch/arm64/util/header.c
> new file mode 100644
> index 0000000..4e25498
> --- /dev/null
> +++ b/tools/perf/arch/arm64/util/header.c
> @@ -0,0 +1,59 @@
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <api/fs/fs.h>
> +#include "header.h"
> +
> +#define MIDR "/regs/identification/midr_el1"
> +#define MIDR_SIZE 128
> +
> +char *get_cpuid_str(struct perf_pmu *pmu)
> +{
> +	char *buf = malloc(MIDR_SIZE);
> +	char *temp = NULL;
> +	char path[PATH_MAX];
> +	const char *sysfs = sysfs__mountpoint();
> +	int cpu, ret;
> +	unsigned long long midr;
> +	struct cpu_map *cpus;
> +	FILE *file;
> +
> +	if (!pmu->cpus)
> +		return NULL;
> +
> +	if (!sysfs)
> +		return NULL;
> +
> +	/* read midr from list of cpus mapped to this pmu */
> +	cpus = cpu_map__get(pmu->cpus);
> +	for (cpu = 0; cpu < cpus->nr; cpu++) {
> +		ret = snprintf(path, PATH_MAX,
> +				"%s/devices/system/cpu/cpu%d"MIDR,
> +				sysfs, cpus->map[cpu]);
> +		if (ret == PATH_MAX) {
> +			pr_err("sysfs path crossed PATH_MAX(%d) size\n", PATH_MAX);
> +			goto err;
> +		}
> +
> +		file = fopen(path, "r");
> +		if (!file)
> +			continue;
> +
> +		temp = fgets(buf, MIDR_SIZE, file);
> +		fclose(file);
> +		if (!temp)
> +			continue;
> +
> +		/* Ignore/clear Variant[23:20] and
> +		 * Revision[3:0] of MIDR
> +		 */
> +		midr = strtoll(buf, NULL, 16);
> +		midr &= (~(0xf << 20 | 0xf));
> +		snprintf(buf, MIDR_SIZE, "0x%016llx", midr);
> +		cpu_map__put(cpus);
> +		return buf;
> +	}
> +
> +err:	cpu_map__put(cpus);
> +	free(buf);
> +	return NULL;
> +}

Might just be me, but I found this really difficult to read. I think it
works, but having the return at the end of loop is really counter-intuitive.

I'll leave it up to Arnaldo, since I can't find any functional problems
with this series from the arm64 side.

Cheers,

Will

Re: [PATCH v4 2/4] perf tools arm64: Add support for get_cpuid_str function.

[Arnaldo Carvalho de Melo]

Em Thu, Aug 10, 2017 at 06:15:50PM +0100, Will Deacon escreveu:
> On Thu, Jul 20, 2017 at 11:26:06AM +0530, Ganapatrao Kulkarni wrote:
> > function get_cpuid_str returns MIDR string of the first online
> > cpu from the range of cpus associated with the pmu core device.
> > 
> > Signed-off-by: Ganapatrao Kulkarni <ganapatrao.kulkarni@cavium.com>
> > ---
> >  tools/perf/arch/arm64/util/Build    |  1 +
> >  tools/perf/arch/arm64/util/header.c | 59 +++++++++++++++++++++++++++++++++++++
> >  2 files changed, 60 insertions(+)
> >  create mode 100644 tools/perf/arch/arm64/util/header.c
> > 
> > diff --git a/tools/perf/arch/arm64/util/Build b/tools/perf/arch/arm64/util/Build
> > index cef6fb3..b1ab72d 100644
> > --- a/tools/perf/arch/arm64/util/Build
> > +++ b/tools/perf/arch/arm64/util/Build
> > @@ -1,3 +1,4 @@
> > +libperf-y += header.o
> >  libperf-$(CONFIG_DWARF)     += dwarf-regs.o
> >  libperf-$(CONFIG_LOCAL_LIBUNWIND) += unwind-libunwind.o
> >  
> > diff --git a/tools/perf/arch/arm64/util/header.c b/tools/perf/arch/arm64/util/header.c
> > new file mode 100644
> > index 0000000..4e25498
> > --- /dev/null
> > +++ b/tools/perf/arch/arm64/util/header.c
> > @@ -0,0 +1,59 @@
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <api/fs/fs.h>
> > +#include "header.h"
> > +
> > +#define MIDR "/regs/identification/midr_el1"
> > +#define MIDR_SIZE 128
> > +
> > +char *get_cpuid_str(struct perf_pmu *pmu)
> > +{
> > +	char *buf = malloc(MIDR_SIZE);
> > +	char *temp = NULL;
> > +	char path[PATH_MAX];
> > +	const char *sysfs = sysfs__mountpoint();
> > +	int cpu, ret;
> > +	unsigned long long midr;
> > +	struct cpu_map *cpus;
> > +	FILE *file;
> > +
> > +	if (!pmu->cpus)
> > +		return NULL;
> > +
> > +	if (!sysfs)
> > +		return NULL;
> > +
> > +	/* read midr from list of cpus mapped to this pmu */
> > +	cpus = cpu_map__get(pmu->cpus);
> > +	for (cpu = 0; cpu < cpus->nr; cpu++) {
> > +		ret = snprintf(path, PATH_MAX,
> > +				"%s/devices/system/cpu/cpu%d"MIDR,
> > +				sysfs, cpus->map[cpu]);
> > +		if (ret == PATH_MAX) {
> > +			pr_err("sysfs path crossed PATH_MAX(%d) size\n", PATH_MAX);
> > +			goto err;
> > +		}
> > +
> > +		file = fopen(path, "r");
> > +		if (!file)
> > +			continue;
> > +
> > +		temp = fgets(buf, MIDR_SIZE, file);
> > +		fclose(file);
> > +		if (!temp)
> > +			continue;
> > +
> > +		/* Ignore/clear Variant[23:20] and
> > +		 * Revision[3:0] of MIDR
> > +		 */
> > +		midr = strtoll(buf, NULL, 16);
> > +		midr &= (~(0xf << 20 | 0xf));
> > +		snprintf(buf, MIDR_SIZE, "0x%016llx", midr);
> > +		cpu_map__put(cpus);
> > +		return buf;
> > +	}
> > +
> > +err:	cpu_map__put(cpus);
> > +	free(buf);
> > +	return NULL;
> > +}
> 
> Might just be me, but I found this really difficult to read. I think it
> works, but having the return at the end of loop is really counter-intuitive.
> 
> I'll leave it up to Arnaldo, since I can't find any functional problems
> with this series from the arm64 side.

And it uses malloc(128) and doesn't check its return as well, can you
please rewrite this having just one return, one refcount drop, etc,
outside of the loop?

And that fgets() return may be an error, don't you have to check it more
carefully?

Also please read the snprintf() man page, it doesn't return the number
of chars it actually wrote, but the number of chars it _would_ write, I
suggest you use scnprintf() instead.

Also it doesn't count the terminating null byte on what it returns, so
the check is flawed in that regard as well.

- Arnaldo

Re: [PATCH v4 2/4] perf tools arm64: Add support for get_cpuid_str function.

[Ganapatrao Kulkarni]

On Thu, Aug 10, 2017 at 11:39 PM, Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
> Em Thu, Aug 10, 2017 at 06:15:50PM +0100, Will Deacon escreveu:
>> On Thu, Jul 20, 2017 at 11:26:06AM +0530, Ganapatrao Kulkarni wrote:
>> > function get_cpuid_str returns MIDR string of the first online
>> > cpu from the range of cpus associated with the pmu core device.
>> >
>> > Signed-off-by: Ganapatrao Kulkarni <ganapatrao.kulkarni@cavium.com>
>> > ---
>> >  tools/perf/arch/arm64/util/Build    |  1 +
>> >  tools/perf/arch/arm64/util/header.c | 59 +++++++++++++++++++++++++++++++++++++
>> >  2 files changed, 60 insertions(+)
>> >  create mode 100644 tools/perf/arch/arm64/util/header.c
>> >
>> > diff --git a/tools/perf/arch/arm64/util/Build b/tools/perf/arch/arm64/util/Build
>> > index cef6fb3..b1ab72d 100644
>> > --- a/tools/perf/arch/arm64/util/Build
>> > +++ b/tools/perf/arch/arm64/util/Build
>> > @@ -1,3 +1,4 @@
>> > +libperf-y += header.o
>> >  libperf-$(CONFIG_DWARF)     += dwarf-regs.o
>> >  libperf-$(CONFIG_LOCAL_LIBUNWIND) += unwind-libunwind.o
>> >
>> > diff --git a/tools/perf/arch/arm64/util/header.c b/tools/perf/arch/arm64/util/header.c
>> > new file mode 100644
>> > index 0000000..4e25498
>> > --- /dev/null
>> > +++ b/tools/perf/arch/arm64/util/header.c
>> > @@ -0,0 +1,59 @@
>> > +#include <stdio.h>
>> > +#include <stdlib.h>
>> > +#include <api/fs/fs.h>
>> > +#include "header.h"
>> > +
>> > +#define MIDR "/regs/identification/midr_el1"
>> > +#define MIDR_SIZE 128
>> > +
>> > +char *get_cpuid_str(struct perf_pmu *pmu)
>> > +{
>> > +   char *buf = malloc(MIDR_SIZE);
>> > +   char *temp = NULL;
>> > +   char path[PATH_MAX];
>> > +   const char *sysfs = sysfs__mountpoint();
>> > +   int cpu, ret;
>> > +   unsigned long long midr;
>> > +   struct cpu_map *cpus;
>> > +   FILE *file;
>> > +
>> > +   if (!pmu->cpus)
>> > +           return NULL;
>> > +
>> > +   if (!sysfs)
>> > +           return NULL;
>> > +
>> > +   /* read midr from list of cpus mapped to this pmu */
>> > +   cpus = cpu_map__get(pmu->cpus);
>> > +   for (cpu = 0; cpu < cpus->nr; cpu++) {
>> > +           ret = snprintf(path, PATH_MAX,
>> > +                           "%s/devices/system/cpu/cpu%d"MIDR,
>> > +                           sysfs, cpus->map[cpu]);
>> > +           if (ret == PATH_MAX) {
>> > +                   pr_err("sysfs path crossed PATH_MAX(%d) size\n", PATH_MAX);
>> > +                   goto err;
>> > +           }
>> > +
>> > +           file = fopen(path, "r");
>> > +           if (!file)
>> > +                   continue;
>> > +
>> > +           temp = fgets(buf, MIDR_SIZE, file);
>> > +           fclose(file);
>> > +           if (!temp)
>> > +                   continue;
>> > +
>> > +           /* Ignore/clear Variant[23:20] and
>> > +            * Revision[3:0] of MIDR
>> > +            */
>> > +           midr = strtoll(buf, NULL, 16);
>> > +           midr &= (~(0xf << 20 | 0xf));
>> > +           snprintf(buf, MIDR_SIZE, "0x%016llx", midr);
>> > +           cpu_map__put(cpus);
>> > +           return buf;
>> > +   }
>> > +
>> > +err:       cpu_map__put(cpus);
>> > +   free(buf);
>> > +   return NULL;
>> > +}
>>
>> Might just be me, but I found this really difficult to read. I think it
>> works, but having the return at the end of loop is really counter-intuitive.
>>
>> I'll leave it up to Arnaldo, since I can't find any functional problems
>> with this series from the arm64 side.
>
> And it uses malloc(128) and doesn't check its return as well, can you
> please rewrite this having just one return, one refcount drop, etc,
> outside of the loop?
>
> And that fgets() return may be an error, don't you have to check it more
> carefully?
>
> Also please read the snprintf() man page, it doesn't return the number
> of chars it actually wrote, but the number of chars it _would_ write, I
> suggest you use scnprintf() instead.
>
> Also it doesn't count the terminating null byte on what it returns, so
> the check is flawed in that regard as well.

Thanks Arnaldo and Will for the comments.
I will rework this function as suggested in next version.

>
> - Arnaldo
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

thanks
Ganapat

Re: [PATCH v4 3/4] perf utils: Add helper function is_pmu_core to detect PMU CORE devices

[Jonathan Cameron]

On Thu, 20 Jul 2017 11:26:07 +0530
Ganapatrao Kulkarni <ganapatrao.kulkarni@cavium.com> wrote:

> On some platforms, PMU core devices sysfs name is not cpu.
> Adding function is_pmu_core to detect as core device using
> core device specific hints in sysfs.
> 
> For arm64 platforms, all core devices have file "cpus" in sysfs.
> 
> Signed-off-by: Ganapatrao Kulkarni <ganapatrao.kulkarni@cavium.com>

One really trivial point inline.  Otherwise looks good to me.

Jonathan

> ---
>  tools/perf/util/pmu.c | 44 ++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 40 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index aefdbd1..0057d1c 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -506,6 +506,39 @@ static struct cpu_map *pmu_cpumask(const char *name)
>  }
>  
>  /*
> + *  PMU CORE devices have different name other than cpu in sysfs on some
> + *  platforms. looking for possible sysfs files to identify as core device.
> + */
> +static int is_pmu_core(const char *name)
> +{
> +	struct stat st;
> +	char path[PATH_MAX];
> +	const char *sysfs = sysfs__mountpoint();
> +	const char **template;
> +	const char *templates[] = {
> +		 "%s/bus/event_source/devices/%s/cpus",
> +		 NULL
> +	};
> +
> +	if (!sysfs)
> +		return 0;
> +
> +	/* Look for cpu sysfs */
> +	snprintf(path, PATH_MAX, "%s/bus/event_source/devices/cpu", sysfs);
> +	if ((stat(path, &st) == 0) &&
> +			(strncmp(name, "cpu", strlen("cpu")) == 0))
> +		return 1;
> +
> +	for (template = templates; *template; template++) {
> +		snprintf(path, PATH_MAX, *template, sysfs, name);
> +		if (stat(path, &st) == 0)
> +			return 1;
> +	}

Adding generic infrastructure for one entry where we already know the other
case doesn't fit the pattern, seems a little premature.

If we can't make these two fit it seems like we should just hard code
the second case as well for now.

Refactoring to this may be make sense once we have a few different cases
that do share a common form (of including the name)

You could actually put both options in templates and rely on the fact that
snprintf will ignore excess parameters, but that feels fragile.

> +
> +	return 0;
> +}
> +
> +/*
>   * Return the CPU id as a raw string.
>   *
>   * Each architecture should provide a more precise id string that
> @@ -558,15 +591,18 @@ static void pmu_add_cpu_aliases(struct list_head *head, const char *name,
>  	 */
>  	i = 0;
>  	while (1) {
> -		const char *pname;
>  
>  		pe = &map->table[i++];
>  		if (!pe->name)
>  			break;
>  
> -		pname = pe->pmu ? pe->pmu : "cpu";
> -		if (strncmp(pname, name, strlen(pname)))
> -			continue;
> +		if (!is_pmu_core(name)) {
> +			/* check for uncore devices */
> +			if (pe->pmu == NULL)
> +				continue;
> +			if (strncmp(pe->pmu, name, strlen(pe->pmu)))
> +				continue;
> +		}
>  
>  		/* need type casts to override 'const' */
>  		__perf_pmu__new_alias(head, NULL, (char *)pe->name,