Messages in this thread | | | Subject | Re: [PATCH] perf vendor events arm64: Add hip08 implementation defined PMU core events | From | John Garry <> | Date | Wed, 18 Oct 2017 17:37:03 +0100 |
| |
On 18/10/2017 11:49, Will Deacon wrote: > On Wed, Oct 18, 2017 at 10:25:39AM +0100, John Garry wrote: >> On 17/10/2017 13:59, Will Deacon wrote: >>> Hi Shaokun, >>> >>> Thanks for the patch. One comment below. >>> >>> On Tue, Oct 17, 2017 at 03:01:39PM +0800, Shaokun Zhang wrote: >>>> This is a short list of useful implementation defined PMU events of >>>> hip08, other supported events are not listed in this JSON file. >>>> >>>> This patch is dependent on Cavium's patch-v9 (Add support for >>>> ThunderX2 pmu events using json files), Link: >>>> https://www.spinics.net/lists/arm-kernel/msg611895.html >>>> >>>> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com> >>>> Cc: Peter Zijlstra <peterz@infradead.org> >>>> Cc: Ingo Molnar <mingo@redhat.com> >>>> Cc: Arnaldo Carvalho de Melo <acme@kernel.org> >>>> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> >>>> Cc: Will Deacon <will.deacon@arm.com> >>>> Cc: Ganapatrao Kulkarni <ganapatrao.kulkarni@cavium.com> >>>> Cc: John Garry <john.garry@huawei.com> >>>> --- >>>> .../arch/arm64/hisilicon/hip08-imp-def.json | 176 +++++++++++++++++++++ >>>> tools/perf/pmu-events/arch/arm64/mapfile.csv | 1 + >>>> 2 files changed, 177 insertions(+) >>>> create mode 100644 tools/perf/pmu-events/arch/arm64/hisilicon/hip08-imp-def.json >>>> >>>> diff --git a/tools/perf/pmu-events/arch/arm64/hisilicon/hip08-imp-def.json b/tools/perf/pmu-events/arch/arm64/hisilicon/hip08-imp-def.json >>>> new file mode 100644 >>>> index 0000000..6bb31da >>>> --- /dev/null >>>> +++ b/tools/perf/pmu-events/arch/arm64/hisilicon/hip08-imp-def.json >>>> @@ -0,0 +1,176 @@ >>>> +[ >>>> + { >>>> + "PublicDescription": "Attributable Level 1 data cache access, read", >>>> + "EventCode": "0x40", >>>> + "EventName": "L1D_CACHE_RD", >>>> + "BriefDescription": "L1D cache access, read", >>>> + }, >>>> + { >>>> + "PublicDescription": "Attributable Level 1 data cache access, write", >>>> + "EventCode": "0x41", >>>> + "EventName": "L1D_CACHE_WR", >>>> + "BriefDescription": "L1D cache access, write", >>>> + }, >>> >>> So these are the same as the events in cavium/thunderx2-imp-def.json and >>> should be factored out. In fact, ARM recommends event numbers for 0x40-0xBF, >>> so the best thing would be to have those defined in their own file, then >>> have a way for the various CPU-specific .json files to pick and chose the >>> events they need from there. >> >> Right, this seems reasonable. Just need to check on feasible. >> >> In terms of coordinating this work, shall we do it? Will arm64+ThunderX >> support be accepted as is? > > Yes, that would be my preference if you don't mind.
Cool.
I am looking at this topic now. But I am doubting the folder structure again.
Firstly, we still have this comment in the README: All the topic JSON files for a CPU model/family should be in a separate sub directory.
Now, when thunderx3 or hip09 comes along, I assume that their jsons will similarly go into cavium and hisilcon folders, respectively. So, for example, we add thunderx3 json, like this: arm64/cavium/thunderx3-imp-def.json [ { "PublicDescription": "foo", "EventCode": "0x40", "EventName": "bar", "BriefDescription": "sieve", }, ]
#Family-model,Version,Filename,EventType 0x00000000420f5160,v1,cavium,core 0x00000000420f5161,v1,cavium,core
Then we have generated pmu_events.c, like this: #include "../../pmu-events/pmu-events.h" struct pmu_event pme_cavium[] = { { .name = "bar", .event = "event=0x40", .desc = "sieve", .topic = "thunderx3 imp def", .long_desc = "foo", }, { .name = "l1d_cache_rd", .event = "event=0x40", .desc = "L1D cache read", .topic = "thunderx2 imp def", .long_desc = "Attributable Level 1 data cache access, read", }, { .name = "l1d_cache_wr", .event = "event=0x41", .desc = "L1D cache write", .topic = "thunderx2 imp def", .long_desc = "Attributable Level 1 data cache access, write ", },
[ ... ]
{ .name = 0, .event = 0, .desc = 0, }, }; struct pmu_events_map pmu_events_map[] = { { .cpuid = "0x00000000420f5160", .version = "v1", .type = "core", .table = pme_cavium }, { .cpuid = "0x00000000420f5161", .version = "v1", .type = "core", .table = pme_cavium }, { .cpuid = 0, .version = 0, .type = 0, .table = 0, }, };
It doesn't look right, espcially since we have conflicting definitions for event 0x40. We really should have table per cpu.
John
> > Will > > . >
| |