Re: [PATCH v2 0/2] Fix topdown event weak grouping

[Ian Rogers <irogers@google.com>]

On Fri, May 13, 2022 at 7:25 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
>
> On 5/12/2022 2:13 AM, Ian Rogers wrote:
> > Keep topdown events within a group when a weak group is broken. This
> > is a requirement as topdown events must form a group.
> >
> > Add perf stat testing including for required topdown event group
> > behaviors.
> >
> > Note: as with existing topdown evsel/evlist code topdown events are
> > assumed to be on the PMU "cpu". On Alderlake the PMU "cpu_core" should
> > also be tested. Future changes can fix Alderlake.
>
> I will send a follow-up patch to fix the weak grouping for the hybrid
> platform shortly.
>
> For the non-hybrid platform, the patch set looks good to me.

Thanks. I was looking into things like perf_pmu__for_each_hybrid_pmu:
https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/pmu-hybrid.h?h=perf/core#n13

The issue is that we only really care about "cpu_core" and "cpu" for
topdown events, so using a loop isn't clearly beneficial as it'll
include "cpu_atom". Also, the hybrid parsing code only works for
Intel:
https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/pmu-hybrid.c?h=perf/core#n28
so I have a hard time seeing this kind of code as making things more generic.

What we really want in this case is some way of iterating over PMUs
for CPU like things (note the property is CPU like, not hybrid). There
is an unfortunate naming for ARM where this property got called core:
https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/pmu.c?h=perf/core#n679

The PMUs iterated are architecture specific and should probably be set
up via a function on a weak symbol - the default to just use "cpu".
Unfortunately Intel uses a cpus file in sysfs to indicate hybrid and
that is set up differently on ARM.

So with all that said my hybrid support code is to change:

if (pmu_have_event("cpu", "slots"))

to:

if (pmu_have_event("cpu", "slots")) || pmu_have_event("cpu_core", "slots"))

Longer term I think we need to rethink hybrid. I'm not sure hybrid is
the correct term, we want all hard coded "cpu" to be a set of things
like "cpu", "cpu_core", "cpu_atom" or the appropriate PMU on ARM. As I
said in:
https://lore.kernel.org/lkml/CAP-5=fWRRZsyJZ-gky-FOFz79zW_3r78d_0APpj5sf66HqTpLw@mail.gmail.com/
I think the sane way to do this is to turn all "event sources" into a
sysfs like file system and to have clear discovery rules. The problem
right now is to clean up and transition to that, while maintaining
existing code working. Let me know if there's a better way to have
this conversation. My own work pushing in that direction is currently
gated by these changes landing:
https://lore.kernel.org/lkml/20220511211526.1021908-1-irogers@google.com/

Thanks,
Ian



> Reviewed-by: Kan Liang <kan.liang@linux.intel.com>
>
> Thanks,
> Kan
>
> >
> > v2. Correct behavior wrt pmu prefixed events and avoid the test using
> >      deprecated events: Suggested-by: Liang, Kan <kan.liang@linux.intel.com>
> >
> > Ian Rogers (2):
> >    perf evlist: Keep topdown counters in weak group
> >    perf test: Add basic stat and topdown group test
> >
> >   tools/perf/arch/x86/util/evsel.c | 12 ++++++
> >   tools/perf/tests/shell/stat.sh   | 67 ++++++++++++++++++++++++++++++++
> >   tools/perf/util/evlist.c         | 16 +++++++-
> >   tools/perf/util/evsel.c          | 10 +++++
> >   tools/perf/util/evsel.h          |  3 ++
> >   5 files changed, 106 insertions(+), 2 deletions(-)
> >   create mode 100755 tools/perf/tests/shell/stat.sh
> >