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

pmda-denki RAPL MSR support #2106

Merged
merged 7 commits into from
Feb 3, 2025

Conversation

christianhorn
Copy link
Collaborator

@christianhorn christianhorn commented Nov 25, 2024

So far, only /sys file system RAPL sources were supported. This adds /dev/msr register support, found on Intel. This makes on many Intel chips an exciting new metric available which reflects the overall systems power consumption - so far we have to use battery-metrics or smart meter for that.

Selinux looks good for the new MSR metrics.

I see just one unsolved issue with these commits: qa/1515 is right now failing. I do not see how to fix it, it seems the reason is that the new version of pmda-denki is outputting more defails to DEBUG-INFO. That's pieces output after initialization just one time and intended. Not sure how to modify the test accordingly.

Regarding man-page: it can be removed from the commit, no useful content was updated (but the tool changed all kinds of formatting).

@natoscott
Copy link
Member

So far, only /sys file system RAPL sources were supported. This adds /dev/msr register support, found on Intel. This makes on many Intel chips an exciting new metric available which reflects the overall systems power consumption - so far we have to use battery-metrics or smart meter for that.

OK. Looking through the code, one minor consideration is whether a common namespace prefix would be better than repeating denki.rapl for these metrics? i.e. naming like denki.rapl.sysfs and denki.rapl.msr ("raplsysfs" and "raplmsr" seems slightly more cryptic than need be)

Selinux looks good for the new MSR metrics.

Excellent.

I see just one unsolved issue with these commits: qa/1515 is right now failing. I do not see how to fix it, it seems the reason is that the new version of pmda-denki is outputting more defails to DEBUG-INFO. That's pieces output after initialization just one time and intended. Not sure how to modify the test accordingly.

That's OK - we just need to adjust the pmdadenki_filter() function to catch these expected outputs - I can help with that if you like. That function also needs to be updated for the changes to denki.rapl{msr,sysfs} metrics regarding "No value(s) available" handling (its out-of-date currently as it only handles denki.rapl as a metric name).

Regarding man-page: it can be removed from the commit, no useful content was updated (but the tool changed all kinds of formatting).

Which tool is that OOC? The whitespace changes generated there are causing CI failures as follows:

$ make check
../../../scripts/man-lint -f pmdadenki.1
mandoc: pmdadenki.1:1:4: STYLE: whitespace at end of input line
mandoc: pmdadenki.1:3:4: STYLE: whitespace at end of input line
mandoc: pmdadenki.1:8:4: STYLE: whitespace at end of input line
mandoc: pmdadenki.1:13:4: STYLE: whitespace at end of input line
mandoc: pmdadenki.1:25:4: STYLE: whitespace at end of input line
mandoc: pmdadenki.1:28:4: STYLE: whitespace at end of input line
mandoc: pmdadenki.1:45:4: STYLE: whitespace at end of input line
mandoc: pmdadenki.1:54:4: STYLE: whitespace at end of input line
mandoc: pmdadenki.1:57:4: STYLE: whitespace at end of input line
mandoc: pmdadenki.1:66:4: STYLE: whitespace at end of input line

So, we'll definitely need to do something about that.

@christianhorn
Copy link
Collaborator Author

christianhorn commented Feb 2, 2025

Thanks for the input. I had separation into rapl.sysfs and rapl.msr considered, but got side tracked later.
Help with "pmdadenki_filter()" would be appreciated. The other pieces should be fixed now.

I used manedit ( http://freshmeat.net/projects/manedit/ ), package from Fedora41. For the committed version I simply edited the current upstream version with an editor. No big changes actually required to the manpage due to the change.

@natoscott
Copy link
Member

Thanks for the input. I had separation into rapl.sysfs and rapl.msr considered, but got side tracked later. Help with "pmdadenki_filter()" would be appreciated. The other pieces should be fixed now.

Great! I've pushed tweaks to the test 1515 filtering and updated the test outputs to match the current denki implementation. Please double check my changes? Thanks!

@natoscott natoscott merged commit 25dd2e3 into performancecopilot:main Feb 3, 2025
24 checks passed
@christianhorn
Copy link
Collaborator Author

Including last commit, all looks good for me, includig qa/1515!

@christianhorn christianhorn deleted the rapl-msr-support branch February 3, 2025 05:28
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