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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAP-5=fXS2r245frPSdzjkQUPMQK_Yrm5+g_cfKQRFiAkoi7Ocg@mail.gmail.com>
Date:   Fri, 13 May 2022 08:18:59 -0700
From:   Ian Rogers <irogers@...gle.com>
To:     "Liang, Kan" <kan.liang@...ux.intel.com>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>,
        Riccardo Mancini <rickyman7@...il.com>,
        Kim Phillips <kim.phillips@....com>,
        Madhavan Srinivasan <maddy@...ux.ibm.com>,
        Shunsuke Nakamura <nakamura.shun@...itsu.com>,
        Florian Fischer <florian.fischer@...q.space>,
        Andi Kleen <ak@...ux.intel.com>,
        John Garry <john.garry@...wei.com>,
        Zhengjun Xing <zhengjun.xing@...ux.intel.com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        James Clark <james.clark@....com>,
        linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
        Stephane Eranian <eranian@...gle.com>
Subject: Re: [PATCH v2 0/2] Fix topdown event weak grouping

On Fri, May 13, 2022 at 7:25 AM Liang, Kan <kan.liang@...ux.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@...ux.intel.com>
>
> Thanks,
> Kan
>
> >
> > v2. Correct behavior wrt pmu prefixed events and avoid the test using
> >      deprecated events: Suggested-by: Liang, Kan <kan.liang@...ux.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
> >