[PATCH] perf metricgroup: Fix for metrics containing duration_time

[PATCH] perf metricgroup: Fix for metrics containing duration_time

[John Garry]

Metrics containing duration_time cause a segfault:

$./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

In commit c2337d67199a ("perf metricgroup: Fix metrics using aliases
covering multiple PMUs"), the logic in find_evsel_group() when iter'ing
events was changed to not only select events in same group, but also for
aliased PMUs.

Checking whether events were for aliased PMUs was done by comparing the
event PMU name. This was not safe for duration_time event, which has no
associated PMU (and no PMU name), so fix by checking if the event PMU name
is set also.

Fixes: c2337d67199a ("perf metricgroup: Fix metrics using aliases covering multiple 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 2e60ee170abc..e6d3452031e5 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -162,6 +162,14 @@ static bool contains_event(struct evsel **metric_events, int num_events,
 	return false;
 }
 
+static bool evsel_same_pmu(struct evsel *ev1, struct evsel *ev2)
+{
+	if (!ev1->pmu_name || !ev2->pmu_name)
+		return false;
+
+	return !strcmp(ev1->pmu_name, ev2->pmu_name);
+}
+
 /**
  * Find a group of events in perf_evlist that correspond to those from a parsed
  * metric expression. Note, as find_evsel_group is called in the same order as
@@ -280,8 +288,7 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
 			 */
 			if (!has_constraint &&
 			    ev->leader != metric_events[i]->leader &&
-			    !strcmp(ev->leader->pmu_name,
-				    metric_events[i]->leader->pmu_name))
+			    evsel_same_pmu(ev->leader, metric_events[i]->leader))
 				break;
 			if (!strcmp(metric_events[i]->name, ev->name)) {
 				set_bit(ev->idx, evlist_used);
-- 
2.26.2

Re: [PATCH] perf metricgroup: Fix for metrics containing duration_time

[John Garry]

On 20/01/2021 16:40, Ian Rogers wrote:
> On Wed, Jan 20, 2021 at 8:23 AM John Garry <john.garry@huawei.com 
> <mailto:john.garry@huawei.com>> wrote:
> 
>     Metrics containing duration_time cause a segfault:
> 
>     $./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
> 
>     In commit c2337d67199a ("perf metricgroup: Fix metrics using aliases
>     covering multiple PMUs"), the logic in find_evsel_group() when iter'ing
>     events was changed to not only select events in same group, but also for
>     aliased PMUs.
> 
>     Checking whether events were for aliased PMUs was done by comparing the
>     event PMU name. This was not safe for duration_time event, which has no
>     associated PMU (and no PMU name), so fix by checking if the event
>     PMU name
>     is set also.
> 
> 
> Thanks for this, it should be fairly easy to add a test. Could we do this?

I don't mind following up with that.

> 
>     Fixes: c2337d67199a ("perf metricgroup: Fix metrics using aliases
>     covering multiple PMUs")
>     Reported-by: Joakim Zhang <qiangqing.zhang@nxp.com
>     <mailto:qiangqing.zhang@nxp.com>>
>     Signed-off-by: John Garry <john.garry@huawei.com
>     <mailto:john.garry@huawei.com>>
> 
>     diff --git a/tools/perf/util/metricgroup.c
>     b/tools/perf/util/metricgroup.c
>     index 2e60ee170abc..e6d3452031e5 100644
>     --- a/tools/perf/util/metricgroup.c
>     +++ b/tools/perf/util/metricgroup.c
>     @@ -162,6 +162,14 @@ static bool contains_event(struct evsel
>     **metric_events, int num_events,
>              return false;
>       }
> 
>     +static bool evsel_same_pmu(struct evsel *ev1, struct evsel *ev2)
>     +{
>     +       if (!ev1->pmu_name || !ev2->pmu_name)
>     +               return false;
> 
> 
> What about the case of "!ev1->pmu_name && !ev2->pmu_name" ?

As far as I know, it should not happen, since duration_time is a special 
event. More below.

> 
> Thanks,
> Ian
> 
>     +
>     +       return !strcmp(ev1->pmu_name, ev2->pmu_name);
>     +}
>     +
>       /**
>        * Find a group of events in perf_evlist that correspond to those
>     from a parsed
>        * metric expression. Note, as find_evsel_group is called in the
>     same order as
>     @@ -280,8 +288,7 @@ static struct evsel *find_evsel_group(struct
>     evlist *perf_evlist,
>                               */
>                              if (!has_constraint &&
>                                  ev->leader != metric_events[i]->leader &&
>     -                           !strcmp(ev->leader->pmu_name,
>     -                                   metric_events[i]->leader->pmu_name))
>     +                           evsel_same_pmu(ev->leader,
>     metric_events[i]->leader))

ev->leader->pmu_name == NULL for only duration_time event. And we don't 
get here for ev == metric_events[i] == duration_time event (as we use 
evlist__for_each_entry_continue() and duration_time is always last in 
metric_events[]), so both event arguments should not have pmu_name == 
NULL. Indeed, I could just check metric_events[i]->leader->pmu_name != 
NULL, but thought it better to check both for safety.

Cheers,
John

>                                      break;
>                              if (!strcmp(metric_events[i]->name,
>     ev->name)) {
>                                      set_bit(ev->idx, evlist_used);
>     -- 
>     2.26.2
> 

Re: [PATCH] perf metricgroup: Fix for metrics containing duration_time

[John Garry]

>      >
>      >
>      > Thanks for this, it should be fairly easy to add a test. Could we
>     do this?
> 
>     I don't mind following up with that.

How about this:

---- >8 ----

 From 38e9d5aa07588d552378d97760b6b79f19d59bbd Mon Sep 17 00:00:00 2001
From: John Garry <john.garry@huawei.com>
Date: Thu, 21 Jan 2021 16:55:17 +0000
Subject: [PATCH] perf test: Add parse-metric memory bandwidth testcase

Event duration_time in a metric expression requires special handling.

Improve test coverage by including a metric whose expression includes
duration_time. The actual metric is copied from the L1D_Cache_Fill_BW
metric on my broadwell machine.

Signed-off-by: John Garry <john.garry@huawei.com>

diff --git a/tools/perf/tests/parse-metric.c 
b/tools/perf/tests/parse-metric.c
index ce7be37f0d88..98b9570c6194 100644
--- a/tools/perf/tests/parse-metric.c
+++ b/tools/perf/tests/parse-metric.c
@@ -11,6 +11,7 @@
  #include "debug.h"
  #include "expr.h"
  #include "stat.h"
+#include <math.h>

  static struct pmu_event pme_test[] = {
  {
@@ -69,6 +70,10 @@ static struct pmu_event pme_test[] = {
  	.metric_expr	= "1/m3",
  	.metric_name	= "M3",
  },
+{
+	.metric_expr	= "64 * l1d.replacement / 1000000000 / duration_time",
+	.metric_name	= "L1D_Cache_Fill_BW",
+},
  {
  	.name	= NULL,
  }
@@ -107,6 +112,8 @@ static void load_runtime_stat(struct runtime_stat 
*st, struct evlist *evlist,
  	evlist__for_each_entry(evlist, evsel) {
  		count = find_value(evsel->name, vals);
  		perf_stat__update_shadow_stats(evsel, count, 0, st);
+		if (!strcmp(evsel->name, "duration_time"))
+			update_stats(&walltime_nsecs_stats, count);
  	}
  }

@@ -321,6 +328,24 @@ static int test_recursion_fail(void)
  	return 0;
  }

+static int test_memory_bandwidth(void)
+{
+	double ratio;
+	struct value vals[] = {
+		{ .event = "l1d.replacement", .val = 304334545 },
+		{ .event = "duration_time",  .val = 1001057587 },
+		{ .event = NULL, },
+	};
+
+	TEST_ASSERT_VAL("failed to compute metric",
+			compute_metric("L1D_Cache_Fill_BW", vals, &ratio) == 0);
+
+	TEST_ASSERT_VAL("L1D_Cache_Fill_BW, wrong ratio",
+			fabs(ratio - 19.45) < 0.01);
+
+	return 0;
+}
+
  static int test_metric_group(void)
  {
  	double ratio1, ratio2;
@@ -353,5 +378,6 @@ int test__parse_metric(struct test *test 
__maybe_unused, int subtest __maybe_unu
  	TEST_ASSERT_VAL("DCache_L2 failed", test_dcache_l2() == 0);
  	TEST_ASSERT_VAL("recursion fail failed", test_recursion_fail() == 0);
  	TEST_ASSERT_VAL("test metric group", test_metric_group() == 0);
+	TEST_ASSERT_VAL("Memory bandwidth", test_memory_bandwidth() == 0);
  	return 0;
  }
-- 
2.26.2

Cheers,
John

Re: [PATCH] perf metricgroup: Fix for metrics containing duration_time

[Jiri Olsa]

On Thu, Jan 21, 2021 at 12:18:38AM +0800, John Garry wrote:
> Metrics containing duration_time cause a segfault:
> 
> $./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
> 
> In commit c2337d67199a ("perf metricgroup: Fix metrics using aliases
> covering multiple PMUs"), the logic in find_evsel_group() when iter'ing
> events was changed to not only select events in same group, but also for
> aliased PMUs.
> 
> Checking whether events were for aliased PMUs was done by comparing the
> event PMU name. This was not safe for duration_time event, which has no
> associated PMU (and no PMU name), so fix by checking if the event PMU name
> is set also.
> 
> Fixes: c2337d67199a ("perf metricgroup: Fix metrics using aliases covering multiple PMUs")
> Reported-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> Signed-off-by: John Garry <john.garry@huawei.com>

Tested/Acked-by: Jiri Olsa <jolsa@redhat.com>

thanks,
jirka

> 
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 2e60ee170abc..e6d3452031e5 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -162,6 +162,14 @@ static bool contains_event(struct evsel **metric_events, int num_events,
>  	return false;
>  }
>  
> +static bool evsel_same_pmu(struct evsel *ev1, struct evsel *ev2)
> +{
> +	if (!ev1->pmu_name || !ev2->pmu_name)
> +		return false;
> +
> +	return !strcmp(ev1->pmu_name, ev2->pmu_name);
> +}
> +
>  /**
>   * Find a group of events in perf_evlist that correspond to those from a parsed
>   * metric expression. Note, as find_evsel_group is called in the same order as
> @@ -280,8 +288,7 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
>  			 */
>  			if (!has_constraint &&
>  			    ev->leader != metric_events[i]->leader &&
> -			    !strcmp(ev->leader->pmu_name,
> -				    metric_events[i]->leader->pmu_name))
> +			    evsel_same_pmu(ev->leader, metric_events[i]->leader))
>  				break;
>  			if (!strcmp(metric_events[i]->name, ev->name)) {
>  				set_bit(ev->idx, evlist_used);
> -- 
> 2.26.2
> 

Re: [PATCH] perf metricgroup: Fix for metrics containing duration_time

[Arnaldo Carvalho de Melo]

Em Wed, Jan 20, 2021 at 10:35:35PM +0100, Jiri Olsa escreveu:
> On Thu, Jan 21, 2021 at 12:18:38AM +0800, John Garry wrote:
> > Metrics containing duration_time cause a segfault:
> > 
> > $./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
> > 
> > In commit c2337d67199a ("perf metricgroup: Fix metrics using aliases
> > covering multiple PMUs"), the logic in find_evsel_group() when iter'ing
> > events was changed to not only select events in same group, but also for
> > aliased PMUs.
> > 
> > Checking whether events were for aliased PMUs was done by comparing the
> > event PMU name. This was not safe for duration_time event, which has no
> > associated PMU (and no PMU name), so fix by checking if the event PMU name
> > is set also.
> > 
> > Fixes: c2337d67199a ("perf metricgroup: Fix metrics using aliases covering multiple PMUs")
> > Reported-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > Signed-off-by: John Garry <john.garry@huawei.com>
> 
> Tested/Acked-by: Jiri Olsa <jolsa@redhat.com>

Thanks, applied.

- Arnaldo

 
> thanks,
> jirka
> 
> > 
> > diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> > index 2e60ee170abc..e6d3452031e5 100644
> > --- a/tools/perf/util/metricgroup.c
> > +++ b/tools/perf/util/metricgroup.c
> > @@ -162,6 +162,14 @@ static bool contains_event(struct evsel **metric_events, int num_events,
> >  	return false;
> >  }
> >  
> > +static bool evsel_same_pmu(struct evsel *ev1, struct evsel *ev2)
> > +{
> > +	if (!ev1->pmu_name || !ev2->pmu_name)
> > +		return false;
> > +
> > +	return !strcmp(ev1->pmu_name, ev2->pmu_name);
> > +}
> > +
> >  /**
> >   * Find a group of events in perf_evlist that correspond to those from a parsed
> >   * metric expression. Note, as find_evsel_group is called in the same order as
> > @@ -280,8 +288,7 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> >  			 */
> >  			if (!has_constraint &&
> >  			    ev->leader != metric_events[i]->leader &&
> > -			    !strcmp(ev->leader->pmu_name,
> > -				    metric_events[i]->leader->pmu_name))
> > +			    evsel_same_pmu(ev->leader, metric_events[i]->leader))
> >  				break;
> >  			if (!strcmp(metric_events[i]->name, ev->name)) {
> >  				set_bit(ev->idx, evlist_used);
> > -- 
> > 2.26.2
> > 
> 

-- 

- Arnaldo