Invert pmc_cpumask()/PMC_OP_GETCPUMASK: return the cpus sharing a pmc's counter hardware (Linux-style), not one cpu per domain. Reference cpu threaded through amd_l3_cpumask()/amd_pkg_cpumask(); new amd_umc_cpumask() for UMC. amd_umc_sharing_key() added as the NodeId seam (delegates to amd_pkg_key() for now). Fixed a virtual-mode PMC_TO_CPU() OOB this would've reintroduced. Rewrote pmcstat's clone loop as a set-cover (old loop assumed a covering set).
Details
- Reviewers
afscoelho_gmail.com anascime_amd.com davichazbh_gmail.com ali_mashtizadeh.com bcr mhorne - Group Reviewers
manpages pmc
Diff Detail
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
@ali_mashtizadeh.com, this is the CPU mask interface we discussed: hwpmc reports the CPUs to allocate each system-mode PMC on (one per L3 cache for L3, one per package for DF and UMC) and pmcstat uses them. Is this what you had in mind? L3 events count all slices by default; tell me if you need per-slice control.
As soon you confirm, we can ask others reviewers to take a look
IIRC the linux interface does the inverse of what were doing here.
We should return the list of cores that share a counter instead of choosing unique cores to attach to. This might make the userspace code a few lines longer for the purpose of this change, but it makes a big difference for the node-id stuff that AMD is using in Zen 6. Recall that the APIC IDs are not guarenteed to match the physical core layout. So the node-ids could be out of order and userspace would have to read the MSR to validate it.
I think this also makes it easier for you to implement node-id for UMC counters, since you'll just find all the cores that have an equal node id to the current allocated counter.
| sys/dev/hwpmc/hwpmc_amd.c | ||
|---|---|---|
| 1371 | Is this valid for big-little and combinations of NPS1-4? Shouldn't you use the domain API so we don't need to patch this later? | |
| 1399 | You'll need to implement node-id maybe in a separate change. | |
Two clarifications:
- I think that’s valid with current cpus for DF counters with respect to NPS ignore me you shouldn’t use domain api except is there any reason we need to check that msr per domain i.e. some imbalance between domains?
- General for everyone else node-id is not domains it’s an MSR used to define the sharing set of UMC counters. It’s been 0 until some zen 6 chips that’s what all this refactoring is for.
Inverted as requested, reference cpu threaded through the AMD grouping functions. amd_umc_sharing_key() is the NodeId seam (CPUID 0x8000001e, not an MSR -- also affects amd_pkg_key()/DF once Zen 6 sockets can host >1 I/O die). Per-domain question doesn't apply: DF/UMC grouping never touches NUMA domains. Verified on hardware (L3 = CCX group, DF/UMC = package). Also fixed an OOB this redesign would've introduced (virtual-mode PMC_TO_CPU() sentinel).
In general this looks fine. Are these search/mask routines being called during any hot paths or only when setup/teardown/userland stuff is being called?
No. The only call sites are:
- pmcstat.c:723 and :758, both in event allocation before any counters are armed.
- The PMC_OP_GETCPUMASK ioctl handler in hwpmc_mod.c:4778, a one-shot userspace syscall.
Nothing here runs from amd_intr() or any per-sample path. It's a setup-time query only.
Looks good to me except removing the XXX and cleanup that comment.
| sys/dev/hwpmc/hwpmc_amd.c | ||
|---|---|---|
| 1277 | Remove the XXX but leave the comment? | |
Done in the latest diff: dropped the XXX and reworded the comment. No functional change; also rebased on current main.