[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Thu, 12 Dec 2019 11:24:05 +0530
From: kajoljain <kjain@...ux.ibm.com>
To: Arnaldo Carvalho de Melo <arnaldo.melo@...il.com>,
Ravi Bangoria <ravi.bangoria@...ux.ibm.com>
Cc: Andi Kleen <ak@...ux.intel.com>, Jin Yao <yao.jin@...ux.intel.com>,
linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Jiri Olsa <jolsa@...nel.org>,
Kan Liang <kan.liang@...ux.intel.com>,
Peter Zijlstra <peterz@...radead.org>,
Madhavan Srinivasan <maddy@...ux.vnet.ibm.com>,
Anju T Sudhakar <anju@...ux.vnet.ibm.com>
Subject: Re: [PATCH] tools/perf/metricgroup: Fix printing event names of
metric group with multiple events
On 12/11/19 7:15 PM, Arnaldo Carvalho de Melo wrote:
> Em Wed, Dec 04, 2019 at 12:25:22PM +0530, Ravi Bangoria escreveu:
>>
>> On 11/20/19 2:10 PM, Kajol Jain wrote:
>>> 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
>>>
>>> In power9 platform:
>>> command:# ./perf stat --metric-only -M translation -C 0 -I 1000 sleep 2
>>> 1.000208486
>>> 2.000368863
>>> 2.001400558
>>>
>>> Similarly in skylake platform:
>>> command:./perf stat --metric-only -M Power -I 1000
>>> 1.000579994
>>> 2.002189493
>>>
>>> With current upstream version, issue is with event name comparison
>>> logic in find_evsel_group(). Current logic is to compare events
>>> belonging to a metric group to the events in perf_evlist.
>>> Since the break statement is missing in the loop used for comparison
>>> between metric group and perf_evlist events, the loop continues to
>>> execute even after getting a pattern match, and end up in discarding
>>> the matches.
>>> Incase of single metric event belongs to metric group, its working fine,
>>> because in case of single event once it compare all events it reaches to
>>> end of perf_evlist.
>>>
>>> Example for single metric event in power9 platform
>>> command:# ./perf stat --metric-only -M branches_per_inst -I 1000 sleep 1
>>> 1.000094653 0.2
>>> 1.001337059 0.0
>>>
>>> Patch fixes the issue by making sure once we found all events
>>> belongs to that metric event matched in find_evsel_group(), we
>>> successfully break from that loop by adding corresponding condition.
>>>
>>> With this patch:
>>> In power9 platform:
>>>
>>> command:# ./perf stat --metric-only -M translation -C 0 -I 1000 sleep 2
>>> result:# time derat_4k_miss_rate_percent derat_4k_miss_ratio
>>> derat_miss_ratio derat_64k_miss_rate_percent derat_64k_miss_ratio
>>> dslb_miss_rate_percent islb_miss_rate_percent
>>> 1.000135672 0.0 0.3
>>> 1.0 0.0 0.2
>>> 0.0 0.0
>>> 2.000380617 0.0 0.0
>>> 0.0 0.0
>>> 0.0 0.0
>>>
>>> command:# ./perf stat --metric-only -M Power -I 1000
>>>
>>> Similarly in skylake platform:
>>> result:# time Turbo_Utilization C3_Core_Residency
>>> C6_Core_Residency C7_Core_Residency C2_Pkg_Residency
>>> C3_Pkg_Residency C6_Pkg_Residency C7_Pkg_Residency
>>> 1.000563580 0.3 0.0
>>> 2.6 44.2 21.9
>>> 0.0 0.0 0.0
>>> 2.002235027 0.4 0.0
>>> 2.7 43.0 20.7
>>> 0.0 0.0 0.0
>>>
>>> Signed-off-by: Kajol Jain <kjain@...ux.ibm.com>
>>> Cc: Alexander Shishkin <alexander.shishkin@...ux.intel.com>
>>> Cc: Andi Kleen <ak@...ux.intel.com>
>>> Cc: Jiri Olsa <jolsa@...nel.org>
>>> Cc: Kan Liang <kan.liang@...ux.intel.com>
>>> Cc: Peter Zijlstra <peterz@...radead.org>
>>> Cc: Jin Yao <yao.jin@...ux.intel.com>
>>> Cc: Madhavan Srinivasan <maddy@...ux.vnet.ibm.com>
>>> Cc: Anju T Sudhakar <anju@...ux.vnet.ibm.com>
>>> Cc: Ravi Bangoria <ravi.bangoria@...ux.ibm.com>
>> Fixes: f01642e4912b ("perf metricgroup: Support multiple events for metricgroup")
>> Reviewed-by: Ravi Bangoria <ravi.bangoria@...ux.ibm.com>
> Tested on a Skylake machine and applied.
>
>> But while looking at the patch, I found that, commit f01642e4912b
>> has (again) screwed up logic for metric with overlapping events.
> Is someone looking into this?
Hi Arnaldo,
I am looking into this issue and will post the fix soon.
Kajol
>
>> $ 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
>>
>> This also needs to be fixed.
>>
>> Ravi
Powered by blists - more mailing lists