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,
-----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
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 Zhangqiangqing.zhang@nxp.com Signed-off-by: John Garryjohn.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 Zhangqiangqing.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
-----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 Zhangqiangqing.zhang@nxp.com Signed-off-by: John Garryjohn.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 16973imx8mm_ddr.write_cycles # 265.2 KB imx8mm_ddr_write.all
2.003150250 28836imx8mm_ddr.read_cycles # 450.6 KB imx8mm_ddr_read.all
2.003150250 6705imx8mm_ddr.write_cycles # 104.8 KB imx8mm_ddr_write.all
For this issue, Tested-by: Joakim Zhangqiangqing.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
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 Zhangqiangqing.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
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
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;
-----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
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
-----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
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.pe...
Thanks,
- Arnaldo
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.pe...
Hi Arnaldo,
Yeah, that looks fine. I gave it a quick spin also without issue.
Cheers, John
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
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
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
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
-
Arnaldo Carvalho de Melo -
Jiri Olsa -
Joakim Zhang -
John Garry