Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <8f846bfe-af5d-d895-0ee4-a5bbbae1fd06@linux.intel.com>
Date:   Wed, 18 May 2022 10:45:11 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.0
Subject: Re: [PATCH 1/5] Revert "perf stat: Support metrics with hybrid
 events"
Content-Language: en-US
To:     Namhyung Kim <namhyung@kernel.org>
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>
References: <20220507053410.3798748-1-irogers@google.com>
 <20220507053410.3798748-2-irogers@google.com> <YnkTIwiVBEGUJRKn@kernel.org>
 <b01a66c4-ec4d-ccb7-8ca7-130d93855177@linux.intel.com>
 <708fab8f-b5c5-8f34-4322-3e337c56b869@linux.intel.com>
 <CAM9d7cgEc9o9y-fH==avzucigmMVw0AwooGw0eufb2+D568-_g@mail.gmail.com>
From:   Xing Zhengjun <zhengjun.xing@linux.intel.com>
In-Reply-To: <CAM9d7cgEc9o9y-fH==avzucigmMVw0AwooGw0eufb2+D568-_g@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 5/18/2022 6:58 AM, Namhyung Kim wrote:
> 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

Yes, we have ever tested it, and it can work. we are changing the 
converter tools to implement it, but it still has some issues that need 
to fix.

-- 
Zhengjun Xing