Message-ID: <CAP-5=fWvdy+_H4hv28j+N1-swfGA5Q0yHDvewvsyh3t7dQb0JA@mail.gmail.com>
Date: Thu, 6 Mar 2025 09:25:31 -0800
From: Ian Rogers <irogers@...gle.com>
To: "Mi, Dapeng" <dapeng1.mi@...ux.intel.com>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>, 
	Arnaldo Carvalho de Melo <acme@...nel.org>, Namhyung Kim <namhyung@...nel.org>, 
	Mark Rutland <mark.rutland@....com>, 
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>, 
	Adrian Hunter <adrian.hunter@...el.com>, Kan Liang <kan.liang@...ux.intel.com>, 
	James Clark <james.clark@...aro.org>, Dominique Martinet <asmadeus@...ewreck.org>, 
	Andi Kleen <ak@...ux.intel.com>, linux-perf-users@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Thomas Falcon <thomas.falcon@...el.com>
Subject: Re: [PATCH v1 2/2] perf parse-events: Corrections to topdown sorting

On Thu, Mar 6, 2025 at 1:17 AM Mi, Dapeng <dapeng1.mi@...ux.intel.com> wrote:
>
>
> On 3/5/2025 4:37 PM, Ian Rogers wrote:
> > In the case of '{instructions,slots},faults,topdown-retiring' the
> > first event that must be grouped, slots, is ignored causing the
> > topdown-retiring event not to be adjacent to the group it needs to be
> > inserted into. Don't ignore the group members when computing the
> > force_grouped_index.
> >
> > Make the force_grouped_index be for the leader of the group it is
> > within and always use it first rather than a group leader index so
> > that topdown events may be sorted from one group into another.
> >
> > Reported-by: Dapeng Mi <dapeng1.mi@...ux.intel.com>
> > Closes: https://lore.kernel.org/lkml/20250224083306.71813-2-dapeng1.mi@linux.intel.com/
> > Signed-off-by: Ian Rogers <irogers@...gle.com>
> > ---
> >  tools/perf/util/parse-events.c | 54 ++++++++++++++++++----------------
> >  1 file changed, 28 insertions(+), 26 deletions(-)
> >
> > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> > index 35e48fe56dfa..cf32abc496e9 100644
> > --- a/tools/perf/util/parse-events.c
> > +++ b/tools/perf/util/parse-events.c
> > @@ -1983,31 +1983,30 @@ static int evlist__cmp(void *_fg_idx, const struct list_head *l, const struct li
> >       bool lhs_has_group, rhs_has_group;
> >
> >       /*
> > -      * First sort by grouping/leader. Read the leader idx only if the evsel
> > -      * is part of a group, by default ungrouped events will be sorted
> > -      * relative to grouped events based on where the first ungrouped event
> > -      * occurs. If both events don't have a group we want to fall-through to
> > -      * the arch specific sorting, that can reorder and fix things like
> > -      * Intel's topdown events.
> > +      * Get the indexes of the 2 events to sort. If the events are
> > +      * in groups then the leader's index is used otherwise the
> > +      * event's index is used. Events in the same group will be
> > +      * sorted by PMU name. An index may be forced for events that
> > +      * must be in the same group, namely Intel topdown events.
> > +      * When everything is identical arch specific sorting is used,
> > +      * that can reorder and fix things like Intel's topdown
> > +      * events.
> >        */
> > -     if (lhs_core->leader != lhs_core || lhs_core->nr_members > 1) {
> > -             lhs_has_group = true;
> > +     lhs_has_group = lhs_core->leader != lhs_core || lhs_core->nr_members > 1;
> > +     if (*force_grouped_idx != -1 && arch_evsel__must_be_in_group(lhs))
> > +             lhs_sort_idx = *force_grouped_idx;
> > +     else if (lhs_has_group)
> >               lhs_sort_idx = lhs_core->leader->idx;
> > -     } else {
> > -             lhs_has_group = false;
> > -             lhs_sort_idx = *force_grouped_idx != -1 && arch_evsel__must_be_in_group(lhs)
> > -                     ? *force_grouped_idx
> > -                     : lhs_core->idx;
> > -     }
> > -     if (rhs_core->leader != rhs_core || rhs_core->nr_members > 1) {
> > -             rhs_has_group = true;
> > +     else
> > +             lhs_sort_idx = lhs_core->idx;
> > +     rhs_has_group = rhs_core->leader != rhs_core || rhs_core->nr_members > 1;
> > +
> > +     if (*force_grouped_idx != -1 && arch_evsel__must_be_in_group(rhs))
> > +             rhs_sort_idx = *force_grouped_idx;
> > +     else if (rhs_has_group)
> >               rhs_sort_idx = rhs_core->leader->idx;
> > -     } else {
> > -             rhs_has_group = false;
> > -             rhs_sort_idx = *force_grouped_idx != -1 && arch_evsel__must_be_in_group(rhs)
> > -                     ? *force_grouped_idx
> > -                     : rhs_core->idx;
> > -     }
> > +     else
> > +             rhs_sort_idx = rhs_core->idx;
> >
> >       if (lhs_sort_idx != rhs_sort_idx)
> >               return lhs_sort_idx - rhs_sort_idx;
> > @@ -2055,10 +2054,13 @@ static int parse_events__sort_events_and_fix_groups(struct list_head *list)
> >                */
> >               pos->core.idx = idx++;
> >
> > -             /* Remember an index to sort all forced grouped events together to. */
> > -             if (force_grouped_idx == -1 && pos == pos_leader && pos->core.nr_members < 2 &&
> > -                 arch_evsel__must_be_in_group(pos))
> > -                     force_grouped_idx = pos->core.idx;
> > +             /*
> > +              * Remember an index to sort all forced grouped events
> > +              * together to. Use the group leader as some events
> > +              * must appear first within the group.
> > +              */
> > +             if (force_grouped_idx == -1 && arch_evsel__must_be_in_group(pos))
> > +                     force_grouped_idx = pos_leader->core.idx;
> >       }
> >
> >       /* Sort events. */
>
> Hi Ian,
>
> With this fix,  this topdown metrics sequence
> "{instructions,slots},faults,topdown-retiring" indeed works on non-hybrid
> platform, like SPR, but it still fails on hybrid platform.
>
> Here is the result on Intel LNL platform.
>
> ./perf stat -e "{instructions,slots},faults,topdown-retiring" true
> WARNING: events were regrouped to match PMUs
>
>  Performance counter stats for 'true':
>
>    *<not supported> *     cpu_core/topdown-retiring/u
>            146,710      instructions:u
>      <not counted>
> cpu_core/slots/u                                                        (0.00%)
>      <not counted>
> instructions:u                                                          (0.00%)
>                 49      faults:u
>            195,855      cpu_atom/topdown-retiring/u
>
>        0.001367139 seconds time elapsed
>
>        0.001402000 seconds user
>        0.000000000 seconds sys
>
> the "cpu_core/topdown-retiring/" event is incorrectly moved to the head and
> becomes the group leader.
>
> To thoroughly fix this issue on hybrid platform, we need an extra below
> change.
>
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 91c2b2e2c6bd..1f7772d4db6e 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -2006,7 +2006,7 @@ static int evlist__cmp(void *_fg_idx, const struct
> list_head *l, const struct li
>                 return lhs_sort_idx - rhs_sort_idx;
>
>         /* Group by PMU if there is a group. Groups can't span PMUs. */
> -       if (lhs_has_group && rhs_has_group) {
> +       if (lhs_has_group || rhs_has_group) {
>                 lhs_pmu_name = lhs->group_pmu_name;
>                 rhs_pmu_name = rhs->group_pmu_name;
>                 ret = strcmp(lhs_pmu_name, rhs_pmu_name);
>
> Besides, since we support this new topdown events sequence regroup, the
> comments and tests are need to be updated accordingly.

Thanks, I was able to reproduce the problem and agree with extending
the tests. I think this needs more comments so I'll update and repost
the patch with those.

Thanks,
Ian

> diff --git a/tools/perf/arch/x86/util/evlist.c
> b/tools/perf/arch/x86/util/evlist.c
> index 447a734e591c..8d7a7c4acd4b 100644
> --- a/tools/perf/arch/x86/util/evlist.c
> +++ b/tools/perf/arch/x86/util/evlist.c
> @@ -39,28 +39,21 @@ int arch_evlist__cmp(const struct evsel *lhs, const
> struct evsel *rhs)
>          *         26,319,024      slots
>          *          2,427,791      instructions
>          *          2,683,508      topdown-retiring
> -        *
> -        * If slots event and topdown metrics events are not in same group, the
> -        * topdown metrics events must be first event after the slots event
> group,
> -        * otherwise topdown metrics events can't be regrouped correctly, e.g.
> -        *
> -        * a. perf stat -e "{instructions,slots},cycles,topdown-retiring"
> -C0 sleep 1
> +        * e. slots event and metrics event are in a group and not adjacent
> +        *    perf stat -e "{instructions,slots},cycles,topdown-retiring"
> -C0 sleep 1
>          *    WARNING: events were regrouped to match PMUs
> -        *     Performance counter stats for 'CPU(s) 0':
> -        *         17,923,134      slots
> -        *          2,154,855      instructions
> -        *          3,015,058      cycles
> -        *    <not supported>      topdown-retiring
> -        *
> -        * If slots event and topdown metrics events are in two groups, the
> group which
> -        * has topdown metrics events must contain only the topdown metrics
> event,
> -        * otherwise topdown metrics event can't be regrouped correctly as
> well, e.g.
> -        *
> -        * a. perf stat -e "{instructions,slots},{topdown-retiring,cycles}"
> -C0 sleep 1
> +        *    Performance counter stats for 'true':
> +        *         78,452,058      slots
> +        *         10,767,929      topdown-retiring
> +        *          9,438,226      instructions
> +        *         13,080,988      cycles
> +        * f. slots event and metrics event are in two groups and not adjacent
> +        *    perf stat -e "{instructions,slots},{cycles,topdown-retiring}"
> -C0 sleep 1
>          *    WARNING: events were regrouped to match PMUs
> -        *    Error:
> -        *    The sys_perf_event_open() syscall returned with 22 (Invalid
> argument) for
> -        *    event (topdown-retiring)
> +        *         68,433,522      slots
> +        *          8,856,102      topdown-retiring
> +        *          7,791,494      instructions
> +        *         11,469,513      cycles
>          */
>         if (topdown_sys_has_perf_metrics() &&
>             (arch_evsel__must_be_in_group(lhs) ||
> arch_evsel__must_be_in_group(rhs))) {
> diff --git a/tools/perf/tests/shell/stat.sh b/tools/perf/tests/shell/stat.sh
> index 68323d636fb7..a1b847c16f07 100755
> --- a/tools/perf/tests/shell/stat.sh
> +++ b/tools/perf/tests/shell/stat.sh
> @@ -97,6 +97,18 @@ test_topdown_groups() {
>      err=1
>      return
>    fi
> +  if perf stat -e '{instructions,slots},cycles,topdown-retiring' true 2>&1
> | grep -E -q "<not supported>"
> +  then
> +    echo "Topdown event group test [Failed non-adjacent topdown metrics
> group not move into slots group]"
> +    err=1
> +    return
> +  fi
> +  if perf stat -e '{instructions,slots},{cycles,topdown-retiring}' true
> 2>&1 | grep -E -q "<not supported>"
> +  then
> +    echo "Topdown event group test [Failed non-adjacent topdown metrics
> group not merge into slots group]"
> +    err=1
> +    return
> +  fi
>    if perf stat -e '{instructions,r400,r8000}' true 2>&1 | grep -E -q "<not
> supported>"
>    then
>      echo "Topdown event group test [Failed raw format slots not reordered
> first]"
>
> Thanks,
>
> Dapeng Mi
>
>

Powered by blists - more mailing lists