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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e866c12a-7328-8524-fd0e-668301da6875@linux.intel.com>
Date:   Wed, 8 Jan 2020 15:25:24 +0800
From:   "Jin, Yao" <yao.jin@...ux.intel.com>
To:     Kajol Jain <kjain@...ux.ibm.com>, acme@...nel.org
Cc:     linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Andi Kleen <ak@...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>,
        Ravi Bangoria <ravi.bangoria@...ux.ibm.com>
Subject: Re: [PATCH] tools/perf/metricgroup: Fix printing event names of
 metric group with multiple events incase of overlapping events



On 1/8/2020 2:58 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 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 static variable 'iterator_perf_evlist'
> to keep track of events which already matched with some group. It points
> to event in perf_evlist from where next match should start. Because we
> need to make sure, we match correct set of events belongs to
> corresponding metric group.
> 
> With this patch:
> In skylake platform:
> 
> command:# ./perf stat -M CoreIPC,Instructions  -C 0 sleep 1
> 
>   Performance counter stats for 'CPU(s) 0':
> 
>         149,481,533      inst_retired.any          #      0.8 CoreIPC
>         186,244,218      cycles
>         149,479,362      inst_retired.any          # 149479362.0
> 							Instructions
> 
>         1.001655885 seconds time elapsed
> 
> command:# ./perf stat -M UPI,IPC sleep 1
>   Performance counter stats for 'CPU(s) 0':
> 
>          16,858,849      uops_retired.retire_slots #      1.3 UPI
>          12,529,178      inst_retired.any
>          12,529,558      inst_retired.any          #      0.3 IPC
>          39,936,071      cpu_clk_unhalted.thread
> 
>         1.001413978 seconds time elapsed
> 
> 
> 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>
> ---
>   tools/perf/util/metricgroup.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 35e151b8359b..58889b0496fb 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -90,16 +90,21 @@ struct egroup {
>   	const char *metric_unit;
>   };
>   
> +static int iterator_perf_evlist;
> +
>   static struct evsel *find_evsel_group(struct evlist *perf_evlist,
>   				      const char **ids,
>   				      int idnum,
>   				      struct evsel **metric_events)
>   {
>   	struct evsel *ev;
> -	int i = 0;
> +	int i = 0, j = 0;
>   	bool leader_found;
>   
>   	evlist__for_each_entry (perf_evlist, ev) {
> +		j++;
> +		if (j <= iterator_perf_evlist)
> +			continue;
>   		if (!strcmp(ev->name, ids[i])) {
>   			if (!metric_events[i])
>   				metric_events[i] = ev;
> @@ -146,6 +151,7 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
>   			}
>   		}
>   	}
> +	iterator_perf_evlist = j;
>   
>   	return metric_events[0];
>   }
> 

Thanks for reporting and fixing this issue.

I just have one question, do we really need a *static variable* to track 
the matched events? Perhaps using an input parameter?

Thanks
Jin Yao