Received-SPF: pass (google.com: domain of linux-kernel+bounces-147962-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240412210756.309828-1-weilin.wang@intel.com> <20240412210756.309828-6-weilin.wang@intel.com>
In-Reply-To: <20240412210756.309828-6-weilin.wang@intel.com>
From: Ian Rogers <irogers@google.com>
Date: Tue, 16 Apr 2024 22:35:48 -0700
Message-ID: <CAP-5=fWX9rttA5m4MtwzTEp-ocw=sdXw34L=PW7rdt+mOdesig@mail.gmail.com>
Subject: Re: [RFC PATCH v5 05/16] perf stat: Add functions to set counter
 bitmaps for hardware-grouping method
To: 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>, 
	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>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Apr 12, 2024 at 2:08=E2=80=AFPM <weilin.wang@intel.com> wrote:
>
> From: Weilin Wang <weilin.wang@intel.com>
>
> Add metricgroup__event_info data structure to represent an event in the
> metric grouping context; the list of counters and the PMU name an event
> should be collected with.
>
> Add functions to parse event counter info from pmu-events and generate a
> list of metricgroup__event_info data to prepare grouping.
>
> Reviewed-by: Ian Rogers <irogers@google.com>
> Signed-off-by: Weilin Wang <weilin.wang@intel.com>
> ---
>  tools/perf/util/metricgroup.c | 219 +++++++++++++++++++++++++++++++++-
>  1 file changed, 216 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.=
c
> index 8047f03b2b1f..1a7ac17f7ae1 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -24,6 +24,7 @@
>  #include <linux/list_sort.h>
>  #include <linux/string.h>
>  #include <linux/zalloc.h>
> +#include <linux/bitmap.h>
>  #include <perf/cpumap.h>
>  #include <subcmd/parse-options.h>
>  #include <api/fs/fs.h>
> @@ -159,6 +160,29 @@ struct metric {
>         struct evlist *evlist;
>  };
>
> +/* Maximum number of counters per PMU*/
> +#define NR_COUNTERS    16
> +
> +/**
> + * An event used in a metric. This info is for metric grouping.
> + */
> +struct metricgroup__event_info {
> +       struct list_head nd;
> +       /** The name of the event. */
> +       const char *name;
> +       /** The name of the pmu the event be collected on. */
> +       const char *pmu_name;
> +       /** The event uses fixed counter. */
> +       bool fixed_counter;
> +       /**
> +        * The event uses special counters that we consider that as free =
counter
> +        * during the event grouping.
> +        */
> +       bool free_counter;
> +       /** The counters the event allowed to be collected on. */
> +       DECLARE_BITMAP(counters, NR_COUNTERS);
> +};
> +
>  /**
>   * Each group is one node in the group string list.
>   */
> @@ -1440,6 +1464,181 @@ static int build_combined_expr_ctx(const struct l=
ist_head *metric_list,
>         return ret;
>  }
>
> +/**
> + * set_counter_bitmap - The counter bitmap: [0-15].
> + */
> +static int set_counter_bitmap(int pos, unsigned long *bitmap)
> +{
> +       if (pos >=3D NR_COUNTERS || pos < 0)
> +               return -EINVAL;
> +       __set_bit(pos, bitmap);
> +       return 0;
> +}
> +
> +static int parse_fixed_counter(const char *counter,
> +                             unsigned long *bitmap,
> +                             bool *fixed)
> +{
> +       int ret =3D -ENOENT;
> +       /* TODO: this pattern maybe different on some other platforms */
> +       const char *pattern =3D "Fixed counter ";
> +       int pos =3D 0;
> +
> +       if (!strncmp(counter, pattern, strlen(pattern))) {
> +               pos =3D atoi(counter + strlen(pattern));
> +               ret =3D set_counter_bitmap(pos, bitmap);
> +               if (ret)
> +                       return ret;
> +               *fixed =3D true;
> +               return 0;
> +       }
> +       return ret;
> +}
> +
> +/**
> + * parse_counter - Parse event counter info from pmu-events and set up b=
itmap
> + * accordingly.
> + *
> + * @counter: counter info string to be parsed.
> + * @bitmap: bitmap to set based on counter info parsed.
> + * @fixed: is set to true if the event uses fixed counter.
> + */
> +static int parse_counter(const char *counter,
> +                       unsigned long *bitmap,
> +                       bool *fixed)
> +{
> +       int ret =3D 0;
> +       char *p;
> +       char *tok;
> +       int pos =3D 0;
> +
> +       ret =3D parse_fixed_counter(counter, bitmap, fixed);
> +       /* ret=3D=3D0 means matched with fixed counter */
> +       if (ret =3D=3D 0)
> +               return ret;
> +
> +       p =3D strdup(counter);
> +       if (!p)
> +               return -ENOMEM;
> +       tok =3D strtok(p, ",");
> +       if (!tok)
> +               return -ENOENT;
> +
> +       while (tok) {
> +               pos =3D atoi(tok);
> +               ret =3D set_counter_bitmap(pos, bitmap);
> +               if (ret)
> +                       return ret;
> +               tok =3D strtok(NULL, ",");
> +       }
> +       return 0;
> +}
> +
> +static struct metricgroup__event_info *event_info__new(const char *name,
> +                                                     const char *pmu_nam=
e,
> +                                                     const char *counter=
,
> +                                                     bool free_counter)
> +{
> +       int ret =3D 0;
> +       char *bit_buf =3D malloc(NR_COUNTERS);
> +       bool fixed_counter =3D false;
> +       struct metricgroup__event_info *e;
> +
> +       e =3D zalloc(sizeof(*e));
> +       if (!e)
> +               return NULL;
> +       if (!pmu_name)
> +               pmu_name =3D "core";
> +
> +       e->name =3D strdup(name);
> +       e->pmu_name =3D strdup(pmu_name);
> +       if (!e->pmu_name || !e->name)
> +               return NULL;
> +       e->free_counter =3D free_counter;
> +       if (free_counter) {
> +               ret =3D set_counter_bitmap(0, e->counters);
> +               if (ret)
> +                       return NULL;
> +       } else {
> +               ret =3D parse_counter(counter, e->counters, &fixed_counte=
r);
> +               if (ret)
> +                       return NULL;
> +               e->fixed_counter =3D fixed_counter;
> +       }
> +
> +       bitmap_scnprintf(e->counters, NR_COUNTERS, bit_buf, NR_COUNTERS);
> +       pr_debug("Event %s requires pmu %s counter: %s bitmap %s, [pmu=3D=
%s]\n",
> +               e->name, e->pmu_name, counter, bit_buf, pmu_name);
> +
> +       return e;
> +}
> +
> +struct metricgroup__add_metric_event_data {
> +       struct list_head *list;
> +       /* pure event name, exclude umask and other info*/
> +       const char *event_name;
> +       /* event name and umask if applicable*/
> +       const char *event_id;
> +};
> +
> +static int metricgroup__add_metric_event_callback(const struct pmu_event=
 *pe,
> +                                                const struct pmu_events_=
table *table __maybe_unused,
> +                                                void *data)
> +{
> +       struct metricgroup__event_info *event;
> +       struct metricgroup__add_metric_event_data *d =3D data;
> +
> +       if (!strcasecmp(pe->name, d->event_name)) {
> +               event =3D event_info__new(d->event_id, pe->pmu, pe->count=
er, /*free_counter=3D*/false);

I'm seeing a build error at this patch:
```
  CC      /tmp/perf/util/metricgroup.o
util/metricgroup.c: In function =E2=80=98metricgroup__add_metric_event_call=
back=E2=80=99:
util/metricgroup.c:1592:65: error: =E2=80=98const struct pmu_event=E2=80=99=
 has no
member named =E2=80=98counter=E2=80=99
 1592 |                 event =3D event_info__new(d->event_id, pe->pmu,
pe->counter, /*free_counter=3D*/false);
      |                                                                 ^~
util/metricgroup.c: In function =E2=80=98get_metricgroup_events=E2=80=99:
util/metricgroup.c:1633:23: error: implicit declaration of function
=E2=80=98pmu_events_table_for_each_event=E2=80=99; did you mean
=E2=80=98pmu_events_table__for_each_event=E2=80=99?
[-Werror=3Dimplicit-function-declaration]
 1633 |                 ret =3D pmu_events_table_for_each_event(table,
      |                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                       pmu_events_table__for_each_event
cc1: all warnings being treated as errors
```
it must be resolved by a later patch,

Thanks,
Ian

> +               if (!event)
> +                       return -ENOMEM;
> +               list_add(&event->nd, d->list);
> +       }
> +
> +       return 0;
> +}
> +
> +/**
> + * get_metricgroup_events - Find counter requirement of events from the
> + * pmu_events table
> + * @full_id: the full event identifiers.
> + * @table: pmu_events table that is searched for event data.
> + * @event_info_list: the list that the new event counter info added to.
> + */
> +static int get_metricgroup_events(const char *full_id,
> +                                const struct pmu_events_table *table,
> +                                struct list_head *event_info_list)
> +{
> +       LIST_HEAD(list);
> +       int ret =3D 0;
> +       const char *id;
> +       const char *rsep, *sep =3D strchr(full_id, '@');
> +
> +       if (sep) {
> +               rsep =3D strchr(full_id, ',');
> +               id =3D strndup(sep + 1, rsep - sep - 1);
> +               if (!id) {
> +                       ret =3D -ENOMEM;
> +                       goto out;
> +               }
> +       } else {
> +               id =3D full_id;
> +       }
> +       {
> +               struct metricgroup__add_metric_event_data data =3D {
> +                       .list =3D &list,
> +                       .event_name =3D id,
> +                       .event_id =3D full_id,
> +               };
> +               ret =3D pmu_events_table_for_each_event(table,
> +                               metricgroup__add_metric_event_callback, &=
data);
> +       }
> +
> +out:
> +       list_splice(&list, event_info_list);
> +       return ret;
> +}
> +
>  /**
>   * hw_aware_build_grouping - Build event groupings by reading counter
>   * requirement of the events and counter available on the system from
> @@ -1453,9 +1652,25 @@ static int hw_aware_build_grouping(struct expr_par=
se_ctx *ctx __maybe_unused,
>                                   const char *modifier __maybe_unused)
>  {
>         int ret =3D 0;
> +       struct hashmap_entry *cur;
> +       LIST_HEAD(pmu_info_list);
> +       LIST_HEAD(event_info_list);
> +       size_t bkt;
> +       const struct pmu_events_table *etable =3D perf_pmu__find_events_t=
able(NULL);
> +
> +#define RETURN_IF_NON_ZERO(x) do { if (x) return x; } while (0)
> +       hashmap__for_each_entry(ctx->ids, cur, bkt) {
> +               const char *id =3D cur->pkey;
> +
> +               pr_debug("found event %s\n", id);
> +
> +               ret =3D get_metricgroup_events(id, etable, &event_info_li=
st);
> +               if (ret)
> +                       return ret;
> +       }
>
> -       pr_debug("This is a placeholder\n");
>         return ret;
> +#undef RETURN_IF_NON_ZERO
>  }
>
>  static void group_str_free(struct metricgroup__group_strs *g)
> @@ -1529,8 +1744,6 @@ static int hw_aware_parse_ids(struct perf_pmu *fake=
_pmu,
>         *out_evlist =3D parsed_evlist;
>         parsed_evlist =3D NULL;
>  err_out:
> -       parse_events_error__exit(&parse_error);
> -       evlist__delete(parsed_evlist);
>         metricgroup__free_grouping_strs(&groupings);
>         return ret;
>  }
> --
> 2.42.0
>