feat(hwmon): implement device reader for architectures with hwmon sensors #2372
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2372 +/- ##
==========================================
- Coverage 90.71% 89.52% -1.19%
==========================================
Files 44 45 +1
Lines 3865 4163 +298
==========================================
+ Hits 3506 3727 +221
- Misses 269 323 +54
- Partials 90 113 +23 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
KaiyiLiu1234
force-pushed
the
hwmon-support
branch
from
8e0a139 to
f7da3ac
Compare
Contributor
|
|
Collaborator
Author
|
TODO: Update documentation to include experimental hwmon enable feature. |
Collaborator
Author
|
TODO: Add unit tests for config.go |
Contributor
|
📊 Profiling reports are ready to be viewed
💻 CPU Comparison with base Kepler💾 Memory Comparison with base Kepler (Inuse)💾 Memory Comparison with base Kepler (Alloc)⬇️ Download the Profiling artifacts from the Actions Summary page 📦 Artifact name: 🔧 Or use GitHub CLI to download artifacts: gh run download 20350200717 -n profile-artifacts-2372 |
Contributor
|
📊 Profiling reports are ready to be viewed
💻 CPU Comparison with base Kepler💾 Memory Comparison with base Kepler (Inuse)💾 Memory Comparison with base Kepler (Alloc)⬇️ Download the Profiling artifacts from the Actions Summary page 📦 Artifact name: 🔧 Or use GitHub CLI to download artifacts: gh run download 20350294686 -n profile-artifacts-2372 |
KaiyiLiu1234
force-pushed
the
hwmon-support
branch
from
f7da3ac to
8b27076
Compare
Contributor
|
|
Contributor
|
📊 Profiling reports are ready to be viewed
💻 CPU Comparison with base Kepler💾 Memory Comparison with base Kepler (Inuse)💾 Memory Comparison with base Kepler (Alloc)⬇️ Download the Profiling artifacts from the Actions Summary page 📦 Artifact name: 🔧 Or use GitHub CLI to download artifacts: gh run download 20350934438 -n profile-artifacts-2372 |
…er sensors Implemented hwmon device reader to allow Kepler to acquire power metrics (watts) from hwmon subsystem. Hwmon enablement added as an experimental feature in config. Signed-off-by: Kaiyi Liu <[email protected]>
KaiyiLiu1234
force-pushed
the
hwmon-support
branch
from
8b27076 to
0fc4ee5
Compare
Contributor
|
|
Contributor
|
📊 Profiling reports are ready to be viewed
💻 CPU Comparison with base Kepler💾 Memory Comparison with base Kepler (Inuse)💾 Memory Comparison with base Kepler (Alloc)⬇️ Download the Profiling artifacts from the Actions Summary page 📦 Artifact name: 🔧 Or use GitHub CLI to download artifacts: gh run download 20361710986 -n profile-artifacts-2372 |
vimalk78
reviewed
Dec 19, 2025
Comment on lines
+60
to
+61
| Zones []string `yaml:"zones"` | ||
| Chips []string `yaml:"chips"` |
Collaborator
There was a problem hiding this comment.
add comments briefly explaining what are zones and chips in hwmon.
are these hwmon concepts or arm concepts?
Comment on lines
+470
to
+492
| func sysReadFile(file string) ([]byte, error) { | ||
| f, err := os.Open(file) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer func() { _ = f.Close() }() | ||
|
|
||
| // On some machines, hwmon drivers are broken and return EAGAIN. This causes | ||
| // Go's os.ReadFile implementation to poll forever. | ||
| // | ||
| // Since we either want to read data or bail immediately, do the simplest | ||
| // possible read using system call directly. | ||
| b := make([]byte, 128) | ||
| n, err := unix.Read(int(f.Fd()), b) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if n < 0 { | ||
| return nil, fmt.Errorf("failed to read file: %q, read returned negative bytes value: %d", file, n) | ||
| } | ||
|
|
||
| return b[:n], nil | ||
| } |
Collaborator
There was a problem hiding this comment.
can we do simple ioutil.ReadFile
in rapl support, procfs also uses filepath and os.ReadFile
https://github.com/prometheus/procfs/blob/master/sysfs/class_powercap.go#L90-L94
Collaborator
Author
There was a problem hiding this comment.
So this is quite interesting. Counterintuitively, you actually should not use os.ReadFile. I initially did this, but according to node exporter themselves, for reading hwmon drivers, you should use system call directly:
// On some machines, hwmon drivers are broken and return EAGAIN. This causes
// Go's os.ReadFile implementation to poll forever.
//
// Since we either want to read data or bail immediately, do the simplest
// possible read using system call directly.
Comment on lines
+522
to
+525
| func (z *hwmonPowerZone) MaxEnergy() Energy { | ||
| // No maximum for power sensors | ||
| return 0 | ||
| } |
Collaborator
There was a problem hiding this comment.
is this function used anywhere? what is the implication of returning 0 as the MaxPower
Collaborator
Author
There was a problem hiding this comment.
This is not MaxPower it is MaxEnergy. This is for satisfying the EnergyZone interface. Essentially, it means that because this zone only exposes Power, Energy and MaxEnergy will return 0.
Collaborator
There was a problem hiding this comment.
is lebel being read in hwmon_power_meter?
Collaborator
Author
There was a problem hiding this comment.
Yes absolutely. We need the label to determine what kind of "zone" this sensor satisfies at the node level (ex. cpu_power, io_power)
| // Returns a map of sensor number -> sensor files | ||
| func (r *sysfsHwmonReader) findSensorsByType(files []os.DirEntry, sensorType string) map[int]map[string]string { | ||
| sensors := make(map[int]map[string]string) | ||
| pattern := regexp.MustCompile(fmt.Sprintf(`^%s(\d+)_(.+)$`, sensorType)) |
Collaborator
There was a problem hiding this comment.
does this get compiled only at startup?
Collaborator
There was a problem hiding this comment.
better move it to package level
| var zones []EnergyZone | ||
| for _, entry := range hwmonDirs { | ||
| // check for valid hwmon devices | ||
| if !entry.IsDir() && !isSymlink(filepath.Join(r.basePath, entry.Name())) { |
Collaborator
There was a problem hiding this comment.
it needs to be a symlink?
| // Scan for power sensors | ||
| files, err := os.ReadDir(hwmonPath) | ||
| if err != nil { | ||
| return nil, err |
Collaborator
There was a problem hiding this comment.
need better error message
|
|
||
| func (r *sysfsHwmonReader) discoverZones(hwmonPath string) ([]EnergyZone, error) { | ||
| // Get chip name | ||
| chipName, err := r.getChipName(hwmonPath) |
Collaborator
There was a problem hiding this comment.
why not filter the chips here itself?
Collaborator
Author
There was a problem hiding this comment.
It can be put there, but I followed rapl's setup which instead placed it in Zones() to be consistent. Please refer to rapl_sysfs_power_meter.go
|
|
||
| // WithChipFilter sets chip names to include for monitoring | ||
| // If empty, all chips are included | ||
| func WithChipFilter(chips []string) HwmonOptionFn { |
Collaborator
There was a problem hiding this comment.
how will the users decide which chip names are to be included here?
Collaborator
Author
There was a problem hiding this comment.
With the config:
experimental:
hwmon:
enabled: true
zones: ["package", "core"] # Filter by zone names
chips: ["k10temp", "amdgpu"] # Filter by chip names
Empty means everything will be included. Chip names are determined based on how node rapl determines them:
- Try to construct name from device path
- Fall back to /sys/class/hwmon/hwmonX/name file
- Fall back to hwmon directory name (hwmon0, hwmon1, etc.)
Collaborator
There was a problem hiding this comment.
lets clearly document what we mean by a "zone". does hwmon_power_meter use sensor as zone?
hwmon0 << chip
├── power1_input << sensor
└── power2_input << sensor
hwmon1
├── power1_input
└── power2_input
Collaborator
Author
There was a problem hiding this comment.
I will update the documentation
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implemented hwmon device reader to allow Kepler to acquire power metrics (watts) from hwmon subsystem. Hwmon enablement added as an experimental feature in config.
Precise changes: