[RESEND PATCH] powertop: fix erroneous C state detection on ARM
by Lorenzo Pieralisi
When the kernel is configured with CONFIG_CPU_IDLE_MULTIPLE_DRIVERS,
the cpuidle directory contains a "driver" subdirectory that confuses
the current code parsing the C states on ARM platforms, ending up
with a C state entry corresponding to the actual driver name instead
of a proper C state entry.
This patch fixes the code by stopping the parsing if the files that
characterise the C states (ie usage) are not found in the respective:
/sys/devices/system/cpu/cpuX/cpuidle
subdirectory containing C states information.
Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi(a)arm.com>
Tested-by: Kevin Hilman <khilman(a)linaro.org>
---
Alexandra, Arjan,
previous posting here:
https://lists.01.org/pipermail/powertop/2014-December/001724.html
Please consider pulling this patch.
Thanks,
Lorenzo
src/cpu/cpu_linux.cpp | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/cpu/cpu_linux.cpp b/src/cpu/cpu_linux.cpp
index e1ff165..78fd2d3 100644
--- a/src/cpu/cpu_linux.cpp
+++ b/src/cpu/cpu_linux.cpp
@@ -83,7 +83,8 @@ void cpu_linux::parse_cstates_start(void)
if (file) {
file >> usage;
file.close();
- }
+ } else
+ continue;
sprintf(filename + len, "/%s/time", entry->d_name);
@@ -172,7 +173,8 @@ void cpu_linux::parse_cstates_end(void)
if (file) {
file >> usage;
file.close();
- }
+ } else
+ continue;
sprintf(filename + len, "/%s/time", entry->d_name);
--
1.9.1
7 years
[Patches] snprintf, PATH_MAX + improvement of handling reporting filenames
by Jaroslav Skarvada
Hi,
I have noticed there are some sprintf that may lead to buffer
overflows, e.g. the following can crash powertop:
# powertop --csv=`printf 'a%.0s' {1..5000}`
I attempted to fix hopefully most of them by converting
the code to snprintf. For data returned by kernel
it's probably unlikely to cause overflows, but why
not cover it all. I also tried to unify buffer
sizes for paths/filenames to PATH_MAX.
The second patch tries to improve handling of reporting
filenames. It works the following way:
powertop --html
generates 'powertop.html' file
powertop --html=myfile.suffix
generates 'myfile.suffix' file
powertop -i 2 --html
generates 'powertop-TIMESTAMPS.html' files
powertop -i 2 --html=myfile.suffix
generates 'myfile-TIMESTAMPS.suffix' files
powertop -i 2 --html=myfile
generates 'myfile-TIMESTAMPS' files
Similarly for CSV.
I think this is more logical behavior
regards
Jaroslav
7 years
[PATCH] Do not exit on MSR read errors
by Jaroslav Škarvada
There are virtual environments not correctly implementing the MSR (e.g.
VirtualBox bug: https://www.virtualbox.org/ticket/14099) or not implementing
it at all (e.g. KVM). PowerTOP just exits in such environments and
cannot run.
The proposed patch makes PowerTOP not to exit on MSR read errors, but still
report them.
Signed-off-by: Jaroslav Škarvada <jskarvad(a)redhat.com>
---
src/cpu/intel_cpus.cpp | 103 +++++++++++++++++++++++++++++--------------------
src/cpu/intel_cpus.h | 1 +
2 files changed, 62 insertions(+), 42 deletions(-)
diff --git a/src/cpu/intel_cpus.cpp b/src/cpu/intel_cpus.cpp
index 1f3647a..69fdc84 100644
--- a/src/cpu/intel_cpus.cpp
+++ b/src/cpu/intel_cpus.cpp
@@ -79,20 +79,20 @@ int is_supported_intel_cpu(int model)
return 0;
}
-static uint64_t get_msr(int cpu, uint64_t offset)
+static int get_msr(int cpu, uint64_t offset, uint64_t *msr)
{
ssize_t retval;
- uint64_t msr;
- retval = read_msr(cpu, offset, &msr);
+ retval = read_msr(cpu, offset, msr);
if (retval < 0) {
reset_display();
fprintf(stderr, _("read_msr cpu%d 0x%llx : "), cpu, (unsigned long long)offset);
fprintf(stderr, "%s\n", strerror(errno));
- exit(-2);
+ *msr = 0;
+ return 0;
}
- return msr;
+ return 1;
}
intel_util::intel_util()
@@ -152,13 +152,18 @@ void nhm_core::measurement_start(void)
last_stamp = 0;
if (this->has_c1_res)
- c1_before = get_msr(first_cpu, MSR_CORE_C1_RESIDENCY);
+ if (!get_msr(first_cpu, MSR_CORE_C1_RESIDENCY, &c1_before))
+ this->has_c1_res = 0;
if (this->has_c3_res)
- c3_before = get_msr(first_cpu, MSR_CORE_C3_RESIDENCY);
- c6_before = get_msr(first_cpu, MSR_CORE_C6_RESIDENCY);
+ if (!get_msr(first_cpu, MSR_CORE_C3_RESIDENCY, &c3_before))
+ this->has_c3_res = 0;
+ if (this->has_c6_res)
+ if (!get_msr(first_cpu, MSR_CORE_C6_RESIDENCY, &c6_before))
+ this->has_c6_res = 0;
if (this->has_c7_res)
- c7_before = get_msr(first_cpu, MSR_CORE_C7_RESIDENCY);
- tsc_before = get_msr(first_cpu, MSR_TSC);
+ if (!get_msr(first_cpu, MSR_CORE_C7_RESIDENCY, &c7_before))
+ this->has_c7_res = 0;
+ get_msr(first_cpu, MSR_TSC, &tsc_before);
if (this->has_c1_res)
insert_cstate("core c1", "C1 (cc1)", 0, c1_before, 1);
@@ -196,13 +201,18 @@ void nhm_core::measurement_end(void)
double ratio;
if (this->has_c1_res)
- c1_after = get_msr(first_cpu, MSR_CORE_C1_RESIDENCY);
+ if (!get_msr(first_cpu, MSR_CORE_C1_RESIDENCY, &c1_after))
+ this->has_c1_res = 0;
if (this->has_c3_res)
- c3_after = get_msr(first_cpu, MSR_CORE_C3_RESIDENCY);
- c6_after = get_msr(first_cpu, MSR_CORE_C6_RESIDENCY);
+ if (!get_msr(first_cpu, MSR_CORE_C3_RESIDENCY, &c3_after))
+ this->has_c3_res = 0;
+ if (this->has_c6_res)
+ if (!get_msr(first_cpu, MSR_CORE_C6_RESIDENCY, &c6_after))
+ this->has_c6_res = 0;
if (this->has_c7_res)
- c7_after = get_msr(first_cpu, MSR_CORE_C7_RESIDENCY);
- tsc_after = get_msr(first_cpu, MSR_TSC);
+ if (!get_msr(first_cpu, MSR_CORE_C7_RESIDENCY, &c7_after))
+ this->has_c7_res = 0;
+ get_msr(first_cpu, MSR_TSC, &tsc_after);
if (this->has_c1_res)
finalize_cstate("core c1", 0, c1_after, 1);
@@ -360,27 +370,31 @@ void nhm_package::measurement_start(void)
last_stamp = 0;
if (this->has_c2c6_res)
- c2_before = get_msr(number, MSR_PKG_C2_RESIDENCY);
+ if (!get_msr(number, MSR_PKG_C2_RESIDENCY, &c2_before))
+ this->has_c2c6_res = 0;
if (this->has_c3_res)
- c3_before = get_msr(number, MSR_PKG_C3_RESIDENCY);
+ if (!get_msr(number, MSR_PKG_C3_RESIDENCY, &c3_before))
+ this->has_c3_res = 0;
/*
* Hack for Braswell where C7 MSR is actually BSW C6
*/
if (this->has_c6c_res)
- c6_before = get_msr(number, MSR_PKG_C7_RESIDENCY);
+ get_msr(number, MSR_PKG_C7_RESIDENCY, &c6_before);
else
- c6_before = get_msr(number, MSR_PKG_C6_RESIDENCY);
+ get_msr(number, MSR_PKG_C6_RESIDENCY, &c6_before);
if (this->has_c7_res)
- c7_before = get_msr(number, MSR_PKG_C7_RESIDENCY);
+ if (!get_msr(number, MSR_PKG_C7_RESIDENCY, &c7_before))
+ this->has_c7_res = 0;
if (this->has_c8c9c10_res) {
- c8_before = get_msr(number, MSR_PKG_C8_RESIDENCY);
- c9_before = get_msr(number, MSR_PKG_C9_RESIDENCY);
- c10_before = get_msr(number, MSR_PKG_C10_RESIDENCY);
+ if (!get_msr(number, MSR_PKG_C8_RESIDENCY, &c8_before) ||
+ !get_msr(number, MSR_PKG_C9_RESIDENCY, &c9_before) ||
+ !get_msr(number, MSR_PKG_C10_RESIDENCY, &c10_before))
+ this->has_c8c9c10_res = 0;
}
- tsc_before = get_msr(first_cpu, MSR_TSC);
+ get_msr(first_cpu, MSR_TSC, &tsc_before);
if (this->has_c2c6_res)
insert_cstate("pkg c2", "C2 (pc2)", 0, c2_before, 1);
@@ -409,24 +423,28 @@ void nhm_package::measurement_end(void)
if (this->has_c2c6_res)
- c2_after = get_msr(number, MSR_PKG_C2_RESIDENCY);
+ if (!get_msr(number, MSR_PKG_C2_RESIDENCY, &c2_after))
+ this->has_c2c6_res = 0;
if (this->has_c3_res)
- c3_after = get_msr(number, MSR_PKG_C3_RESIDENCY);
+ if (!get_msr(number, MSR_PKG_C3_RESIDENCY, &c3_after))
+ this->has_c3_res = 0;
if (this->has_c6c_res)
- c6_after = get_msr(number, MSR_PKG_C7_RESIDENCY);
+ get_msr(number, MSR_PKG_C7_RESIDENCY, &c6_after);
else
- c6_after = get_msr(number, MSR_PKG_C6_RESIDENCY);
+ get_msr(number, MSR_PKG_C6_RESIDENCY, &c6_after);
if (this->has_c7_res)
- c7_after = get_msr(number, MSR_PKG_C7_RESIDENCY);
- if (has_c8c9c10_res) {
- c8_after = get_msr(number, MSR_PKG_C8_RESIDENCY);
- c9_after = get_msr(number, MSR_PKG_C9_RESIDENCY);
- c10_after = get_msr(number, MSR_PKG_C10_RESIDENCY);
+ if (!get_msr(number, MSR_PKG_C7_RESIDENCY, &c7_after))
+ this->has_c7_res = 0;
+ if (this->has_c8c9c10_res) {
+ if (!get_msr(number, MSR_PKG_C8_RESIDENCY, &c8_after) ||
+ !get_msr(number, MSR_PKG_C9_RESIDENCY, &c9_after) ||
+ !get_msr(number, MSR_PKG_C10_RESIDENCY, &c10_after))
+ this->has_c8c9c10_res = 0;
}
- tsc_after = get_msr(first_cpu, MSR_TSC);
+ get_msr(first_cpu, MSR_TSC, &tsc_after);
gettimeofday(&stamp_after, NULL);
@@ -441,7 +459,7 @@ void nhm_package::measurement_end(void)
finalize_cstate("pkg c6", 0, c6_after, 1);
if (this->has_c7_res)
finalize_cstate("pkg c7", 0, c7_after, 1);
- if (has_c8c9c10_res) {
+ if (this->has_c8c9c10_res) {
finalize_cstate("pkg c8", 0, c8_after, 1);
finalize_cstate("pkg c9", 0, c9_after, 1);
finalize_cstate("pkg c10", 0, c10_after, 1);
@@ -497,9 +515,9 @@ void nhm_cpu::measurement_start(void)
last_stamp = 0;
- aperf_before = get_msr(number, MSR_APERF);
- mperf_before = get_msr(number, MSR_MPERF);
- tsc_before = get_msr(number, MSR_TSC);
+ get_msr(number, MSR_APERF, &aperf_before);
+ get_msr(number, MSR_MPERF, &mperf_before);
+ get_msr(number, MSR_TSC, &tsc_before);
insert_cstate("active", _("C0 active"), 0, aperf_before, 1);
@@ -527,9 +545,9 @@ void nhm_cpu::measurement_end(void)
double ratio;
unsigned int i;
- aperf_after = get_msr(number, MSR_APERF);
- mperf_after = get_msr(number, MSR_MPERF);
- tsc_after = get_msr(number, MSR_TSC);
+ get_msr(number, MSR_APERF, &aperf_after);
+ get_msr(number, MSR_MPERF, &mperf_after);
+ get_msr(number, MSR_TSC, &tsc_after);
@@ -582,7 +600,8 @@ char * nhm_cpu::fill_pstate_line(int line_nr, char *buffer)
if (line_nr == LEVEL_C0) {
double F;
- F = 1.0 * (tsc_after - tsc_before) * (aperf_after - aperf_before) / (mperf_after - mperf_before) / time_factor * 1000;
+ F = mperf_after - mperf_before;
+ F = 1.0 * (tsc_after - tsc_before) * (aperf_after - aperf_before) / (F ? F : 1) / time_factor * 1000;
hz_to_human(F, buffer, 1);
return buffer;
}
diff --git a/src/cpu/intel_cpus.h b/src/cpu/intel_cpus.h
index 0331069..e909018 100644
--- a/src/cpu/intel_cpus.h
+++ b/src/cpu/intel_cpus.h
@@ -100,6 +100,7 @@ private:
uint64_t total_stamp;
public:
int has_c1_res;
+ int has_c6_res = 1;
int has_c7_res;
int has_c3_res;
nhm_core(int model);
--
2.1.0
7 years
[RFC 0/9] Use udev for accessing sysfs
by Nanley Chery
From: Nanley Chery <nanley.g.chery(a)intel.com>
This patch series changes PowerTOP's method of accessing sysfs (for
tunables mainly) from explicitly accessing files to using the standard
udev device management library and API.
The current codebase collects data on devices by probing sysfs files
directly with the usual low-level file handling API. This method is
not recommended by the Linux kernel maintainers because the sysfs
organization is not guaranteed to be stable over time. This can lead
to dependent applications breaking with newer kernel releases. Instead
using an abstraction layer, like udev, is suggested
(see https://www.kernel.org/doc/Documentation/sysfs-rules.txt).
The benefits of switching to udev include:
* The removal of libpci and a reduction in repetitive code.
* Improved readability of the tunable code.
* Faster reading of device state values due to caching provided by udev.
I've tried to make this transition as simple as possible by including
the bare minimum of changes required for this feature. There's at least
one more tunable that can be converted (i2c), but I would like to get
feedback on the current implementation first.
Nanley Chery (9):
build with udev library
add udev helper functions
usb_tunable: use udev
syfs_tunable: use udev
runtime_tunable: use udev
replace pciutils usage with udevhwdb
runtime_pm: use udev
remove libpci and most dead code
calibrate: use udev
Android.mk | 1 -
README | 1 -
configure.ac | 20 ++-----
src/Makefile.am | 6 +-
src/calibrate/calibrate.cpp | 44 ++++----------
src/devices/runtime_pm.cpp | 105 +++++++++++---------------------
src/devices/runtime_pm.h | 1 +
src/lib.cpp | 100 +++++++++++++++++++++----------
src/lib.h | 7 ++-
src/main.cpp | 4 +-
src/tuning/runtime.cpp | 139 +++++++++++++------------------------------
src/tuning/runtime.h | 6 +-
src/tuning/tuningsysfs.cpp | 12 +++-
src/tuning/tuningsysfs.h | 1 +
src/tuning/tuningusb.cpp | 142 ++++++++++++++------------------------------
src/tuning/tuningusb.h | 5 +-
16 files changed, 231 insertions(+), 363 deletions(-)
--
2.4.1
7 years, 3 months
Why on some platform or OS, Powertop show only "idle 100%"s in the frequency page?
by Wang, Xiaolong
Hi PowerTop developers,
I've been testing PowerTop on various OS and platforms, and found on some platforms or OS, Powertop show only "idle 100%"s in the frequency page, no other frequencies and their percentage.
For example, currently I'm test RHEL6,
on some new client platforms, which intel_pstate is not supported yet, the frequencies and their percentage could be listed below idle percentage.
but on the mature platforms, where intel_pstate is governing, I could only get "idle 100%" under each Package/Core/CPU.
(well, the "Actual" frequency under each "CPU" looks normal)
I tried Powertop 2.3 and 2.7, and had guessed it should be intel_pstate not providing some necessary info to Powertop.
But it's not the case on my i5-5300U machine, it is governed by intel_pstate, but shows frequencies well.
I tried this with Powertop 2.6.1 and 2.7.
Could you help to explain, why Powertop show only "idle 100%"s in the frequency page?
Thanks~
The following is an example, captured from a Haswell-EP server:
PowerTOP 2.7 Overview Idle stats Frequency stats Device stats Tunables
Package | Core | CPU 0 CPU 4
| | Actual 3.2 GHz 1.9 GHz
Idle 100.0% | Idle 100.0% | Idle 100.0% 100.0%
| s 9 Core | CPU 1 CPU 5
| | Actual 3.4 GHz 2.3 GHz
| Idle 100.0% | Idle 100.0% 100.0%
| Core | CPU 2 CPU 6
| | Actual 2.0 GHz 2.3 GHz
| Idle 100.0% | Idle 100.0% 100.0%
| s Core | CPU 3 CPU 7
| s | Actual 2.3 GHz 2.6 GHz
| Idle 100.0% | Idle 100.0% 100.0%
-Wang, Xiaolong
7 years, 3 months