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 <[email protected]>
Signed-off-by: John Garry <[email protected]>
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 Zhang<[email protected]>
>> Signed-off-by: John Garry<[email protected]>
> 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<[email protected]>
>
> 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 <[email protected]>
> Sent: 2021??1??19?? 19:05
> To: Joakim Zhang <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]
> Cc: [email protected]; [email protected]
> 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<[email protected]>
> >> Signed-off-by: John Garry<[email protected]>
> > 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<[email protected]>
> >
> > 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
> -----Original Message-----
> From: John Garry <[email protected]>
> Sent: 2021??1??19?? 18:04
> To: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]
> Cc: [email protected]; [email protected]; Joakim Zhang
> <[email protected]>; John Garry <[email protected]>
> 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 <[email protected]>
> Signed-off-by: John Garry <[email protected]>
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 <[email protected]>
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:
> 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<[email protected]>
>
> 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 <[email protected]>
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 <[email protected]>
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
> -----Original Message-----
> From: John Garry <[email protected]>
> Sent: 2021年1月20日 1:33
> To: Joakim Zhang <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]
> Cc: [email protected]; [email protected]
> 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 <[email protected]>
> 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 <[email protected]>
>
> 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 <[email protected]>
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 <[email protected]>
>> Sent: 2021年1月20日 1:33
>> To: Joakim Zhang <[email protected]>; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected]
>> Cc: [email protected]; [email protected]
>> 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 <[email protected]>
>> 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 <[email protected]>
>>
>> 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 <[email protected]>
>
> 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 <[email protected]>
> Sent: 2021年1月20日 17:16
> To: Joakim Zhang <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]
> Cc: [email protected]; [email protected]
> Subject: Re: [PATCH] perf metricgroup: Fix system PMU metrics
>
> On 20/01/2021 05:15, Joakim Zhang wrote:
> >
> >> -----Original Message-----
> >> From: John Garry <[email protected]>
> >> Sent: 2021年1月20日 1:33
> >> To: Joakim Zhang <[email protected]>; [email protected];
> >> [email protected]; [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected]; [email protected]
> >> Cc: [email protected]; [email protected]
> >> 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 <[email protected]>
> >> 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 <[email protected]>
> >>
> >> 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 <[email protected]>
> >
> > 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
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
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 <[email protected]>
> 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 <[email protected]>
>
> 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 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 <[email protected]>
> Signed-off-by: John Garry <[email protected]>
Acked-by: Jiri Olsa <[email protected]>
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
>
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 <[email protected]>
> > 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
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
>>
>>
> 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 <[email protected]>
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