* [PATCH] perf metricgroup: Fix system PMU metrics
@ 2021-01-19 10:04 John Garry
2021-01-19 10:56 ` Joakim Zhang
2021-01-20 21:42 ` Jiri Olsa
0 siblings, 2 replies; 16+ messages in thread
From: John Garry @ 2021-01-19 10:04 UTC (permalink / raw)
To: peterz, mingo, acme, mark.rutland, alexander.shishkin, jolsa,
namhyung, irogers, kjain
Cc: linux-kernel, linuxarm, qiangqing.zhang, John Garry
Joakim reports that getting "perf stat" for multiple system PMU metrics
segfaults:
./perf stat -a -I 1000 -M imx8mm_ddr_write.all,imx8mm_ddr_write.all
Segmentation fault
While the same works without issue for a single metric.
The logic in metricgroup__add_metric_sys_event_iter() is broken, in that
add_metric() @m argument should be NULL for each new metric. Fix by not
passing a holder for that, and rather make local in
metricgroup__add_metric_sys_event_iter().
Fixes: be335ec28efa ("perf metricgroup: Support adding metrics for system PMUs")
Reported-by: Joakim Zhang <qiangqing.zhang@nxp.com>
Signed-off-by: John Garry <john.garry@huawei.com>
diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index ee94d3e8dd65..2e60ee170abc 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -766,7 +766,6 @@ int __weak arch_get_runtimeparam(struct pmu_event *pe __maybe_unused)
struct metricgroup_add_iter_data {
struct list_head *metric_list;
const char *metric;
- struct metric **m;
struct expr_ids *ids;
int *ret;
bool *has_match;
@@ -1058,12 +1057,13 @@ static int metricgroup__add_metric_sys_event_iter(struct pmu_event *pe,
void *data)
{
struct metricgroup_add_iter_data *d = data;
+ struct metric *m = NULL;
int ret;
if (!match_pe_metric(pe, d->metric))
return 0;
- ret = add_metric(d->metric_list, pe, d->metric_no_group, d->m, NULL, d->ids);
+ ret = add_metric(d->metric_list, pe, d->metric_no_group, &m, NULL, d->ids);
if (ret)
return ret;
@@ -1114,7 +1114,6 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,
.metric_list = &list,
.metric = metric,
.metric_no_group = metric_no_group,
- .m = &m,
.ids = &ids,
.has_match = &has_match,
.ret = &ret,
--
2.26.2
^ permalink raw reply related [flat|nested] 16+ messages in thread* RE: [PATCH] perf metricgroup: Fix system PMU metrics 2021-01-19 10:04 [PATCH] perf metricgroup: Fix system PMU metrics John Garry @ 2021-01-19 10:56 ` Joakim Zhang 2021-01-19 11:04 ` John Garry ` (2 more replies) 2021-01-20 21:42 ` Jiri Olsa 1 sibling, 3 replies; 16+ messages in thread From: Joakim Zhang @ 2021-01-19 10:56 UTC (permalink / raw) To: John Garry, peterz, mingo, acme, mark.rutland, alexander.shishkin, jolsa, namhyung, irogers, kjain Cc: linux-kernel, linuxarm > -----Original Message----- > From: John Garry <john.garry@huawei.com> > Sent: 2021年1月19日 18:04 > To: peterz@infradead.org; mingo@redhat.com; acme@kernel.org; > mark.rutland@arm.com; alexander.shishkin@linux.intel.com; > jolsa@redhat.com; namhyung@kernel.org; irogers@google.com; > kjain@linux.ibm.com > Cc: linux-kernel@vger.kernel.org; linuxarm@openeuler.org; Joakim Zhang > <qiangqing.zhang@nxp.com>; John Garry <john.garry@huawei.com> > Subject: [PATCH] perf metricgroup: Fix system PMU metrics > > Joakim reports that getting "perf stat" for multiple system PMU metrics > segfaults: > ./perf stat -a -I 1000 -M imx8mm_ddr_write.all,imx8mm_ddr_write.all > Segmentation fault > > While the same works without issue for a single metric. > > The logic in metricgroup__add_metric_sys_event_iter() is broken, in that > add_metric() @m argument should be NULL for each new metric. Fix by not > passing a holder for that, and rather make local in > metricgroup__add_metric_sys_event_iter(). > > Fixes: be335ec28efa ("perf metricgroup: Support adding metrics for system > PMUs") > Reported-by: Joakim Zhang <qiangqing.zhang@nxp.com> > Signed-off-by: John Garry <john.garry@huawei.com> root@imx8mmevk:~# ./perf stat -a -I 1000 -M imx8mm_ddr_read.all,imx8mm_ddr_write .all # time counts unit events 1.001446500 40832 imx8mm_ddr.read_cycles # 638.0 KB imx8mm_ddr_read.all 1.001446500 16973 imx8mm_ddr.write_cycles # 265.2 KB imx8mm_ddr_write.all 2.003150250 28836 imx8mm_ddr.read_cycles # 450.6 KB imx8mm_ddr_read.all 2.003150250 6705 imx8mm_ddr.write_cycles # 104.8 KB imx8mm_ddr_write.all For this issue, Tested-by: Joakim Zhang <qiangqing.zhang@nxp.com> Hi John, It seems have other issue compared to 5.10 kernel after switching to this framework, below metric can't work. "MetricExpr": "(( imx8_ddr0@read\\-cycles@ + imx8_ddr0@write\\-cycles@ ) * 4 * 4 / duration_time) / (750 * 1000000 * 4 * 4)" After change to: "MetricExpr": "(( imx8mm_ddr.read_cycles + imx8mm_ddr.write_cycles ) * 4 * 4 / duration_time) / (750 * 1000000 * 4 * 4)", Best Regards, Joakim Zhang > diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c index > ee94d3e8dd65..2e60ee170abc 100644 > --- a/tools/perf/util/metricgroup.c > +++ b/tools/perf/util/metricgroup.c > @@ -766,7 +766,6 @@ int __weak arch_get_runtimeparam(struct pmu_event > *pe __maybe_unused) struct metricgroup_add_iter_data { > struct list_head *metric_list; > const char *metric; > - struct metric **m; > struct expr_ids *ids; > int *ret; > bool *has_match; > @@ -1058,12 +1057,13 @@ static int > metricgroup__add_metric_sys_event_iter(struct pmu_event *pe, > void *data) > { > struct metricgroup_add_iter_data *d = data; > + struct metric *m = NULL; > int ret; > > if (!match_pe_metric(pe, d->metric)) > return 0; > > - ret = add_metric(d->metric_list, pe, d->metric_no_group, d->m, NULL, > d->ids); > + ret = add_metric(d->metric_list, pe, d->metric_no_group, &m, NULL, > +d->ids); > if (ret) > return ret; > > @@ -1114,7 +1114,6 @@ static int metricgroup__add_metric(const char > *metric, bool metric_no_group, > .metric_list = &list, > .metric = metric, > .metric_no_group = metric_no_group, > - .m = &m, > .ids = &ids, > .has_match = &has_match, > .ret = &ret, > -- > 2.26.2 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] perf metricgroup: Fix system PMU metrics 2021-01-19 10:56 ` Joakim Zhang @ 2021-01-19 11:04 ` John Garry 2021-01-19 11:13 ` Joakim Zhang 2021-01-19 12:02 ` John Garry 2021-01-19 15:47 ` John Garry 2 siblings, 1 reply; 16+ messages in thread From: John Garry @ 2021-01-19 11:04 UTC (permalink / raw) To: Joakim Zhang, peterz, mingo, acme, mark.rutland, alexander.shishkin, jolsa, namhyung, irogers, kjain Cc: linux-kernel, linuxarm On 19/01/2021 10:56, Joakim Zhang wrote: >> Joakim reports that getting "perf stat" for multiple system PMU metrics >> segfaults: >> ./perf stat -a -I 1000 -M imx8mm_ddr_write.all,imx8mm_ddr_write.all >> Segmentation fault >> >> While the same works without issue for a single metric. >> >> The logic in metricgroup__add_metric_sys_event_iter() is broken, in that >> add_metric() @m argument should be NULL for each new metric. Fix by not >> passing a holder for that, and rather make local in >> metricgroup__add_metric_sys_event_iter(). >> >> Fixes: be335ec28efa ("perf metricgroup: Support adding metrics for system >> PMUs") >> Reported-by: Joakim Zhang<qiangqing.zhang@nxp.com> >> Signed-off-by: John Garry<john.garry@huawei.com> > root@imx8mmevk:~# ./perf stat -a -I 1000 -M imx8mm_ddr_read.all,imx8mm_ddr_write .all > # time counts unit events > 1.001446500 40832 imx8mm_ddr.read_cycles # 638.0 KB imx8mm_ddr_read.all > 1.001446500 16973 imx8mm_ddr.write_cycles # 265.2 KB imx8mm_ddr_write.all > 2.003150250 28836 imx8mm_ddr.read_cycles # 450.6 KB imx8mm_ddr_read.all > 2.003150250 6705 imx8mm_ddr.write_cycles # 104.8 KB imx8mm_ddr_write.all > > For this issue, Tested-by: Joakim Zhang<qiangqing.zhang@nxp.com> > > Hi John, > > It seems have other issue compared to 5.10 kernel after switching to this framework, below metric can't work. > "MetricExpr": "(( imx8_ddr0@read\\-cycles@ + imx8_ddr0@write\\-cycles@ ) * 4 * 4 / duration_time) / (750 * 1000000 * 4 * 4)" > After change to: > "MetricExpr": "(( imx8mm_ddr.read_cycles + imx8mm_ddr.write_cycles ) * 4 * 4 / duration_time) / (750 * 1000000 * 4 * 4)", Hmmm... not sure what you mean by "compared to 5.10 kernel". As far as I'm concerned, none of this was supported in 5.10 and metrics did not work for arm64. Support for sys PMU events+metrics only came in 5.11-rc. Anyway, can you share the full metric event which you say does not work, not just the "MetricExpr"? Thanks, John ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH] perf metricgroup: Fix system PMU metrics 2021-01-19 11:04 ` John Garry @ 2021-01-19 11:13 ` Joakim Zhang 0 siblings, 0 replies; 16+ messages in thread From: Joakim Zhang @ 2021-01-19 11:13 UTC (permalink / raw) To: John Garry, peterz, mingo, acme, mark.rutland, alexander.shishkin, jolsa, namhyung, irogers, kjain Cc: linux-kernel, linuxarm > -----Original Message----- > From: John Garry <john.garry@huawei.com> > Sent: 2021年1月19日 19:05 > To: Joakim Zhang <qiangqing.zhang@nxp.com>; peterz@infradead.org; > mingo@redhat.com; acme@kernel.org; mark.rutland@arm.com; > alexander.shishkin@linux.intel.com; jolsa@redhat.com; > namhyung@kernel.org; irogers@google.com; kjain@linux.ibm.com > Cc: linux-kernel@vger.kernel.org; linuxarm@openeuler.org > Subject: Re: [PATCH] perf metricgroup: Fix system PMU metrics > > On 19/01/2021 10:56, Joakim Zhang wrote: > >> Joakim reports that getting "perf stat" for multiple system PMU > >> metrics > >> segfaults: > >> ./perf stat -a -I 1000 -M imx8mm_ddr_write.all,imx8mm_ddr_write.all > >> Segmentation fault > >> > >> While the same works without issue for a single metric. > >> > >> The logic in metricgroup__add_metric_sys_event_iter() is broken, in > >> that > >> add_metric() @m argument should be NULL for each new metric. Fix by > >> not passing a holder for that, and rather make local in > >> metricgroup__add_metric_sys_event_iter(). > >> > >> Fixes: be335ec28efa ("perf metricgroup: Support adding metrics for > >> system > >> PMUs") > >> Reported-by: Joakim Zhang<qiangqing.zhang@nxp.com> > >> Signed-off-by: John Garry<john.garry@huawei.com> > > root@imx8mmevk:~# ./perf stat -a -I 1000 -M > imx8mm_ddr_read.all,imx8mm_ddr_write > > .all > > # time counts unit events > > 1.001446500 40832 > imx8mm_ddr.read_cycles # 638.0 KB imx8mm_ddr_read.all > > 1.001446500 16973 > imx8mm_ddr.write_cycles # 265.2 KB imx8mm_ddr_write.all > > 2.003150250 28836 > imx8mm_ddr.read_cycles # 450.6 KB imx8mm_ddr_read.all > > 2.003150250 6705 > imx8mm_ddr.write_cycles # 104.8 KB imx8mm_ddr_write.all > > > > For this issue, Tested-by: Joakim Zhang<qiangqing.zhang@nxp.com> > > > > Hi John, > > > > It seems have other issue compared to 5.10 kernel after switching to this > framework, below metric can't work. > > "MetricExpr": "(( imx8_ddr0@read\\-cycles@ + imx8_ddr0@write\\-cycles@ ) > * 4 * 4 / duration_time) / (750 * 1000000 * 4 * 4)" > > After change to: > > "MetricExpr": "(( imx8mm_ddr.read_cycles + imx8mm_ddr.write_cycles ) * > > 4 * 4 / duration_time) / (750 * 1000000 * 4 * 4)", > > Hmmm... not sure what you mean by "compared to 5.10 kernel". As far as I'm > concerned, none of this was supported in 5.10 and metrics did not work for > arm64. Support for sys PMU events+metrics only came in 5.11-rc. Yes, 5.10 doesn't support ARM64. I add some code let it work locally. And, "MetricExpr": "(( imx8_ddr0@read\\-cycles@ + imx8_ddr0@write\\-cycles@ ) * 4 * 4 / duration_time) / (750 * 1000000 * 4 * 4)" Above metric expression can work fine. > Anyway, can you share the full metric event which you say does not work, not > just the "MetricExpr"? OK, Could help check below metric? Thanks. "MetricExpr": "(( imx8_ddr0@read\\-cycles@ + imx8_ddr0@write\\-cycles@ ) * 4 * 4 / duration_time) / (750 * 1000000 * 4 * 4)" or "MetricExpr": "(( imx8mm_ddr.read_cycles + imx8mm_ddr.write_cycles ) * 4 * 4 / duration_time) / (750 * 1000000 * 4 * 4)" Best Regards, Joakim Zhang > Thanks, > John ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] perf metricgroup: Fix system PMU metrics 2021-01-19 10:56 ` Joakim Zhang 2021-01-19 11:04 ` John Garry @ 2021-01-19 12:02 ` John Garry 2021-01-19 15:47 ` John Garry 2 siblings, 0 replies; 16+ messages in thread From: John Garry @ 2021-01-19 12:02 UTC (permalink / raw) To: Joakim Zhang, peterz, mingo, acme, mark.rutland, alexander.shishkin, jolsa, namhyung, irogers, kjain Cc: linux-kernel, linuxarm On 19/01/2021 10:56, Joakim Zhang wrote: > root@imx8mmevk:~# ./perf stat -a -I 1000 -M imx8mm_ddr_read.all,imx8mm_ddr_write .all > # time counts unit events > 1.001446500 40832 imx8mm_ddr.read_cycles # 638.0 KB imx8mm_ddr_read.all > 1.001446500 16973 imx8mm_ddr.write_cycles # 265.2 KB imx8mm_ddr_write.all > 2.003150250 28836 imx8mm_ddr.read_cycles # 450.6 KB imx8mm_ddr_read.all > 2.003150250 6705 imx8mm_ddr.write_cycles # 104.8 KB imx8mm_ddr_write.all > > For this issue, Tested-by: Joakim Zhang<qiangqing.zhang@nxp.com> > > Hi John, > > It seems have other issue compared to 5.10 kernel after switching to this framework, below metric can't work. > "MetricExpr": "(( imx8_ddr0@read\\-cycles@ + imx8_ddr0@write\\-cycles@ ) * 4 * 4 / duration_time) / (750 * 1000000 * 4 * 4)" > After change to: > "MetricExpr": "(( imx8mm_ddr.read_cycles + imx8mm_ddr.write_cycles ) * 4 * 4 / duration_time) / (750 * 1000000 * 4 * 4)", > OK, I can have a look. So is there anything else, apart from this, which doesn't work now? Thanks, john ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] perf metricgroup: Fix system PMU metrics 2021-01-19 10:56 ` Joakim Zhang 2021-01-19 11:04 ` John Garry 2021-01-19 12:02 ` John Garry @ 2021-01-19 15:47 ` John Garry 2021-01-19 17:33 ` John Garry 2 siblings, 1 reply; 16+ messages in thread From: John Garry @ 2021-01-19 15:47 UTC (permalink / raw) To: Joakim Zhang, peterz, mingo, acme, mark.rutland, alexander.shishkin, jolsa, namhyung, irogers, kjain Cc: linux-kernel, linuxarm On 19/01/2021 10:56, Joakim Zhang wrote: > It seems have other issue compared to 5.10 kernel after switching to this framework, below metric can't work. > "MetricExpr": "(( imx8_ddr0@read\\-cycles@ + imx8_ddr0@write\\-cycles@ ) * 4 * 4 / duration_time) / (750 * 1000000 * 4 * 4)" > After change to: > "MetricExpr": "(( imx8mm_ddr.read_cycles + imx8mm_ddr.write_cycles ) * 4 * 4 / duration_time) / (750 * 1000000 * 4 * 4)", It seems that any metric which includes "duration_time" is broken, even on x86: john@localhost:~/acme/tools/perf> sudo ./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 Seems to be from my commit c2337d67199 ("perf metricgroup: Fix metrics using aliases covering multiple PMUs") I'll look to fix it now. Thanks, John ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] perf metricgroup: Fix system PMU metrics 2021-01-19 15:47 ` John Garry @ 2021-01-19 17:33 ` John Garry 2021-01-20 5:15 ` Joakim Zhang 2021-01-20 12:56 ` Jiri Olsa 0 siblings, 2 replies; 16+ messages in thread From: John Garry @ 2021-01-19 17:33 UTC (permalink / raw) To: Joakim Zhang, peterz, mingo, acme, mark.rutland, alexander.shishkin, jolsa, namhyung, irogers, kjain Cc: linux-kernel, linuxarm On 19/01/2021 15:47, John Garry wrote: > On 19/01/2021 10:56, Joakim Zhang wrote: >> It seems have other issue compared to 5.10 kernel after switching to >> this framework, below metric can't work. >> "MetricExpr": "(( imx8_ddr0@read\\-cycles@ + imx8_ddr0@write\\-cycles@ >> ) * 4 * 4 / duration_time) / (750 * 1000000 * 4 * 4)" >> After change to: >> "MetricExpr": "(( imx8mm_ddr.read_cycles + imx8mm_ddr.write_cycles ) * >> 4 * 4 / duration_time) / (750 * 1000000 * 4 * 4)", > > It seems that any metric which includes "duration_time" is broken, even > on x86: > > john@localhost:~/acme/tools/perf> sudo ./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 > > > Seems to be from my commit c2337d67199 ("perf metricgroup: Fix metrics > using aliases covering multiple PMUs") > > I'll look to fix it now. > Please try this: From 2380f1ef0250e6818b3dbc7bff4a868810875e2a Mon Sep 17 00:00:00 2001 From: John Garry <john.garry@huawei.com> Date: Tue, 19 Jan 2021 17:29:54 +0000 Subject: [PATCH] perf metricgroup: Fix metric support for duration_time For a metric using duration_time, the strcmp() check when finding identical events in metric_events[] is broken, as it does not consider that the event pmu_name is NULL - it would be for duration_time. As such, add a NULL check here for event pmu_name. Signed-off-by: John Garry <john.garry@huawei.com> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c index ee94d3e8dd65..277adff8017f 100644 --- a/tools/perf/util/metricgroup.c +++ b/tools/perf/util/metricgroup.c @@ -280,6 +280,8 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist, */ if (!has_constraint && ev->leader != metric_events[i]->leader && + ev->leader->pmu_name && + metric_events[i]->leader->pmu_name && !strcmp(ev->leader->pmu_name, metric_events[i]->leader->pmu_name)) break; -- 2.26.2 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* RE: [PATCH] perf metricgroup: Fix system PMU metrics 2021-01-19 17:33 ` John Garry @ 2021-01-20 5:15 ` Joakim Zhang 2021-01-20 9:15 ` John Garry 2021-01-20 12:56 ` Jiri Olsa 1 sibling, 1 reply; 16+ messages in thread From: Joakim Zhang @ 2021-01-20 5:15 UTC (permalink / raw) To: John Garry, peterz, mingo, acme, mark.rutland, alexander.shishkin, jolsa, namhyung, irogers, kjain Cc: linux-kernel, linuxarm > -----Original Message----- > From: John Garry <john.garry@huawei.com> > Sent: 2021年1月20日 1:33 > To: Joakim Zhang <qiangqing.zhang@nxp.com>; peterz@infradead.org; > mingo@redhat.com; acme@kernel.org; mark.rutland@arm.com; > alexander.shishkin@linux.intel.com; jolsa@redhat.com; > namhyung@kernel.org; irogers@google.com; kjain@linux.ibm.com > Cc: linux-kernel@vger.kernel.org; linuxarm@openeuler.org > Subject: Re: [PATCH] perf metricgroup: Fix system PMU metrics > > On 19/01/2021 15:47, John Garry wrote: > > On 19/01/2021 10:56, Joakim Zhang wrote: > >> It seems have other issue compared to 5.10 kernel after switching to > >> this framework, below metric can't work. > >> "MetricExpr": "(( imx8_ddr0@read\\-cycles@ + > >> imx8_ddr0@write\\-cycles@ > >> ) * 4 * 4 / duration_time) / (750 * 1000000 * 4 * 4)" > >> After change to: > >> "MetricExpr": "(( imx8mm_ddr.read_cycles + imx8mm_ddr.write_cycles ) > >> * > >> 4 * 4 / duration_time) / (750 * 1000000 * 4 * 4)", > > > > It seems that any metric which includes "duration_time" is broken, > > even on x86: > > > > john@localhost:~/acme/tools/perf> sudo ./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 > > > > > > Seems to be from my commit c2337d67199 ("perf metricgroup: Fix metrics > > using aliases covering multiple PMUs") > > > > I'll look to fix it now. > > > > Please try this: > > From 2380f1ef0250e6818b3dbc7bff4a868810875e2a Mon Sep 17 00:00:00 > 2001 > From: John Garry <john.garry@huawei.com> > Date: Tue, 19 Jan 2021 17:29:54 +0000 > Subject: [PATCH] perf metricgroup: Fix metric support for duration_time > > For a metric using duration_time, the strcmp() check when finding identical > events in metric_events[] is broken, as it does not consider that the > event pmu_name is NULL - it would be for duration_time. > > As such, add a NULL check here for event pmu_name. > > Signed-off-by: John Garry <john.garry@huawei.com> > > diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c > index ee94d3e8dd65..277adff8017f 100644 > --- a/tools/perf/util/metricgroup.c > +++ b/tools/perf/util/metricgroup.c > @@ -280,6 +280,8 @@ static struct evsel *find_evsel_group(struct evlist > *perf_evlist, > */ > if (!has_constraint && > ev->leader != metric_events[i]->leader && > + ev->leader->pmu_name && > + metric_events[i]->leader->pmu_name && > !strcmp(ev->leader->pmu_name, > metric_events[i]->leader->pmu_name)) > break; > -- > 2.26.2 > > For this patch: Tested-by: Joakim Zhang <qiangqing.zhang@nxp.com> Hi John, Jolsa, Is there any way to avoid breaking exist metric expressions? If not, it will always happened after metricgroup changes. I recall that Jolsa mentioned it before, but I don’t remember it very clearly. Thanks a lot for John's bug fix. Best Regards, Joakim Zhang ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] perf metricgroup: Fix system PMU metrics 2021-01-20 5:15 ` Joakim Zhang @ 2021-01-20 9:15 ` John Garry 2021-01-20 10:19 ` Joakim Zhang 2021-01-21 20:31 ` Arnaldo Carvalho de Melo 0 siblings, 2 replies; 16+ messages in thread From: John Garry @ 2021-01-20 9:15 UTC (permalink / raw) To: Joakim Zhang, peterz, mingo, acme, mark.rutland, alexander.shishkin, jolsa, namhyung, irogers, kjain Cc: linux-kernel, linuxarm On 20/01/2021 05:15, Joakim Zhang wrote: > >> -----Original Message----- >> From: John Garry <john.garry@huawei.com> >> Sent: 2021年1月20日 1:33 >> To: Joakim Zhang <qiangqing.zhang@nxp.com>; peterz@infradead.org; >> mingo@redhat.com; acme@kernel.org; mark.rutland@arm.com; >> alexander.shishkin@linux.intel.com; jolsa@redhat.com; >> namhyung@kernel.org; irogers@google.com; kjain@linux.ibm.com >> Cc: linux-kernel@vger.kernel.org; linuxarm@openeuler.org >> Subject: Re: [PATCH] perf metricgroup: Fix system PMU metrics >> >> On 19/01/2021 15:47, John Garry wrote: >>> On 19/01/2021 10:56, Joakim Zhang wrote: >>>> It seems have other issue compared to 5.10 kernel after switching to >>>> this framework, below metric can't work. >>>> "MetricExpr": "(( imx8_ddr0@read\\-cycles@ + >>>> imx8_ddr0@write\\-cycles@ >>>> ) * 4 * 4 / duration_time) / (750 * 1000000 * 4 * 4)" >>>> After change to: >>>> "MetricExpr": "(( imx8mm_ddr.read_cycles + imx8mm_ddr.write_cycles ) >>>> * >>>> 4 * 4 / duration_time) / (750 * 1000000 * 4 * 4)", >>> >>> It seems that any metric which includes "duration_time" is broken, >>> even on x86: >>> >>> john@localhost:~/acme/tools/perf> sudo ./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 >>> >>> >>> Seems to be from my commit c2337d67199 ("perf metricgroup: Fix metrics >>> using aliases covering multiple PMUs") >>> >>> I'll look to fix it now. >>> >> >> Please try this: >> >> From 2380f1ef0250e6818b3dbc7bff4a868810875e2a Mon Sep 17 00:00:00 >> 2001 >> From: John Garry <john.garry@huawei.com> >> Date: Tue, 19 Jan 2021 17:29:54 +0000 >> Subject: [PATCH] perf metricgroup: Fix metric support for duration_time >> >> For a metric using duration_time, the strcmp() check when finding identical >> events in metric_events[] is broken, as it does not consider that the >> event pmu_name is NULL - it would be for duration_time. >> >> As such, add a NULL check here for event pmu_name. >> >> Signed-off-by: John Garry <john.garry@huawei.com> >> >> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c >> index ee94d3e8dd65..277adff8017f 100644 >> --- a/tools/perf/util/metricgroup.c >> +++ b/tools/perf/util/metricgroup.c >> @@ -280,6 +280,8 @@ static struct evsel *find_evsel_group(struct evlist >> *perf_evlist, >> */ >> if (!has_constraint && >> ev->leader != metric_events[i]->leader && >> + ev->leader->pmu_name && >> + metric_events[i]->leader->pmu_name && >> !strcmp(ev->leader->pmu_name, >> metric_events[i]->leader->pmu_name)) >> break; >> -- >> 2.26.2 >> >> > > For this patch: Tested-by: Joakim Zhang <qiangqing.zhang@nxp.com> > > Hi John, Jolsa, > > Is there any way to avoid breaking exist metric expressions? If not, it will always happened after metricgroup changes. > They are not normally broken like that. Normally we test beforehand, but these cases were missed here by me. However if you were testing them previously, then it would be expected that you had tested them again for the final patchset which was merged. Anyway, we can look to add metric tests for these. @Arnaldo, I will send separate formal patch for this today. Thanks, John ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH] perf metricgroup: Fix system PMU metrics 2021-01-20 9:15 ` John Garry @ 2021-01-20 10:19 ` Joakim Zhang 2021-01-21 20:31 ` Arnaldo Carvalho de Melo 1 sibling, 0 replies; 16+ messages in thread From: Joakim Zhang @ 2021-01-20 10:19 UTC (permalink / raw) To: John Garry, peterz, mingo, acme, mark.rutland, alexander.shishkin, jolsa, namhyung, irogers, kjain Cc: linux-kernel, linuxarm > -----Original Message----- > From: John Garry <john.garry@huawei.com> > Sent: 2021年1月20日 17:16 > To: Joakim Zhang <qiangqing.zhang@nxp.com>; peterz@infradead.org; > mingo@redhat.com; acme@kernel.org; mark.rutland@arm.com; > alexander.shishkin@linux.intel.com; jolsa@redhat.com; > namhyung@kernel.org; irogers@google.com; kjain@linux.ibm.com > Cc: linux-kernel@vger.kernel.org; linuxarm@openeuler.org > Subject: Re: [PATCH] perf metricgroup: Fix system PMU metrics > > On 20/01/2021 05:15, Joakim Zhang wrote: > > > >> -----Original Message----- > >> From: John Garry <john.garry@huawei.com> > >> Sent: 2021年1月20日 1:33 > >> To: Joakim Zhang <qiangqing.zhang@nxp.com>; peterz@infradead.org; > >> mingo@redhat.com; acme@kernel.org; mark.rutland@arm.com; > >> alexander.shishkin@linux.intel.com; jolsa@redhat.com; > >> namhyung@kernel.org; irogers@google.com; kjain@linux.ibm.com > >> Cc: linux-kernel@vger.kernel.org; linuxarm@openeuler.org > >> Subject: Re: [PATCH] perf metricgroup: Fix system PMU metrics > >> > >> On 19/01/2021 15:47, John Garry wrote: > >>> On 19/01/2021 10:56, Joakim Zhang wrote: > >>>> It seems have other issue compared to 5.10 kernel after switching > >>>> to this framework, below metric can't work. > >>>> "MetricExpr": "(( imx8_ddr0@read\\-cycles@ + > >>>> imx8_ddr0@write\\-cycles@ > >>>> ) * 4 * 4 / duration_time) / (750 * 1000000 * 4 * 4)" > >>>> After change to: > >>>> "MetricExpr": "(( imx8mm_ddr.read_cycles + imx8mm_ddr.write_cycles > >>>> ) > >>>> * > >>>> 4 * 4 / duration_time) / (750 * 1000000 * 4 * 4)", > >>> > >>> It seems that any metric which includes "duration_time" is broken, > >>> even on x86: > >>> > >>> john@localhost:~/acme/tools/perf> sudo ./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 > >>> > >>> > >>> Seems to be from my commit c2337d67199 ("perf metricgroup: Fix > >>> metrics using aliases covering multiple PMUs") > >>> > >>> I'll look to fix it now. > >>> > >> > >> Please try this: > >> > >> From 2380f1ef0250e6818b3dbc7bff4a868810875e2a Mon Sep 17 > 00:00:00 > >> 2001 > >> From: John Garry <john.garry@huawei.com> > >> Date: Tue, 19 Jan 2021 17:29:54 +0000 > >> Subject: [PATCH] perf metricgroup: Fix metric support for > >> duration_time > >> > >> For a metric using duration_time, the strcmp() check when finding > >> identical events in metric_events[] is broken, as it does not > >> consider that the event pmu_name is NULL - it would be for duration_time. > >> > >> As such, add a NULL check here for event pmu_name. > >> > >> Signed-off-by: John Garry <john.garry@huawei.com> > >> > >> diff --git a/tools/perf/util/metricgroup.c > >> b/tools/perf/util/metricgroup.c index ee94d3e8dd65..277adff8017f > >> 100644 > >> --- a/tools/perf/util/metricgroup.c > >> +++ b/tools/perf/util/metricgroup.c > >> @@ -280,6 +280,8 @@ static struct evsel *find_evsel_group(struct > >> evlist *perf_evlist, > >> */ > >> if (!has_constraint && > >> ev->leader != metric_events[i]->leader && > >> + ev->leader->pmu_name && > >> + metric_events[i]->leader->pmu_name && > >> !strcmp(ev->leader->pmu_name, > >> metric_events[i]->leader->pmu_name)) > >> break; > >> -- > >> 2.26.2 > >> > >> > > > > For this patch: Tested-by: Joakim Zhang <qiangqing.zhang@nxp.com> > > > > Hi John, Jolsa, > > > > Is there any way to avoid breaking exist metric expressions? If not, it will > always happened after metricgroup changes. > > > > They are not normally broken like that. Normally we test beforehand, but these > cases were missed here by me. However if you were testing them previously, > then it would be expected that you had tested them again for the final patchset > which was merged. Yes, John, sorry. I have not did the fully test before, this could be avoided. Best Regards, Joakim Zhang > Anyway, we can look to add metric tests for these. > > @Arnaldo, I will send separate formal patch for this today. > > Thanks, > John ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] perf metricgroup: Fix system PMU metrics 2021-01-20 9:15 ` John Garry 2021-01-20 10:19 ` Joakim Zhang @ 2021-01-21 20:31 ` Arnaldo Carvalho de Melo 2021-01-21 21:40 ` John Garry 1 sibling, 1 reply; 16+ messages in thread From: Arnaldo Carvalho de Melo @ 2021-01-21 20:31 UTC (permalink / raw) To: John Garry Cc: Joakim Zhang, peterz, mingo, mark.rutland, alexander.shishkin, jolsa, namhyung, irogers, kjain, linux-kernel, linuxarm Em Wed, Jan 20, 2021 at 09:15:54AM +0000, John Garry escreveu: > On 20/01/2021 05:15, Joakim Zhang wrote: > > For this patch: Tested-by: Joakim Zhang <qiangqing.zhang@nxp.com> > > Hi John, Jolsa, > > Is there any way to avoid breaking exist metric expressions? If not, it will always happened after metricgroup changes. > They are not normally broken like that. Normally we test beforehand, but > these cases were missed here by me. However if you were testing them > previously, then it would be expected that you had tested them again for the > final patchset which was merged. > Anyway, we can look to add metric tests for these. > @Arnaldo, I will send separate formal patch for this today. Hi John, can you please take a look at my tmp.perf/urgent branch and see if all is well, i.e. the versions of these patches are the ones that should be merged and that all the patches discussed are there? For your convenience: https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/log/?h=tmp.perf/urgent Thanks, - Arnaldo ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] perf metricgroup: Fix system PMU metrics 2021-01-21 20:31 ` Arnaldo Carvalho de Melo @ 2021-01-21 21:40 ` John Garry 0 siblings, 0 replies; 16+ messages in thread From: John Garry @ 2021-01-21 21:40 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Joakim Zhang, peterz, mingo, mark.rutland, alexander.shishkin, jolsa, namhyung, irogers, kjain, linux-kernel, linuxarm On 21/01/2021 20:31, Arnaldo Carvalho de Melo wrote: >> They are not normally broken like that. Normally we test beforehand, but >> these cases were missed here by me. However if you were testing them >> previously, then it would be expected that you had tested them again for the >> final patchset which was merged. > >> Anyway, we can look to add metric tests for these. > >> @Arnaldo, I will send separate formal patch for this today. > Hi John, can you please take a look at my tmp.perf/urgent branch and see > if all is well, i.e. the versions of these patches are the ones that > should be merged and that all the patches discussed are there? > > For your convenience: > > https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/log/?h=tmp.perf/urgent Hi Arnaldo, Yeah, that looks fine. I gave it a quick spin also without issue. Cheers, John ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] perf metricgroup: Fix system PMU metrics 2021-01-19 17:33 ` John Garry 2021-01-20 5:15 ` Joakim Zhang @ 2021-01-20 12:56 ` Jiri Olsa 2021-01-20 13:07 ` John Garry 2021-02-05 9:56 ` John Garry 1 sibling, 2 replies; 16+ messages in thread From: Jiri Olsa @ 2021-01-20 12:56 UTC (permalink / raw) To: John Garry Cc: Joakim Zhang, peterz, mingo, acme, mark.rutland, alexander.shishkin, namhyung, irogers, kjain, linux-kernel, linuxarm On Tue, Jan 19, 2021 at 05:33:13PM +0000, John Garry wrote: SNIP > Please try this: > > From 2380f1ef0250e6818b3dbc7bff4a868810875e2a Mon Sep 17 00:00:00 2001 > From: John Garry <john.garry@huawei.com> > Date: Tue, 19 Jan 2021 17:29:54 +0000 > Subject: [PATCH] perf metricgroup: Fix metric support for duration_time > > For a metric using duration_time, the strcmp() check when finding identical > events in metric_events[] is broken, as it does not consider that the > event pmu_name is NULL - it would be for duration_time. > > As such, add a NULL check here for event pmu_name. > > Signed-off-by: John Garry <john.garry@huawei.com> > > diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c > index ee94d3e8dd65..277adff8017f 100644 > --- a/tools/perf/util/metricgroup.c > +++ b/tools/perf/util/metricgroup.c > @@ -280,6 +280,8 @@ static struct evsel *find_evsel_group(struct evlist > *perf_evlist, > */ > if (!has_constraint && > ev->leader != metric_events[i]->leader && > + ev->leader->pmu_name && > + metric_events[i]->leader->pmu_name && > !strcmp(ev->leader->pmu_name, > metric_events[i]->leader->pmu_name)) > break; > -- > 2.26.2 > > > that's fixing the issue for me, this was crashing: # perf stat -a -I 1000 -M L1D_Cache_Fill_BW,L2_Cache_Fill_BW could you please send it formaly, so it can be merged? I can't reproduce the original patch issue and I need to check the code in more depth thanks, jirka ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] perf metricgroup: Fix system PMU metrics 2021-01-20 12:56 ` Jiri Olsa @ 2021-01-20 13:07 ` John Garry 2021-02-05 9:56 ` John Garry 1 sibling, 0 replies; 16+ messages in thread From: John Garry @ 2021-01-20 13:07 UTC (permalink / raw) To: Jiri Olsa Cc: Joakim Zhang, peterz, mingo, acme, mark.rutland, alexander.shishkin, namhyung, irogers, kjain, linux-kernel, linuxarm On 20/01/2021 12:56, Jiri Olsa wrote: > that's fixing the issue for me, this was crashing: > # perf stat -a -I 1000 -M L1D_Cache_Fill_BW,L2_Cache_Fill_BW > > could you please send it formaly, so it can be merged? > I will do it today. But I want to make sure that the logic is correct, so checking it all again, i.e. I can fix the segfault but the logic may be wrong. I think Ian wrote this code originally or made the most recent rework (which I tried to fix), so would like him to check also. > I can't reproduce the original patch issue and I need > to check the code in more depth It should only occur for metrics when using system PMUs, i.e. match via identifier file, so I really doubt your system has them. I'd like to add a test for this - I need to think how... Cheers, John ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] perf metricgroup: Fix system PMU metrics 2021-01-20 12:56 ` Jiri Olsa 2021-01-20 13:07 ` John Garry @ 2021-02-05 9:56 ` John Garry 1 sibling, 0 replies; 16+ messages in thread From: John Garry @ 2021-02-05 9:56 UTC (permalink / raw) To: Jiri Olsa Cc: Joakim Zhang, peterz, mingo, acme, mark.rutland, alexander.shishkin, namhyung, irogers, kjain, linux-kernel, linuxarm >> >> > that's fixing the issue for me, this was crashing: > # perf stat -a -I 1000 -M L1D_Cache_Fill_BW,L2_Cache_Fill_BW > To cover this scenario (multiple metrics listed), how about a simple addition like: ---->8---- From bd2ded1b0ef4962a9443cd180eed4e5c5b75ae5f Mon Sep 17 00:00:00 2001 From: John Garry <john.garry@huawei.com> Date: Fri, 5 Feb 2021 09:50:54 +0000 Subject: [PATCH] perf test: Add parse-metric list test scenario diff --git a/tools/perf/tests/parse-metric.c b/tools/perf/tests/parse-metric.c index ce7be37f0d88..0626e389354c 100644 --- a/tools/perf/tests/parse-metric.c +++ b/tools/perf/tests/parse-metric.c @@ -201,6 +201,13 @@ static int compute_metric_group(const char *name, struct value *vals, return __compute_metric(name, vals, name1, ratio1, name2, ratio2); } +static int compute_metric_list(const char *list, struct value *vals, + const char *name1, double *ratio1, + const char *name2, double *ratio2) +{ + return __compute_metric(list, vals, name1, ratio1, name2, ratio2); +} + static int test_ipc(void) { double ratio; @@ -279,7 +286,7 @@ static int test_cache_miss_cycles(void) */ static int test_dcache_l2(void) { - double ratio; + double ratio, ratio1, ratio2; struct value vals[] = { { .event = "l2_rqsts.demand_data_rd_hit", .val = 100 }, { .event = "l2_rqsts.pf_hit", .val = 200 }, @@ -301,6 +308,15 @@ static int test_dcache_l2(void) TEST_ASSERT_VAL("DCache_L2_Misses failed, wrong ratio", ratio == 0.7); + + TEST_ASSERT_VAL("failed to compute metric", + compute_metric_list("DCache_L2_Hits,DCache_L2_Misses", vals, "DCache_L2_Hits", &ratio1, "DCache_L2_Misses", &ratio2) == 0); + + TEST_ASSERT_VAL("DCache_L2_Hits failed, wrong ratio", + ratio1 == 0.3); + + TEST_ASSERT_VAL("DCache_L2_Misses failed, wrong ratio", + ratio2 == 0.7); return 0; } ----8<---- > could you please send it formaly, so it can be merged? > > I can't reproduce the original patch issue and I need > to check the code in more depth Thanks, John ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] perf metricgroup: Fix system PMU metrics 2021-01-19 10:04 [PATCH] perf metricgroup: Fix system PMU metrics John Garry 2021-01-19 10:56 ` Joakim Zhang @ 2021-01-20 21:42 ` Jiri Olsa 1 sibling, 0 replies; 16+ messages in thread From: Jiri Olsa @ 2021-01-20 21:42 UTC (permalink / raw) To: John Garry Cc: peterz, mingo, acme, mark.rutland, alexander.shishkin, namhyung, irogers, kjain, linux-kernel, linuxarm, qiangqing.zhang On Tue, Jan 19, 2021 at 06:04:15PM +0800, John Garry wrote: > Joakim reports that getting "perf stat" for multiple system PMU metrics > segfaults: > ./perf stat -a -I 1000 -M imx8mm_ddr_write.all,imx8mm_ddr_write.all > Segmentation fault > > While the same works without issue for a single metric. > > The logic in metricgroup__add_metric_sys_event_iter() is broken, in that > add_metric() @m argument should be NULL for each new metric. Fix by not > passing a holder for that, and rather make local in > metricgroup__add_metric_sys_event_iter(). > > Fixes: be335ec28efa ("perf metricgroup: Support adding metrics for system PMUs") > Reported-by: Joakim Zhang <qiangqing.zhang@nxp.com> > Signed-off-by: John Garry <john.garry@huawei.com> Acked-by: Jiri Olsa <jolsa@redhat.com> thanks, jirka > > diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c > index ee94d3e8dd65..2e60ee170abc 100644 > --- a/tools/perf/util/metricgroup.c > +++ b/tools/perf/util/metricgroup.c > @@ -766,7 +766,6 @@ int __weak arch_get_runtimeparam(struct pmu_event *pe __maybe_unused) > struct metricgroup_add_iter_data { > struct list_head *metric_list; > const char *metric; > - struct metric **m; > struct expr_ids *ids; > int *ret; > bool *has_match; > @@ -1058,12 +1057,13 @@ static int metricgroup__add_metric_sys_event_iter(struct pmu_event *pe, > void *data) > { > struct metricgroup_add_iter_data *d = data; > + struct metric *m = NULL; > int ret; > > if (!match_pe_metric(pe, d->metric)) > return 0; > > - ret = add_metric(d->metric_list, pe, d->metric_no_group, d->m, NULL, d->ids); > + ret = add_metric(d->metric_list, pe, d->metric_no_group, &m, NULL, d->ids); > if (ret) > return ret; > > @@ -1114,7 +1114,6 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group, > .metric_list = &list, > .metric = metric, > .metric_no_group = metric_no_group, > - .m = &m, > .ids = &ids, > .has_match = &has_match, > .ret = &ret, > -- > 2.26.2 > ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2021-02-06 0:56 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-19 10:04 [PATCH] perf metricgroup: Fix system PMU metrics John Garry 2021-01-19 10:56 ` Joakim Zhang 2021-01-19 11:04 ` John Garry 2021-01-19 11:13 ` Joakim Zhang 2021-01-19 12:02 ` John Garry 2021-01-19 15:47 ` John Garry 2021-01-19 17:33 ` John Garry 2021-01-20 5:15 ` Joakim Zhang 2021-01-20 9:15 ` John Garry 2021-01-20 10:19 ` Joakim Zhang 2021-01-21 20:31 ` Arnaldo Carvalho de Melo 2021-01-21 21:40 ` John Garry 2021-01-20 12:56 ` Jiri Olsa 2021-01-20 13:07 ` John Garry 2021-02-05 9:56 ` John Garry 2021-01-20 21:42 ` 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.