Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 20 Jul 2020 21:16:25 +0200
From:   Jiri Olsa <jolsa@redhat.com>
To:     kajoljain <kjain@linux.ibm.com>
Cc:     Jiri Olsa <jolsa@kernel.org>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        lkml <linux-kernel@vger.kernel.org>,
        Ingo Molnar <mingo@kernel.org>,
        Namhyung Kim <namhyung@kernel.org>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Peter Zijlstra <a.p.zijlstra@chello.nl>,
        Michael Petlan <mpetlan@redhat.com>,
        Andi Kleen <ak@linux.intel.com>,
        John Garry <john.garry@huawei.com>,
        "Paul A. Clarke" <pc@us.ibm.com>,
        Stephane Eranian <eranian@google.com>,
        Ian Rogers <irogers@google.com>
Subject: Re: [PATCHv3 00/19] perf metric: Add support to reuse metric
Message-ID: <20200720191625.GL760733@krava>
References: <20200719181320.785305-1-jolsa@kernel.org>
 <dbe59791-937d-de95-4ba0-c34e7a1cd273@linux.ibm.com>
 <20200720072237.GC760733@krava>
 <6cb72b48-5244-9faf-a9e5-67858c732b83@linux.ibm.com>
 <20200720081943.GE760733@krava>
 <dd465647-da63-c473-9944-bdfec2abe484@linux.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <dd465647-da63-c473-9944-bdfec2abe484@linux.ibm.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Jul 20, 2020 at 02:32:40PM +0530, kajoljain wrote:
> 
> 
> On 7/20/20 1:49 PM, Jiri Olsa wrote:
> > On Mon, Jul 20, 2020 at 01:39:24PM +0530, kajoljain wrote:
> > 
> > SNIP
> > 
> >> This is with your perf/metric branch:
> >> command# ./perf stat -M PowerBUS_Frequency -C 0 -I 1000
> >> assertion failed at util/metricgroup.c:709
> >> #           time             counts unit events
> >>      1.000054545          7,807,505      hv_24x7/pm_pb_cyc,chip=0/ #      2.0 GHz  PowerBUS_Frequency_0
> >>      1.000054545          7,807,485      hv_24x7/pm_pb_cyc,chip=1/                                   
> >>      2.000232761          7,807,500      hv_24x7/pm_pb_cyc,chip=0/ #      2.0 GHz  PowerBUS_Frequency_0
> >>      2.000232761          7,807,478      hv_24x7/pm_pb_cyc,chip=1/                                   
> >>      3.000363762          7,799,665      hv_24x7/pm_pb_cyc,chip=0/ #      1.9 GHz  PowerBUS_Frequency_0
> >>      3.000363762          7,807,502      hv_24x7/pm_pb_cyc,chip=1/                                   
> >> ^C     3.259418599          2,022,150      hv_24x7/pm_pb_cyc,chip=0/ #      0.5 GHz  PowerBUS_Frequency_0
> >>      3.259418599          2,022,164      hv_24x7/pm_pb_cyc,chip=1/                                   
> >>
> >>  Performance counter stats for 'CPU(s) 0':
> >>
> >>         25,436,820      hv_24x7/pm_pb_cyc,chip=0/ #      6.4 GHz  PowerBUS_Frequency_0
> >>         25,444,629      hv_24x7/pm_pb_cyc,chip=1/                                   
> >>
> >>        3.259505529 seconds time elapsed
> > 
> > I found the bug, we are not adding runtime metrics as standalone ones,
> > but as referenced metrics.. will fix and try to add test for that
> > 
> > as for testing.. do I need some special ppc server to have support for this? 
> 
> Hi jiri,
>     We need power9 lpar machine and need to make sure `CONFIG_HV_PERF_CTRS` is
> enabled.

could you please try with following patch on top?

thanks,
jirka


---
diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 6f179b9903a0..03aa4bd4a38b 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -820,11 +820,11 @@ static int add_metric(struct list_head *metric_list,
 		      struct expr_id *parent,
 		      struct expr_ids *ids);
 
-static int resolve_metric(struct metric *m,
-			  bool metric_no_group,
-			  struct list_head *metric_list,
-			  struct pmu_events_map *map,
-			  struct expr_ids *ids)
+static int __resolve_metric(struct metric *m,
+			    bool metric_no_group,
+			    struct list_head *metric_list,
+			    struct pmu_events_map *map,
+			    struct expr_ids *ids)
 {
 	struct hashmap_entry *cur;
 	size_t bkt;
@@ -869,6 +869,23 @@ static int resolve_metric(struct metric *m,
 	return 0;
 }
 
+static int resolve_metric(bool metric_no_group,
+			  struct list_head *metric_list,
+			  struct pmu_events_map *map,
+			  struct expr_ids *ids)
+{
+	struct metric *m;
+	int err;
+
+	list_for_each_entry(m, metric_list, nd) {
+		err = __resolve_metric(m, metric_no_group, metric_list, map, ids);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
 static int add_metric(struct list_head *metric_list,
 		      struct pmu_event *pe,
 		      bool metric_no_group,
@@ -876,6 +893,7 @@ static int add_metric(struct list_head *metric_list,
 		      struct expr_id *parent,
 		      struct expr_ids *ids)
 {
+	struct metric *orig = *m;
 	int ret = 0;
 
 	pr_debug("metric expr %s for %s\n", pe->metric_expr, pe->metric_name);
@@ -892,7 +910,7 @@ static int add_metric(struct list_head *metric_list,
 		 * those events to metric_list.
 		 */
 
-		for (j = 0; j < count && !ret; j++) {
+		for (j = 0; j < count && !ret; j++, *m = orig) {
 			ret = __add_metric(metric_list, pe, metric_no_group, j, m, parent, ids);
 		}
 	}
@@ -907,8 +925,8 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,
 
 {
 	struct expr_ids ids = { 0 };
+	struct metric *m = NULL;
 	struct pmu_event *pe;
-	struct metric *m;
 	LIST_HEAD(list);
 	int i, ret;
 	bool has_match = false;
@@ -925,7 +943,7 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,
 		 * Process any possible referenced metrics
 		 * included in the expression.
 		 */
-		ret = resolve_metric(m, metric_no_group,
+		ret = resolve_metric(metric_no_group,
 				     &list, map, &ids);
 		if (ret)
 			return ret;