MIME-Version: 1.0
References: <20230302212531.1043318-1-irogers@google.com> <20230302212531.1043318-3-irogers@google.com>
 <962de75d-8e27-9b47-662e-e324b3ba5812@linux.intel.com>
In-Reply-To: <962de75d-8e27-9b47-662e-e324b3ba5812@linux.intel.com>
From:   Ian Rogers <irogers@google.com>
Date:   Fri, 3 Mar 2023 08:44:15 -0800
Message-ID: <CAP-5=fXiRtKF661e=-4dX30ooV7uKQbidjaaDhjckVRdjw7CzA@mail.gmail.com>
Subject: Re: [PATCH v2 02/10] perf stat: Don't remove all grouped events when
 CPU maps disagree
To:     "Liang, Kan" <kan.liang@linux.intel.com>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@kernel.org>,
        Namhyung Kim <namhyung@kernel.org>,
        Zhengjun Xing <zhengjun.xing@linux.intel.com>,
        Ravi Bangoria <ravi.bangoria@amd.com>,
        Adrian Hunter <adrian.hunter@intel.com>,
        "Steinar H. Gunderson" <sesse@google.com>,
        Kim Phillips <kim.phillips@amd.com>,
        Florian Fischer <florian.fischer@muhq.space>,
        James Clark <james.clark@arm.com>,
        Suzuki Poulouse <suzuki.poulose@arm.com>,
        Sean Christopherson <seanjc@google.com>,
        Leo Yan <leo.yan@linaro.org>,
        John Garry <john.g.garry@oracle.com>,
        Kajol Jain <kjain@linux.ibm.com>,
        linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org,
        Stephane Eranian <eranian@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Mar 3, 2023 at 7:50=E2=80=AFAM Liang, Kan <kan.liang@linux.intel.co=
m> 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_writ=
e/,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=3D0x1/}, 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@google.com>
> > ---
> >  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, str=
uct evsel *b)
> >
> >  static void evlist__check_cpu_maps(struct evlist *evlist)
> >  {
> > -     struct evsel *evsel, *pos, *leader;
> > -     char buf[1024];
> > +     struct evsel *evsel, *warned_leader =3D NULL;
> >
> >       if (evlist__has_hybrid(evlist))
> >               evlist__warn_hybrid_group(evlist);
> >
> >       evlist__for_each_entry(evlist, evsel) {
> > -             leader =3D evsel__leader(evsel);
> > +             struct evsel *leader =3D evsel__leader(evsel);
> >
> >               /* Check that leader matches cpus with each member. */
> >               if (leader =3D=3D 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 !=3D leader) {
> > +                     char buf[200];
> > +
> > +                     pr_warning("WARNING: grouped events cpus do not m=
atch.\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 =3D leader;
> > +             }
> >               if (verbose > 0) {
> > +                     char buf[200];
> > +
> >                       cpu_map__snprint(leader->core.cpus, buf, sizeof(b=
uf));
> >                       pr_warning("     %s: %s\n", leader->name, buf);
> >                       cpu_map__snprint(evsel->core.cpus, buf, sizeof(bu=
f));
> >                       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);
> >       }
> >  }
> >