pmc.h.
ClosedPublic
Actions

Authored by pauamma_gundo.com on May 28 2022, 6:58 PM.

Details

Reviewers

mhorne
gjb
carlavilla
luporl

Group Reviewers

manpages
pmc
ARM
arm64
PowerPC

Commits

rG5faf9f4dff8d: Refresh CPU types and classes from sys/sys/pmc.h.

Summary

While here, fix a few nits.

Assuming that the Arm7 trademark covers ARMv7 as well.

Inspired by reviewing D35342.

Sources for trademark info

https://www.arm.com/company/policies/trademarks (no Arm8, curiously)
https://www.ibm.com/legal/copytrade?mhsrc=ibmsearch_a&mhq=trademark

Test Plan

man # and eyeball.
mandoc -T lint
igor

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

pauamma_gundo.com created this revision.May 28 2022, 6:58 PM

Herald added a subscriber: imp. · View Herald TranscriptMay 28 2022, 6:58 PM

pauamma_gundo.com requested review of this revision.May 28 2022, 6:58 PM

Harbormaster completed remote builds in B45757: Diff 106450.May 28 2022, 6:58 PM

lwhsu added reviewers: ARM, arm64, PowerPC.May 30 2022, 4:04 AM

Thank you for this!

lib/libpmc/pmc.3
176	I believe this also covers POWER9 CPUs.

This revision is now accepted and ready to land.May 30 2022, 2:49 PM

pauamma_gundo.com added inline comments.May 30 2022, 8:03 PM

lib/libpmc/pmc.3
176	I believe this also covers POWER9 CPUs. Maybe add that to sys/sys/pmc.h in that case? I went by the current content: __PMC_CPU(PPC_POWER8, 0x390, "IBM POWER8") \

luporl added a subscriber: luporl.May 31 2022, 4:22 PM

luporl added inline comments.

lib/libpmc/pmc.3
176	It does cover POWER9 CPUs too. The reason there is only PPC_POWER8 class in sys/sys/pmc.h is that POWER9 counters are programmed exactly like the POWER8 ones. There are different counters between POWER8 and POWER9, but this only changes the value that must be set to access them. This is handled somewhere in the PMU code, where it queries the model of current CPU.

Waiting for an answer to my inline question about pmc_close_logfile to update the diff, and I'd still like manpages approval after that.

lib/libpmc/pmc.3
176	Thanks. Will add as a trademark, then.
479	While I'm touching this file: shouldn't this be `pmc_close_logfile` instead?

mhorne added inline comments.Jun 2 2022, 12:24 PM

lib/libpmc/pmc.3
479	It seems that `pmc_configure_logfile(-1)` is used to flush and close any open logfile for the process. The exact difference in intent/functionality between this and `pmc_close_logfile()` is not obvious on a quick glance. So, I think it is okay to leave this as-is for now.

Add POWER9 trademark

This revision now requires review to proceed.Jun 3 2022, 12:17 AM

Harbormaster completed remote builds in B45827: Diff 106587.Jun 3 2022, 12:17 AM

pauamma_gundo.com added reviewers: gjb, carlavilla.Oct 24 2022, 2:28 PM

Review from manpages, ARM, arm64, and PowerPC would be good too.

No enough experience on this topic. I prefer to wait for the approval from src

The PowerPC parts look good.

This revision is now accepted and ready to land.Nov 1 2022, 1:48 PM

mhorne accepted this revision.Nov 7 2022, 2:48 PM

mhorne added inline comments.

lib/libpmc/pmc.3
162	Making a note for future work: we ought to find a better solution for this list of AMD and Intel processors. What is here is so outdated as to not be useful (or worse, misleading); instead we need to find the words to communicate that "most AMD and Intel processors are supported", but avoid trying (and failing) to maintain a list of specific CPU models.

pauamma_gundo.com added inline comments.Nov 7 2022, 6:39 PM

lib/libpmc/pmc.3
162	avoid trying (and failing) to maintain a list of specific CPU models. Is there an implicit definition that could work instead? rummages into dmesg.boot Like whatever test outputs "TSC: P-state invariant, performance statistics" at boot? (13.1 amd64)