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

[kajoljain]

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