Re: [PATCH v4] tools/perf/metricgroup: Fix printing event names of metric group with multiple events incase of overlapping events

From: kajoljain
Date: Thu Feb 20 2020 - 04:54:15 EST



On 2/17/20 8:41 AM, Joakim Zhang wrote:
>
>> -----Original Message-----
>> From: linux-perf-users-owner@xxxxxxxxxxxxxxx
>> <linux-perf-users-owner@xxxxxxxxxxxxxxx> On Behalf Of Kajol Jain
>> Sent: 2020年2月12日 13:41
>> To: acme@xxxxxxxxxx
>> Cc: linux-kernel@xxxxxxxxxxxxxxx; linux-perf-users@xxxxxxxxxxxxxxx;
>> kjain@xxxxxxxxxxxxx; Jiri Olsa <jolsa@xxxxxxxxxx>; Alexander Shishkin
>> <alexander.shishkin@xxxxxxxxxxxxxxx>; Andi Kleen <ak@xxxxxxxxxxxxxxx>; Kan
>> Liang <kan.liang@xxxxxxxxxxxxxxx>; Peter Zijlstra <peterz@xxxxxxxxxxxxx>; Jin
>> Yao <yao.jin@xxxxxxxxxxxxxxx>; Madhavan Srinivasan
>> <maddy@xxxxxxxxxxxxxxxxxx>; Anju T Sudhakar <anju@xxxxxxxxxxxxxxxxxx>;
>> Ravi Bangoria <ravi.bangoria@xxxxxxxxxxxxx>
>> Subject: [PATCH v4] tools/perf/metricgroup: Fix printing event names of metric
>> group with multiple events incase of overlapping events
>>
>> Commit f01642e4912b ("perf metricgroup: Support multiple events for
>> metricgroup") introduced support for multiple events in a metric group. But
>> with the current upstream, metric events names are not printed properly
>> incase we try to run multiple metric groups with overlapping event.
>>
>> With current upstream version, incase of overlapping metric events issue is, we
>> always start our comparision logic from start.
>> So, the events which already matched with some metric group also take part in
>> comparision logic. Because of that when we have overlapping events, we end
>> up matching current metric group event with already matched one.
>>
>> For example, in skylake machine we have metric event CoreIPC and
>> Instructions. Both of them need 'inst_retired.any' event value.
>> As events in Instructions is subset of events in CoreIPC, they endup in pointing
>> to same 'inst_retired.any' value.
>>
>> In skylake platform:
>>
>> command:# ./perf stat -M CoreIPC,Instructions -C 0 sleep 1
>>
>> Performance counter stats for 'CPU(s) 0':
>>
>> 1,254,992,790 inst_retired.any # 1254992790.0
>> Instructions
>> # 1.3
>> CoreIPC
>> 977,172,805 cycles
>> 1,254,992,756 inst_retired.any
>>
>> 1.000802596 seconds time elapsed
>>
>> command:# sudo ./perf stat -M UPI,IPC sleep 1
>>
>> Performance counter stats for 'sleep 1':
>>
>> 948,650 uops_retired.retire_slots
>> 866,182 inst_retired.any # 0.7 IPC
>> 866,182 inst_retired.any
>> 1,175,671 cpu_clk_unhalted.thread
>>
>> Patch fixes the issue by adding a new bool pointer 'evlist_used' to keep track of
>> events which already matched with some group by setting it true.
>> So, we skip all used events in list when we start comparision logic.
>> Patch also make some changes in comparision logic, incase we get a match
>> miss, we discard the whole match and start again with first event id in metric
>> event.
>>
>> With this patch:
>> In skylake platform:
>>
>> command:# ./perf stat -M CoreIPC,Instructions -C 0 sleep 1
>>
>> Performance counter stats for 'CPU(s) 0':
>>
>> 3,348,415 inst_retired.any # 0.3 CoreIPC
>> 11,779,026 cycles
>> 3,348,381 inst_retired.any # 3348381.0
>> Instructions
>>
>> 1.001649056 seconds time elapsed
>>
>> command:# ./perf stat -M UPI,IPC sleep 1
>>
>> Performance counter stats for 'sleep 1':
>>
>> 1,023,148 uops_retired.retire_slots # 1.1 UPI
>> 924,976 inst_retired.any
>> 924,976 inst_retired.any # 0.6 IPC
>> 1,489,414 cpu_clk_unhalted.thread
>>
>> 1.003064672 seconds time elapsed
>>
>> Signed-off-by: Kajol Jain <kjain@xxxxxxxxxxxxx>
>> Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
>> Cc: Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>
>> Cc: Andi Kleen <ak@xxxxxxxxxxxxxxx>
>> Cc: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
>> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
>> Cc: Jin Yao <yao.jin@xxxxxxxxxxxxxxx>
>> Cc: Madhavan Srinivasan <maddy@xxxxxxxxxxxxxxxxxx>
>> Cc: Anju T Sudhakar <anju@xxxxxxxxxxxxxxxxxx>
>> Cc: Ravi Bangoria <ravi.bangoria@xxxxxxxxxxxxx>
>> ---
>> tools/perf/util/metricgroup.c | 50 ++++++++++++++++++++++-------------
>> 1 file changed, 31 insertions(+), 19 deletions(-)
>
> Hi Kajol,
>
> I am not sure if it is good to ask a question here :-)
>
> I encountered a perf metricgroup issue, the result is incorrect when the metric includes more than 2 events.
>
> git log --oneline tools/perf/util/metricgroup.c
> 3635b27cc058 perf metricgroup: Fix printing event names of metric group with multiple events
> f01642e4912b perf metricgroup: Support multiple events for metricgroup
> 287f2649f791 perf metricgroup: Scale the metric result
>
> I did a simple test, below is the JSON file and result.
> [
> {
> "PublicDescription": "Calculate DDR0 bus actual utilization which vary from DDR0 controller clock frequency",
> "BriefDescription": "imx8qm: ddr0 bus actual utilization",
> "MetricName": "imx8qm-ddr0-bus-util",
> "MetricExpr": "( imx8_ddr0\\/read\\-cycles\\/ + imx8_ddr0\\/write\\-cycles\\/ )",
> "MetricGroup": "i.MX8QM_DDR0_BUS_UTIL"
> }
> ]
> ./perf stat -I 1000 -M imx8qm-ddr0-bus-util
> # time counts unit events
> 1.000104250 16720 imx8_ddr0/read-cycles/ # 22921.0 imx8qm-ddr0-bus-util
> 1.000104250 6201 imx8_ddr0/write-cycles/
> 2.000525625 8316 imx8_ddr0/read-cycles/ # 12785.5 imx8qm-ddr0-bus-util
> 2.000525625 2738 imx8_ddr0/write-cycles/
> 3.000819125 1056 imx8_ddr0/read-cycles/ # 4136.7 imx8qm-ddr0-bus-util
> 3.000819125 303 imx8_ddr0/write-cycles/
> 4.001103750 6260 imx8_ddr0/read-cycles/ # 9149.8 imx8qm-ddr0-bus-util
> 4.001103750 2317 imx8_ddr0/write-cycles/
> 5.001392750 2084 imx8_ddr0/read-cycles/ # 4516.0 imx8qm-ddr0-bus-util
> 5.001392750 601 imx8_ddr0/write-cycles/
>
> You can see that only the first result is correct, could this be reproduced at you side?

Hi Joakim,
Will try to look into it from my side.
Thanks,
Kajol
>
> Thanks a lot!
>
> Best Regards,
> Joakim Zhang
>