2017-08-16 07:11:17

by Ganapatrao Kulkarni

[permalink] [raw]
Subject: [PATCH v5 0/4] Add support for ThunderX2 pmu events using json files

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

v5:
- Addressed comments from Arnaldo.
- Rebased to 4.13-rc5

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 | 61 +++++++++++++++++++++
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, 188 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


2017-08-16 07:11:24

by Ganapatrao Kulkarni

[permalink] [raw]
Subject: [PATCH v5 1/4] perf utils: passing pmu as a parameter to function get_cpuid_str

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 <[email protected]>
---
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

2017-08-16 07:11:28

by Ganapatrao Kulkarni

[permalink] [raw]
Subject: [PATCH v5 2/4] perf tools arm64: Add support for get_cpuid_str function.

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 <[email protected]>
---
tools/perf/arch/arm64/util/Build | 1 +
tools/perf/arch/arm64/util/header.c | 61 +++++++++++++++++++++++++++++++++++++
2 files changed, 62 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..713ef17
--- /dev/null
+++ b/tools/perf/arch/arm64/util/header.c
@@ -0,0 +1,61 @@
+#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 = NULL;
+ char *temp = NULL;
+ char path[PATH_MAX];
+ const char *sysfs = sysfs__mountpoint();
+ int cpu;
+ unsigned long long midr = 0;
+ struct cpu_map *cpus;
+ FILE *file;
+
+ if (!sysfs || !pmu->cpus)
+ return NULL;
+
+ buf = malloc(MIDR_SIZE);
+ if (!buf)
+ 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++) {
+ scnprintf(path, PATH_MAX, "%s/devices/system/cpu/cpu%d"MIDR,
+ sysfs, cpus->map[cpu]);
+
+ file = fopen(path, "r");
+ if (!file) {
+ pr_err("fopen failed for file %s\n", path);
+ continue;
+ }
+
+ temp = fgets(buf, MIDR_SIZE, file);
+ if (!temp)
+ continue;
+ fclose(file);
+
+ /* Ignore/clear Variant[23:20] and
+ * Revision[3:0] of MIDR
+ */
+ midr = strtoll(buf, NULL, 16);
+ midr &= (~(0xf << 20 | 0xf));
+ scnprintf(buf, MIDR_SIZE, "0x%016llx", midr);
+ /* got midr break loop */
+ break;
+ }
+
+ if (!midr) {
+ free(buf);
+ buf = NULL;
+ }
+
+ cpu_map__put(cpus);
+ return buf;
+}
--
2.9.4

2017-08-16 07:11:36

by Ganapatrao Kulkarni

[permalink] [raw]
Subject: [PATCH v5 3/4] perf utils: Add helper function is_pmu_core to detect PMU CORE devices

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.

Tested-by: Shaokun Zhang <[email protected]>
Signed-off-by: Ganapatrao Kulkarni <[email protected]>
---
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

2017-08-16 07:11:51

by Ganapatrao Kulkarni

[permalink] [raw]
Subject: [PATCH v5 4/4] perf vendor events arm64: Add ThunderX2 implementation defined pmu core events

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

Signed-off-by: Ganapatrao Kulkarni <[email protected]>
---
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

2017-08-22 04:22:19

by Ganapatrao Kulkarni

[permalink] [raw]
Subject: Re: [PATCH v5 0/4] Add support for ThunderX2 pmu events using json files

Hi Arnaldo, Will,

are there any comments on this series?


On Wed, Aug 16, 2017 at 12:40 PM, Ganapatrao Kulkarni
<[email protected]> wrote:
> Extending json/jevent framework for parsing arm64 event files.
> Adding jevents for ThunderX2 implementation defined PMU events.
>
> v5:
> - Addressed comments from Arnaldo.
> - Rebased to 4.13-rc5
>
> 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 | 61 +++++++++++++++++++++
> 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, 188 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
>

thanks
Ganapat

2017-08-23 09:23:54

by John Garry

[permalink] [raw]
Subject: Re: [PATCH v5 0/4] Add support for ThunderX2 pmu events using json files

On 22/08/2017 05:21, Ganapatrao Kulkarni wrote:
> Hi Arnaldo, Will,
>
> are there any comments on this series?
>

Hi Ganapatrao,

Is it possible to have vendor sub-folder in
tools/perf/pmu-events/arch/arm64 folder (like arm64 device tree files)?
We (HiSilicon) want to add support for our hip08 SoC, and I don't think
"hip08" or "<internal code name for core>" are good arch sub-folder names.

I am not sure if this way discussed before, but the changelog does not
mention it. And I think that we had the same location for thunderx2 in
v1 series.

We should also consider that in theory the events could change per SoC
using the same custom ARMv8 custom implementation.

Thanks,
John

>
> On Wed, Aug 16, 2017 at 12:40 PM, Ganapatrao Kulkarni
> <[email protected]> wrote:
>> Extending json/jevent framework for parsing arm64 event files.
>> Adding jevents for ThunderX2 implementation defined PMU events.
>>
>> v5:
>> - Addressed comments from Arnaldo.
>> - Rebased to 4.13-rc5
>>
>> 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 | 61 +++++++++++++++++++++
>> 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, 188 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
>>
>
> thanks
> Ganapat
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
> .
>


2017-08-23 10:18:08

by Will Deacon

[permalink] [raw]
Subject: Re: [PATCH v5 2/4] perf tools arm64: Add support for get_cpuid_str function.

On Wed, Aug 16, 2017 at 12:40:46PM +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 <[email protected]>
> ---
> tools/perf/arch/arm64/util/Build | 1 +
> tools/perf/arch/arm64/util/header.c | 61 +++++++++++++++++++++++++++++++++++++
> 2 files changed, 62 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..713ef17
> --- /dev/null
> +++ b/tools/perf/arch/arm64/util/header.c
> @@ -0,0 +1,61 @@
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <api/fs/fs.h>
> +#include "header.h"
> +
> +#define MIDR "/regs/identification/midr_el1"
> +#define MIDR_SIZE 128

Why is this so big?

> +char *get_cpuid_str(struct perf_pmu *pmu)
> +{
> + char *buf = NULL;
> + char *temp = NULL;
> + char path[PATH_MAX];
> + const char *sysfs = sysfs__mountpoint();
> + int cpu;
> + unsigned long long midr = 0;
> + struct cpu_map *cpus;
> + FILE *file;
> +
> + if (!sysfs || !pmu->cpus)
> + return NULL;
> +
> + buf = malloc(MIDR_SIZE);
> + if (!buf)
> + 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++) {
> + scnprintf(path, PATH_MAX, "%s/devices/system/cpu/cpu%d"MIDR,
> + sysfs, cpus->map[cpu]);
> +
> + file = fopen(path, "r");
> + if (!file) {
> + pr_err("fopen failed for file %s\n", path);

pr_debug instead? It's not fatal if a CPU is hotplugged off and you can't
read its midr.

> + continue;
> + }
> +
> + temp = fgets(buf, MIDR_SIZE, file);
> + if (!temp)
> + continue;

Do you actually need a temporary variable for this?

> + fclose(file);
> +
> + /* Ignore/clear Variant[23:20] and
> + * Revision[3:0] of MIDR
> + */
> + midr = strtoll(buf, NULL, 16);
> + midr &= (~(0xf << 20 | 0xf));

If midr really needs to be unsigned long long (u64 might be better), then
you need to fix these immediates to have a ULL suffix.

Can you use the GENMASK_ULL macro to generate the MIDR mask, or is that
not available here?

> + scnprintf(buf, MIDR_SIZE, "0x%016llx", midr);
> + /* got midr break loop */
> + break;
> + }
> +
> + if (!midr) {
> + free(buf);
> + buf = NULL;

If you want the pr_err, here would seem more appropriate because at this
point we've actually failed.

Will

2017-08-24 07:23:12

by Ganapatrao Kulkarni

[permalink] [raw]
Subject: Re: [PATCH v5 0/4] Add support for ThunderX2 pmu events using json files

Hi John,

On Wed, Aug 23, 2017 at 2:52 PM, John Garry <[email protected]> wrote:
> On 22/08/2017 05:21, Ganapatrao Kulkarni wrote:
>>
>> Hi Arnaldo, Will,
>>
>> are there any comments on this series?
>>
>
> Hi Ganapatrao,
>
> Is it possible to have vendor sub-folder in tools/perf/pmu-events/arch/arm64
> folder (like arm64 device tree files)? We (HiSilicon) want to add support
> for our hip08 SoC, and I don't think "hip08" or "<internal code name for
> core>" are good arch sub-folder names.

at present the directory structure is in-line with as done for x86 and powerpc.
there can be separate patchset to have directory hierarchy as
suggested by you, if every one agrees on it!

>
> I am not sure if this way discussed before, but the changelog does not
> mention it. And I think that we had the same location for thunderx2 in v1
> series.
>
> We should also consider that in theory the events could change per SoC using
> the same custom ARMv8 custom implementation.
>
> Thanks,
> John
>
>>
>> On Wed, Aug 16, 2017 at 12:40 PM, Ganapatrao Kulkarni
>> <[email protected]> wrote:
>>>
>>> Extending json/jevent framework for parsing arm64 event files.
>>> Adding jevents for ThunderX2 implementation defined PMU events.
>>>
>>> v5:
>>> - Addressed comments from Arnaldo.
>>> - Rebased to 4.13-rc5
>>>
>>> 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 | 61
>>> +++++++++++++++++++++
>>> 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, 188 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
>>>
>>
>> thanks
>> Ganapat
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> [email protected]
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
>> .
>>
>
>

thanks
Ganapat

2017-08-24 09:56:44

by Ganapatrao Kulkarni

[permalink] [raw]
Subject: Re: [PATCH v5 2/4] perf tools arm64: Add support for get_cpuid_str function.

On Wed, Aug 23, 2017 at 3:47 PM, Will Deacon <[email protected]> wrote:
> On Wed, Aug 16, 2017 at 12:40:46PM +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 <[email protected]>
>> ---
>> tools/perf/arch/arm64/util/Build | 1 +
>> tools/perf/arch/arm64/util/header.c | 61 +++++++++++++++++++++++++++++++++++++
>> 2 files changed, 62 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..713ef17
>> --- /dev/null
>> +++ b/tools/perf/arch/arm64/util/header.c
>> @@ -0,0 +1,61 @@
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <api/fs/fs.h>
>> +#include "header.h"
>> +
>> +#define MIDR "/regs/identification/midr_el1"
>> +#define MIDR_SIZE 128
>
> Why is this so big?

ok, i will reduce it to exact size(19)
>
>> +char *get_cpuid_str(struct perf_pmu *pmu)
>> +{
>> + char *buf = NULL;
>> + char *temp = NULL;
>> + char path[PATH_MAX];
>> + const char *sysfs = sysfs__mountpoint();
>> + int cpu;
>> + unsigned long long midr = 0;
>> + struct cpu_map *cpus;
>> + FILE *file;
>> +
>> + if (!sysfs || !pmu->cpus)
>> + return NULL;
>> +
>> + buf = malloc(MIDR_SIZE);
>> + if (!buf)
>> + 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++) {
>> + scnprintf(path, PATH_MAX, "%s/devices/system/cpu/cpu%d"MIDR,
>> + sysfs, cpus->map[cpu]);
>> +
>> + file = fopen(path, "r");
>> + if (!file) {
>> + pr_err("fopen failed for file %s\n", path);
>
> pr_debug instead? It's not fatal if a CPU is hotplugged off and you can't
> read its midr.

ok
>
>> + continue;
>> + }
>> +
>> + temp = fgets(buf, MIDR_SIZE, file);
>> + if (!temp)
>> + continue;
>
> Do you actually need a temporary variable for this?

ok
>
>> + fclose(file);
>> +
>> + /* Ignore/clear Variant[23:20] and
>> + * Revision[3:0] of MIDR
>> + */
>> + midr = strtoll(buf, NULL, 16);
>> + midr &= (~(0xf << 20 | 0xf));
>
> If midr really needs to be unsigned long long (u64 might be better), then
> you need to fix these immediates to have a ULL suffix.

yes, will use u64
>
> Can you use the GENMASK_ULL macro to generate the MIDR mask, or is that
> not available here?

it is not defined in tools/include/linux/bitops.h
>
>> + scnprintf(buf, MIDR_SIZE, "0x%016llx", midr);
>> + /* got midr break loop */
>> + break;
>> + }
>> +
>> + if (!midr) {
>> + free(buf);
>> + buf = NULL;
>
> If you want the pr_err, here would seem more appropriate because at this
> point we've actually failed.

thanks, makes sense.
>
> Will
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

thanks
Ganapat

2017-08-24 11:10:20

by John Garry

[permalink] [raw]
Subject: Re: [PATCH v5 0/4] Add support for ThunderX2 pmu events using json files

On 24/08/2017 08:23, Ganapatrao Kulkarni wrote:
> Hi John,
>
> On Wed, Aug 23, 2017 at 2:52 PM, John Garry <[email protected]> wrote:
>> On 22/08/2017 05:21, Ganapatrao Kulkarni wrote:
>>>
>>> Hi Arnaldo, Will,
>>>
>>> are there any comments on this series?
>>>
>>
>> Hi Ganapatrao,
>>
>> Is it possible to have vendor sub-folder in tools/perf/pmu-events/arch/arm64
>> folder (like arm64 device tree files)? We (HiSilicon) want to add support
>> for our hip08 SoC, and I don't think "hip08" or "<internal code name for
>> core>" are good arch sub-folder names.
>
> at present the directory structure is in-line with as done for x86 and powerpc.
> there can be separate patchset to have directory hierarchy as
> suggested by you, if every one agrees on it!

Fine.

BTW, Shaokun has a json file for our hip08 platform waiting, based on
this current patchset. We can send it now if anyone wants to see another
arm64 json.

Much appreciated,
John

>
>>
>> I am not sure if this way discussed before, but the changelog does not
>> mention it. And I think that we had the same location for thunderx2 in v1
>> series.
>>
>> We should also consider that in theory the events could change per SoC using
>> the same custom ARMv8 custom implementation.
>>
>> Thanks,
>> John
>>
>>>
>>> On Wed, Aug 16, 2017 at 12:40 PM, Ganapatrao Kulkarni
>>> <[email protected]> wrote:
>>>>
>>>> Extending json/jevent framework for parsing arm64 event files.
>>>> Adding jevents for ThunderX2 implementation defined PMU events.
>>>>
>>>> v5:
>>>> - Addressed comments from Arnaldo.
>>>> - Rebased to 4.13-rc5
>>>>
>>>> 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 | 61
>>>> +++++++++++++++++++++
>>>> 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, 188 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
>>>>
>>>
>>> thanks
>>> Ganapat
>>>
>>> _______________________________________________
>>> linux-arm-kernel mailing list
>>> [email protected]
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>
>>> .
>>>
>>
>>
>
> thanks
> Ganapat
>
> .
>