[PATCH] perf metricgroup: Fix system PMU metrics

[PATCH] perf metricgroup: Fix system PMU metrics

[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

RE: [PATCH] perf metricgroup: Fix system PMU metrics

[Joakim Zhang]

> -----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

Re: [PATCH] perf metricgroup: Fix system PMU metrics

[John Garry]

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

RE: [PATCH] perf metricgroup: Fix system PMU metrics

[Joakim Zhang]

> -----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

Re: [PATCH] perf metricgroup: Fix system PMU metrics

[John Garry]

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

Re: [PATCH] perf metricgroup: Fix system PMU metrics

[John Garry]

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

Re: [PATCH] perf metricgroup: Fix system PMU metrics

[John Garry]

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

RE: [PATCH] perf metricgroup: Fix system PMU metrics

[Joakim Zhang]

> -----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

Re: [PATCH] perf metricgroup: Fix system PMU metrics

[John Garry]

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

RE: [PATCH] perf metricgroup: Fix system PMU metrics

[Joakim Zhang]

> -----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

Re: [PATCH] perf metricgroup: Fix system PMU metrics

[Jiri Olsa]

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

Re: [PATCH] perf metricgroup: Fix system PMU metrics

[John Garry]

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

Re: [PATCH] perf metricgroup: Fix system PMU metrics

[Jiri Olsa]

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
> 

Re: [PATCH] perf metricgroup: Fix system PMU metrics

[Arnaldo Carvalho de Melo]

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

Re: [PATCH] perf metricgroup: Fix system PMU metrics

[John Garry]

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

Re: [PATCH] perf metricgroup: Fix system PMU metrics

[John Garry]

>>
>>
> 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