Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 3 commits into from

Conversation

girardm
Copy link
Contributor

@girardm girardm commented Aug 30, 2019

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
Copy link
Member

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
Copy link
Contributor Author

girardm commented Sep 3, 2019

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

He @girardm - I have applied some portions of this PR, with modifications, and kept you as a co-author of the commits (3e9b225 and ba34d1f). 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 (4b690da). 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.

@connorimes connorimes closed this Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants