Received-SPF: pass (google.com: domain of linux-kernel+bounces-66177-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240213075256.1983638-1-namhyung@kernel.org> <20240213075256.1983638-4-namhyung@kernel.org>
In-Reply-To: <20240213075256.1983638-4-namhyung@kernel.org>
From: Ian Rogers <irogers@google.com>
Date: Wed, 14 Feb 2024 16:08:10 -0800
Message-ID: <CAP-5=fVK3vvNoaGHPep8NCweGw9cztKBGQh5+0bVX91PhWp5Eg@mail.gmail.com>
Subject: Re: [PATCH 3/4] perf hist: Do not use event index in hpp__fmt()
To: Namhyung Kim <namhyung@kernel.org>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>, Jiri Olsa <jolsa@kernel.org>, 
	Adrian Hunter <adrian.hunter@intel.com>, Peter Zijlstra <peterz@infradead.org>, 
	Ingo Molnar <mingo@kernel.org>, LKML <linux-kernel@vger.kernel.org>, 
	linux-perf-users@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Feb 12, 2024 at 11:52=E2=80=AFPM Namhyung Kim <namhyung@kernel.org>=
 wrote:
>
> The __hpp__fmt() is to print period values in a hist entry.  It handles
> event groups using linked pair entries.  Until now, it used event index
> to print values of group members.  But we want to make it more robust
> and support groups even if some members in the group were removed.

I'm unclear how it breaks currently. The evsel idx is set the evlist
nr_entries on creation and not updated by a remove. A remove may
change a groups leader, should the remove also make the next entry's
index idx that of the previous group leader?

> Let's use an index table from evsel to value array so that we can skip
> dummy events in the output later.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/ui/hist.c | 34 ++++++++++++++++++++++++++++------
>  1 file changed, 28 insertions(+), 6 deletions(-)
>
> diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
> index 5f4c110d840f..9c4c738edde1 100644
> --- a/tools/perf/ui/hist.c
> +++ b/tools/perf/ui/hist.c
> @@ -48,15 +48,30 @@ static int __hpp__fmt(struct perf_hpp *hpp, struct hi=
st_entry *he,
>         if (evsel__is_group_event(evsel)) {
>                 int idx;
>                 struct hist_entry *pair;
> -               int nr_members =3D evsel->core.nr_members;
> +               int nr_members =3D evsel->core.nr_members - 1;

A comment on the -1 would be useful.

Thanks,
Ian


>                 union {
>                         u64 period;
>                         double percent;
>                 } *val;
> +               struct evsel *member;
> +               struct evsel **idx_table;
>
>                 val =3D calloc(nr_members, sizeof(*val));
>                 if (val =3D=3D NULL)
> -                       return 0;
> +                       goto out;
> +
> +               idx_table =3D calloc(nr_members, sizeof(*idx_table));
> +               if (idx_table =3D=3D NULL)
> +                       goto out;
> +
> +               /*
> +                * Build an index table for each evsel to the val array.
> +                * It cannot use evsel->core.idx because removed events m=
ight
> +                * create a hole so the index is not consecutive anymore.
> +                */
> +               idx =3D 0;
> +               for_each_group_member(member, evsel)
> +                       idx_table[idx++] =3D member;
>
>                 /* collect values in the group members */
>                 list_for_each_entry(pair, &he->pairs.head, pairs.node) {
> @@ -66,8 +81,15 @@ static int __hpp__fmt(struct perf_hpp *hpp, struct his=
t_entry *he,
>                         if (!total)
>                                 continue;
>
> -                       evsel =3D hists_to_evsel(pair->hists);
> -                       idx =3D evsel__group_idx(evsel);
> +                       member =3D hists_to_evsel(pair->hists);
> +                       for (idx =3D 0; idx < nr_members; idx++) {
> +                               if (idx_table[idx] =3D=3D member)
> +                                       break;
> +                       }
> +
> +                       /* this should not happen */
> +                       if (idx =3D=3D nr_members)
> +                               continue;
>
>                         if (fmt_percent)
>                                 val[idx].percent =3D 100.0 * period / tot=
al;
> @@ -75,8 +97,7 @@ static int __hpp__fmt(struct perf_hpp *hpp, struct hist=
_entry *he,
>                                 val[idx].period =3D period;
>                 }
>
> -               /* idx starts from 1 to skip the leader event */
> -               for (idx =3D 1; idx < nr_members; idx++) {
> +               for (idx =3D 0; idx < nr_members; idx++) {
>                         if (fmt_percent) {
>                                 ret +=3D hpp__call_print_fn(hpp, print_fn=
,
>                                                           fmt, len, val[i=
dx].percent);
> @@ -89,6 +110,7 @@ static int __hpp__fmt(struct perf_hpp *hpp, struct his=
t_entry *he,
>                 free(val);
>         }
>
> +out:
>         /*
>          * Restore original buf and size as it's where caller expects
>          * the result will be saved.
> --
> 2.43.0.687.g38aa6559b0-goog
>