linux-kernel - Re: [PATCH RFC 4/9] perf jevents: Add sys_events_find_events

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAP-5=fX+rz928LtFs2MWYUH=6Mcvz0XQcLRkO-n9BnVnX4RYWw@mail.gmail.com>
Date:   Wed, 12 Jul 2023 10:52:31 -0700
From:   Ian Rogers <irogers@...gle.com>
To:     John Garry <john.g.garry@...cle.com>
Cc:     acme@...nel.org, namhyung@...nel.org, jolsa@...nel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
        renyu.zj@...ux.alibaba.com, shangxiaojing@...wei.com,
        kjain@...ux.ibm.com, kan.liang@...ux.intel.com
Subject: Re: [PATCH RFC 4/9] perf jevents: Add sys_events_find_events_table()

On Wed, Jul 12, 2023 at 3:55 AM John Garry <john.g.garry@...cle.com> wrote:
>
>
> >>>> MetricExpr: "cpu_core@...tructions@ / cpu_core@...les@",
> >> I did not know that it was possible to state that an event is for a
> >> specific PMU type in this fashion - is this feature new? Does it work
> >> only for known terms, like cycles and instructions?
> > It has been in metrics a long time (I didn't choose that @ was the /
> > replacement 😄 ). It should work for all events.
> >
>
> Good to know.
>
> >>>> The @ is used to avoid parsing confusion with / meaning divide. The
> >>>> PMUs for the events are explicitly listed here. We could say the PMU
> >>>> is implied but then it gets complex for uncore events, for metrics
> >>>> that mix core and uncore events.
> >> So this works ok for IPC and CPU PMUs as we want the same event for many
> >> PMU types and naturally it would have the same name.
> >>
> >> I am still not sure that sys event metrics need to specify a PMU.
> > There was a similar thought for hybrid metrics. The PMU could be
> > implied from the PMU of the metric. I think there can be confusion
> > from an implied PMU, for example the cycles event without a PMU will
> > open two events on a hybrid CPU. If we imply the PMU then it can mean
> > just 1 PMU, but if the PMU doesn't have the event presumably it means
> > the multiple PMU behavior.
> >
> > In parse-events there is existing logic to wildcard events but to
> > ignore those that don't match a given PMU. This is used to support the
> > --cputype option in builtin-stat.c, there is a similar option for
> > builtin-list.c. We can use this so that events in a metric only match
> > the PMU of the metric. Currently there are core metrics but whose
> > events are all uncore like:
> > https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/pmu-events/arch/x86/alderlake/adl-metrics.json?h=perf-tools-next*n1802__;Iw!!ACWV5N9M2RV99hQ!MjhanGd4AVsAl6d8weFktNHgeOptrgeBDyooXlpeW-J1TQ0e2BwzvqO4BTFEjs_gRzuWTPfnhW_jLx1pIJc$
> >
> > So we'd need to move these metrics to be on the appropriate uncore
> > PMU. Supporting >1 PMU
>
> To be crystal clear, when you say ">1 PMU", do you mean ">1 PMU instance
> of the same type" or ">1 PMU type"?

So I'm meaning that if you have a MetricExpr where the events are from
>1 PMU, for example memory bandwidth coming from uncore PMUs and then
instructions from a core PMU, and you do something like a ratio
between these two.

>   in a metric wouldn't work though as it would
> > appear the event was missing. Having the metric specify the PMU avoids
> > these problems, but is verbose.
>
> The message I'm getting - correct me if I am wrong - is that you would
> still prefer the PMU specified per event in metric expr, right? We don't
> do that exactly for sys PMU metrics today - we specify "Unit" instead, like:
>
> MetricExpr: "sys_pmu_bar_event_foo1 + sys_pmu_bar_event_foo2"
> Compat: "baz"
> Unit:"sys_pmu_bar"
>
> And so you prefer something like the following, right?
> MetricExpr: "sys_pmu_foo@...1@ + sys_pmu_foo@...2@"
>
> If so, I think that is ok - I just want to get rid of Unit and Compat.

I think we're agreeing :-) I think Unit may be useful, say on Intel
hybrid I want the tma_fround_bound metric on just cpu_atom. Currently
the use of Unit is messy for metrics, ie uncore metrics are associated
with core PMUs, and what to do with a MetricExpr with >1 PMU. I think
we're learning from trying. I'm just hoping the migration to a sysfs
style layout will still be possible, as I can see lots of upside in
terms of testing, 1 approach, etc.

Thanks,
Ian

> Thanks,
> John