[PATCH 1/3] powertop: Use cpufreqlib for getting cpufreqstats info

[PATCH 1/3] powertop: Use cpufreqlib for getting cpufreqstats info

[Thomas Renninger]

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.


Signed-off-by: Thomas Renninger <trenn@suse.de>
CC: Amit Arora <amit.arora@linaro.org>
CC: auke@linux.intel.com
CC: power@bughost.org
CC: cpufreq@vger.kernel.org
---
 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

Re: [PATCH 1/3] powertop: Use cpufreqlib for getting cpufreqstats info

[Amit Arora]

Hello Thomas,

On Wed, Aug 25, 2010 at 7:53 PM, Thomas Renninger <trenn@suse.de> 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@suse.de>
> CC: Amit Arora <amit.arora@linaro.org>
> CC: auke@linux.intel.com
> CC: power@bughost.org
> CC: cpufreq@vger.kernel.org
> ---
>  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
>
>

Re: [PATCH 1/3] powertop: Use cpufreqlib for getting cpufreqstats info

[Thomas Renninger]

On Thursday 26 August 2010 09:14:35 Amit Arora wrote:
> Hello Thomas,
> 
> On Wed, Aug 25, 2010 at 7:53 PM, Thomas Renninger <trenn@suse.de>
> wrote:
..
> But, if you are suggesting this as alternate to my previous post
> (titled "Make PowerTOP generic"), then it doesn't meet the purpose.
No, I am not suggesting this as a replacement of your code.
Only overlap is to make max frequencies dynamic.
And because the code overlaps/conflicts it made sense
to send this as part of this thread.

But I can rebase my patch(es) if your stuff is figured out, no problem.

    Thomas

Re: [PATCH 1/3] powertop: Use cpufreqlib for getting cpufreqstats info

[Amit Arora]

On Thu, Aug 26, 2010 at 4:28 PM, Thomas Renninger <trenn@suse.de> wrote:
> On Thursday 26 August 2010 09:14:35 Amit Arora wrote:
>> Hello Thomas,
>>
>> On Wed, Aug 25, 2010 at 7:53 PM, Thomas Renninger <trenn@suse.de>
>> wrote:
> ..
>> But, if you are suggesting this as alternate to my previous post
>> (titled "Make PowerTOP generic"), then it doesn't meet the purpose.
> No, I am not suggesting this as a replacement of your code.
> Only overlap is to make max frequencies dynamic.
> And because the code overlaps/conflicts it made sense
> to send this as part of this thread.

OK.

> But I can rebase my patch(es) if your stuff is figured out, no problem.

No issues. Either way is fine. I can also rebase my patch, if it gets
accepted later. Its not huge change, anyways. :)

Thanks!
Regards,
Amit Arora

>    Thomas
>