Received-SPF: pass (google.com: domain of linux-kernel+bounces-143368-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
From: weilin.wang@intel.com
To: weilin.wang@intel.com,
	Ian Rogers <irogers@google.com>,
	Kan Liang <kan.liang@linux.intel.com>,
	Namhyung Kim <namhyung@kernel.org>,
	Arnaldo Carvalho de Melo <acme@kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@redhat.com>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Jiri Olsa <jolsa@kernel.org>,
	Adrian Hunter <adrian.hunter@intel.com>
Cc: linux-perf-users@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	Perry Taylor <perry.taylor@intel.com>,
	Samantha Alt <samantha.alt@intel.com>,
	Caleb Biggers <caleb.biggers@intel.com>
Subject: [RFC PATCH v5 15/16] perf stat: use tool event helper function in metricgroup__build_event_string
Date: Fri, 12 Apr 2024 14:07:55 -0700
Message-ID: <20240412210756.309828-16-weilin.wang@intel.com>
In-Reply-To: <20240412210756.309828-1-weilin.wang@intel.com>
References: <20240412210756.309828-1-weilin.wang@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit

From: Weilin Wang <weilin.wang@intel.com>

We want to include all used tool events in each event group to ensure
sharing events so that help improve multiplexing.

This change updates parse_id and metricgroupg__build_event_string to use
the get_tool_event_str helper function to generate tool event string
instead of inserting temporary tool event ids and generate string from
the tool event ids.

Signed-off-by: Weilin Wang <weilin.wang@intel.com>
---
 tools/perf/util/metricgroup.c | 61 +++++++++++++----------------------
 1 file changed, 22 insertions(+), 39 deletions(-)

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 86b6528e5a44..39746d18f078 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -774,6 +774,18 @@ static int get_tool_event_str(struct strbuf *events,
 	int i = 0;
 	int ret;
 
+	/*
+	 * We may fail to share events between metrics because a tool
+	 * event isn't present in one metric. For example, a ratio of
+	 * cache misses doesn't need duration_time but the same events
+	 * may be used for a misses per second. Events without sharing
+	 * implies multiplexing, that is best avoided, so place
+	 * all tool events in every group.
+	 * This function helps place all tool events in every group by
+	 * generating the tool event strbuf that to be added in event
+	 * group strings.
+	 */
+
 	perf_tool_event__for_each_event(i) {
 		if (tool_events[i]) {
 			const char *tmp = strdup(perf_tool_event__to_str(i));
@@ -795,15 +807,18 @@ static int get_tool_event_str(struct strbuf *events,
 static int metricgroup__build_event_string(struct strbuf *events,
 					   const struct expr_parse_ctx *ctx,
 					   const char *modifier,
-					   bool group_events)
+					   bool group_events,
+					   const bool tool_events[PERF_TOOL_MAX])
 {
 	struct hashmap_entry *cur;
 	size_t bkt;
 	bool no_group = true, has_tool_events = false;
-	bool tool_events[PERF_TOOL_MAX] = {false};
 	int ret = 0;
+	struct strbuf tool_event_str = STRBUF_INIT;
 
 #define RETURN_IF_NON_ZERO(x) do { if (x) return x; } while (0)
+	ret = get_tool_event_str(&tool_event_str, tool_events, &has_tool_events);
+	RETURN_IF_NON_ZERO(ret);
 
 	hashmap__for_each_entry(ctx->ids, cur, bkt) {
 		const char *sep, *rsep, *id = cur->pkey;
@@ -814,8 +829,6 @@ static int metricgroup__build_event_string(struct strbuf *events,
 		/* Always move tool events outside of the group. */
 		ev = perf_tool_event__from_str(id);
 		if (ev != PERF_TOOL_NONE) {
-			has_tool_events = true;
-			tool_events[ev] = true;
 			continue;
 		}
 		/* Separate events with commas and open the group if necessary. */
@@ -879,19 +892,8 @@ static int metricgroup__build_event_string(struct strbuf *events,
 		RETURN_IF_NON_ZERO(ret);
 	}
 	if (has_tool_events) {
-		int i;
-
-		perf_tool_event__for_each_event(i) {
-			if (tool_events[i]) {
-				if (!no_group) {
-					ret = strbuf_addch(events, ',');
-					RETURN_IF_NON_ZERO(ret);
-				}
-				no_group = false;
-				ret = strbuf_addstr(events, perf_tool_event__to_str(i));
-				RETURN_IF_NON_ZERO(ret);
-			}
-		}
+		ret = strbuf_addstr(events, tool_event_str.buf);
+		RETURN_IF_NON_ZERO(ret);
 	}
 
 	return ret;
@@ -2421,32 +2423,13 @@ static int parse_ids(bool metric_no_merge, struct perf_pmu *fake_pmu,
 
 	*out_evlist = NULL;
 	if (!metric_no_merge || hashmap__size(ids->ids) == 0) {
-		bool added_event = false;
-		int i;
 		/*
-		 * We may fail to share events between metrics because a tool
-		 * event isn't present in one metric. For example, a ratio of
-		 * cache misses doesn't need duration_time but the same events
-		 * may be used for a misses per second. Events without sharing
-		 * implies multiplexing, that is best avoided, so place
-		 * all tool events in every group.
-		 *
-		 * Also, there may be no ids/events in the expression parsing
+		 * There may be no ids/events in the expression parsing
 		 * context because of constant evaluation, e.g.:
 		 *    event1 if #smt_on else 0
 		 * Add a tool event to avoid a parse error on an empty string.
 		 */
-		perf_tool_event__for_each_event(i) {
-			if (tool_events[i]) {
-				char *tmp = strdup(perf_tool_event__to_str(i));
-
-				if (!tmp)
-					return -ENOMEM;
-				ids__insert(ids->ids, tmp);
-				added_event = true;
-			}
-		}
-		if (!added_event && hashmap__size(ids->ids) == 0) {
+		if (hashmap__size(ids->ids) == 0) {
 			char *tmp = strdup("duration_time");
 
 			if (!tmp)
@@ -2455,7 +2438,7 @@ static int parse_ids(bool metric_no_merge, struct perf_pmu *fake_pmu,
 		}
 	}
 	ret = metricgroup__build_event_string(&events, ids, modifier,
-					      group_events);
+					      group_events, tool_events);
 	if (ret)
 		return ret;
 
-- 
2.42.0