In include/perfmon/pfmlib.h new processor enumerations are suppose to be placed right before:
/* MUST ADD NEW PMU MODELS HERE */
This is to avoid changing the existing enum values between versions.
As a sanity check diffed arm_cortex_a53_events.h and arm_cavium_tx2_events.h to see if anything was way off on the common events. Looks cortex a53 might need to update some of the name and descriptions. Also checked the names against the kernel perf tool entries in armv8-recommended.json. BUS_ACCESS_LD should be changed to BUS_ACCESS_RD, BUS_ACCESS_ST should also match up but doesn't.
Is there a reason to include "CHAIN" event? It wouldn't be usable in libpfm
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Is there a pointer to documentation describing the events? If that is available, it would be good to mention that in the comments of file with the events.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Thanks for the thorough review. I'll fix the issues you mentioned.
The only documentation I can reference is the ARM Architecture Reference Manual
ARMv8, for the ARMv8-a architecture profile, ARM DDI 0487B.a ID033117. We have
an Optimization Guide, but it's only available through Support with an NDA. Apparently,
I'm not the first to encounter this long standing issue. I'll have to submit another pull request
at some future time when more detailed documentation is available.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
In include/perfmon/pfmlib.h new processor enumerations are suppose to be placed right before:
/* MUST ADD NEW PMU MODELS HERE */
This is to avoid changing the existing enum values between versions.
As a sanity check diffed arm_cortex_a53_events.h and arm_cavium_tx2_events.h to see if anything was way off on the common events. Looks cortex a53 might need to update some of the name and descriptions. Also checked the names against the kernel perf tool entries in armv8-recommended.json. BUS_ACCESS_LD should be changed to BUS_ACCESS_RD, BUS_ACCESS_ST should also match up but doesn't.
Is there a reason to include "CHAIN" event? It wouldn't be usable in libpfm
Is there a pointer to documentation describing the events? If that is available, it would be good to mention that in the comments of file with the events.
Should add some checks in tests/valide_arm64.c to check that some of the thunderx 2 events are there.
Thanks for the thorough review. I'll fix the issues you mentioned.
The only documentation I can reference is the ARM Architecture Reference Manual
ARMv8, for the ARMv8-a architecture profile, ARM DDI 0487B.a ID033117. We have
an Optimization Guide, but it's only available through Support with an NDA. Apparently,
I'm not the first to encounter this long standing issue. I'll have to submit another pull request
at some future time when more detailed documentation is available.
I think I've rebased and applied the appropriate changes. I'm not sure about the workflow,
but the earlier merge request can be cancelled.
I think in the future that you can use an interactive rebase as described in the following URL to massage the patch to cleanly include the revisions:
https://robots.thoughtbot.com/git-interactive-rebase-squash-amend-rewriting-history