* [PATCH] tools/perf/metricgroup: Fix printing event names of metric group with multiple events incase of overlapping events
@ 2020-01-08 6:58 Kajol Jain
2020-01-08 7:25 ` Jin, Yao
0 siblings, 1 reply; 7+ messages in thread
From: Kajol Jain @ 2020-01-08 6:58 UTC (permalink / raw)
To: acme
Cc: linux-kernel, linux-perf-users, kjain, Alexander Shishkin,
Andi Kleen, Jiri Olsa, Kan Liang, Peter Zijlstra, Jin Yao,
Madhavan Srinivasan, Anju T Sudhakar, Ravi Bangoria
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@linux.ibm.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Kan Liang <kan.liang@linux.intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Jin Yao <yao.jin@linux.intel.com>
Cc: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
Cc: Anju T Sudhakar <anju@linux.vnet.ibm.com>
Cc: Ravi Bangoria <ravi.bangoria@linux.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];
}
--
2.21.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] tools/perf/metricgroup: Fix printing event names of metric group with multiple events incase of overlapping events
2020-01-08 6:58 [PATCH] tools/perf/metricgroup: Fix printing event names of metric group with multiple events incase of overlapping events Kajol Jain
@ 2020-01-08 7:25 ` Jin, Yao
2020-01-08 9:11 ` kajoljain
0 siblings, 1 reply; 7+ messages in thread
From: Jin, Yao @ 2020-01-08 7:25 UTC (permalink / raw)
To: Kajol Jain, acme
Cc: linux-kernel, linux-perf-users, Alexander Shishkin, Andi Kleen,
Jiri Olsa, Kan Liang, Peter Zijlstra, Madhavan Srinivasan,
Anju T Sudhakar, Ravi Bangoria
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@linux.ibm.com>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Andi Kleen <ak@linux.intel.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Kan Liang <kan.liang@linux.intel.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Jin Yao <yao.jin@linux.intel.com>
> Cc: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
> Cc: Anju T Sudhakar <anju@linux.vnet.ibm.com>
> Cc: Ravi Bangoria <ravi.bangoria@linux.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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] tools/perf/metricgroup: Fix printing event names of metric group with multiple events incase of overlapping events
2020-01-08 7:25 ` Jin, Yao
@ 2020-01-08 9:11 ` kajoljain
2020-01-08 16:02 ` Jiri Olsa
0 siblings, 1 reply; 7+ messages in thread
From: kajoljain @ 2020-01-08 9:11 UTC (permalink / raw)
To: Jin, Yao, acme
Cc: linux-kernel, linux-perf-users, Alexander Shishkin, Andi Kleen,
Jiri Olsa, Kan Liang, Peter Zijlstra, Madhavan Srinivasan,
Anju T Sudhakar, Ravi Bangoria
On 1/8/20 12:55 PM, Jin, Yao wrote:
>
>
> 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@linux.ibm.com>
>> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>> Cc: Andi Kleen <ak@linux.intel.com>
>> Cc: Jiri Olsa <jolsa@kernel.org>
>> Cc: Kan Liang <kan.liang@linux.intel.com>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Cc: Jin Yao <yao.jin@linux.intel.com>
>> Cc: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
>> Cc: Anju T Sudhakar <anju@linux.vnet.ibm.com>
>> Cc: Ravi Bangoria <ravi.bangoria@linux.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?
Hi Jin,
The other way I come up with to solve this issue is, making change in
perf_evlist itself by adding some flag in event name, to keep track of
matched events.
As if we change event name itself, next time when we compare it won't
matched. But in that case we need to remove those flag later. Which will
increase the
complexity. If you have any suggestions, please let me know.
Thanks,
Kajol
>
> Thanks
> Jin Yao
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] tools/perf/metricgroup: Fix printing event names of metric group with multiple events incase of overlapping events
2020-01-08 9:11 ` kajoljain
@ 2020-01-08 16:02 ` Jiri Olsa
2020-01-20 8:53 ` kajoljain
0 siblings, 1 reply; 7+ messages in thread
From: Jiri Olsa @ 2020-01-08 16:02 UTC (permalink / raw)
To: kajoljain
Cc: Jin, Yao, acme, linux-kernel, linux-perf-users,
Alexander Shishkin, Andi Kleen, Jiri Olsa, Kan Liang,
Peter Zijlstra, Madhavan Srinivasan, Anju T Sudhakar,
Ravi Bangoria
On Wed, Jan 08, 2020 at 02:41:35PM +0530, kajoljain wrote:
SNIP
> > > - 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?
>
> Hi Jin,
>
> The other way I come up with to solve this issue is, making change in
> perf_evlist itself by adding some flag in event name, to keep track of
> matched events.
>
> As if we change event name itself, next time when we compare it won't
> matched. But in that case we need to remove those flag later. Which will
> increase the
>
> complexity. If you have any suggestions, please let me know.
we already keep evsel::cpu_iter for similar concept
so I guess we could have some iterator_perf_evlist variable in evlist..
that is if we don't find other solution (other than static varable)
thanks,
jirka
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] tools/perf/metricgroup: Fix printing event names of metric group with multiple events incase of overlapping events
2020-01-08 16:02 ` Jiri Olsa
@ 2020-01-20 8:53 ` kajoljain
2020-01-20 9:23 ` Jiri Olsa
0 siblings, 1 reply; 7+ messages in thread
From: kajoljain @ 2020-01-20 8:53 UTC (permalink / raw)
To: Jiri Olsa
Cc: Jin, Yao, acme, linux-kernel, linux-perf-users,
Alexander Shishkin, Andi Kleen, Jiri Olsa, Kan Liang,
Peter Zijlstra, Madhavan Srinivasan, Anju T Sudhakar,
Ravi Bangoria
On 1/8/20 9:32 PM, Jiri Olsa wrote:
> On Wed, Jan 08, 2020 at 02:41:35PM +0530, kajoljain wrote:
>
> SNIP
>
>>>> - 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?
>> Hi Jin,
>>
>> The other way I come up with to solve this issue is, making change in
>> perf_evlist itself by adding some flag in event name, to keep track of
>> matched events.
>>
>> As if we change event name itself, next time when we compare it won't
>> matched. But in that case we need to remove those flag later. Which will
>> increase the
>>
>> complexity. If you have any suggestions, please let me know.
> we already keep evsel::cpu_iter for similar concept
>
> so I guess we could have some iterator_perf_evlist variable in evlist..
> that is if we don't find other solution (other than static varable)
Hi Jiri,
Thanks for reviewing the patch. I checked 'evsel::cpu_iter'
variable, I think it added recently and I am not able to find any
similar kind of variable in
evlist. Please let me know if my understanding is fine. Do you
want me to add new variable in evlist itself or there is any other way
possible.
Thanks,
Kajol
> thanks,
> jirka
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] tools/perf/metricgroup: Fix printing event names of metric group with multiple events incase of overlapping events
2020-01-20 8:53 ` kajoljain
@ 2020-01-20 9:23 ` Jiri Olsa
2020-01-20 9:25 ` Jiri Olsa
0 siblings, 1 reply; 7+ messages in thread
From: Jiri Olsa @ 2020-01-20 9:23 UTC (permalink / raw)
To: kajoljain
Cc: Jin, Yao, acme, linux-kernel, linux-perf-users,
Alexander Shishkin, Andi Kleen, Jiri Olsa, Kan Liang,
Peter Zijlstra, Madhavan Srinivasan, Anju T Sudhakar,
Ravi Bangoria
On Mon, Jan 20, 2020 at 02:23:19PM +0530, kajoljain wrote:
>
> On 1/8/20 9:32 PM, Jiri Olsa wrote:
> > On Wed, Jan 08, 2020 at 02:41:35PM +0530, kajoljain wrote:
> >
> > SNIP
> >
> > > > > - 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?
> > > Hi Jin,
> > >
> > > The other way I come up with to solve this issue is, making change in
> > > perf_evlist itself by adding some flag in event name, to keep track of
> > > matched events.
> > >
> > > As if we change event name itself, next time when we compare it won't
> > > matched. But in that case we need to remove those flag later. Which will
> > > increase the
> > >
> > > complexity. If you have any suggestions, please let me know.
> > we already keep evsel::cpu_iter for similar concept
> >
> > so I guess we could have some iterator_perf_evlist variable in evlist..
> > that is if we don't find other solution (other than static varable)
>
> Hi Jiri,
>
> Thanks for reviewing the patch. I checked 'evsel::cpu_iter'
> variable, I think it added recently and I am not able to find any similar
> kind of variable in
>
> evlist. Please let me know if my understanding is fine. Do you want
> me to add new variable in evlist itself or there is any other way possible.
please check Arnaldo's tree:
git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git perf/core
jirka
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] tools/perf/metricgroup: Fix printing event names of metric group with multiple events incase of overlapping events
2020-01-20 9:23 ` Jiri Olsa
@ 2020-01-20 9:25 ` Jiri Olsa
0 siblings, 0 replies; 7+ messages in thread
From: Jiri Olsa @ 2020-01-20 9:25 UTC (permalink / raw)
To: kajoljain
Cc: Jin, Yao, acme, linux-kernel, linux-perf-users,
Alexander Shishkin, Andi Kleen, Jiri Olsa, Kan Liang,
Peter Zijlstra, Madhavan Srinivasan, Anju T Sudhakar,
Ravi Bangoria
On Mon, Jan 20, 2020 at 10:23:56AM +0100, Jiri Olsa wrote:
> On Mon, Jan 20, 2020 at 02:23:19PM +0530, kajoljain wrote:
> >
> > On 1/8/20 9:32 PM, Jiri Olsa wrote:
> > > On Wed, Jan 08, 2020 at 02:41:35PM +0530, kajoljain wrote:
> > >
> > > SNIP
> > >
> > > > > > - 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?
> > > > Hi Jin,
> > > >
> > > > The other way I come up with to solve this issue is, making change in
> > > > perf_evlist itself by adding some flag in event name, to keep track of
> > > > matched events.
> > > >
> > > > As if we change event name itself, next time when we compare it won't
> > > > matched. But in that case we need to remove those flag later. Which will
> > > > increase the
> > > >
> > > > complexity. If you have any suggestions, please let me know.
> > > we already keep evsel::cpu_iter for similar concept
> > >
> > > so I guess we could have some iterator_perf_evlist variable in evlist..
> > > that is if we don't find other solution (other than static varable)
> >
> > Hi Jiri,
> >
> > Thanks for reviewing the patch. I checked 'evsel::cpu_iter'
> > variable, I think it added recently and I am not able to find any similar
> > kind of variable in
> >
> > evlist. Please let me know if my understanding is fine. Do you want
> > me to add new variable in evlist itself or there is any other way possible.
ah sry, it's in 'struct evsel' ;-)
jirka
>
> please check Arnaldo's tree:
> git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git perf/core
>
> jirka
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-01-20 9:25 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-08 6:58 [PATCH] tools/perf/metricgroup: Fix printing event names of metric group with multiple events incase of overlapping events Kajol Jain
2020-01-08 7:25 ` Jin, Yao
2020-01-08 9:11 ` kajoljain
2020-01-08 16:02 ` Jiri Olsa
2020-01-20 8:53 ` kajoljain
2020-01-20 9:23 ` Jiri Olsa
2020-01-20 9:25 ` Jiri Olsa
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.