Received-SPF: pass (google.com: domain of linux-kernel+bounces-37293-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20231212230224.1473300-1-weilin.wang@intel.com> <20231212230224.1473300-10-weilin.wang@intel.com>
In-Reply-To: <20231212230224.1473300-10-weilin.wang@intel.com>
From: Ian Rogers <irogers@google.com>
Date: Wed, 24 Jan 2024 07:57:18 -0800
Message-ID: <CAP-5=fVc6UwFKezDKQuwe7yLhL5vofSKWthwLRByTEeqoeUs9w@mail.gmail.com>
Subject: Re: [RFC PATCH v3 08/18] perf stat: Add functions to get counter info
To: weilin.wang@intel.com
Cc: Peter Zijlstra <peterz@infradead.org>, Ingo Molnar <mingo@redhat.com>, 
	Arnaldo Carvalho de Melo <acme@kernel.org>, Alexander Shishkin <alexander.shishkin@linux.intel.com>, 
	Jiri Olsa <jolsa@kernel.org>, Namhyung Kim <namhyung@kernel.org>, 
	Adrian Hunter <adrian.hunter@intel.com>, Kan Liang <kan.liang@linux.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>, Mark Rutland <mark.rutland@arm.com>, 
	Yang Jihong <yangjihong1@huawei.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Dec 12, 2023 at 3:03=E2=80=AFPM <weilin.wang@intel.com> wrote:
>
> From: Weilin Wang <weilin.wang@intel.com>
>
> Add data structure metricgroup__pmu_counters to represent hardware counte=
rs
> available in the system.
>
> Add functions to parse pmu-events and create the list of pmu_info_list to
> hold the counter information of the system.
>
> Add functions to free pmu_info_list and event_info_list before exit
> grouping for hardware-grouping method
>
> This method would fall back to normal grouping when event json files do n=
ot
> support hardware aware grouping.
>
> Signed-off-by: Weilin Wang <weilin.wang@intel.com>
> ---
>  tools/perf/util/metricgroup.c | 85 +++++++++++++++++++++++++++++++++--
>  tools/perf/util/metricgroup.h | 15 +++++++
>  2 files changed, 97 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.=
c
> index 24268882d355..a393584c7a73 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -1507,6 +1507,27 @@ static int parse_counter(const char *counter,
>         return 0;
>  }
>
> +static void metricgroup__free_event_info(struct list_head
> +                                       *event_info_list)
> +{
> +       struct metricgroup__event_info *e, *tmp;
> +
> +       list_for_each_entry_safe(e, tmp, event_info_list, nd) {
> +               list_del_init(&e->nd);
> +               free(e);
> +       }
> +}
> +
> +static void metricgroup__free_pmu_info(struct list_head *pmu_info_list)
> +{
> +       struct metricgroup__pmu_counters *p, *tmp;
> +
> +       list_for_each_entry_safe(p, tmp, pmu_info_list, nd) {
> +               list_del_init(&p->nd);
> +               free(p);
> +       }
> +}
> +
>  static struct metricgroup__event_info *event_info__new(const char *name,
>                                                       const char *pmu_nam=
e,
>                                                       const char *counter=
,
> @@ -1525,7 +1546,7 @@ static struct metricgroup__event_info *event_info__=
new(const char *name,
>
>         e->name =3D name;
>         e->free_counter =3D free_counter;
> -       e->pmu_name =3D strdup(pmu_name);
> +       e->pmu_name =3D pmu_name;
>         if (free_counter) {
>                 ret =3D set_counter_bitmap(0, e->counters);
>                 if (ret)
> @@ -1560,6 +1581,8 @@ static int metricgroup__add_metric_event_callback(c=
onst struct pmu_event *pe,
>         struct metricgroup__add_metric_event_data *d =3D data;
>
>         if (!strcasecmp(pe->name, d->event_name)) {
> +               if (!pe->counter)
> +                       return -EINVAL;
>                 event =3D event_info__new(d->event_id, pe->pmu, pe->count=
er, /*free_counter=3D*/false);
>                 if (!event)
>                         return -ENOMEM;
> @@ -1599,7 +1622,7 @@ static int get_metricgroup_events(const char *full_=
id,
>                         .event_name =3D id,
>                         .event_id =3D full_id,
>                 };
> -               ret =3D pmu_events_table_for_each_event(table,
> +               ret =3D pmu_events_table__for_each_event(table, /*pmu=3D*=
/NULL,
>                                 metricgroup__add_metric_event_callback, &=
data);
>         }
>
> @@ -1608,6 +1631,57 @@ static int get_metricgroup_events(const char *full=
_id,
>         return ret;
>  }
>
> +static struct metricgroup__pmu_counters *pmu_layout__new(const struct pm=
u_layout *pl)
> +{
> +       struct metricgroup__pmu_counters *l;
> +
> +       l =3D zalloc(sizeof(*l));
> +
> +       if (!l)
> +               return NULL;
> +
> +       l->name =3D pl->pmu;
> +       l->size =3D pl->size;
> +       l->fixed_size =3D pl->fixed_size;
> +       pr_debug("create new pmu_layout: [pmu]=3D%s, [gp_size]=3D%ld, [fi=
xed_size]=3D%ld\n",
> +               l->name, l->size, l->fixed_size);
> +       return l;
> +}
> +
> +static int metricgroup__add_pmu_layout_callback(const struct pmu_layout =
*pl,
> +                                               void *data)
> +{
> +       struct metricgroup__pmu_counters *pmu;
> +       struct list_head *d =3D data;
> +       int ret =3D 0;
> +
> +       pmu =3D pmu_layout__new(pl);
> +       if (!pmu)
> +               return -ENOMEM;
> +       list_add(&pmu->nd, d);
> +       return ret;
> +}
> +
> +/**
> + * get_pmu_counter_layouts - Find counter info of the architecture from
> + * the pmu_layouts table
> + * @pmu_info_list: the list that the new counter info of a pmu is added =
to.
> + * @table: pmu_layouts table that is searched for counter info.
> + */
> +static int get_pmu_counter_layouts(struct list_head *pmu_info_list,
> +                                  const struct pmu_layouts_table
> +                                  *table)
> +{
> +       LIST_HEAD(list);
> +       int ret;
> +
> +       ret =3D pmu_layouts_table__for_each_layout(table,
> +                                               metricgroup__add_pmu_layo=
ut_callback, &list);
> +
> +       list_splice(&list, pmu_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
> @@ -1626,6 +1700,7 @@ static int hw_aware_build_grouping(struct expr_pars=
e_ctx *ctx __maybe_unused,
>         LIST_HEAD(event_info_list);
>         size_t bkt;
>         const struct pmu_events_table *etable =3D perf_pmu__find_events_t=
able(NULL);
> +       const struct pmu_layouts_table *ltable =3D perf_pmu__find_layouts=
_table(NULL);
>
>  #define RETURN_IF_NON_ZERO(x) do { if (x) return x; } while (0)
>         hashmap__for_each_entry(ctx->ids, cur, bkt) {
> @@ -1635,9 +1710,13 @@ static int hw_aware_build_grouping(struct expr_par=
se_ctx *ctx __maybe_unused,
>
>                 ret =3D get_metricgroup_events(id, etable, &event_info_li=
st);
>                 if (ret)
> -                       return ret;
> +                       goto err_out;
>         }
> +       ret =3D get_pmu_counter_layouts(&pmu_info_list, ltable);
>
> +err_out:
> +       metricgroup__free_event_info(&event_info_list);
> +       metricgroup__free_pmu_info(&pmu_info_list);
>         return ret;
>  #undef RETURN_IF_NON_ZERO
>  }
> diff --git a/tools/perf/util/metricgroup.h b/tools/perf/util/metricgroup.=
h
> index 3704545c9a11..802ca15e7c6b 100644
> --- a/tools/perf/util/metricgroup.h
> +++ b/tools/perf/util/metricgroup.h
> @@ -94,6 +94,21 @@ struct metricgroup__event_info {
>         DECLARE_BITMAP(counters, NR_COUNTERS);
>  };
>
> +/**
> + * A node is the counter availability of a pmu.
> + * This info is built up at the beginning from JSON file and
> + * used as a reference in metric grouping process.
> + */
> +struct metricgroup__pmu_counters {
> +       struct list_head nd;
> +       /** The name of the pmu the event collected on. */
> +       const char *name;
> +       //DECLARE_BITMAP(counter_bits, NR_COUNTERS);

nit: this can go.

> +       /** The number of gp counters in the pmu. */
> +       size_t size;
> +       size_t fixed_size;

nit: same naming concern.

Thanks,
Ian

> +};
> +
>  /**
>   * Each group is one node in the group string list.
>   */
> --
> 2.39.3
>