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

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

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

...

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