Re: [PATCH 1/5] Revert "perf stat: Support metrics with hybrid events"

From: Namhyung Kim <namhyung@kernel.org>
To: Xing Zhengjun <zhengjun.xing@linux.intel.com>
Cc: "Liang, Kan" <kan.liang@linux.intel.com>,
	Arnaldo Carvalho de Melo <acme@kernel.org>,
	Ian Rogers <irogers@google.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@redhat.com>,
	Mark Rutland <mark.rutland@arm.com>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Jiri Olsa <jolsa@kernel.org>,
	Riccardo Mancini <rickyman7@gmail.com>,
	Kim Phillips <kim.phillips@amd.com>,
	Madhavan Srinivasan <maddy@linux.ibm.com>,
	Shunsuke Nakamura <nakamura.shun@fujitsu.com>,
	Florian Fischer <florian.fischer@muhq.space>,
	Andi Kleen <ak@linux.intel.com>,
	John Garry <john.garry@huawei.com>,
	Adrian Hunter <adrian.hunter@intel.com>,
	James Clark <james.clark@arm.com>,
	linux-perf-users <linux-perf-users@vger.kernel.org>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	Stephane Eranian <eranian@google.com>
Subject: Re: [PATCH 1/5] Revert "perf stat: Support metrics with hybrid events"
Date: Tue, 17 May 2022 15:58:09 -0700	[thread overview]
Message-ID: <CAM9d7cgEc9o9y-fH==avzucigmMVw0AwooGw0eufb2+D568-_g@mail.gmail.com> (raw)
In-Reply-To: <708fab8f-b5c5-8f34-4322-3e337c56b869@linux.intel.com>

Hello,

On Tue, May 10, 2022 at 2:31 AM Xing Zhengjun
<zhengjun.xing@linux.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@INST_RETIRED.ANY@" already..

Or could it be done when creating JSON files?

Thanks,
Namhyung