Re: [PATCH] perf metricgroup: Fix for metrics containing duration_time

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Wed, Jan 20, 2021 at 10:35:35PM +0100, Jiri Olsa escreveu:
> On Thu, Jan 21, 2021 at 12:18:38AM +0800, John Garry wrote:
> > Metrics containing duration_time cause a segfault:
> > 
> > $./perf stat -v -M L1D_Cache_Fill_BW sleep 1
> > Using CPUID GenuineIntel-6-3D-4
> > metric expr 64 * l1d.replacement / 1000000000 / duration_time for L1D_Cache_Fill_BW
> > found event duration_time
> > found event l1d.replacement
> > adding {l1d.replacement}:W,duration_time
> > l1d.replacement -> cpu/umask=0x1,(null)=0x1e8483,event=0x51/
> > Segmentation fault
> > 
> > In commit c2337d67199a ("perf metricgroup: Fix metrics using aliases
> > covering multiple PMUs"), the logic in find_evsel_group() when iter'ing
> > events was changed to not only select events in same group, but also for
> > aliased PMUs.
> > 
> > Checking whether events were for aliased PMUs was done by comparing the
> > event PMU name. This was not safe for duration_time event, which has no
> > associated PMU (and no PMU name), so fix by checking if the event PMU name
> > is set also.
> > 
> > Fixes: c2337d67199a ("perf metricgroup: Fix metrics using aliases covering multiple PMUs")
> > Reported-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > Signed-off-by: John Garry <john.garry@huawei.com>
> 
> Tested/Acked-by: Jiri Olsa <jolsa@redhat.com>

Thanks, applied.

- Arnaldo

 
> thanks,
> jirka
> 
> > 
> > diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> > index 2e60ee170abc..e6d3452031e5 100644
> > --- a/tools/perf/util/metricgroup.c
> > +++ b/tools/perf/util/metricgroup.c
> > @@ -162,6 +162,14 @@ static bool contains_event(struct evsel **metric_events, int num_events,
> >  	return false;
> >  }
> >  
> > +static bool evsel_same_pmu(struct evsel *ev1, struct evsel *ev2)
> > +{
> > +	if (!ev1->pmu_name || !ev2->pmu_name)
> > +		return false;
> > +
> > +	return !strcmp(ev1->pmu_name, ev2->pmu_name);
> > +}
> > +
> >  /**
> >   * Find a group of events in perf_evlist that correspond to those from a parsed
> >   * metric expression. Note, as find_evsel_group is called in the same order as
> > @@ -280,8 +288,7 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> >  			 */
> >  			if (!has_constraint &&
> >  			    ev->leader != metric_events[i]->leader &&
> > -			    !strcmp(ev->leader->pmu_name,
> > -				    metric_events[i]->leader->pmu_name))
> > +			    evsel_same_pmu(ev->leader, metric_events[i]->leader))
> >  				break;
> >  			if (!strcmp(metric_events[i]->name, ev->name)) {
> >  				set_bit(ev->idx, evlist_used);
> > -- 
> > 2.26.2
> > 
> 

-- 

- Arnaldo