Re: [PATCH v2 02/10] perf stat: Don't remove all grouped events when CPU maps disagree

[Ian Rogers]

On Fri, Mar 3, 2023 at 7:50 AM Liang, Kan <kan.liang@xxxxxxxxxxxxxxx> wrote:
>
>
>
> On 2023-03-02 4:25 p.m., Ian Rogers wrote:
> > If the events in an evlist's CPU map differ then the entire group is
> > removed. For example:
> >
> > ```
> > $ perf stat -e '{imc_free_running/data_read/,imc_free_running/data_write/,cs}' -a sleep 1
> > WARNING: grouped events cpus do not match, disabling group:
> >   anon group { imc_free_running/data_read/, imc_free_running/data_write/, cs }
> > ```
> >
> > Change the behavior so that just the events not matching the leader
> > are removed. So in the example above, just 'cs' will be removed.
> >
> > Modify the warning so that it is produced once for each group, rather
> > than once for the entire evlist. Shrink the scope and size of the
> > warning text buffer.
>
> For the uncore, we usually have to create a group for each uncore PMU.
> The number of groups may be big. For example, on ICX, we have 40 CHA
> PMUs. For SPR, there should be more CHAs. If we have something like
> {cycles,uncore_cha/event=0x1/}, is the warning shown 40 times on ICX?
> If so, it should be very annoying.
>
> Maybe it's better to keep the current behavior which only print a
> warning once and notify the users that perf will re-group the events.
> For the details, they can get it from the -v option.

Thanks Kan, I could imagine that but I was also worried about cases
where there are multiple groups like:

```
$ perf stat -e '{imc_free_running/data_read/,cs},{uncore_clock/clockticks/,cs}'
-a sleep 1
WARNING: grouped events cpus do not match.
Events with CPUs not matching the leader will be removed from the group.
 anon group { imc_free_running/data_read/, cs }
WARNING: grouped events cpus do not match.
Events with CPUs not matching the leader will be removed from the group.
 anon group { uncore_clock/clockticks/, cs }

Performance counter stats for 'system wide':

         1,255.75 MiB  imc_free_running/data_read/
            7,571      cs
    1,327,285,527      uncore_clock/clockticks/
            7,571      cs

      1.002772882 seconds time elapsed
```

Knowing that both groups were broken there feels like a value add.
Given that this is a warning, and it can be fixed by moving the event
out of the group or forcing the CPUs, I lean toward being
informative/spammy as the spam is somewhat straightforwardly fixed on
the command line.

Thanks,
Ian

> Thanks,
> Kan
> >
> > Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>
> > ---
> >  tools/perf/builtin-stat.c | 24 +++++++++++++++---------
> >  1 file changed, 15 insertions(+), 9 deletions(-)
> >
> > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> > index d70b1ec88594..5c12ae5efce5 100644
> > --- a/tools/perf/builtin-stat.c
> > +++ b/tools/perf/builtin-stat.c
> > @@ -181,14 +181,13 @@ static bool cpus_map_matched(struct evsel *a, struct evsel *b)
> >
> >  static void evlist__check_cpu_maps(struct evlist *evlist)
> >  {
> > -     struct evsel *evsel, *pos, *leader;
> > -     char buf[1024];
> > +     struct evsel *evsel, *warned_leader = NULL;
> >
> >       if (evlist__has_hybrid(evlist))
> >               evlist__warn_hybrid_group(evlist);
> >
> >       evlist__for_each_entry(evlist, evsel) {
> > -             leader = evsel__leader(evsel);
> > +             struct evsel *leader = evsel__leader(evsel);
> >
> >               /* Check that leader matches cpus with each member. */
> >               if (leader == evsel)
> > @@ -197,19 +196,26 @@ static void evlist__check_cpu_maps(struct evlist *evlist)
> >                       continue;
> >
> >               /* If there's mismatch disable the group and warn user. */
> > -             WARN_ONCE(1, "WARNING: grouped events cpus do not match, disabling group:\n");
> > -             evsel__group_desc(leader, buf, sizeof(buf));
> > -             pr_warning("  %s\n", buf);
> > -
> > +             if (warned_leader != leader) {
> > +                     char buf[200];
> > +
> > +                     pr_warning("WARNING: grouped events cpus do not match.\n"
> > +                             "Events with CPUs not matching the leader will "
> > +                             "be removed from the group.\n");
> > +                     evsel__group_desc(leader, buf, sizeof(buf));
> > +                     pr_warning("  %s\n", buf);
> > +                     warned_leader = leader;
> > +             }
> >               if (verbose > 0) {
> > +                     char buf[200];
> > +
> >                       cpu_map__snprint(leader->core.cpus, buf, sizeof(buf));
> >                       pr_warning("     %s: %s\n", leader->name, buf);
> >                       cpu_map__snprint(evsel->core.cpus, buf, sizeof(buf));
> >                       pr_warning("     %s: %s\n", evsel->name, buf);
> >               }
> >
> > -             for_each_group_evsel(pos, leader)
> > -                     evsel__remove_from_group(pos, leader);
> > +             evsel__remove_from_group(evsel, leader);
> >       }
> >  }
> >