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
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 | 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
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
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 <[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
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
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 <[email protected]>
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
>
can this be merged/queued to next, if no further comments please?
On Wed, Jul 26, 2017 at 8:19 AM, Zhangshaokun
<[email protected]> 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 <[email protected]>
>
> 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
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
thanks
Ganapat
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> > ---
> > 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
On Thu, Aug 10, 2017 at 11:39 PM, Arnaldo Carvalho de Melo
<[email protected]> 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 <[email protected]>
>> > ---
>> > 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
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
thanks
Ganapat
On Thu, 20 Jul 2017 11:26:07 +0530
Ganapatrao Kulkarni <[email protected]> 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 <[email protected]>
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,