[3/4] perf hist: Do not use event index in hpp__fmt()

  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.

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(-)
  

On Mon, Feb 12, 2024 at 11:52 PM 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 hist_entry *he,
>         if (evsel__is_group_event(evsel)) {
>                 int idx;
>                 struct hist_entry *pair;
> -               int nr_members = evsel->core.nr_members;
> +               int nr_members = 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 = calloc(nr_members, sizeof(*val));
>                 if (val == NULL)
> -                       return 0;
> +                       goto out;
> +
> +               idx_table = calloc(nr_members, sizeof(*idx_table));
> +               if (idx_table == NULL)
> +                       goto out;
> +
> +               /*
> +                * Build an index table for each evsel to the val array.
> +                * It cannot use evsel->core.idx because removed events might
> +                * create a hole so the index is not consecutive anymore.
> +                */
> +               idx = 0;
> +               for_each_group_member(member, evsel)
> +                       idx_table[idx++] = 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 hist_entry *he,
>                         if (!total)
>                                 continue;
>
> -                       evsel = hists_to_evsel(pair->hists);
> -                       idx = evsel__group_idx(evsel);
> +                       member = hists_to_evsel(pair->hists);
> +                       for (idx = 0; idx < nr_members; idx++) {
> +                               if (idx_table[idx] == member)
> +                                       break;
> +                       }
> +
> +                       /* this should not happen */
> +                       if (idx == nr_members)
> +                               continue;
>
>                         if (fmt_percent)
>                                 val[idx].percent = 100.0 * period / total;
> @@ -75,8 +97,7 @@ static int __hpp__fmt(struct perf_hpp *hpp, struct hist_entry *he,
>                                 val[idx].period = period;
>                 }
>
> -               /* idx starts from 1 to skip the leader event */
> -               for (idx = 1; idx < nr_members; idx++) {
> +               for (idx = 0; idx < nr_members; idx++) {
>                         if (fmt_percent) {
>                                 ret += hpp__call_print_fn(hpp, print_fn,
>                                                           fmt, len, val[idx].percent);
> @@ -89,6 +110,7 @@ static int __hpp__fmt(struct perf_hpp *hpp, struct hist_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
>
  

On Wed, Feb 14, 2024 at 4:08 PM Ian Rogers <irogers@google.com> wrote:
>
> On Mon, Feb 12, 2024 at 11:52 PM 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?

The evsel__group_idx() returns evsel->idx - leader->idx.
If it has a group event {A, B, C} then the index would be 0, 1, 2.
If it removes B, the group would be {A, C} with index 0 and 2.
The nr_members is 2 now so it cannot use index 2 for C.

Note that we cannot change the index of C because some information
like annotation histogram relies on the index.

>
> > 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 hist_entry *he,
> >         if (evsel__is_group_event(evsel)) {
> >                 int idx;
> >                 struct hist_entry *pair;
> > -               int nr_members = evsel->core.nr_members;
> > +               int nr_members = evsel->core.nr_members - 1;
>
> A comment on the -1 would be useful.

The 'nr_members' includes the leader which is already printed.
In this code, we want to print member events only, hence -1.
I'll add it to the comment.

Thanks,
Namhyung

>
> Thanks,
> Ian
>
>
> >                 union {
> >                         u64 period;
> >                         double percent;
> >                 } *val;
> > +               struct evsel *member;
> > +               struct evsel **idx_table;
> >
> >                 val = calloc(nr_members, sizeof(*val));
> >                 if (val == NULL)
> > -                       return 0;
> > +                       goto out;
> > +
> > +               idx_table = calloc(nr_members, sizeof(*idx_table));
> > +               if (idx_table == NULL)
> > +                       goto out;
> > +
> > +               /*
> > +                * Build an index table for each evsel to the val array.
> > +                * It cannot use evsel->core.idx because removed events might
> > +                * create a hole so the index is not consecutive anymore.
> > +                */
> > +               idx = 0;
> > +               for_each_group_member(member, evsel)
> > +                       idx_table[idx++] = 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 hist_entry *he,
> >                         if (!total)
> >                                 continue;
> >
> > -                       evsel = hists_to_evsel(pair->hists);
> > -                       idx = evsel__group_idx(evsel);
> > +                       member = hists_to_evsel(pair->hists);
> > +                       for (idx = 0; idx < nr_members; idx++) {
> > +                               if (idx_table[idx] == member)
> > +                                       break;
> > +                       }
> > +
> > +                       /* this should not happen */
> > +                       if (idx == nr_members)
> > +                               continue;
> >
> >                         if (fmt_percent)
> >                                 val[idx].percent = 100.0 * period / total;
> > @@ -75,8 +97,7 @@ static int __hpp__fmt(struct perf_hpp *hpp, struct hist_entry *he,
> >                                 val[idx].period = period;
> >                 }
> >
> > -               /* idx starts from 1 to skip the leader event */
> > -               for (idx = 1; idx < nr_members; idx++) {
> > +               for (idx = 0; idx < nr_members; idx++) {
> >                         if (fmt_percent) {
> >                                 ret += hpp__call_print_fn(hpp, print_fn,
> >                                                           fmt, len, val[idx].percent);
> > @@ -89,6 +110,7 @@ static int __hpp__fmt(struct perf_hpp *hpp, struct hist_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
> >
  

On Wed, Feb 14, 2024 at 9:26 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Wed, Feb 14, 2024 at 4:08 PM Ian Rogers <irogers@google.com> wrote:
> >
> > On Mon, Feb 12, 2024 at 11:52 PM 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?
>
> The evsel__group_idx() returns evsel->idx - leader->idx.
> If it has a group event {A, B, C} then the index would be 0, 1, 2.
> If it removes B, the group would be {A, C} with index 0 and 2.
> The nr_members is 2 now so it cannot use index 2 for C.
>
> Note that we cannot change the index of C because some information
> like annotation histogram relies on the index.

Ugh, the whole index thing is just messy - perhaps these days we could
have a hashmap with the evsel as a key instead. I remember that I also
forced the idx here:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/parse-events.c?h=perf-tools-next#n2049
If it were invariant that the idx were always the position of an event
in the evlist then I think life would be easier, but that won't help
for the arrays of counters that need the index to be constant. I guess
this is why the previous work to do this skipped evsels rather than
removed them.

Thanks,
Ian


> >
> > > 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 hist_entry *he,
> > >         if (evsel__is_group_event(evsel)) {
> > >                 int idx;
> > >                 struct hist_entry *pair;
> > > -               int nr_members = evsel->core.nr_members;
> > > +               int nr_members = evsel->core.nr_members - 1;
> >
> > A comment on the -1 would be useful.
>
> The 'nr_members' includes the leader which is already printed.
> In this code, we want to print member events only, hence -1.
> I'll add it to the comment.
>
> Thanks,
> Namhyung
>
> >
> > Thanks,
> > Ian
> >
> >
> > >                 union {
> > >                         u64 period;
> > >                         double percent;
> > >                 } *val;
> > > +               struct evsel *member;
> > > +               struct evsel **idx_table;
> > >
> > >                 val = calloc(nr_members, sizeof(*val));
> > >                 if (val == NULL)
> > > -                       return 0;
> > > +                       goto out;
> > > +
> > > +               idx_table = calloc(nr_members, sizeof(*idx_table));
> > > +               if (idx_table == NULL)
> > > +                       goto out;
> > > +
> > > +               /*
> > > +                * Build an index table for each evsel to the val array.
> > > +                * It cannot use evsel->core.idx because removed events might
> > > +                * create a hole so the index is not consecutive anymore.
> > > +                */
> > > +               idx = 0;
> > > +               for_each_group_member(member, evsel)
> > > +                       idx_table[idx++] = 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 hist_entry *he,
> > >                         if (!total)
> > >                                 continue;
> > >
> > > -                       evsel = hists_to_evsel(pair->hists);
> > > -                       idx = evsel__group_idx(evsel);
> > > +                       member = hists_to_evsel(pair->hists);
> > > +                       for (idx = 0; idx < nr_members; idx++) {
> > > +                               if (idx_table[idx] == member)
> > > +                                       break;
> > > +                       }
> > > +
> > > +                       /* this should not happen */
> > > +                       if (idx == nr_members)
> > > +                               continue;
> > >
> > >                         if (fmt_percent)
> > >                                 val[idx].percent = 100.0 * period / total;
> > > @@ -75,8 +97,7 @@ static int __hpp__fmt(struct perf_hpp *hpp, struct hist_entry *he,
> > >                                 val[idx].period = period;
> > >                 }
> > >
> > > -               /* idx starts from 1 to skip the leader event */
> > > -               for (idx = 1; idx < nr_members; idx++) {
> > > +               for (idx = 0; idx < nr_members; idx++) {
> > >                         if (fmt_percent) {
> > >                                 ret += hpp__call_print_fn(hpp, print_fn,
> > >                                                           fmt, len, val[idx].percent);
> > > @@ -89,6 +110,7 @@ static int __hpp__fmt(struct perf_hpp *hpp, struct hist_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
> > >
  

On Wed, Feb 14, 2024 at 11:54 PM Ian Rogers <irogers@google.com> wrote:
>
> On Wed, Feb 14, 2024 at 9:26 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > On Wed, Feb 14, 2024 at 4:08 PM Ian Rogers <irogers@google.com> wrote:
> > >
> > > On Mon, Feb 12, 2024 at 11:52 PM 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?
> >
> > The evsel__group_idx() returns evsel->idx - leader->idx.
> > If it has a group event {A, B, C} then the index would be 0, 1, 2.
> > If it removes B, the group would be {A, C} with index 0 and 2.
> > The nr_members is 2 now so it cannot use index 2 for C.
> >
> > Note that we cannot change the index of C because some information
> > like annotation histogram relies on the index.
>
> Ugh, the whole index thing is just messy - perhaps these days we could
> have a hashmap with the evsel as a key instead. I remember that I also
> forced the idx here:
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/parse-events.c?h=perf-tools-next#n2049
> If it were invariant that the idx were always the position of an event
> in the evlist then I think life would be easier, but that won't help
> for the arrays of counters that need the index to be constant. I guess
> this is why the previous work to do this skipped evsels rather than
> removed them.

Actually I have a patch series to convert the annotation histogram
to a hash map.  It'd reduce memory usage as well.  Will post.

I think removing evsel is not a common operation and should be
done with care.  In this patchset, it removed (dummy) events after
processing all samples.  I can make the code to skip those event
when printing the result but it'd be much easier if it can remove the
unnecessary events.

Thanks,
Namhyung