Re: [PATCH 3/4] perf vendor events arm64: Add AmpereOne metrics

[Ilkka Koskinen <ilkka@os.amperecomputing.com>]

On Mon, 7 Aug 2023, John Garry wrote:
> On 04/08/2023 20:59, Ilkka Koskinen wrote:
>> 
>> Hi John
>> 
>> On Fri, 4 Aug 2023, John Garry wrote:
>>> On 03/08/2023 22:13, Ilkka Koskinen wrote:
>>>> This patch adds AmpereOne metrics. The metrics also work around
>>>> the issue related to some of the events.
>
> Would these events be any metrics added which are not a "Topdown"? I guess 
> no, since there are many, but I just don't know.
>
>>> 
>>> Just curious, are these events/metrics described in some 
>>> publically-available document?
>> 
>> I quickly checked that and there are a spreadsheet and a document 
>> available, which list the supported PMUs, their events and metrics in the 
>> customer connect website but that requires registering.
>> 
>
> OK, thanks for the info. I ask is it always worthwhile mentioning a link in 
> the changelog if publicly available.

I can certainly add a comment that the events are available at the 
customer connect website.

>
>
> Just a few minor comments:
>
> On 03/08/2023 22:13, Ilkka Koskinen wrote:
>> This patch adds AmpereOne metrics. The metrics also work around
>> the issue related to some of the events.
>>
>> Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
>> ---
>>   .../arch/arm64/ampere/ampereone/metrics.json  | 362 ++++++++++++++++++
>>   1 file changed, 362 insertions(+)
>>
>
> ...
>
>> +    {
>> +	"MetricExpr": "CRYPTO_SPEC / OP_SPEC",
>> +	"BriefDescription": "Proportion of crypto data processing 
> operations",
>> +	"MetricGroup": "Instruction",
>> +	"MetricName": "Crypto mix"
>> +    },
>> +    {
>> +	"MetricExpr": "VFP_SPEC / (duration_time *1000000000)",
>> +	"BriefDescription": "Giga-floating point operations per second",
>> +	"MetricGroup": "Instruction",
>> +	"MetricName": "GFLOPS_ISSUED"
>> +    },
>> +    {
>> +	"MetricExpr": "DP_SPEC / OP_SPEC",
>> +	"BriefDescription": "Proportion of integer data processing 
> operations",
>> +	"MetricGroup": "Instruction",
>> +	"MetricName": "Integer mix"
>> +    },
>> +    {
>> +	"MetricExpr": "INST_RETIRED / CPU_CYCLES",
>> +	"BriefDescription": "Instructions per cycle",
>> +	"MetricGroup": "Instruction",
>> +	"MetricName": "IPC"
>> +    },
>> +    {
>> +	"MetricExpr": "LD_SPEC / OP_SPEC",
>> +	"BriefDescription": "Proportion of load operations",
>> +	"MetricGroup": "Instruction",
>> +	"MetricName": "Load mix"
>> +    },
>> +    {
>> +	"MetricExpr": "LDST_SPEC/ OP_SPEC",
>
> mega nit: missing whitespace before '/'

I'll fix it.

>
>> +	"BriefDescription": "Proportion of load & store operations",
>> +	"MetricGroup": "Instruction",
>> +	"MetricName": "Load-store mix"
>> +    },
>> +    {
>> +	"MetricExpr": "INST_RETIRED / (duration_time * 1000000)",
>
> I think that we may use 1e6 here for shorthand - it helps avoid mistakes with 
> too few or many '0's :)

Oh, that's great. I don't think anyone needed to use those in arm64 and I 
guess I didn't realize to take a look at other architectures. I'll change 
all the numbers.

>
>> +	"BriefDescription": "Millions of instructions per second",
>> +	"MetricGroup": "Instruction",
>> +	"MetricName": "MIPS_RETIRED"
>> +    },
>> +    {
>> +	"MetricExpr": "INST_SPEC / (duration_time * 1000000)",
>> +	"BriefDescription": "Millions of instructions per second",
>> +	"MetricGroup": "Instruction",
>> +	"MetricName": "MIPS_UTILIZATION"
>> +    },
>> +    {
>> +	"MetricExpr": "PC_WRITE_SPEC / OP_SPEC",
>> +	"BriefDescription": "Proportion of software change of PC operations",
>> +	"MetricGroup": "Instruction",
>> +	"MetricName": "PC write mix"
>> +    },
>> +    {
>> +	"MetricExpr": "ST_SPEC / OP_SPEC",
>> +	"BriefDescription": "Proportion of store operations",
>> +	"MetricGroup": "Instruction",
>> +	"MetricName": "Store mix"
>> +    },
>> +    {
>> +	"MetricExpr": "VFP_SPEC / OP_SPEC",
>> +	"BriefDescription": "Proportion of FP operations",
>> +	"MetricGroup": "Instruction",
>> +	"MetricName": "VFP mix"
>> +    },
>> +    {
>> +	"MetricExpr": "1 - (OP_RETIRED/ (CPU_CYCLES * 4))",
>> +	"BriefDescription": "Proportion of slots lost",
>> +	"MetricGroup": "Speculation / TDA",
>> +	"MetricName": "CPU lost"
>> +    },
>> +    {
>> +	"MetricExpr": "OP_RETIRED/ (CPU_CYCLES * 4)",
>> +	"BriefDescription": "Proportion of slots retiring",
>> +	"MetricGroup": "Speculation / TDA",
>> +	"MetricName": "CPU utilization"
>> +    },
>> +    {
>> +	"MetricExpr": "OP_RETIRED - OP_SPEC",
>> +	"BriefDescription": "Operations lost due to misspeculation",
>> +	"MetricGroup": "Speculation / TDA",
>> +	"MetricName": "Operations lost"
>> +    },
>> +    {
>> +	"MetricExpr": "1 - (OP_RETIRED / OP_SPEC)",
>> +	"BriefDescription": "Proportion of operations lost",
>> +	"MetricGroup": "Speculation / TDA",
>> +	"MetricName": "Operations lost (ratio)"
>> +    },
>> +    {
>> +	"MetricExpr": "OP_RETIRED / OP_SPEC",
>> +	"BriefDescription": "Proportion of operations retired",
>> +	"MetricGroup": "Speculation / TDA",
>> +	"MetricName": "Operations retired"
>> +    },
>> +    {
>> +	"MetricExpr": "STALL_BACKEND_CACHE / CPU_CYCLES",
>> +	"BriefDescription": "Proportion of cycles stalled and no operations 
> issued to backend and cache miss",
>> +	"MetricGroup": "Stall",
>> +	"MetricName": "Stall backend cache cycles"
>> +    },
>> +    {
>> +	"MetricExpr": "STALL_BACKEND_RESOURCE / CPU_CYCLES",
>> +	"BriefDescription": "Proportion of cycles stalled and no operations 
> issued to backend and resource full",
>> +	"MetricGroup": "Stall",
>> +	"MetricName": "Stall backend resource cycles"
>> +    },
>> +    {
>> +	"MetricExpr": "STALL_BACKEND_TLB / CPU_CYCLES",
>> +	"BriefDescription": "Proportion of cycles stalled and no operations 
> issued to backend and TLB miss",
>> +	"MetricGroup": "Stall",
>> +	"MetricName": "Stall backend tlb cycles"
>> +    },
>> +    {
>> +	"MetricExpr": "STALL_FRONTEND_CACHE / CPU_CYCLES",
>> +	"BriefDescription": "Proportion of cycles stalled and no ops 
> delivered from frontend and cache miss",
>> +	"MetricGroup": "Stall",
>> +	"MetricName": "Stall frontend cache cycles"
>> +    },
>> +    {
>> +	"MetricExpr": "STALL_FRONTEND_TLB / CPU_CYCLES",
>> +	"BriefDescription": "Proportion of cycles stalled and no ops 
> delivered from frontend and TLB miss",
>> +	"MetricGroup": "Stall",
>> +	"MetricName": "Stall frontend tlb cycles"
>> +    },
>> +    {
>> +	"MetricExpr": "DTLB_WALK / L1D_TLB",
>> +	"BriefDescription": "D-side walk per d-side translation request",
>> +	"MetricGroup": "TLB",
>> +	"MetricName": "DTLB walks"
>> +    },
>> +    {
>> +	"MetricExpr": "ITLB_WALK / L1I_TLB",
>> +	"BriefDescription": "I-side walk per i-side translation request",
>> +	"MetricGroup": "TLB",
>> +	"MetricName": "ITLB walks"
>> +    },
>> +    {
>> +        "MetricExpr": "STALL_SLOT_BACKEND / (CPU_CYCLES * 4)",
>> +        "BriefDescription": "Fraction of slots backend bound",
>> +        "MetricGroup": "TopDownL1",
>
> @Ian, should this be "Default;TopDownL1"?
>
>> +        "MetricName": "backend"
>
> How about use consistent names with other other archs and arm64 platforms, 
> like "backend_bound"? I did not check all names, but please consider this.
>
> If 'perf topdown' is ever supported for arm64, we would prob rely on 
> metricgroups, so would need use a fixed standard name here. Note that x86 
> uses custom kernel events for this instead.

That's an excellent point. I'll reach out to our architect and we'll 
change the names and groups in the patch and the document to be more 
consistent to the existing ones.

>> +    },
>> +    {
>> +        "MetricExpr": "1 - (retiring + lost + backend)",
>> +        "BriefDescription": "Fraction of slots frontend bound",
>> +        "MetricGroup": "TopDownL1",
>> +        "MetricName": "frontend"
>
> As above, it would be "frontend_bound"

I'll fix it.

Cheers, Ilkka

>
>> +    },
>> +    {
>> +        "MetricExpr": "((OP_SPEC - OP_RETIRED) / (CPU_CYCLES * 4))",
>> +        "BriefDescription": "Fraction of slots lost due to 
> misspeculation",
>> +        "MetricGroup": "TopDownL1",
>> +        "MetricName": "lost"
>> +    },
>> +    {
>
>