Messages in this thread | | | Date | Wed, 18 May 2022 10:45:11 +0800 | Subject | Re: [PATCH 1/5] Revert "perf stat: Support metrics with hybrid events" | From | Xing Zhengjun <> |
| |
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
| |