Hello Thomas,
On Wed, Aug 25, 2010 at 7:53 PM, Thomas Renninger <trenn@xxxxxxx> wrote:
> Sharing code is a good idea.
> cpufrequtils/libs exist for years and every distribution
> should have them. It's also well maintained and ideally is
> the only userspace tool which needs adjusting if cpufreq
> sysfs api changes at some time.
I agree that it probably makes sense to use the libcpufreq to get
cpufreq information, and don't have any issue with this.
But, if you are suggesting this as alternate to my previous post
(titled "Make PowerTOP generic"), then it doesn't meet the purpose.
The C and P states in powertop still remain hard coded and it doesn't
work well on some of the ARM SoCs (even with this patch applied).
I will probably share some output from one of the ARM boards (OMAP3),
with your patch applied and then with my patch applied on top. This
should help clarify the issue I wanted to target.
PowerTOP output (with and without your below patch applied) :
==========================================
>>>>>>
PowerTOP version 1.13 (C) 2007 Intel Corporation
Cn Avg residency
C0 (cpu running) ( 0.0%)
C0 199.2ms (100.0%)
C1 0.0ms ( 0.0%)
C2 0.0ms ( 0.0%)
C3 0.0ms ( 0.0%)
C4 0.0ms ( 0.0%)
Wakeups-from-idle per second : 5.0 interval: 5.0s
<<<<<<
PowerTOP output with my patch applied on top :
====================================
>>>>>>
PowerTOP version 1.13 (C) 2007 Intel Corporation
Cn Avg residency P-states (frequencies)
C0 (cpu running) ( 0.0%)
C0 226.3ms (100.0%)
C1 0.0ms ( 0.0%)
C2 0.0ms ( 0.0%)
C3 0.0ms ( 0.0%)
C4 0.0ms ( 0.0%)
C5 0.0ms ( 0.0%)
C6 0.0ms ( 0.0%)
Wakeups-from-idle per second : 4.4 interval: 5.0s
<<<<<<
Please observe the number of C states displayed in the two sets of output above.
Thanks!
Regards,
Amit Arora
> Signed-off-by: Thomas Renninger <trenn@xxxxxxx>
> CC: Amit Arora <amit.arora@xxxxxxxxxx>
> CC: auke@xxxxxxxxxxxxxxx
> CC: power@xxxxxxxxxxx
> CC: cpufreq@xxxxxxxxxxxxxxx
> ---
> Makefile | 1 +
> cpufreqstats.c | 52 ++++++++++++++++++++--------------------------------
> 2 files changed, 21 insertions(+), 32 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index e4e1671..ee10a84 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -6,6 +6,7 @@ MANDIR=/usr/share/man/man8
> WARNFLAGS=-Wall -Wshadow -W -Wformat -Wimplicit-function-declaration -Wimplicit-int
> CFLAGS?=-O1 -g ${WARNFLAGS}
> CC?=gcc
> +LDFLAGS = -lcpufreq
>
> CFLAGS+=-D VERSION=\"$(VERSION)\"
>
> diff --git a/cpufreqstats.c b/cpufreqstats.c
> index d10b047..303c3cf 100644
> --- a/cpufreqstats.c
> +++ b/cpufreqstats.c
> @@ -29,6 +29,7 @@
> #include <stdint.h>
> #include <sys/types.h>
> #include <dirent.h>
> +#include <cpufreq.h>
>
> #include "powertop.h"
>
> @@ -100,15 +101,11 @@ static char *HzToHuman(unsigned long hz)
>
> void do_cpufreq_stats(void)
> {
> - DIR *dir;
> - struct dirent *dirent;
> - FILE *file;
> - char filename[PATH_MAX];
> - char line[1024];
> -
> - int ret = 0;
> + struct cpufreq_stats *freq_stats, *tmp;
> + int cpu = 0, ret = 0;
> int maxfreq = 0;
> uint64_t total_time = 0;
> + unsigned long long time_dummy = 0;
>
> memcpy(&oldfreqs, &freqs, sizeof(freqs));
> memset(&cpufreqstrings, 0, sizeof(cpufreqstrings));
> @@ -117,30 +114,21 @@ void do_cpufreq_stats(void)
> for (ret = 0; ret<16; ret++)
> freqs[ret].count = 0;
>
> - dir = opendir("/sys/devices/system/cpu");
> - if (!dir)
> + freq_stats = cpufreq_get_stats(0, &time_dummy);
> + if (!freq_stats)
> + /* Probably cpufreq_stats not compiled in or no cpufreq
> + support, printing a debug message is appropriate */
> return;
>
> - while ((dirent = readdir(dir))) {
> - int i;
> - if (dirent->d_name[0]=='.')
> - continue;
> - sprintf(filename, "/sys/devices/system/cpu/%s/cpufreq/stats/time_in_state", dirent->d_name);
> - file = fopen(filename, "r");
> - if (!file)
> - continue;
> - memset(line, 0, 1024);
> -
> - i = 0;
> - while (!feof(file)) {
> - uint64_t f,count;
> - char *c;
> - if (fgets(line, 1023,file)==NULL)
> - break;
> - f = strtoull(line, &c, 10);
> - if (!c)
> - break;
> - count = strtoull(c, NULL, 10);
> + /* cpu loop */
> + while (freq_stats) {
> + int i = 0;
> + tmp = freq_stats;
> + /* freq loop */
> + for (;freq_stats; freq_stats = freq_stats->next) {
> +
> + unsigned long f = freq_stats->frequency;
> + unsigned long long count = freq_stats->time_in_state;
>
> if (freqs[i].frequency && freqs[i].frequency != f) {
> zap();
> @@ -156,11 +144,11 @@ void do_cpufreq_stats(void)
> if (i>15)
> break;
> }
> - fclose(file);
> + cpu++;
> + cpufreq_put_stats(tmp);
> + freq_stats = cpufreq_get_stats(cpu, &time_dummy);
> }
>
> - closedir(dir);
> -
> for (ret = 0; ret < 16; ret++) {
> delta[ret].count = freqs[ret].count - oldfreqs[ret].count;
> total_time += delta[ret].count;
> --
> 1.6.4.2
>
>
--
To unsubscribe from this list: send the line "unsubscribe cpufreq" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
