Messages in this thread | | | Date | Tue, 22 Nov 2022 11:53:00 +0000 | Subject | Re: [PATCH RFC 0/6] Add metrics for neoverse-n2 | From | James Clark <> |
| |
On 22/11/2022 07:11, Jing Zhang wrote: > > > 在 2022/11/21 下午7:51, James Clark 写道: >> >> >> On 16/11/2022 15:26, Jing Zhang wrote: >>> >>> >>> 在 2022/11/16 下午7:19, James Clark 写道: >>>> >>>> >>>> On 31/10/2022 11:11, Jing Zhang wrote: >>>>> This series add six metricgroups for neoverse-n2, among which, the >>>>> formula of topdown L1 is from the document: >>>>> https://documentation-service.arm.com/static/60250c7395978b529036da86?token= >>>>> >>>>> Since neoverse-n2 does not yet support topdown L2, metricgroups such >>>>> as Cache, TLB, Branch, InstructionsMix, and PEutilization are added to >>>>> help further analysis of performance bottlenecks. >>>>> >>>> >>>> Hi Jing, >>>> >>>> Thanks for working on this, these metrics look ok to me in general, >>>> although we're currently working on publishing standardised metrics >>>> across all new cores as part of a new project in Arm. This will include >>>> N2, and our ones are very similar (or almost identical) to yours, >>>> barring slightly different group names, metric names, and differences in >>>> things like outputting topdown metrics as percentages. >>>> >>>> We plan to publish our standard metrics some time in the next 2 months. >>>> Would you consider holding off on merging this change so that we have >>>> consistant group names and units going forward? Otherwise N2 would be> the odd one out. I will send you the metrics when they are ready, and we >>>> will have a script to generate perf jsons from them, so you can review. >>>> >>> >>> Do you mean that after you release the new standard metrics, I remake my >>> patch referring to them, such as consistent group names and unit? >> >> Hi Jing, >> >> I was planning to submit the patch myself, but there will be a script to >> generate perf json files, so no manual work would be needed. Although >> this is complicated by the fact that we won't be publishing the fixed >> TopdownL1 metrics that you have for the existing N2 silicon so there >> would be a one time copy paste to fix that part. >> >>> >>> >>>> We also have a slightly different forumula for one of the top down >>>> metrics which I think would be slightly more accurate. We don't have >>> >>> >>> The v2 version of the patchset updated the formula of topdown L1. >>> Link: https://lore.kernel.org/all/1668411720-3581-1-git-send-email-renyu.zj@linux.alibaba.com/ >>> >>> The formula of the v2 version is more accurate than v1, and it has been >>> verified in our test environment. Can you share your formula first and we >>> can discuss it together? :) >> >> I was looking at v2 but replied to the root of the thread by mistake. I >> also had it the wrong way round. So your version corrects for the errata >> on the current version of N2 (as you mentioned in the commit message). >> Our version would be if there is a future new silicon revision with that >> fixed, but it does have an extra improvement by subtracting the branch >> mispredicts. >> >> Perf doesn't currently match the jsons based on silicon revision, so >> we'd have to add something in for that if a fixed silicon version is >> released. But this is another problem for another time. >> > > Hi James, > > Let's do what Ian said, and you can improve it later with the standard metrics, > after the fixed silicon version is released. >
Ok that's fine by me. I do have one update about our publishing progress to share. This is the (currently empty) repo that we will be holding our metrics in: https://gitlab.arm.com/telemetry-solution/telemetry-solution
We'll also have the conversion script in there as well. So there has at least been some progress and we're getting close. I will keep you updated when it is populated.
> >> This is the frontend bound metric we have for future revisions: >> >> "100 * ( (STALL_SLOT_FRONTEND/(CPU_CYCLES * 5)) - ((BR_MIS_PRED * >> 4)/CPU_CYCLES) )" >> >> Other changes are, for example, your 'wasted' metric, we have >> 'bad_speculation', and without the >> cycles subtraction: >> >> 100 * ( ((1 - (OP_RETIRED/OP_SPEC)) * (1 - (STALL_SLOT/(CPU_CYCLES * >> 5)))) + ((BR_MIS_PRED * 4)/CPU_CYCLES) ) >> > > Thanks for sharing your metric version, But I still wonder, is BR_MIS_PRED not classified > as frontend bound?
We're counting branch mispredicts as an extra cost so we subtract it from frontend_bound because branch related stalls are covered by bad_speculation where we have added BR_MIS_PRED instead of subtracting.
Unfortunately I'm just the middle man here, I didn't actually work directly on producing these metrics so I hope nothing gets lost in my explanation.
> How do you judge the extra improvement by subtracting branch mispredicts?
As far as I know the repo that I mentioned above will have some benchmarks and tooling that were used to validate our version. So it should be apparent by running those.
> >> And some more details filled in around the units, for example: >> >> { >> "MetricName": "bad_speculation", >> "MetricExpr": "100 * ( ((1 - (OP_RETIRED/OP_SPEC)) * (1 - >> (STALL_SLOT/(CPU_CYCLES * 5)))) + ((BR_MIS_PRED * 4)/CPU_CYCLES) )", >> "BriefDescription": "Bad Speculation", >> "PublicDescription": "This metric is the percentage of total >> slots that executed operations and didn't retire due to a pipeline >> flush.\nThis indicates cycles that were utilized but inefficiently.", >> "MetricGroup": "TopdownL1", >> "ScaleUnit": "1percent of slots" >> }, >> > > My "wasted" metric was changed according to the arm documentation description, it was originally > "bad_speculation". I will change "wasted" back to "bad_speculation", if you wish.
Yeah that would be good. I think since that document we've tried to align names more to what was already out there and bad_speculation was probably judged to be a better description. For example it's already used in tools/perf/pmu-events/arch/arm64/hisilicon/hip08/metrics.json
> > > Thanks, > Jing > > >> So ignoring the errata issue, the main reason to hold off is for >> consistency and churn because these metrics in this format will be >> released for all cores going forwards. >> >> Thanks >> James >>
| |