From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Ian Rogers <irogers@google.com>
Cc: weilin.wang@intel.com, Kan Liang <kan.liang@linux.intel.com>,
Namhyung Kim <namhyung@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>,
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: Re: [RFC PATCH v5 02/16] perf stat: Add basic functions for the hardware aware grouping
Date: Wed, 17 Apr 2024 13:24:04 -0300 [thread overview]
Message-ID: <Zh_3pCLXfL-faTpx@x1> (raw)
In-Reply-To: <CAP-5=fUTHPu_vaNyeaq9uDwf7Hq79khFZymFJK2w39k-9RdutQ@mail.gmail.com>
On Tue, Apr 16, 2024 at 09:56:38PM -0700, Ian Rogers wrote:
> On Fri, Apr 12, 2024 at 2:08 PM <weilin.wang@intel.com> wrote:
> >
> > From: Weilin Wang <weilin.wang@intel.com>
> >
> > Add the first set of functions for the hardware aware grouping method. Function
> > hw_aware_parse_groups() is the entry point of this metric grouping method. It
> > does metric grouping on a combined list of events and will create a list of
> > grouping strings as final results of the grouping method. These grouping strings
> > will be used in the same manner as existing metric grouping process.
> >
> > This method will fall back to normal grouping when hardware aware grouping
> > return with err so that perf stat still executes and returns with correct
> > result.
> >
> > Signed-off-by: Weilin Wang <weilin.wang@intel.com>
>
> Reviewed-by: Ian Rogers <irogers@google.com>
This one isn't applying:
⬢[acme@toolbox perf-tools-next]$ git log -10 --oneline tools/perf/util/metricgroup.c
b6226e7e6daa823d (HEAD -> perf-tools-next) perf stat: Add new field in stat_config to enable hardware aware grouping
4b5ee6db2d3cd624 perf metrics: Remove the "No_group" metric group
97b6b4ac1c5dd42a perf metrics: Fix segv for metrics with no events
d4be39cadef0dbba perf metrics: Fix metric matching
a59fb796a36bb6c2 perf metrics: Compute unmerged uncore metrics individually
7bbe8f0071dfa23f perf tools: Fix calloc() arguments to address error introduced in gcc-14
e2b005d6ec0e738d perf metrics: Avoid segv if default metricgroup isn't set
54409997d4b99ab6 perf metric: "Compat" supports regular expression matching identifiers
4000519eb0c66594 perf pmu-events: Add extra underscore to function names
1c0e47956a8e1109 perf metrics: Sort the Default metricgroup
⬢[acme@toolbox perf-tools-next]$
So just the first patch was applied so far, I'll push what I have to
tmp.perf-tools-next and then later to perf-tools-next so that this work
can be continued from there.
- Arnaldo
> > ---
> > tools/perf/util/metricgroup.c | 217 +++++++++++++++++++++++++++++++++-
> > 1 file changed, 216 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> > index 11613450725a..8047f03b2b1f 100644
> > --- a/tools/perf/util/metricgroup.c
> > +++ b/tools/perf/util/metricgroup.c
> > @@ -159,6 +159,14 @@ struct metric {
> > struct evlist *evlist;
> > };
> >
> > +/**
> > + * Each group is one node in the group string list.
> > + */
> > +struct metricgroup__group_strs {
> > + struct list_head nd;
> > + struct strbuf grouping_str;
> > +};
> > +
> > static void metric__watchdog_constraint_hint(const char *name, bool foot)
> > {
> > static bool violate_nmi_constraint;
> > @@ -1432,6 +1440,101 @@ static int build_combined_expr_ctx(const struct list_head *metric_list,
> > return ret;
> > }
> >
> > +/**
> > + * hw_aware_build_grouping - Build event groupings by reading counter
> > + * requirement of the events and counter available on the system from
> > + * pmu-events.
> > + * @ctx: the event identifiers parsed from metrics.
> > + * @groupings: header to the list of final event grouping.
> > + * @modifier: any modifiers added to the events.
> > + */
> > +static int hw_aware_build_grouping(struct expr_parse_ctx *ctx __maybe_unused,
> > + struct list_head *groupings __maybe_unused,
> > + const char *modifier __maybe_unused)
> > +{
> > + int ret = 0;
> > +
> > + pr_debug("This is a placeholder\n");
> > + return ret;
> > +}
> > +
> > +static void group_str_free(struct metricgroup__group_strs *g)
> > +{
> > + if (!g)
> > + return;
> > +
> > + strbuf_release(&g->grouping_str);
> > + free(g);
> > +}
> > +
> > +static void metricgroup__free_grouping_strs(struct list_head
> > + *grouping_strs)
> > +{
> > + struct metricgroup__group_strs *g, *tmp;
> > +
> > + list_for_each_entry_safe(g, tmp, grouping_strs, nd) {
> > + list_del_init(&g->nd);
> > + group_str_free(g);
> > + }
> > +}
> > +
> > +/**
> > + * hw_aware_parse_ids - Build the event string for the ids and parse them
> > + * creating an evlist. The encoded metric_ids are decoded. Events are placed
> > + * into groups based on event counter requirements and counter availabilities of
> > + * the system.
> > + * @metric_no_merge: is metric sharing explicitly disabled.
> > + * @fake_pmu: used when testing metrics not supported by the current CPU.
> > + * @ids: the event identifiers parsed from a metric.
> > + * @modifier: any modifiers added to the events.
> > + * @out_evlist: the created list of events.
> > + */
> > +static int hw_aware_parse_ids(struct perf_pmu *fake_pmu,
> > + struct expr_parse_ctx *ids, const char *modifier,
> > + struct evlist **out_evlist)
> > +{
> > + struct parse_events_error parse_error;
> > + struct evlist *parsed_evlist;
> > + LIST_HEAD(groupings);
> > + struct metricgroup__group_strs *group;
> > + int ret;
> > +
> > + *out_evlist = NULL;
> > + ret = hw_aware_build_grouping(ids, &groupings, modifier);
> > + if (ret) {
> > + metricgroup__free_grouping_strs(&groupings);
> > + return ret;
> > + }
> > +
> > + parsed_evlist = evlist__new();
> > + if (!parsed_evlist) {
> > + ret = -ENOMEM;
> > + goto err_out;
> > + }
> > + list_for_each_entry(group, &groupings, nd) {
> > + struct strbuf *events = &group->grouping_str;
> > +
> > + pr_debug("Parsing metric events '%s'\n", events->buf);
> > + parse_events_error__init(&parse_error);
> > + ret = __parse_events(parsed_evlist, events->buf, /*pmu_filter=*/NULL,
> > + &parse_error, fake_pmu, /*warn_if_reordered=*/false);
> > + if (ret) {
> > + parse_events_error__print(&parse_error, events->buf);
> > + goto err_out;
> > + }
> > + ret = decode_all_metric_ids(parsed_evlist, modifier);
> > + if (ret)
> > + goto err_out;
> > + }
> > + *out_evlist = parsed_evlist;
> > + parsed_evlist = NULL;
> > +err_out:
> > + parse_events_error__exit(&parse_error);
> > + evlist__delete(parsed_evlist);
> > + metricgroup__free_grouping_strs(&groupings);
> > + return ret;
> > +}
> > +
> > /**
> > * parse_ids - Build the event string for the ids and parse them creating an
> > * evlist. The encoded metric_ids are decoded.
> > @@ -1520,6 +1623,113 @@ static int parse_ids(bool metric_no_merge, struct perf_pmu *fake_pmu,
> > return ret;
> > }
> >
> > +static int hw_aware_parse_groups(struct evlist *perf_evlist,
> > + const char *pmu, const char *str,
> > + bool metric_no_threshold,
> > + const char *user_requested_cpu_list,
> > + bool system_wide,
> > + struct perf_pmu *fake_pmu,
> > + struct rblist *metric_events_list,
> > + const struct pmu_metrics_table *table)
> > +{
> > + struct evlist *combined_evlist = NULL;
> > + LIST_HEAD(metric_list);
> > + struct metric *m;
> > + int ret;
> > + bool metric_no_group = false;
> > + bool metric_no_merge = false;
> > +
> > + if (metric_events_list->nr_entries == 0)
> > + metricgroup__rblist_init(metric_events_list);
> > + ret = metricgroup__add_metric_list(pmu, str, metric_no_group, metric_no_threshold,
> > + user_requested_cpu_list,
> > + system_wide, &metric_list, table);
> > + if (ret)
> > + goto out;
> > +
> > + /* Sort metrics from largest to smallest. */
> > + list_sort(NULL, &metric_list, metric_list_cmp);
> > +
> > + if (!metric_no_merge) {
> > + struct expr_parse_ctx *combined = NULL;
> > +
> > + ret = build_combined_expr_ctx(&metric_list, &combined);
> > +
> > + if (!ret && combined && hashmap__size(combined->ids)) {
> > + ret = hw_aware_parse_ids(fake_pmu, combined,
> > + /*modifier=*/NULL,
> > + &combined_evlist);
> > + }
> > +
> > + if (combined)
> > + expr__ctx_free(combined);
> > + if (ret)
> > + goto out;
> > + }
> > +
> > + list_for_each_entry(m, &metric_list, nd) {
> > + struct metric_expr *expr;
> > + struct metric_event *me;
> > + struct evsel **metric_events;
> > +
> > + ret = setup_metric_events(fake_pmu ? "all" : m->pmu, m->pctx->ids,
> > + combined_evlist, &metric_events);
> > + if (ret) {
> > + pr_debug("Cannot resolve IDs for %s: %s\n",
> > + m->metric_name, m->metric_expr);
> > + goto out;
> > + }
> > +
> > + me = metricgroup__lookup(metric_events_list, metric_events[0], true);
> > +
> > + expr = malloc(sizeof(struct metric_expr));
> > + if (!expr) {
> > + ret = -ENOMEM;
> > + free(metric_events);
> > + goto out;
> > + }
> > +
> > + expr->metric_refs = m->metric_refs;
> > + m->metric_refs = NULL;
> > + expr->metric_expr = m->metric_expr;
> > + if (m->modifier) {
> > + char *tmp;
> > +
> > + if (asprintf(&tmp, "%s:%s", m->metric_name, m->modifier) < 0)
> > + expr->metric_name = NULL;
> > + else
> > + expr->metric_name = tmp;
> > + } else {
> > + expr->metric_name = strdup(m->metric_name);
> > + }
> > +
> > + if (!expr->metric_name) {
> > + ret = -ENOMEM;
> > + free(metric_events);
> > + goto out;
> > + }
> > + expr->metric_threshold = m->metric_threshold;
> > + expr->metric_unit = m->metric_unit;
> > + expr->metric_events = metric_events;
> > + expr->runtime = m->pctx->sctx.runtime;
> > + list_add(&expr->nd, &me->head);
> > + }
> > +
> > + if (combined_evlist) {
> > + evlist__splice_list_tail(perf_evlist, &combined_evlist->core.entries);
> > + evlist__delete(combined_evlist);
> > + }
> > +
> > + list_for_each_entry(m, &metric_list, nd) {
> > + if (m->evlist)
> > + evlist__splice_list_tail(perf_evlist, &m->evlist->core.entries);
> > + }
> > +
> > +out:
> > + metricgroup__free_metrics(&metric_list);
> > + return ret;
> > +}
> > +
> > static int parse_groups(struct evlist *perf_evlist,
> > const char *pmu, const char *str,
> > bool metric_no_group,
> > @@ -1698,10 +1908,15 @@ int metricgroup__parse_groups(struct evlist *perf_evlist,
> > if (!table)
> > return -EINVAL;
> > if (hardware_aware_grouping) {
> > + int ret;
>
> nit: there should be a '\n' between variables and code,
> scripts/checkpatch.pl sould catch this.
>
> > pr_debug("Use hardware aware grouping instead of traditional metric grouping method\n");
> > + ret = hw_aware_parse_groups(perf_evlist, pmu, str,
> > + metric_no_threshold, user_requested_cpu_list, system_wide,
> > + /*fake_pmu=*/NULL, metric_events, table);
> > + if (!ret)
> > + return 0;
> > }
> >
> > -
>
> nit: unneeded whitespace change.
>
> Thanks,
> Ian
>
> > return parse_groups(perf_evlist, pmu, str, metric_no_group, metric_no_merge,
> > metric_no_threshold, user_requested_cpu_list, system_wide,
> > /*fake_pmu=*/NULL, metric_events, table);
> > --
> > 2.42.0
> >
next prev parent reply other threads:[~2024-04-17 16:24 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-12 21:07 [RFC PATCH v5 00/16] Perf stat metric grouping with hardware information weilin.wang
2024-04-12 21:07 ` [RFC PATCH v5 01/16] perf stat: Add new field in stat_config to enable hardware aware grouping weilin.wang
2024-04-17 3:49 ` Ian Rogers
2024-04-17 16:21 ` Arnaldo Carvalho de Melo
2024-04-12 21:07 ` [RFC PATCH v5 02/16] perf stat: Add basic functions for the " weilin.wang
2024-04-17 4:56 ` Ian Rogers
2024-04-17 16:24 ` Arnaldo Carvalho de Melo [this message]
2024-04-12 21:07 ` [RFC PATCH v5 03/16] perf pmu-events: Add functions in jevent.py to parse counter and event info for " weilin.wang
2024-04-17 5:42 ` Ian Rogers
2024-04-12 21:07 ` [RFC PATCH v5 04/16] find_bit: add _find_last_and_bit() to support finding the most significant set bit weilin.wang
2024-04-12 21:07 ` [RFC PATCH v5 05/16] perf stat: Add functions to set counter bitmaps for hardware-grouping method weilin.wang
2024-04-17 5:35 ` Ian Rogers
2024-04-12 21:07 ` [RFC PATCH v5 06/16] perf stat: Add functions to get counter info weilin.wang
2024-04-12 21:07 ` [RFC PATCH v5 07/16] perf stat: Add functions to create new group and assign events into groups weilin.wang
2024-04-12 21:07 ` [RFC PATCH v5 08/16] perf stat: Add build string function and topdown events handling in hardware-grouping weilin.wang
2024-04-17 5:56 ` Ian Rogers
2024-04-12 21:07 ` [RFC PATCH v5 09/16] perf stat: Add function to handle special events " weilin.wang
2024-04-17 6:12 ` Ian Rogers
2024-04-12 21:07 ` [RFC PATCH v5 10/16] perf stat: Add function to combine metrics for hardware-grouping weilin.wang
2024-04-12 21:07 ` [RFC PATCH v5 11/16] perf stat: Add partial support on MSR in hardware-grouping weilin.wang
2024-04-12 21:07 ` [RFC PATCH v5 12/16] perf stat: Handle NMI " weilin.wang
2024-04-12 21:07 ` [RFC PATCH v5 13/16] perf stat: Code refactoring " weilin.wang
2024-04-12 21:07 ` [RFC PATCH v5 14/16] perf stat: Add tool events support " weilin.wang
2024-04-12 21:07 ` [RFC PATCH v5 15/16] perf stat: use tool event helper function in metricgroup__build_event_string weilin.wang
2024-04-17 6:36 ` Ian Rogers
2024-04-12 21:07 ` [RFC PATCH v5 16/16] perf stat: Add hardware-grouping cmd option to perf stat weilin.wang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Zh_3pCLXfL-faTpx@x1 \
--to=acme@kernel.org \
--cc=adrian.hunter@intel.com \
--cc=alexander.shishkin@linux.intel.com \
--cc=caleb.biggers@intel.com \
--cc=irogers@google.com \
--cc=jolsa@kernel.org \
--cc=kan.liang@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=namhyung@kernel.org \
--cc=perry.taylor@intel.com \
--cc=peterz@infradead.org \
--cc=samantha.alt@intel.com \
--cc=weilin.wang@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).