Messages in this thread | | | From | Ian Rogers <> | Date | Wed, 10 Apr 2024 10:47:12 -0700 | Subject | Re: [RFC PATCH v4 14/15] perf stat: Add tool events support in hardware-grouping |
| |
On Tue, Apr 9, 2024 at 1:51 PM Wang, Weilin <weilin.wang@intel.com> wrote: > > > > > -----Original Message----- > > From: Ian Rogers <irogers@google.com> > > Sent: Saturday, March 23, 2024 10:56 PM > > To: Wang, Weilin <weilin.wang@intel.com> > > Cc: 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>; Hunter, Adrian <adrian.hunter@intel.com>; linux-perf- > > users@vger.kernel.org; linux-kernel@vger.kernel.org; Taylor, Perry > > <perry.taylor@intel.com>; Alt, Samantha <samantha.alt@intel.com>; Biggers, > > Caleb <caleb.biggers@intel.com>; Mark Rutland <mark.rutland@arm.com> > > Subject: Re: [RFC PATCH v4 14/15] perf stat: Add tool events support in > > hardware-grouping > > > > On Thu, Feb 8, 2024 at 7:14 PM <weilin.wang@intel.com> wrote: > > > > > > From: Weilin Wang <weilin.wang@intel.com> > > > > > > Add tool events into default_core grouping strings if find tool events so > > > that metrics use tool events could be correctly calculated. Need this step > > > to support TopdownL4-L5. > > > > > > Signed-off-by: Weilin Wang <weilin.wang@intel.com> > > > --- > > > tools/perf/util/metricgroup.c | 49 > > ++++++++++++++++++++++++++++++++++- > > > 1 file changed, 48 insertions(+), 1 deletion(-) > > > > > > diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c > > > index cfdbb5f7fb77..e5b8456d0405 100644 > > > --- a/tools/perf/util/metricgroup.c > > > +++ b/tools/perf/util/metricgroup.c > > > @@ -1486,6 +1486,35 @@ static void find_tool_events(const struct > > list_head *metric_list, > > > } > > > } > > > > > > +/** > > > + * get_tool_event_str - Generate and return a string with all the used tool > > > + * event names. > > > + */ > > > +static int get_tool_event_str(struct strbuf *events, > > > + const bool tool_events[PERF_TOOL_MAX], > > > + bool *has_tool_event) > > > +{ > > > + int i = 0; > > > + int ret; > > > + > > > + perf_tool_event__for_each_event(i) { > > > + if (tool_events[i]) { > > > + const char *tmp = strdup(perf_tool_event__to_str(i)); > > > + > > > + if (!tmp) > > > + return -ENOMEM; > > > + *has_tool_event = true; > > > + ret = strbuf_addstr(events, ","); > > > + if (ret) > > > + return ret; > > > + ret = strbuf_addstr(events, tmp); > > > + if (ret) > > > + return ret; > > > + } > > > + } > > > + return 0; > > > +} > > > + > > > /** > > > * build_combined_expr_ctx - Make an expr_parse_ctx with > > all !group_events > > > * metric IDs, as the IDs are held in a set, > > > @@ -2049,6 +2078,7 @@ static int assign_event_grouping(struct > > metricgroup__event_info *e, > > > > > > static int hw_aware_metricgroup__build_event_string(struct list_head > > *group_strs, > > > const char *modifier, > > > + const bool tool_events[PERF_TOOL_MAX], > > > struct list_head *groups) > > > { > > > struct metricgroup__pmu_group_list *p; > > > @@ -2056,8 +2086,12 @@ static int > > hw_aware_metricgroup__build_event_string(struct list_head *group_strs > > > struct metricgroup__group_events *ge; > > > bool no_group = true; > > > int ret = 0; > > > + struct strbuf tool_event_str = STRBUF_INIT; > > > + bool has_tool_event = false; > > > > > > #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_event); > > > > Does metricgroup__build_event_string need updating to use this helper > > function too? > > Hi Ian, > > In parse_ids, tool events are inserted to the ids that passed to > metricgroup__build_event_string. If we want to use this function here, I think > we also need to update the code in parse_ids to not insert tool events. > > I could add this change if you think we should do this update.
I like code-reuse and sharing logic whenever possible, it seems potentially messy though. I'll leave it up to your judgement as to whether it is worth cleaning up. Perhaps there should be a TODO if not.
Thanks, Ian
> Thanks, > Weilin > > > > > Thanks, > > Ian > > > > > + RETURN_IF_NON_ZERO(ret); > > > > > > list_for_each_entry(p, groups, nd) { > > > list_for_each_entry(g, &p->group_head, nd) { > > > @@ -2129,6 +2163,12 @@ static int > > hw_aware_metricgroup__build_event_string(struct list_head *group_strs > > > } > > > ret = strbuf_addf(events, "}:W"); > > > RETURN_IF_NON_ZERO(ret); > > > + > > > + if (!strcmp(p->pmu_name, "default_core") && has_tool_event) > > { > > > + ret = strbuf_addstr(events, tool_event_str.buf); > > > + RETURN_IF_NON_ZERO(ret); > > > + } > > > + > > > pr_debug("events-buf: %s\n", events->buf); > > > list_add_tail(&new_group_str->nd, group_strs); > > > } > > > @@ -2214,6 +2254,7 @@ static int hw_aware_build_grouping(struct > > expr_parse_ctx *ctx, > > > if (ret) > > > goto err_out; > > > } > > > + > > > ret = get_pmu_counter_layouts(&pmu_info_list, ltable); > > > if (ret) > > > goto err_out; > > > @@ -2259,6 +2300,7 @@ static void > > metricgroup__free_grouping_strs(struct list_head > > > */ > > > static int hw_aware_parse_ids(struct perf_pmu *fake_pmu, > > > struct expr_parse_ctx *ids, const char *modifier, > > > + const bool tool_events[PERF_TOOL_MAX], > > > struct evlist **out_evlist) > > > { > > > struct parse_events_error parse_error; > > > @@ -2272,7 +2314,8 @@ static int hw_aware_parse_ids(struct perf_pmu > > *fake_pmu, > > > ret = hw_aware_build_grouping(ids, &grouping); > > > if (ret) > > > goto out; > > > - ret = hw_aware_metricgroup__build_event_string(&grouping_str, > > modifier, &grouping); > > > + ret = hw_aware_metricgroup__build_event_string(&grouping_str, > > modifier, > > > + tool_events, &grouping); > > > if (ret) > > > goto out; > > > > > > @@ -2407,6 +2450,7 @@ static int hw_aware_parse_groups(struct evlist > > *perf_evlist, > > > struct evlist *combined_evlist = NULL; > > > LIST_HEAD(metric_list); > > > struct metric *m; > > > + bool tool_events[PERF_TOOL_MAX] = {false}; > > > int ret; > > > bool metric_no_group = false; > > > bool metric_no_merge = false; > > > @@ -2425,11 +2469,14 @@ static int hw_aware_parse_groups(struct evlist > > *perf_evlist, > > > if (!metric_no_merge) { > > > struct expr_parse_ctx *combined = NULL; > > > > > > + find_tool_events(&metric_list, tool_events); > > > + > > > ret = hw_aware_build_combined_expr_ctx(&metric_list, > > &combined); > > > > > > if (!ret && combined && hashmap__size(combined->ids)) { > > > ret = hw_aware_parse_ids(fake_pmu, combined, > > > /*modifier=*/NULL, > > > + tool_events, > > > &combined_evlist); > > > } > > > > > > -- > > > 2.42.0 > > >
| |