Re: [RFC v2 4/4] perf tools: Support "branch-misses:pp" on arm64

[Tan Xiaojun]

On 2019/10/25 0:29, James Clark wrote:
> Hi Xiaojun,
> 
> This looks really good. I tried this, but got an error:
> 
> ÂÂÂ ./perf record -e branch-misses:p ls
> ÂÂÂ Error:
> ÂÂÂ The branch-misses:p event is not supported.

Hi, James,

I can't reproduce this problem. If the current system doesn't support spe, it shouldn't report an error. I use the latest codes of the mainline:

commit f116b96685a046a89c25d4a6ba2da489145c8888 (mainline/master)
Merge: f632bfaa33ed 603d9299da32
Author: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
Date:   Thu Oct 24 06:13:45 2019 -0400

    Merge tag 'mfd-fixes-5.4' of git://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd

I will go and see why this will be reported.

> 
> 
> I would have expected to use the event name that is listed in the SPE documentation for branch misses which is br_mis_pred or br_mis_pred_retired:
> 
> ÂÂÂ E[7], byte 0 bit [7]
> ÂÂÂ Mispredicted. The defined values of this bit are:
> ÂÂÂ 0 Did not cause correction to the predicted program flow.
> ÂÂÂ 1 A branch that caused a correction to the predicted program flow.
> 
> ÂÂÂ If PMUv3 is implemented this Event is required to be implemented consistently with either BR_MIS_PRED or BR_MIS_PRED_RETIRED.
> 

Do you mean that I can add these as new events to perf? If we think of them as new events, what should we do if the user does not add :pp for them?
(Or for these events, users can only add :pp to use them?)

> 
> +ÂÂÂÂÂÂ if (!strcmp(perf_env__arch(evlist->env), "arm64")
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ && evsel->core.attr.config == PERF_COUNT_HW_BRANCH_MISSES
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ && evsel->core.attr.precise_ip) {
> 
> As I mentioned above PERF_COUNT_HW_BRANCH_MISSESdoesn't seem to match up with the actual event counter that is associated with this SPE event (BR_MIS_PRED). The fix for this is probably as simple as adding an OR for the other aliases for branch mispredicts.

What you mean is that we can filter with spe events(like BR_MIS_PRED) first, and if we have other events that are exactly the same(no more for now), then we can handle them by adding OR in the future?

> 
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ pmu = perf_pmu__find("arm_spe_0");
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ if (pmu) {
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ evsel->pmu_name = pmu->name;
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ evsel->core.attr.type = PERF_RECORD_AUXTRACE;
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ evsel->core.attr.config = SPE_ATTR_TS_ENABLE
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ | SPE_ATTR_PA_ENABLE
> 
> I wouldn't set physical addresses by default as this requires root. I would leave that to the user if they want to manually configure SPE.

Yes. You are right. I got a error for this case. I will fix it.

------------------
./perf record -e branch-misses:p ls
Error:
You may not have permission to collect stats.
...
------------------

Thanks.
Xiaojun.

> 
> I have only looked briefly and I will do some more testing.
> 
> 
> Thanks
> James
> 
>