[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAP-5=fXM41WooXuzhToLQhfYN2OSmoLEHRZqP2v2yowQWbySig@mail.gmail.com>
Date: Tue, 17 May 2022 17:08:56 -0700
From: Ian Rogers <irogers@...gle.com>
To: Namhyung Kim <namhyung@...nel.org>
Cc: Xing Zhengjun <zhengjun.xing@...ux.intel.com>,
"Liang, Kan" <kan.liang@...ux.intel.com>,
Arnaldo Carvalho de Melo <acme@...nel.org>,
Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...hat.com>,
Mark Rutland <mark.rutland@....com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Jiri Olsa <jolsa@...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>,
Adrian Hunter <adrian.hunter@...el.com>,
James Clark <james.clark@....com>,
linux-perf-users <linux-perf-users@...r.kernel.org>,
linux-kernel <linux-kernel@...r.kernel.org>,
Stephane Eranian <eranian@...gle.com>
Subject: Re: [PATCH 1/5] Revert "perf stat: Support metrics with hybrid events"
On Tue, May 17, 2022 at 3:58 PM Namhyung Kim <namhyung@...nel.org> wrote:
>
> Hello,
>
> On Tue, May 10, 2022 at 2:31 AM Xing Zhengjun
> <zhengjun.xing@...ux.intel.com> wrote:
> >
> >
> >
> > On 5/10/2022 5:55 AM, Liang, Kan wrote:
> > >
> > >
> > > On 5/9/2022 9:12 AM, Arnaldo Carvalho de Melo wrote:
> > >> Em Fri, May 06, 2022 at 10:34:06PM -0700, Ian Rogers escreveu:
> > >>> This reverts commit 60344f1a9a597f2e0efcd57df5dad0b42da15e21.
> > >>
> > >> I picked this from the cover letter and added to this revert, to justify
> > >> it:
> > >>
> > >> "Hybrid metrics place a PMU at the end of the parse string. This is
> > >> also where tool events are placed. The behavior of the parse string
> > >> isn't clear and so revert the change for now."
> > >>
> > >
> > > I think the original patch used a "#" to indicate the PMU name, which
> > > can be used to distinguish between the tool events and the PMU name.
> > > To be honest, I'm not sure what's unclear here. Could you please clarify?
> > >
> > > With this revert, the issue mentioned in the original patch must be
> > > broken on ADL. I don't see a replacement fix in this patch series.
> > > Could you please propose a solution for the issue to replace the #PMU
> > > name solution?
> > >
> > > Thanks,
> > > Kan
> >
> > I am surprised the origin patch is reverted during discussion and though
> > the discussion still has no conclusion.
> > Let me introduce the purpose of the origin patch.
> > For a hybrid system such as ADL, if both the metrics and the formula are
> > different for the different PMUs, without this patch, the metric and
> > event parser should work ok, nothing should be special for the hybrid.
> > In fact, both "cpu_core" and "cpu_atom" may have the same metrics--the
> > same metric name, even the same formula for the metrics. For example,
> > both "cpu_core" and "cpu_atom" have metrics "IpBranch" and "IPC", For
> > "IpBranch", both "cpu_core" and "cpu_atom" has the same metric name and
> > their formula also is the same, the event name is the same though they
> > belong to different PMUs. The old metric and event parser can not handle
> > this kind of metric, that's why we need this patch.
> >
> > "MetricExpr": "INST_RETIRED.ANY / BR_INST_RETIRED.ALL_BRANCHES",
> > "MetricName": "IpBranch",
> > "Unit": "cpu_core"
> >
> > "MetricExpr": "INST_RETIRED.ANY / BR_INST_RETIRED.ALL_BRANCHES",
> > "MetricName": "IpBranch",
> > "Unit": "cpu_atom"
> >
> >
> > "MetricExpr": "INST_RETIRED.ANY / CPU_CLK_UNHALTED.THREAD",
> > "MetricName": "IPC",
> > "Unit": "cpu_core"
> >
> > "MetricExpr": "INST_RETIRED.ANY / CPU_CLK_UNHALTED.CORE",
> > "MetricName": "IPC",
> > "Unit": "cpu_atom"
> >
> > Except for the above two metrics, there are still a lot of similar
> > metrics, "CPU_Utilization"...
> >
> > The original patch expanded the metric group string by adding
> > "#PMU_name" to indicate the PMU name, which can be used to distinguish
> > between the tool events and the PMU name, then the metric and event
> > parser can parse the right PMU for the events.
> >
> > With the patch all the ADL metrics can pass, without the patch, a lot of
> > metrics will fail. I don't think it's a good idea to revert it before
> > the new solution is proposed.
>
> Just an idea. Can we add a pmu prefix when it resolves the event
> for a metric if it has the "Unit"? It seems we can support something
> like "cpu_core@...T_RETIRED.ANY@" already..
>
> Or could it be done when creating JSON files?
Yep. The format for the events in the metric is the same as for
parse-events, we copy+paste the strings from one to the other. The @
in the json is used in place of slash (/) as slash is used in metrics
for division.
Thanks,
Ian
> Thanks,
> Namhyung
Powered by blists - more mailing lists