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.

Sign up for GitHub

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
wants to merge 1 commit into from
Closed

cpu_plugin: Add property "uncore_max_freq_khz" #576

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
113 changes: 113 additions & 0 deletions tuned/plugins/plugin_cpu.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@

cpuidle_states_path = "/sys/devices/system/cpu/cpu0/cpuidle"

class Die:
def __init__(self):
self.cpus = set()
self.new_values = dict()

class CPULatencyPlugin(hotplug.Plugin):
"""
`cpu`::
Expand Down Expand Up @@ -185,6 +190,10 @@ class CPULatencyPlugin(hotplug.Plugin):
pm_qos_resume_latency_us=100
----
Allows any C-state with a resume latency less than value.
[cpu]
uncore_max_freq_khz=0
----
Set the maximum uncore frequency that hardware will use.
"""

def __init__(self, *args, **kwargs):
Expand All @@ -199,15 +208,64 @@ def __init__(self, *args, **kwargs):
self._has_intel_pstate = False
self._has_amd_pstate = False
self._has_pm_qos_resume_latency_us = None
self._has_topology = False

self._uncore_max_delta_khz_save = None
self._min_perf_pct_save = None
self._max_perf_pct_save = None
self._no_turbo_save = None
self._governors_map = {}
self._cmd = commands()

self._topology = dict()
self._topology_config_wrong = False

self._flags = None

def _cpu_get_package_die_id(self, device):
dir = '/sys/devices/system/cpu/%s/topology/' % device
if not os.path.exists(dir):
return (-1, -1)
die_id = self._str2int(self._cmd.read_file(dir + "/die_id"))
package_id = self._str2int(self._cmd.read_file(dir + "/physical_package_id"))
return (package_id, die_id)

def _init_topology(self):
for device in self._free_devices:
package_id, die_id = self._cpu_get_package_die_id(device)
if package_id == -1:
return

if package_id not in self._topology:
self._topology[package_id] = dict()
if die_id not in self._topology[package_id]:
self._topology[package_id][die_id] = Die()

self._topology[package_id][die_id].cpus |= {device}
self._has_topology = True

def _check_topology(self):
if not self._has_topology:
return

# All assigned devices has to be part of the same die's
assigned_devices = self._assigned_devices
while len(assigned_devices) > 0:
device = assigned_devices.pop()
assigned_devices.add(device)

package_id, die_id = self._cpu_get_package_die_id(device)
if package_id == -1:
self._topology_config_wrong = True
return

cpus = self._topology[package_id][die_id].cpus
if len(cpus - assigned_devices) > 0:
self._topology_config_wrong = True
return

assigned_devices -= cpus

def _init_devices(self):
self._devices_supported = True
self._free_devices = set()
Expand All @@ -216,6 +274,7 @@ def _init_devices(self):
self._free_devices.add(device.sys_name)

self._assigned_devices = set()
self._init_topology()

def _get_device_objects(self, devices):
return [self._hardware_inventory.get_device("cpu", x) for x in devices]
Expand All @@ -230,6 +289,7 @@ def _get_config_options(self):
"governor" : None,
"sampling_down_factor" : None,
"energy_perf_bias" : None,
"uncore_max_freq_khz" : None,
"min_perf_pct" : None,
"max_perf_pct" : None,
"no_turbo" : None,
Expand Down Expand Up @@ -343,6 +403,8 @@ def _instance_init(self, instance):
except IndexError:
instance._first_device = None

self._check_topology()

def _instance_cleanup(self, instance):
if instance._first_instance:
if self._has_pm_qos:
Expand Down Expand Up @@ -582,6 +644,57 @@ def _get_sampling_down_factor(self, device, ignore_missing=False):
return None
return self._cmd.read_file(path).strip()

@command_set("uncore_max_freq_khz", per_device=True)
def _set_uncore_max_freq_khz(self, uncore_max_freq_khz, device, sim):
Comment on lines +647 to +648
Copy link

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.

Copy link

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.

Copy link

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.

if not self._has_topology or self._topology_config_wrong:
return None

package_id, die_id = self._cpu_get_package_die_id(device)
if package_id == -1:
return None

die = self._topology[package_id][die_id]
die.new_values[device] = uncore_max_freq_khz

# Update the sysfs file only when cpus on die are set
if (len(die.new_values) != len(die.cpus)):
return uncore_max_freq_khz
die.new_values = dict()

dir = '/sys/devices/system/cpu/intel_uncore_frequency/package_%02d_die_%02d/' % (package_id, die_id)
file = dir + 'max_freq_khz'
if not os.path.exists(file):
log.error("Failed to set uncore_max_freq_khz because % file does not exist." % file)
return None

initial_min_freq_khz = self._str2int(self._cmd.read_file(dir + '/initial_min_freq_khz'))
initial_max_freq_khz = self._str2int(self._cmd.read_file(dir + '/initial_max_freq_khz'))
max_freq_khz = self._str2int(uncore_max_freq_khz)
if initial_min_freq_khz is None or initial_max_freq_khz is None or max_freq_khz is None:
return None

if max_freq_khz < initial_min_freq_khz or max_freq_khz > initial_max_freq_khz:
log.error("Failed to set uncore_max_freq_khz, value not in range (%d, %d)" % (initial_max_freq_khz, initial_max_freq_khz))
return None

self._cmd.write_to_file(file, max_freq_khz)
log.info('Set %s for %s' % (max_freq_khz, file))

return uncore_max_freq_khz

@command_get("uncore_max_freq_khz")
def _get_uncore_max_freq_khz(self, device, ignore_missing=False):
package_id, die_id = self._cpu_get_package_die_id(device)
if package_id == -1:
return None

file = '/sys/devices/system/cpu/intel_uncore_frequency/package_%02d_die_%02d/max_freq_khz' % (package_id , die_id)
if not os.path.exists(file):
return None

uncore_max_freq_khz = self._cmd.read_file(file)
return uncore_max_freq_khz

def _try_set_energy_perf_bias(self, cpu_id, value):
(retcode, out, err_msg) = self._cmd.execute(
["x86_energy_perf_policy",
Expand Down