uncore: Allow to configure uncore ELC control - #796
spandruvada wants to merge 1 commit into
Conversation
|
Thank you @spandruvada for addressing the issues in #731, I guess we can close it now. @yarda , do we want to include this in the next FDP? Thanks. |
|
LGTM |
|
The CentOS 7 CI failure is unrelated. |
The Efficiency Latency Control (ELC) feature improves performance per watt. With this feature hardware power management algorithms optimize trade-off between latency and power consumption. Add additional configuration for uncore plugin to include attributes to configure ELC. Refer to Intel White Paper Link: Linux kernel documentation: https://docs.kernel.org/admin-guide/pm/intel_uncore_frequency_scaling.html Example configuration: [uncore_all] type=uncore elc_floor_freq_khz=2400000 elc_high_threshold_enable=1 elc_high_threshold_percent=95 elc_low_threshold_percent=10
3bbdb8d to
a4f0364
Compare
|
Fixed the issue pointed by @yarda . Thanks for review. |
|
/packit build |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe uncore plugin now documents ELC controls, collects their sysfs values, registers configuration options, and provides handlers for reading and setting floor frequency, threshold percentages, and high-threshold enable state. ChangesUncore ELC controls
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tuned/plugins/plugin_uncore.py`:
- Around line 102-112: Update _get_all so missing ELC sysfs files do not cause
the entire frequency-value read to return None. Keep max_freq_khz and
min_freq_khz retrieval and validation independent, and obtain
elc_floor_freq_khz, elc_high_threshold_enable, elc_high_threshold_percent, and
elc_low_threshold_percent through optional ELC-specific getters or a separate
helper that tolerates unavailable files.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e82f4e71-151d-4d9a-9791-9bb10ef7b130
📒 Files selected for processing (1)
tuned/plugins/plugin_uncore.py
| elc_floor_freq_khz = self._get(device, "elc_floor_freq_khz") | ||
| elc_high_threshold_enable = self._get(device, "elc_high_threshold_enable") | ||
| elc_high_threshold_percent = self._get(device, "elc_high_threshold_percent") | ||
| elc_low_threshold_percent = self._get(device, "elc_low_threshold_percent") | ||
|
|
||
| except (OSError, IOError): | ||
| log.error("fail to read uncore frequency values") | ||
| return None | ||
| return (initial_max_freq_khz, initial_min_freq_khz, max_freq_khz, min_freq_khz) | ||
| return (initial_max_freq_khz, initial_min_freq_khz, max_freq_khz, min_freq_khz, | ||
| elc_floor_freq_khz, elc_high_threshold_enable, elc_high_threshold_percent, | ||
| elc_low_threshold_percent) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve existing frequency controls when ELC files are absent.
Lines 102-105 make _get_all require all ELC sysfs files. If a system supports max_freq_khz and min_freq_khz but does not expose ELC, _get_all returns None. _validate_khz_value then rejects existing max/min configuration before it writes either file.
Treat unavailable ELC values as optional, or read ELC values only from ELC handlers.
Proposed direction
- elc_floor_freq_khz = self._get(device, "elc_floor_freq_khz")
- elc_high_threshold_enable = self._get(device, "elc_high_threshold_enable")
- elc_high_threshold_percent = self._get(device, "elc_high_threshold_percent")
- elc_low_threshold_percent = self._get(device, "elc_low_threshold_percent")
+ # Keep `_get_all` limited to values required by max/min validation.Move ELC reads into their respective getters or a separate optional ELC-value helper.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tuned/plugins/plugin_uncore.py` around lines 102 - 112, Update _get_all so
missing ELC sysfs files do not cause the entire frequency-value read to return
None. Keep max_freq_khz and min_freq_khz retrieval and validation independent,
and obtain elc_floor_freq_khz, elc_high_threshold_enable,
elc_high_threshold_percent, and elc_low_threshold_percent through optional
ELC-specific getters or a separate helper that tolerates unavailable files.
There was a problem hiding this comment.
IMHO this can happen that the basic uncore knobs exist but the ELC ones don't, @spandruvada can you please address it? Otherwise LGTM.
This branch has not been deployed
3 participants
The Efficiency Latency Control (ELC) feature improves performance per watt. With this feature hardware power management algorithms optimize trade-off between latency and power consumption.
Add additional configuration for uncore plugin to include attributes to configure ELC.
Refer to Intel White Paper
Link:
Linux kernel documentation:
https://docs.kernel.org/admin-guide/pm/intel_uncore_frequency_scaling.html
Example configuration:
[uncore_all]
type=uncore
elc_floor_freq_khz=2400000
elc_high_threshold_enable=1
elc_high_threshold_percent=95
elc_low_threshold_percent=10