Received-SPF: pass (google.com: domain of linux-kernel+bounces-112596-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240209031441.943012-1-weilin.wang@intel.com> <20240209031441.943012-14-weilin.wang@intel.com>
In-Reply-To: <20240209031441.943012-14-weilin.wang@intel.com>
From: Ian Rogers <irogers@google.com>
Date: Sat, 23 Mar 2024 22:46:33 -0700
Message-ID: <CAP-5=fVEDik60cFZHevWDj4OdNynB+LF6r7jkuqc1x6M=4qyGg@mail.gmail.com>
Subject: Re: [RFC PATCH v4 13/15] perf stat: Code refactoring in hardware-grouping
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>, 
	Mark Rutland <mark.rutland@arm.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Feb 8, 2024 at 7:14=E2=80=AFPM <weilin.wang@intel.com> wrote:
>
> From: Weilin Wang <weilin.wang@intel.com>
>
> Decouple the step to generate final grouping strings out from the
> build_grouping step so that we could do single metric grouping and then
> merge groups if needed later.
>
> Signed-off-by: Weilin Wang <weilin.wang@intel.com>

Reviewed-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/perf/util/metricgroup.c | 50 +++++++++++++++++------------------
>  1 file changed, 25 insertions(+), 25 deletions(-)
>
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.=
c
> index f1eb73957765..cfdbb5f7fb77 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -1896,9 +1896,10 @@ static int find_and_set_counters(struct metricgrou=
p__event_info *e,
>  {
>         int ret;
>         unsigned long find_bit =3D 0;
> -
> -       if (e->taken_alone && current_group->taken_alone)
> +       if (e->taken_alone && current_group->taken_alone) {
> +               pr_debug("current group with taken alone event already\n"=
);
>                 return -ENOSPC;
> +       }
>         if (e->free_counter)
>                 return 0;
>         if (e->fixed_counter) {
> @@ -2017,7 +2018,8 @@ static int assign_event_grouping(struct metricgroup=
__event_info *e,
>
>         list_for_each_entry(g, groups, nd) {
>                 if (!strcasecmp(g->pmu_name, e->pmu_name)) {
> -                       pr_debug("found group for event %s in pmu %s\n", =
e->name, g->pmu_name);
> +                       pr_debug("found group header for event %s in pmu =
%s\n",
> +                               e->name, g->pmu_name);
>                         pmu_group_head =3D g;
>                         break;
>                 }
> @@ -2146,26 +2148,22 @@ static int hw_aware_metricgroup__build_event_stri=
ng(struct list_head *group_strs
>   */
>  static int create_grouping(struct list_head *pmu_info_list,
>                           struct list_head *event_info_list,
> -                         struct list_head *groupings,
> -                         const char *modifier)
> +                         struct list_head *grouping)
>  {
>         int ret =3D 0;
>         struct metricgroup__event_info *e;
> -       LIST_HEAD(groups);
>         char *bit_buf =3D malloc(NR_COUNTERS);
>
> -       //TODO: for each new core group, we should consider to add events=
 that uses fixed counters
> +       //TODO: for each new core group, we could consider to add events =
that
> +       //uses fixed counters
>         list_for_each_entry(e, event_info_list, nd) {
>                 bitmap_scnprintf(e->counters, NR_COUNTERS, bit_buf, NR_CO=
UNTERS);
>                 pr_debug("Event name %s, [pmu]=3D%s, [counters]=3D%s, [ta=
ken_alone]=3D%d\n",
>                         e->name, e->pmu_name, bit_buf, e->taken_alone);
> -               ret =3D assign_event_grouping(e, pmu_info_list, &groups);
> +               ret =3D assign_event_grouping(e, pmu_info_list, grouping)=
;
>                 if (ret)
> -                       goto out;
> +                       return ret;
>         }
> -       ret =3D hw_aware_metricgroup__build_event_string(groupings, modif=
ier, &groups);
> -out:
> -       metricgroup__free_group_list(&groups);
>         return ret;
>  };
>
> @@ -2186,9 +2184,8 @@ static bool is_special_event(const char *id)
>   * @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_un=
used,
> -                                 struct list_head *groupings __maybe_unu=
sed,
> -                                 const char *modifier __maybe_unused)
> +static int hw_aware_build_grouping(struct expr_parse_ctx *ctx,
> +                                  struct list_head *grouping)
>  {
>         int ret =3D 0;
>         struct hashmap_entry *cur;
> @@ -2220,8 +2217,7 @@ static int hw_aware_build_grouping(struct expr_pars=
e_ctx *ctx __maybe_unused,
>         ret =3D get_pmu_counter_layouts(&pmu_info_list, ltable);
>         if (ret)
>                 goto err_out;
> -       ret =3D create_grouping(&pmu_info_list, &event_info_list, groupin=
gs,
> -                            modifier);
> +       ret =3D create_grouping(&pmu_info_list, &event_info_list, groupin=
g);
>
>  err_out:
>         metricgroup__free_event_info(&event_info_list);
> @@ -2267,23 +2263,25 @@ static int hw_aware_parse_ids(struct perf_pmu *fa=
ke_pmu,
>  {
>         struct parse_events_error parse_error;
>         struct evlist *parsed_evlist;
> -       LIST_HEAD(groupings);
> +       LIST_HEAD(grouping_str);
> +       LIST_HEAD(grouping);
>         struct metricgroup__group_strs *group;
>         int ret;
>
>         *out_evlist =3D NULL;
> -       ret =3D hw_aware_build_grouping(ids, &groupings, modifier);
> -       if (ret) {
> -               metricgroup__free_grouping_strs(&groupings);
> -               return ret;
> -       }
> +       ret =3D hw_aware_build_grouping(ids, &grouping);
> +       if (ret)
> +               goto out;
> +       ret =3D hw_aware_metricgroup__build_event_string(&grouping_str, m=
odifier, &grouping);
> +       if (ret)
> +               goto out;
>
>         parsed_evlist =3D evlist__new();
>         if (!parsed_evlist) {
>                 ret =3D -ENOMEM;
>                 goto err_out;
>         }
> -       list_for_each_entry(group, &groupings, nd) {
> +       list_for_each_entry(group, &grouping_str, nd) {
>                 struct strbuf *events =3D &group->grouping_str;
>
>                 pr_debug("Parsing metric events '%s'\n", events->buf);
> @@ -2303,7 +2301,9 @@ static int hw_aware_parse_ids(struct perf_pmu *fake=
_pmu,
>  err_out:
>         parse_events_error__exit(&parse_error);
>         evlist__delete(parsed_evlist);
> -       metricgroup__free_grouping_strs(&groupings);
> +out:
> +       metricgroup__free_group_list(&grouping);
> +       metricgroup__free_grouping_strs(&grouping_str);
>         return ret;
>  }
>
> --
> 2.42.0
>