From 1f5f9fa59c93d29a5ced58768395952b226a4709 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavol=20=C5=BD=C3=A1=C4=8Dik?= Date: Thu, 11 Apr 2024 10:09:05 +0200 Subject: [PATCH 1/5] Fix logs in `commands.read_file` Print the name of the file being read instead of its descriptor. --- tuned/utils/commands.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tuned/utils/commands.py b/tuned/utils/commands.py index db8d8b1e..be6451da 100644 --- a/tuned/utils/commands.py +++ b/tuned/utils/commands.py @@ -124,9 +124,9 @@ def write_to_file(self, f, data, makedir = False, no_error = False): def read_file(self, f, err_ret = "", no_error = False): old_value = err_ret try: - f = open(f, "r") - old_value = f.read() - f.close() + fd = open(f, "r") + old_value = fd.read() + fd.close() except (OSError,IOError) as e: if not no_error: self._error("Error when reading file '%s': '%s'" % (f, e)) From ea9fce1e90805413c613904d481b69b1582a6e58 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavol=20=C5=BD=C3=A1=C4=8Dik?= Date: Wed, 10 Apr 2024 16:26:01 +0200 Subject: [PATCH 2/5] Simplify reading/writing from/to sysctl by using existing functions We can use `read_file` and `write_to_file` to handle all manipulation of sysctl parameters. This does not change the behavior of _read_sysctl and _write_sysctl, but it alters some of their debug/error messages. --- tuned/plugins/plugin_sysctl.py | 45 +++++++--------------------------- 1 file changed, 9 insertions(+), 36 deletions(-) diff --git a/tuned/plugins/plugin_sysctl.py b/tuned/plugins/plugin_sysctl.py index 96854fd0..5f54857b 100644 --- a/tuned/plugins/plugin_sysctl.py +++ b/tuned/plugins/plugin_sysctl.py @@ -166,45 +166,18 @@ def _get_sysctl_path(self, option): def _read_sysctl(self, option): path = self._get_sysctl_path(option) - try: - with open(path, "r") as f: - line = "" - for i, line in enumerate(f): - if i > 0: - log.error("Failed to read sysctl parameter '%s', multi-line values are unsupported" - % option) - return None - value = line.strip() - log.debug("Value of sysctl parameter '%s' is '%s'" - % (option, value)) - return value - except (OSError, IOError) as e: - if e.errno == errno.ENOENT: - log.error("Failed to read sysctl parameter '%s', the parameter does not exist" - % option) - else: - log.error("Failed to read sysctl parameter '%s': %s" - % (option, str(e))) + content = self._cmd.read_file(path, err_ret=None) + if content is None: return None + content = content.strip() + if len(content.split("\n")) > 1: + log.error("Failed to read sysctl parameter '%s', multi-line values are unsupported" % option) + return None + return content def _write_sysctl(self, option, value, ignore_missing = False): path = self._get_sysctl_path(option) if os.path.basename(path) in DEPRECATED_SYSCTL_OPTIONS: - log.error("Refusing to set deprecated sysctl option %s" - % option) - return False - try: - log.debug("Setting sysctl parameter '%s' to '%s'" - % (option, value)) - with open(path, "w") as f: - f.write(value) - return True - except (OSError, IOError) as e: - if e.errno == errno.ENOENT: - log_func = log.debug if ignore_missing else log.error - log_func("Failed to set sysctl parameter '%s' to '%s', the parameter does not exist" - % (option, value)) - else: - log.error("Failed to set sysctl parameter '%s' to '%s': %s" - % (option, value, str(e))) + log.error("Refusing to set deprecated sysctl option %s" % option) return False + return self._cmd.write_to_file(path, value, no_error=[errno.ENOENT] if ignore_missing else False) From a7e4ce5e459599f9ec2317c5e3dc7fdd5c6ddb4e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavol=20=C5=BD=C3=A1=C4=8Dik?= Date: Wed, 10 Apr 2024 16:19:22 +0200 Subject: [PATCH 3/5] Add an option to skip `write_to_file` if the content would not change In some situations it may be desirable to check if a file write is necessary by comparing the new file content with the existing one (e.g., when the write always causes an inter-processor interrupt). This commit adds an option to perform such a check in `write_to_file`. If it succeeds, the write is skipped. --- tuned/utils/commands.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tuned/utils/commands.py b/tuned/utils/commands.py index be6451da..94603939 100644 --- a/tuned/utils/commands.py +++ b/tuned/utils/commands.py @@ -90,7 +90,7 @@ def re_lookup(self, d, s, r = None): return list(d.values())[mo.lastindex - 1] return None - def write_to_file(self, f, data, makedir = False, no_error = False): + def write_to_file(self, f, data, makedir = False, no_error = False, ignore_same = False): """Write data to a file. Parameters: @@ -98,6 +98,7 @@ def write_to_file(self, f, data, makedir = False, no_error = False): data -- data to write makedir -- if True and the path doesn't exist, it will be created no_error -- if True errors are silenced, it can be also list of ignored errnos + ignore_same -- if True and the write would not change the file, it is skipped Return: bool -- True on success @@ -110,6 +111,9 @@ def write_to_file(self, f, data, makedir = False, no_error = False): try: if makedir: os.makedirs(d) + if ignore_same and self.read_file(f, no_error=True).strip() == str(data): + self._debug("Skipping the write to file '%s', the content would not change" % f) + return True fd = open(f, "w") fd.write(str(data)) fd.close() From ec85884a04109788c01bed7940fddc0d82d8ca0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavol=20=C5=BD=C3=A1=C4=8Dik?= Date: Wed, 10 Apr 2024 16:26:54 +0200 Subject: [PATCH 4/5] Check that writes are necessary if they may cause redundant IPIs Resolves: RHEL-25613 --- tuned/plugins/plugin_cpu.py | 8 ++++---- tuned/plugins/plugin_sysctl.py | 2 +- tuned/plugins/plugin_sysfs.py | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/tuned/plugins/plugin_cpu.py b/tuned/plugins/plugin_cpu.py index 9475854f..98ea6647 100644 --- a/tuned/plugins/plugin_cpu.py +++ b/tuned/plugins/plugin_cpu.py @@ -359,7 +359,7 @@ def _get_intel_pstate_attr(self, attr): def _set_intel_pstate_attr(self, attr, val): if val is not None: - self._cmd.write_to_file("/sys/devices/system/cpu/intel_pstate/%s" % attr, val) + self._cmd.write_to_file("/sys/devices/system/cpu/intel_pstate/%s" % attr, val, ignore_same=True) def _getset_intel_pstate_attr(self, attr, value): if value is None: @@ -524,7 +524,7 @@ def _set_governor(self, governors, device, sim, remove): log.info("setting governor '%s' on cpu '%s'" % (governor, device)) self._cmd.write_to_file("/sys/devices/system/cpu/%s/cpufreq/scaling_governor" - % device, governor, no_error = [errno.ENOENT] if remove else False) + % device, governor, no_error = [errno.ENOENT] if remove else False, ignore_same=True) break elif not sim: log.debug("Ignoring governor '%s' on cpu '%s', it is not supported" @@ -621,7 +621,7 @@ def _set_energy_perf_bias(self, energy_perf_bias, device, sim, remove): for val in vals: val = val.strip() if self._cmd.write_to_file(energy_perf_bias_path, val, \ - no_error = [errno.ENOENT] if remove else False): + no_error = [errno.ENOENT] if remove else False, ignore_same=True): log.info("energy_perf_bias successfully set to '%s' on cpu '%s'" % (val, device)) break @@ -756,7 +756,7 @@ def _set_energy_performance_preference(self, energy_performance_preference, devi for val in vals: if val in avail_vals: self._cmd.write_to_file(self._pstate_preference_path(cpu_id), val, \ - no_error = [errno.ENOENT] if remove else False) + no_error = [errno.ENOENT] if remove else False, ignore_same=True) log.info("Setting energy_performance_preference value '%s' for cpu '%s'" % (val, device)) break else: diff --git a/tuned/plugins/plugin_sysctl.py b/tuned/plugins/plugin_sysctl.py index 5f54857b..3947d275 100644 --- a/tuned/plugins/plugin_sysctl.py +++ b/tuned/plugins/plugin_sysctl.py @@ -180,4 +180,4 @@ def _write_sysctl(self, option, value, ignore_missing = False): if os.path.basename(path) in DEPRECATED_SYSCTL_OPTIONS: log.error("Refusing to set deprecated sysctl option %s" % option) return False - return self._cmd.write_to_file(path, value, no_error=[errno.ENOENT] if ignore_missing else False) + return self._cmd.write_to_file(path, value, no_error=[errno.ENOENT] if ignore_missing else False, ignore_same=True) diff --git a/tuned/plugins/plugin_sysfs.py b/tuned/plugins/plugin_sysfs.py index 4f14e2ad..e405c123 100644 --- a/tuned/plugins/plugin_sysfs.py +++ b/tuned/plugins/plugin_sysfs.py @@ -87,4 +87,4 @@ def _read_sysfs(self, sysfs_file): return None def _write_sysfs(self, sysfs_file, value): - return self._cmd.write_to_file(sysfs_file, value) + return self._cmd.write_to_file(sysfs_file, value, ignore_same=True) From 167c6f50c2232f157511a59e508f0ec69d77cc9f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavol=20=C5=BD=C3=A1=C4=8Dik?= Date: Tue, 28 May 2024 12:57:26 +0200 Subject: [PATCH 5/5] Do not check for x86_energy_perf_policy if it won't be used If the CPU supports hwp_epp, we should always be able to access EPB via its sysfs knob, so we don't have to check if we have x86_energy_perf_policy because we don't need it to manipulate EPB. --- tuned/plugins/plugin_cpu.py | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/tuned/plugins/plugin_cpu.py b/tuned/plugins/plugin_cpu.py index 98ea6647..e282e3d4 100644 --- a/tuned/plugins/plugin_cpu.py +++ b/tuned/plugins/plugin_cpu.py @@ -196,7 +196,8 @@ def __init__(self, *args, **kwargs): self._is_x86 = False self._is_intel = False self._is_amd = False - self._has_energy_perf_bias = False + self._has_hwp_epp = False + self._has_energy_perf_policy_and_bias = False self._has_intel_pstate = False self._has_amd_pstate = False self._has_pm_qos_resume_latency_us = None @@ -259,9 +260,13 @@ def _check_arch(self): else: log.info("We are running on %s (non x86)" % self._arch) + self._has_hwp_epp = consts.CFG_CPU_EPP_FLAG in self._get_cpuinfo_flags() + if self._is_intel: - # Check for x86_energy_perf_policy, ignore if not available / supported - self._check_energy_perf_bias() + # When hwp_epp is not supported, we check for EPB via x86_energy_perf_policy. + # When it is supported, EPB should be accessible via sysfs. + if not self._has_hwp_epp: + self._check_energy_perf_policy_and_bias() # Check for intel_pstate self._check_intel_pstate() @@ -269,15 +274,15 @@ def _check_arch(self): # Check for amd-pstate self._check_amd_pstate() - def _check_energy_perf_bias(self): - self._has_energy_perf_bias = False + def _check_energy_perf_policy_and_bias(self): + """Check for EPB via x86_energy_perf_policy, warn if the tool is not available or EPB unsupported.""" retcode_unsupported = 1 retcode, out = self._cmd.execute(["x86_energy_perf_policy", "-r"], no_errors = [errno.ENOENT, retcode_unsupported]) # With recent versions of the tool, a zero exit code is # returned even if EPB is not supported. The output is empty # in that case, however. if retcode == 0 and out != "": - self._has_energy_perf_bias = True + self._has_energy_perf_policy_and_bias = True elif retcode < 0: log.warning("unable to run x86_energy_perf_policy tool, ignoring CPU energy performance bias, is the tool installed?") else: @@ -614,7 +619,7 @@ def _set_energy_perf_bias(self, energy_perf_bias, device, sim, remove): # It should be writen straight to sysfs energy_perf_bias file if requested on newer processors # see rhbz#2095829 - if consts.CFG_CPU_EPP_FLAG in self._get_cpuinfo_flags(): + if self._has_hwp_epp: energy_perf_bias_path = self._energy_perf_bias_path(cpu_id) if os.path.exists(energy_perf_bias_path): if not sim: @@ -634,7 +639,7 @@ def _set_energy_perf_bias(self, energy_perf_bias, device, sim, remove): log.error("Failed to set energy_perf_bias on cpu '%s' because energy_perf_bias file does not exist." % device) return None - elif self._has_energy_perf_bias: + elif self._has_energy_perf_policy_and_bias: if not sim: for val in vals: val = val.strip() @@ -690,11 +695,11 @@ def _get_energy_perf_bias(self, device, ignore_missing=False): log.debug("%s is not online, skipping" % device) return None cpu_id = device.lstrip("cpu") - if consts.CFG_CPU_EPP_FLAG in self._get_cpuinfo_flags(): + if self._has_hwp_epp: energy_perf_bias_path = self._energy_perf_bias_path(cpu_id) if os.path.exists(energy_perf_bias_path): energy_perf_bias = self._energy_perf_policy_to_human_v2(self._cmd.read_file(energy_perf_bias_path)) - elif self._has_energy_perf_bias: + elif self._has_energy_perf_policy_and_bias: retcode, lines = self._cmd.execute(["x86_energy_perf_policy", "-c", cpu_id, "-r"]) if retcode == 0: for line in lines.splitlines():