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

cpu_plugin: Add property "uncore_max_freq_khz" #576

Closed

Conversation

sgruszka
Copy link
Contributor

@sgruszka sgruszka commented Dec 4, 2023

Add a property "uncore_max_delta_khz" to cpu_plugin, so that uncore maximum frequency can be set. The property is exported per die, hence add extra logic to map it to per cpu setting and check for wrong config.

Refer to the following link to get details uncore frequency scaling: https://docs.kernel.org/admin-guide/pm/intel_uncore_frequency_scaling.html

This extends PR #519 to make uncore frequency defined per cpu as proposed in #519 (comment)

For now, for simplicity , uncore max_freq_khz is configured as direct value. Also uncore's that are not defined as package_NN_die_MM are not covered in this PR.

Add a property "uncore_max_delta_khz" to cpu_plugin, so that uncore
maximum frequency can be set. The property is experted per die,
hance add extra logic to map per cpu setting to per die setting and
check for wrong config.

Refer to the following link to get details uncore freqnency scaling:
https://docs.kernel.org/admin-guide/pm/intel_uncore_frequency_scaling.html

Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
Comment on lines +647 to +648
@command_set("uncore_max_freq_khz", per_device=True)
def _set_uncore_max_freq_khz(self, uncore_max_freq_khz, device, sim):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allowing per-cpu configuration of something that cannot be configured for individual cpus does not make sense to me. The uncore frequency is configurable for each package/die combination - not for individual cpus.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I generally agree with that. From simplicity perspective I think better would be separate plugin that allow configure individual uncore's based on name in sysfs i.e. package_NN_die_MM or uncoreNN. I'm ok to implement such plugin, if that's better.
However @yarda requested per cpus option. My understanding is that, this is more friendly for users using CPU-partitioning profiles , as they could then easier match cpus on which partitioning setting are used.

Anyway I'm ok with both implementations, but would like to know which is preferable, @yarda could you please provide feedback here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the new plugin i.e. plugin_intel_uncore.py would be created ,
it could have config/device specification in configs files like this:

[intel_uncore]
max_freq_khz_delta=200000

[intel_io_uncores]
type=intel_uncore
devices=uncore*
max_freq_khz = 4000000

[intel_package_2]
type=intel_uncore
devices=package_02_die_00
max_freq_khz_procentage=95

The device names would correspond to entries in /sys/devices/system/cpu/intel_uncore_frequency/
The frequency specification could be any: percentage, direct value or delta.

Would that be ok? Feedback appreciated.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with doing this in the existing cpu plugin - but with a single value for all CPUs (i.e. don't set "per_device=True"). Would that work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think yes, that is basically how it was done is original PR #519 - we have one global settings and apply it to all uncore's .

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the info. Then it probably doesn't make sense to add this functionality to the CPU plugin which works over CPUs and we probably need the uncore TuneD plugin that will work over devices which has uncore control.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thanks for feedback. Going to work on the plugin ... should be relatively easy to implement .

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The matcher function could have flexible syntax, e.g.:

...
devices=${f:uncore2devs:02}  # package 02, any die
...
devices=${f:uncore2devs:02:00}  # package 02, die 00
...
devices=${f:uncore2devs:0*:0*}  # any package starting with 0, any die starting with 0
...

etc. This selector function would go under the tuned/profiles/functions directory, where built-in functions live.

I think this could be nice addition for profiles witch will use uncore plugin and also do cpu partitioning. I'll try to implement this as well.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yarda - you said "The cpu plugin has 'per CPU' granularity. So the uncore_max_freq_khz needs to be settable per CPU, otherwise we could break things / TuneD logic. If it's unacceptable we will probably need new plugin with the package/die granularity."

But what does the "per_device" option do when creating the command?
@command_set("uncore_max_freq_khz", per_device=True)

Doesn't setting "per_device=False" allow a command to be defined that cannot be settable per CPU? Or is it for some other purpose?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

per_device=False (non-device commands) are mostly used for non-device plugins (e.g. bootloader). For device based plugins it can be used for setting some property of the plugin instance (for example delay, path prefix, units, ...) to modify device commands behavior of the instance. It shouldn't interfere with other instances of the plugin. It also doesn't support hotplug, because hotplug is device based thing, not instance based.

In case of uncore implementation as individual plugin you could for example in the TuneD profile define uncore plugin instance for high performance and uncore plugin instance for powersave (similarly as we now do with the CPUs) and later move dynamically packages/dies between the instances as needed through the runtime API. This wouldn't work if implemented in the cpu plugin with the non-device command. You could also use the built-in regex matcher to match the packages/dies via regular expressions in the TuneD profile, i.e. you could work with them in TuneD as with other devices.

@sgruszka
Copy link
Contributor Author

Created new PR #590 with intel uncore plugin , closing this one.

@sgruszka sgruszka closed this Jan 30, 2024
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.

4 participants