Re: [PATCH 2/4] perf stat: Always keep perf metrics topdown events in a group

[Liang, Kan]

On 5/13/2022 12:32 PM, Ian Rogers wrote:
> On Fri, May 13, 2022 at 8:16 AM <kan.liang@xxxxxxxxxxxxxxx> wrote:
> > From: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
> >
> > If any member in a group has a different cpu mask than the other
> > members, the current perf stat disables group. when the perf metrics
> > topdown events are part of the group, the below <not supported> error
> > will be triggered.
> >
> > $ perf stat -e "{slots,topdown-retiring,uncore_imc_free_running_0/dclk/}" -a sleep 1
> > WARNING: grouped events cpus do not match, disabling group:
> >    anon group { slots, topdown-retiring, uncore_imc_free_running_0/dclk/ }
> >
> >   Performance counter stats for 'system wide':
> >
> >         141,465,174      slots
> >     <not supported>      topdown-retiring
> >       1,605,330,334      uncore_imc_free_running_0/dclk/
> >
> > The perf metrics topdown events must always be grouped with a slots
> > event as leader.
> >
> > With the patch, the topdown events aren't broken from the group for the
> > splitting.
> >
> > $ perf stat -e "{slots,topdown-retiring,uncore_imc_free_running_0/dclk/}" -a sleep 1
> > WARNING: grouped events cpus do not match, disabling group:
> >    anon group { slots, topdown-retiring, uncore_imc_free_running_0/dclk/ }
> >
> >   Performance counter stats for 'system wide':
> >
> >         346,110,588      slots
> >         124,608,256      topdown-retiring
> >       1,606,869,976      uncore_imc_free_running_0/dclk/
> >
> >         1.003877592 seconds time elapsed
>
> Nice! This is based on:
> https://lore.kernel.org/lkml/20220512061308.1152233-2-irogers@xxxxxxxxxx/
> You may end up with a group with the leader having a group count of 1
> (itself). I explicitly zeroed that in the change above, but this may
> be unnecessary. Maybe we should move this code to helper functions for
> sharing and consistency on what the leader count should be.

I think the current code has already did evsel->core.leader->nr_members 
= 0; for the leader. So I don't change it.

Yes, a helper function seems reasonable. I will add it in V2.

Thanks,
Kan
> Thanks,
> Ian
>
> > Fixes: a9a1790247bd ("perf stat: Ensure group is defined on top of the same cpu mask")
> > Signed-off-by: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
> > ---
> >   tools/perf/builtin-stat.c | 7 +++++--
> >   1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> > index a96f106dc93a..af2248868a4f 100644
> > --- a/tools/perf/builtin-stat.c
> > +++ b/tools/perf/builtin-stat.c
> > @@ -272,8 +272,11 @@ static void evlist__check_cpu_maps(struct evlist *evlist)
> >                  }
> >
> >                  for_each_group_evsel(pos, leader) {
> > -                       evsel__set_leader(pos, pos);
> > -                       pos->core.nr_members = 0;
> > +                       if (!evsel__must_be_in_group(pos) && pos != leader) {
> > +                               evsel__set_leader(pos, pos);
> > +                               pos->core.nr_members = 0;
> > +                               leader->core.nr_members--;
> > +                       }
> >                  }
> >                  evsel->core.leader->nr_members = 0;
> >          }
> > --
> > 2.35.1