search

powercap / raplcap

RAPL power capping C interface with multiple implementations
BSD 3-Clause "New" or "Revised" License
45 stars 9 forks source link

Adds the decoding of the "locked" field of msr #5

Closed girardm closed 5 years ago

girardm commented 5 years ago

Adds the decoding of the "locked" field of the msr register, helping the user to understand why these settings are not taken into account.

connorimes commented 5 years ago

Hi @girardm, thanks for the PR!

The CI checks failed because the new top-level function was not implemented for the other raplcap backends. The primary reason I never implemented getting/setting the locked field in the top-level raplcap.h interface is because the powercap backend (which uses the sysfs interface exposed by the kernel) does not support it. (powercap is the other primary backend for raplcap.) In short, a capability needs to be available to both msr and powercap backends to qualify for inclusion in raplcap.h.

So as it is right now, I can't merge this PR. If you're willing to re-work the PR a bit, you can add the functionality to the msr backend exclusively via msr/raplcap-msr.h (instead of raplcap.h), which already exposes some lower-level details only available by reading the MSRs directly. The function would be renamed from raplcap_is_zone_locked to raplcap_msr_is_zone_locked. Code that always links against libraplcap-msr can then also include that additional header (that code just wouldn't be portable between raplcap backends, which may be acceptable for many use cases). Because the functionality wouldn't be in raplcap.h, the rapl-configure binaries would not be printing out locked status, but I'd be amenable to having a compile-time option to include getting/setting the locked bit so that rapl-configure-msr can expose the functionality, but rapl-configure-powercap does not.

girardm commented 5 years ago

Hi Imes, Thanks for your comments. Let me introduce myself quickly : I am Marc Girard from AtoS compagnie. I am french (so my english is perfectible ;-)). My team and me develop a tool to monitor / optimize power consumption on AtoS clusters. It's my first contribution on GitHub. I am affraid I forgot raplcap-msr.c from my local repository. I hope I could push a new PR before end of next week. Howerer I thinks I found a bug in raplcap-msr-common.c, MSR power limits are 15 bits wide so PL_MASK should be define to 0x7FFF instead of 0x7FF

connorimes commented 5 years ago

It looks like you're correct, that does appear to be a bug. Thank you for catching it. Since you found the issue, I'm happy to give you the opportunity to fix it so you can get credit for it in the git log -- please open a separate pull request that just fixes that issue and I'll merge it ASAP. Otherwise I will fix the issue myself soon.

connorimes commented 5 years ago

The build still fails because reading the "locked" status is not implemented for the powercap backend (and, in fact, cannot be). Please see my first response above for more details. If it is difficult to understand what I described and suggested as a workaround, I will try to explain better. Thanks again.

connorimes commented 5 years ago

He @girardm - I have applied some portions of this PR, with modifications, and kept you as a co-author of the commits (3e9b2250309ba1ac25d75033ef66efe30fdcd194 and ba34d1fa805f1077c11029361cf5be2344cbc596). Both locked and clamping statuses are now available.

I have hidden this additional output of rapl-configure-msr behind the (currently undocumented) CMake option RAPLCAP_CONFIGURE_MSR_EXTRA so that, by default, it still produces the same output as rapl-configure with other backends (4b690dac8867e4ba51ee76f6f8300c866bbac699). You can enable the new output by re-running cmake to enable it, e.g., cmake -DRAPLCAP_CONFIGURE_MSR_EXTRA=ON .. and then rebuilding.

Please let me know how it goes, and feel free to submit other PRs in the future. Cheers.