Page MenuHomeFreeBSD

hwpmc: add RAPL energy-counter class (AMD + Intel)
AcceptedPublic

Authored by afscoelho_gmail.com on Fri, Jul 3, 6:00 PM.
Tags
None
Referenced Files
F163532692: D58028.id181916.diff
Fri, Jul 24, 5:16 AM
F163512744: D58028.id181916.diff
Fri, Jul 24, 12:01 AM
Unknown Object (File)
Thu, Jul 23, 7:12 AM
Unknown Object (File)
Wed, Jul 22, 10:08 PM
Unknown Object (File)
Wed, Jul 22, 4:05 PM
Unknown Object (File)
Wed, Jul 22, 6:03 AM
Unknown Object (File)
Sun, Jul 19, 7:23 AM
Unknown Object (File)
Sat, Jul 18, 7:32 PM

Details

Summary

Add hwpmc_rapl.c/.h implementing PMC_CLASS_RAPL, a read-only
system-scope class modeled on TSC and wired into x86 AMD and Intel MD
init. A per-vendor MSR table covers AMD/Hygon and Intel; energy is
reported in microjoules, with the Intel server 2^-16 J DRAM unit
handled and 32-bit wraps recovered into a 64-bit accumulator.

The overflow guard follows the PMC lifetime: armed on the first
allocated PMC, callout_drain()d on the last release, and each tick only
rendezvouses CPUs holding one. Per-CPU spin locks guard the accumulator
against torn reads on i386. PMC_CAP_DOMWIDE lets pmcstat(8) allocate
one counter per NUMA domain instead of per CPU.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 74773
Build 71656: arc lint + arc unit

Event Timeline

sys/dev/hwpmc/hwpmc_amd.c
1134

You should probably check the cpuid flag for the AMD's RAPL and decide whether you initialize it at all.

sys/dev/hwpmc/hwpmc_intel.c
283

Again, I think this is dependent on the cpuid for thermal sensors.

sys/dev/hwpmc/hwpmc_rapl.c
145

Ah you have a circular dependency between the two functions can you move this definition to the top of the file.

193

This is already done for all system mode PMC's by the common infrastructure.

510

Ah I didn't realize you were checking if the counters exist in your add function that looks good, but still think we shouldn't add the class if there's no counters present.

542

I think amd and intel also have integrated gpu power reporting. Should be a couple extra lines if you look at on of the APU PPRs.

sys/i386/include/pmc_mdep.h
50

RAPL...

sys/dev/hwpmc/hwpmc_rapl.c
537

Shouldn't you depend on the GETCAPS call that I added to advertise different capabilities per counter?

afscoelho_gmail.com added inline comments.
sys/dev/hwpmc/hwpmc_intel.c
283

Intel doesn't enumerate RAPL via CPUID. I think rdmsr_safe() of MSR_RAPL_POWER_UNIT is the best check here. That's now factored into pmc_rapl_present() helper used on AMD, called before the class slot is reserved

sys/dev/hwpmc/hwpmc_rapl.c
193

The common check in pmc_do_op_pmcallocate() is skipped when the security.bsd.unprivileged_syspmcs tunable is set. I did the check intentional so energy counters remain privileged even in this case

510

The class is already dropped on such hardware, pmc_rapl_initialize() returns ENXIO when no energy MSR responds

537

But all RAPL rows carry identical capabilities (PMC_CAP_READ|PMC_CAP_DOMWIDE), and rows whose MSR doesn't respond are never created

542

I can add a RAPL_ENERGY_GPU event now, but I have no hardware to test it. On AMD I couldn't find an MSR based GPU domain: From what I saw GPU power on APUs is reported via the SMU (linux amdgpu) rather than RAPL MSRs . Do you know some specific PPR ?

Gate the RAPL class on a pmc_rapl_present() probe (AMDPM_RAPL CPUID bit on AMD/Hygon, unit-MSR rdmsr_safe on both vendors) instead of reserving a slot and dropping it on failure; move the guard tick prototype to the top of hwpmc_rapl.c; note RAPL in the amd64/i386 pmc_mdep.h class lists.

This revision is now accepted and ready to land.Thu, Jul 9, 6:21 PM
sys/dev/hwpmc/hwpmc_rapl.c
147

I suggest adding a mtx_assert(&rapl_alloc_mtx, MA_OWNED) assertion to rapl_guard_schedule.

471–472

LGTM. Ali, I will follow your lead that this is the most useful way to expose these counters.

Please also bump PMC_VERSION_MINOR in sys/sys/pmc.h.

sys/dev/hwpmc/hwpmc_rapl.c
252–256

I know it was copied from elsewhere, but please use these instead.

446–449
481–482
553

Does it need to be rdmsr_safe()? After all this code comes after pmc_rapl_present().

This revision now requires review to proceed.Mon, Jul 13, 4:25 PM
afscoelho_gmail.com added inline comments.
sys/dev/hwpmc/hwpmc_rapl.c
147

right, in fact using mtx_assert makes it consistent with the comment : )

553

You are right, it does not need to be rdmsr_safe ! Thanks for noticing it

LGTM. Ali, I will follow your lead that this is the most useful way to expose these counters.

Please also bump PMC_VERSION_MINOR in sys/sys/pmc.h.

Right, bumped PMC_VERSION_MINOR to 0x02.

mhorne added inline comments.
sys/dev/hwpmc/hwpmc_rapl.c
1–10
sys/dev/hwpmc/hwpmc_rapl.h
1–8

Latest preferred formatting for the license header: https://docs.freebsd.org/en/articles/license-guide/

With your approval, I can make the above tweak when merging.

This revision is now accepted and ready to land.Thu, Jul 16, 6:18 PM

LGTM. Ali, I will follow your lead that this is the most useful way to expose these counters.

Please also bump PMC_VERSION_MINOR in sys/sys/pmc.h.

Right, bumped PMC_VERSION_MINOR to 0x02.

sys/dev/hwpmc/hwpmc_rapl.h
1–8

Latest preferred formatting for the license header: https://docs.freebsd.org/en/articles/license-guide/

With your approval, I can make the above tweak when merging.

Sure, please go ahead! and sorry for the delay answer, for some reason I didn't see the email : )
Thanks!