"Pallipadi, Venkatesh" <[EMAIL PROTECTED]> writes:
> Nice timing for the patch. I am working on cpuidle patches that will
> add things like C1 idle residency time in /sysfs and was asking
> Arjan about switching powertop to use cpuidle interfaces :-).
Great! My platform reports residency for all the states in sysfs
already, so powertop using sysfs is very useful to me.
> One major comment is that with the patch C-state timing in powertop
> seems to be off, Due to different units of time in /proc/acpi and
> sysfs cpuidle. /proc/acpi C-state timings are in ACPI PM timer
> ticks and cpuidle exports it in micro-seconds.
Yes, the percentage numbers are indeed off. I was just noticing the
same problem. I'm working right now on changing those calculations to
use usecs instead of ticks.
I assume the hardcoded frequency in powertop (3579.545) is the
frequency of the ACPI timer? I'm pretty ignorant of ACPI, but am
guessing that's what those calculations are for.
One other problem I just discoverd is that cpuidle currently stores
the cumulative residency time in an unsigned long. Since this is
tracking usecs, it should probably be a unsigned long long for longer
running systems (patch attached)
Kevin
>>-----Original Message-----
>>From: Kevin Hilman [mailto:[EMAIL PROTECTED]
>>Sent: Thursday, January 31, 2008 10:31 AM
>>To: [email protected]
>>Cc: Adam Belay; Pallipadi, Venkatesh
>>Subject: [PATCH/RFC] powertop non-ACPI: use CPUidle for C-state details
>>
>>[ resend to [email protected] instead of [EMAIL PROTECTED] ]
>>
>>On systems without ACPI, query the CPUidle sysfs interface for C-state
>>usage/duration details. It uses the CPUIdle stateN/usage and
>>stateN/time files for the 'usage' and 'duration' values used for
>>calculations in powertop.
>>
>>For now, if both ACPI and CPUidle are present, it will read data from
>>the ACPI /proc interface instead of CPUidle, but if ACPI is not
>>present, it will fallback to CPUidle.
>>
>>Tested against a CPUidle enabled 2.6.24 non-x86 kernel (TI OMAP3.)
>>
>>Patch against powertop-1.9.
>>
>>Signed-off-by: Kevin Hilman <[EMAIL PROTECTED]>
>>
>>diff -ruN powertop-1.9/powertop.c powertop-1.9-KJH/powertop.c
>>--- powertop-1.9/powertop.c 2008-01-30 17:24:27.000000000 -0800
>>+++ powertop-1.9-KJH/powertop.c 2008-01-30
>>21:02:54.000000000 -0800
>>@@ -35,6 +35,7 @@
>> #include <assert.h>
>> #include <locale.h>
>> #include <time.h>
>>+#include <sys/stat.h>
>>
>> #include "powertop.h"
>>
>>@@ -236,7 +237,7 @@
>> fclose(file);
>> }
>>
>>-static void read_data(uint64_t * usage, uint64_t * duration)
>>+static void read_data_acpi(uint64_t * usage, uint64_t * duration)
>> {
>> DIR *dir;
>> struct dirent *entry;
>>@@ -286,6 +287,97 @@
>> closedir(dir);
>> }
>>
>>+static void read_data_cpuidle(uint64_t * usage, uint64_t * duration)
>>+{
>>+ DIR *dir;
>>+ struct dirent *entry;
>>+ FILE *file = NULL;
>>+ char line[4096];
>>+ char filename[128], *f;
>>+ int len, clevel = 0;
>>+
>>+ memset(usage, 0, 64);
>>+ memset(duration, 0, 64);
>>+
>>+ dir = opendir("/sys/devices/system/cpu");
>>+ if (!dir)
>>+ return;
>>+
>>+ /* Loop over cpuN entries */
>>+ while ((entry = readdir(dir))) {
>>+ if (strlen(entry->d_name) < 3)
>>+ continue;
>>+
>>+ if (!isdigit(entry->d_name[3]))
>>+ continue;
>>+
>>+ len = sprintf(filename,
>>"/sys/devices/system/cpu/%s/cpuidle",
>>+ entry->d_name);
>>+
>>+ dir = opendir(filename);
>>+ if (!dir)
>>+ return;
>>+
>>+ /* For each C-state, there is a stateX directory which
>>+ * contains a 'usage' and a 'time' (duration) file */
>>+ while ((entry = readdir(dir))) {
>>+ if (strlen(entry->d_name) < 3)
>>+ continue;
>>+ sprintf(filename + len, "/%s/usage",
>>entry->d_name);
>>+ file = fopen(filename, "r");
>>+ if (!file)
>>+ continue;
>>+
>>+ memset(line, 0, 4096);
>>+ f = fgets(line, 4096, file);
>>+ fclose(file);
>>+ if (f == NULL)
>>+ break;
>>+
>>+ usage[clevel] += 1+strtoull(line, NULL, 10);
>>+
>>+ sprintf(filename + len, "/%s/time",
>>entry->d_name);
>>+ file = fopen(filename, "r");
>>+ if (!file)
>>+ continue;
>>+
>>+ memset(line, 0, 4096);
>>+ f = fgets(line, 4096, file);
>>+ fclose(file);
>>+ if (f == NULL)
>>+ break;
>>+
>>+ duration[clevel] += 1+strtoull(line, NULL, 10);
>>+
>>+ clevel++;
>>+ if (clevel > maxcstate)
>>+ maxcstate = clevel;
>>+
>>+ }
>>+
>>+ }
>>+ closedir(dir);
>>+}
>>+
>>+static void read_data(uint64_t * usage, uint64_t * duration)
>>+{
>>+ int r;
>>+ struct stat s;
>>+
>>+ /* First, check for ACPI */
>>+ r = stat("/proc/acpi/processor", &s);
>>+ if (!r) {
>>+ read_data_acpi(usage, duration);
>>+ return;
>>+ }
>>+
>>+ /* Then check for CPUidle */
>>+ r = stat("/sys/devices/system/cpu/cpuidle", &s);
>>+ if (!r) {
>>+ read_data_cpuidle(usage, duration);
>>+ }
>>+}
>>+
>> void stop_timerstats(void)
>> {
>> FILE *file;
>>@@ -530,7 +622,7 @@
>> memset(&cstate_lines, 0, sizeof(cstate_lines));
>> topcstate = -4;
>> if (totalevents == 0 && maxcstate <= 1) {
>>- sprintf(cstate_lines[5],_("< Detailed
>>C-state information is only available on Mobile CPUs (laptops) >\n"));
>>+ sprintf(cstate_lines[5],_("< Detailed
>>C-state information is not available.>\n"));
>> } else {
>> double sleept, percentage;;
>> c0 = sysconf(_SC_NPROCESSORS_ONLN) *
>>ticktime * 1000 * FREQ - totalticks;
>>
CPUidle: Increase size of cumulative residency time to unsigned long long
Signed-off-by: Kevin Hilman <[EMAIL PROTECTED]>
---
commit aa7647c12180b98e266bd10d82376f705835d160
tree 0fe55db37c7d90804e8a190ec7df5bfd8d309343
parent 0a5233ccff3c8723f5328c33f686ca3c87113898
author Kevin Hilman <[EMAIL PROTECTED]> Thu, 31 Jan 2008 16:05:52 -0800
committer Kevin Hilman <[EMAIL PROTECTED]> Thu, 31 Jan 2008 16:05:52 -0800
drivers/cpuidle/sysfs.c | 6 +++++-
include/linux/cpuidle.h | 2 +-
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c
index 0f3515e..a3e8e22 100644
--- a/drivers/cpuidle/sysfs.c
+++ b/drivers/cpuidle/sysfs.c
@@ -218,6 +218,11 @@ static ssize_t show_state_##_name(struct cpuidle_state *state, char *buf) \
return sprintf(buf, "%u\n", state->_name);\
}
+static ssize_t show_state_time(struct cpuidle_state *state, char *buf)
+{
+ return sprintf(buf, "%llu\n", state->time);
+}
+
static ssize_t show_state_name(struct cpuidle_state *state, char *buf)
{
return sprintf(buf, "%s\n", state->name);
@@ -226,7 +231,6 @@ static ssize_t show_state_name(struct cpuidle_state *state, char *buf)
define_show_state_function(exit_latency)
define_show_state_function(power_usage)
define_show_state_function(usage)
-define_show_state_function(time)
define_one_state_ro(name, show_state_name);
define_one_state_ro(latency, show_state_exit_latency);
define_one_state_ro(power, show_state_power_usage);
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 23f063a..7d30264 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -37,7 +37,7 @@ struct cpuidle_state {
unsigned int target_residency; /* in US */
unsigned int usage;
- unsigned int time; /* in US */
+ unsigned long long time; /* in US */
int (*enter) (struct cpuidle_device *dev,
struct cpuidle_state *state);
_______________________________________________
Power mailing list
[email protected]
http://www.bughost.org/mailman/listinfo/power
