[PATCH 0/6] cpupower tools: Fix minor bugs and warnings

[PATCH 0/6] cpupower tools: Fix minor bugs and warnings

[Palmer Cox]

Patches 1 and 2 of the series fix a simple issues with the Makefiles
and .gitignore file. Patch 3 fixes a couple trivial warnings. Patch
4 fixes some error checking issues. Path 5 fixes a malloc issue. Patch
6 fixes a sign comparison error by updating how the number of cpu packages
are calculated which I believe also fixes an issue that would arrise if the
package id values are non-contiguous on a particular system.

Palmer Cox (6):
      cpupower tools: Remove brace expansion from clean target
      cpupower tools: Update .gitignore for files created in the debug directories
      cpupower tools: Fix minor warnings
      cpupower tools: Fix issues with sysfs_topology_read_file
      cpupower tools: Fix malloc of cpu_info structure
      cpupower tools: Fix warning and a bug with the cpu package count

 tools/power/cpupower/.gitignore                    |    7 +++
 tools/power/cpupower/Makefile                      |    3 +-
 tools/power/cpupower/debug/i386/Makefile           |    5 +-
 tools/power/cpupower/utils/helpers/helpers.h       |   17 ++++---
 tools/power/cpupower/utils/helpers/sysfs.c         |   19 -------
 tools/power/cpupower/utils/helpers/topology.c      |   53 +++++++++++---------
 .../cpupower/utils/idle_monitor/cpupower-monitor.c |    3 +-
 7 files changed, 52 insertions(+), 55 deletions(-)

[PATCH 1/6] cpupower tools: Remove brace expansion from clean target

[Palmer Cox]

The clean targets from the cpupower tools' Makefiles use brace expansion to
remove some generated files. However, the default shells on many systems do
not support this feature resulting in some generated files not being removed
by clean.
---
 tools/power/cpupower/Makefile            |    3 ++-
 tools/power/cpupower/debug/i386/Makefile |    5 ++++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/power/cpupower/Makefile b/tools/power/cpupower/Makefile
index a93e06c..44b7a06 100644
--- a/tools/power/cpupower/Makefile
+++ b/tools/power/cpupower/Makefile
@@ -253,7 +253,8 @@ clean:
 	 | xargs rm -f
 	-rm -f $(OUTPUT)cpupower
 	-rm -f $(OUTPUT)libcpupower.so*
-	-rm -rf $(OUTPUT)po/*.{gmo,pot}
+	-rm -rf $(OUTPUT)po/*.gmo
+	-rm -rf $(OUTPUT)po/*.pot
 	$(MAKE) -C bench O=$(OUTPUT) clean
 
 
diff --git a/tools/power/cpupower/debug/i386/Makefile b/tools/power/cpupower/debug/i386/Makefile
index 3ba158f..c05cc0a 100644
--- a/tools/power/cpupower/debug/i386/Makefile
+++ b/tools/power/cpupower/debug/i386/Makefile
@@ -26,7 +26,10 @@ $(OUTPUT)powernow-k8-decode: powernow-k8-decode.c
 all: $(OUTPUT)centrino-decode $(OUTPUT)dump_psb $(OUTPUT)intel_gsic $(OUTPUT)powernow-k8-decode
 
 clean:
-	rm -rf $(OUTPUT){centrino-decode,dump_psb,intel_gsic,powernow-k8-decode}
+	rm -rf $(OUTPUT)centrino-decode
+	rm -rf $(OUTPUT)dump_psb
+	rm -rf $(OUTPUT)intel_gsic
+	rm -rf $(OUTPUT)powernow-k8-decode
 
 install:
 	$(INSTALL) -d $(DESTDIR)${bindir}
-- 
1.7.9.5

[PATCH 2/6] cpupower tools: Update .gitignore for files created in the debug directories

[Palmer Cox]

The files generated by the Makefiles in the debug directories aren't listed
in the .gitignore file in the root of the cpupower tool which causes these
files to show up in the output of 'git status'.
---
 tools/power/cpupower/.gitignore |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tools/power/cpupower/.gitignore b/tools/power/cpupower/.gitignore
index 8a83dd2..d42073f 100644
--- a/tools/power/cpupower/.gitignore
+++ b/tools/power/cpupower/.gitignore
@@ -20,3 +20,10 @@ utils/cpufreq-set.o
 utils/cpufreq-aperf.o
 cpupower
 bench/cpufreq-bench
+debug/kernel/Module.symvers
+debug/i386/centrino-decode
+debug/i386/dump_psb
+debug/i386/intel_gsic
+debug/i386/powernow-k8-decode
+debug/x86_64/centrino-decode
+debug/x86_64/powernow-k8-decode
-- 
1.7.9.5

[PATCH 3/6] cpupower tools: Fix minor warnings

[Palmer Cox]

Fix minor warnings reported with GCC 4.6:
* The sysfs_write_file function is unused - remove it.
* The pr_mon_len in the print_header function is unsed - remove it.
---
 tools/power/cpupower/utils/helpers/sysfs.c         |   19 -------------------
 .../cpupower/utils/idle_monitor/cpupower-monitor.c |    3 +--
 2 files changed, 1 insertion(+), 21 deletions(-)

diff --git a/tools/power/cpupower/utils/helpers/sysfs.c b/tools/power/cpupower/utils/helpers/sysfs.c
index 96e28c1..38ab916 100644
--- a/tools/power/cpupower/utils/helpers/sysfs.c
+++ b/tools/power/cpupower/utils/helpers/sysfs.c
@@ -37,25 +37,6 @@ unsigned int sysfs_read_file(const char *path, char *buf, size_t buflen)
 	return (unsigned int) numread;
 }
 
-static unsigned int sysfs_write_file(const char *path,
-				     const char *value, size_t len)
-{
-	int fd;
-	ssize_t numwrite;
-
-	fd = open(path, O_WRONLY);
-	if (fd == -1)
-		return 0;
-
-	numwrite = write(fd, value, len);
-	if (numwrite < 1) {
-		close(fd);
-		return 0;
-	}
-	close(fd);
-	return (unsigned int) numwrite;
-}
-
 /*
  * Detect whether a CPU is online
  *
diff --git a/tools/power/cpupower/utils/idle_monitor/cpupower-monitor.c b/tools/power/cpupower/utils/idle_monitor/cpupower-monitor.c
index 0d6571e..7a657f3 100644
--- a/tools/power/cpupower/utils/idle_monitor/cpupower-monitor.c
+++ b/tools/power/cpupower/utils/idle_monitor/cpupower-monitor.c
@@ -84,7 +84,7 @@ int fill_string_with_spaces(char *s, int n)
 void print_header(int topology_depth)
 {
 	int unsigned mon;
-	int state, need_len, pr_mon_len;
+	int state, need_len;
 	cstate_t s;
 	char buf[128] = "";
 	int percent_width = 4;
@@ -93,7 +93,6 @@ void print_header(int topology_depth)
 	printf("%s|", buf);
 
 	for (mon = 0; mon < avail_monitors; mon++) {
-		pr_mon_len = 0;
 		need_len = monitors[mon]->hw_states_num * (percent_width + 3)
 			- 1;
 		if (mon != 0) {
-- 
1.7.9.5

[PATCH 4/6] cpupower tools: Fix issues with sysfs_topology_read_file

[Palmer Cox]

Fix a variety of issues with sysfs_topology_read_file:
* The return value of sysfs_topology_read_file function was not properly
  being checked for failure.
* The function was reading int valued sysfs variables and then returning
  their value. So, even if a function was trying to check the return value
  of this function, a caller would not be able to tell an failure code apart
  from reading a negative value. This also conflicted with the comment on the
  function which said that a return value of 0 indicated success.
* The function was parsing int valued sysfs values with strtoul instead of
  strtol.
* The function was non-static even though it was only used in the
  file it was declared in.
---
 tools/power/cpupower/utils/helpers/topology.c |   21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/tools/power/cpupower/utils/helpers/topology.c b/tools/power/cpupower/utils/helpers/topology.c
index 4eae2c4..216f3e3 100644
--- a/tools/power/cpupower/utils/helpers/topology.c
+++ b/tools/power/cpupower/utils/helpers/topology.c
@@ -20,9 +20,8 @@
 #include <helpers/sysfs.h>
 
 /* returns -1 on failure, 0 on success */
-int sysfs_topology_read_file(unsigned int cpu, const char *fname)
+static int sysfs_topology_read_file(unsigned int cpu, const char *fname, int *result)
 {
-	unsigned long value;
 	char linebuf[MAX_LINE_LEN];
 	char *endp;
 	char path[SYSFS_PATH_MAX];
@@ -31,10 +30,10 @@ int sysfs_topology_read_file(unsigned int cpu, const char *fname)
 			 cpu, fname);
 	if (sysfs_read_file(path, linebuf, MAX_LINE_LEN) == 0)
 		return -1;
-	value = strtoul(linebuf, &endp, 0);
+	*result = strtol(linebuf, &endp, 0);
 	if (endp == linebuf || errno == ERANGE)
 		return -1;
-	return value;
+	return 0;
 }
 
 struct cpuid_core_info {
@@ -82,13 +81,19 @@ int get_cpu_topology(struct cpupower_topology *cpu_top)
 	for (cpu = 0; cpu < cpus; cpu++) {
 		cpu_top->core_info[cpu].cpu = cpu;
 		cpu_top->core_info[cpu].is_online = sysfs_is_cpu_online(cpu);
-		cpu_top->core_info[cpu].pkg =
-			sysfs_topology_read_file(cpu, "physical_package_id");
+		if(sysfs_topology_read_file(
+			cpu,
+			"physical_package_id",
+			&(cpu_top->core_info[cpu].pkg)) < 0)
+			return -1;
 		if ((int)cpu_top->core_info[cpu].pkg != -1 &&
 		    cpu_top->core_info[cpu].pkg > cpu_top->pkgs)
 			cpu_top->pkgs = cpu_top->core_info[cpu].pkg;
-		cpu_top->core_info[cpu].core =
-			sysfs_topology_read_file(cpu, "core_id");
+		if(sysfs_topology_read_file(
+			cpu,
+			"core_id",
+			&(cpu_top->core_info[cpu].core)) < 0)
+			return -1;
 	}
 	cpu_top->pkgs++;
 
-- 
1.7.9.5

[PATCH 5/6] cpupower tools: Fix malloc of cpu_info structure

[Palmer Cox]

The cpu_info member of cpupower_topology was being declared as an unnamed
structure. This member was then being malloced using the size of the
parent cpupower_topology * the number of cpus. This works
because cpu_info is smaller than cpupower_topology. However, there is
no guarantee that will always be the case. Making cpu_info its own
top level structure (named cpuid_core_info) allows for mallocing the actual
size of this structure. This also lets us get rid of a redefinition of
the structure in topology.c with slightly different field names.
---
 tools/power/cpupower/utils/helpers/helpers.h  |   17 +++++++++--------
 tools/power/cpupower/utils/helpers/topology.c |   14 +++-----------
 2 files changed, 12 insertions(+), 19 deletions(-)

diff --git a/tools/power/cpupower/utils/helpers/helpers.h b/tools/power/cpupower/utils/helpers/helpers.h
index 2eb584c..f84985f 100644
--- a/tools/power/cpupower/utils/helpers/helpers.h
+++ b/tools/power/cpupower/utils/helpers/helpers.h
@@ -92,6 +92,14 @@ extern int get_cpu_info(unsigned int cpu, struct cpupower_cpu_info *cpu_info);
 extern struct cpupower_cpu_info cpupower_cpu_info;
 /* cpuid and cpuinfo helpers  **************************/
 
+struct cpuid_core_info {
+	int pkg;
+	int core;
+	int cpu;
+
+	/* flags */
+	unsigned int is_online:1;
+};
 
 /* CPU topology/hierarchy parsing ******************/
 struct cpupower_topology {
@@ -101,14 +109,7 @@ struct cpupower_topology {
 	unsigned int threads; /* per core */
 
 	/* Array gets mallocated with cores entries, holding per core info */
-	struct {
-		int pkg;
-		int core;
-		int cpu;
-
-		/* flags */
-		unsigned int is_online:1;
-	} *core_info;
+	struct cpuid_core_info *core_info;
 };
 
 extern int get_cpu_topology(struct cpupower_topology *cpu_top);
diff --git a/tools/power/cpupower/utils/helpers/topology.c b/tools/power/cpupower/utils/helpers/topology.c
index 216f3e3..4e2b583 100644
--- a/tools/power/cpupower/utils/helpers/topology.c
+++ b/tools/power/cpupower/utils/helpers/topology.c
@@ -36,14 +36,6 @@ static int sysfs_topology_read_file(unsigned int cpu, const char *fname, int *re
 	return 0;
 }
 
-struct cpuid_core_info {
-	unsigned int pkg;
-	unsigned int thread;
-	unsigned int cpu;
-	/* flags */
-	unsigned int is_online:1;
-};
-
 static int __compare(const void *t1, const void *t2)
 {
 	struct cpuid_core_info *top1 = (struct cpuid_core_info *)t1;
@@ -52,9 +44,9 @@ static int __compare(const void *t1, const void *t2)
 		return -1;
 	else if (top1->pkg > top2->pkg)
 		return 1;
-	else if (top1->thread < top2->thread)
+	else if (top1->core < top2->core)
 		return -1;
-	else if (top1->thread > top2->thread)
+	else if (top1->core > top2->core)
 		return 1;
 	else if (top1->cpu < top2->cpu)
 		return -1;
@@ -74,7 +66,7 @@ int get_cpu_topology(struct cpupower_topology *cpu_top)
 {
 	int cpu, cpus = sysconf(_SC_NPROCESSORS_CONF);
 
-	cpu_top->core_info = malloc(sizeof(struct cpupower_topology) * cpus);
+	cpu_top->core_info = malloc(sizeof(struct cpuid_core_info) * cpus);
 	if (cpu_top->core_info == NULL)
 		return -ENOMEM;
 	cpu_top->pkgs = cpu_top->cores = 0;
-- 
1.7.9.5

[PATCH 6/6] cpupower tools: Fix warning and a bug with the cpu package count

[Palmer Cox]

The pkgs member of cpupower_topology is being used as the number of
cpu packages. As the comment in get_cpu_topology notes, the package ids
are not guaranteed to be contiguous. So, simply setting pkgs to the value
of the highest physical_package_id doesn't actually provide a count of
the number of cpu packages. Instead, calculate pkgs by setting it to
the number of distinct physical_packge_id values which is pretty easy
to do after the core_info structs are sorted. Calculating pkgs this
way also has the nice benefit of getting rid of a sign comparison warning
that GCC 4.6 was reporting.
---
 tools/power/cpupower/utils/helpers/topology.c |   18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/tools/power/cpupower/utils/helpers/topology.c b/tools/power/cpupower/utils/helpers/topology.c
index 4e2b583..fd3cc4d 100644
--- a/tools/power/cpupower/utils/helpers/topology.c
+++ b/tools/power/cpupower/utils/helpers/topology.c
@@ -64,7 +64,7 @@ static int __compare(const void *t1, const void *t2)
  */
 int get_cpu_topology(struct cpupower_topology *cpu_top)
 {
-	int cpu, cpus = sysconf(_SC_NPROCESSORS_CONF);
+	int cpu, last_pkg, cpus = sysconf(_SC_NPROCESSORS_CONF);
 
 	cpu_top->core_info = malloc(sizeof(struct cpuid_core_info) * cpus);
 	if (cpu_top->core_info == NULL)
@@ -78,20 +78,28 @@ int get_cpu_topology(struct cpupower_topology *cpu_top)
 			"physical_package_id",
 			&(cpu_top->core_info[cpu].pkg)) < 0)
 			return -1;
-		if ((int)cpu_top->core_info[cpu].pkg != -1 &&
-		    cpu_top->core_info[cpu].pkg > cpu_top->pkgs)
-			cpu_top->pkgs = cpu_top->core_info[cpu].pkg;
 		if(sysfs_topology_read_file(
 			cpu,
 			"core_id",
 			&(cpu_top->core_info[cpu].core)) < 0)
 			return -1;
 	}
-	cpu_top->pkgs++;
 
 	qsort(cpu_top->core_info, cpus, sizeof(struct cpuid_core_info),
 	      __compare);
 
+	/* Count the number of distinct pkgs values. This works
+	   becuase the primary sort of of the core_info structs was just
+	   done by pkg value. */
+        last_pkg = cpu_top->core_info[0].pkg;
+        for(cpu = 1; cpu < cpus; cpu++) {
+                if(cpu_top->core_info[cpu].pkg != last_pkg) {
+			last_pkg = cpu_top->core_info[cpu].pkg;
+                        cpu_top->pkgs++;
+		}
+        }
+        cpu_top->pkgs++;
+
 	/* Intel's cores count is not consecutively numbered, there may
 	 * be a core_id of 3, but none of 2. Assume there always is 0
 	 * Get amount of cores by counting duplicates in a package
-- 
1.7.9.5

Re: [PATCH 6/6] cpupower tools: Fix warning and a bug with the cpu package count

[Thomas Renninger]

On Tuesday 07 August 2012 04:24:48 Palmer Cox wrote:
> The pkgs member of cpupower_topology is being used as the number of
> cpu packages. As the comment in get_cpu_topology notes, the package ids
> are not guaranteed to be contiguous. So, simply setting pkgs to the value
> of the highest physical_package_id doesn't actually provide a count of
> the number of cpu packages. Instead, calculate pkgs by setting it to
> the number of distinct physical_packge_id values which is pretty easy
> to do after the core_info structs are sorted. Calculating pkgs this
> way also has the nice benefit of getting rid of a sign comparison warning
> that GCC 4.6 was reporting.
> ---
>  tools/power/cpupower/utils/helpers/topology.c |   18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/power/cpupower/utils/helpers/topology.c b/tools/power/cpupower/utils/helpers/topology.c
> index 4e2b583..fd3cc4d 100644
> --- a/tools/power/cpupower/utils/helpers/topology.c
> +++ b/tools/power/cpupower/utils/helpers/topology.c
> @@ -64,7 +64,7 @@ static int __compare(const void *t1, const void *t2)
>   */
>  int get_cpu_topology(struct cpupower_topology *cpu_top)
>  {
> -	int cpu, cpus = sysconf(_SC_NPROCESSORS_CONF);
> +	int cpu, last_pkg, cpus = sysconf(_SC_NPROCESSORS_CONF);
>  
>  	cpu_top->core_info = malloc(sizeof(struct cpuid_core_info) * cpus);
>  	if (cpu_top->core_info == NULL)
> @@ -78,20 +78,28 @@ int get_cpu_topology(struct cpupower_topology *cpu_top)
>  			"physical_package_id",
>  			&(cpu_top->core_info[cpu].pkg)) < 0)
>  			return -1;
> -		if ((int)cpu_top->core_info[cpu].pkg != -1 &&
> -		    cpu_top->core_info[cpu].pkg > cpu_top->pkgs)
> -			cpu_top->pkgs = cpu_top->core_info[cpu].pkg;
>  		if(sysfs_topology_read_file(
>  			cpu,
>  			"core_id",
>  			&(cpu_top->core_info[cpu].core)) < 0)
>  			return -1;
>  	}
> -	cpu_top->pkgs++;
>  
>  	qsort(cpu_top->core_info, cpus, sizeof(struct cpuid_core_info),
>  	      __compare);
>  
> +	/* Count the number of distinct pkgs values. This works
> +	   becuase the primary sort of of the core_info structs was just
becuase ... of of ... struct instead of structs

Otherwise the first 4 patches look like rather nice and straight forward
cleanups/fixes.
Feel free to add a Reviewed-by: Thomas Renninger <trenn@suse.de>

Let me have a closer look at patch 5 and 6. I had problems getting rid of
the compiler warning, looks like you found an easy way to clean this up.
I will be back and have time for this in the beginning of next week.

On which platforms (topology) did you test this?

   Thomas

Re: [PATCH 6/6] cpupower tools: Fix warning and a bug with the cpu package count

[Palmer Cox]

On Thu, Aug 09, 2012 at 12:07:36PM +0200, Thomas Renninger wrote:
> On Tuesday 07 August 2012 04:24:48 Palmer Cox wrote:
> > The pkgs member of cpupower_topology is being used as the number of
> > cpu packages. As the comment in get_cpu_topology notes, the package ids
> > are not guaranteed to be contiguous. So, simply setting pkgs to the value
> > of the highest physical_package_id doesn't actually provide a count of
> > the number of cpu packages. Instead, calculate pkgs by setting it to
> > the number of distinct physical_packge_id values which is pretty easy
> > to do after the core_info structs are sorted. Calculating pkgs this
> > way also has the nice benefit of getting rid of a sign comparison warning
> > that GCC 4.6 was reporting.
> > ---
> >  tools/power/cpupower/utils/helpers/topology.c |   18 +++++++++++++-----
> >  1 file changed, 13 insertions(+), 5 deletions(-)
> > 
> > diff --git a/tools/power/cpupower/utils/helpers/topology.c b/tools/power/cpupower/utils/helpers/topology.c
> > index 4e2b583..fd3cc4d 100644
> > --- a/tools/power/cpupower/utils/helpers/topology.c
> > +++ b/tools/power/cpupower/utils/helpers/topology.c
> > @@ -64,7 +64,7 @@ static int __compare(const void *t1, const void *t2)
> >   */
> >  int get_cpu_topology(struct cpupower_topology *cpu_top)
> >  {
> > -	int cpu, cpus = sysconf(_SC_NPROCESSORS_CONF);
> > +	int cpu, last_pkg, cpus = sysconf(_SC_NPROCESSORS_CONF);
> >  
> >  	cpu_top->core_info = malloc(sizeof(struct cpuid_core_info) * cpus);
> >  	if (cpu_top->core_info == NULL)
> > @@ -78,20 +78,28 @@ int get_cpu_topology(struct cpupower_topology *cpu_top)
> >  			"physical_package_id",
> >  			&(cpu_top->core_info[cpu].pkg)) < 0)
> >  			return -1;
> > -		if ((int)cpu_top->core_info[cpu].pkg != -1 &&
> > -		    cpu_top->core_info[cpu].pkg > cpu_top->pkgs)
> > -			cpu_top->pkgs = cpu_top->core_info[cpu].pkg;
> >  		if(sysfs_topology_read_file(
> >  			cpu,
> >  			"core_id",
> >  			&(cpu_top->core_info[cpu].core)) < 0)
> >  			return -1;
> >  	}
> > -	cpu_top->pkgs++;
> >  
> >  	qsort(cpu_top->core_info, cpus, sizeof(struct cpuid_core_info),
> >  	      __compare);
> >  
> > +	/* Count the number of distinct pkgs values. This works
> > +	   becuase the primary sort of of the core_info structs was just
> becuase ... of of ... struct instead of structs

Oof. I'm not winning any grammar medals for this. Thanks for
noticing.
> 
> Otherwise the first 4 patches look like rather nice and straight forward
> cleanups/fixes.
> Feel free to add a Reviewed-by: Thomas Renninger <trenn@suse.de>

Will do. Thanks!
> 
> Let me have a closer look at patch 5 and 6. I had problems getting rid of
> the compiler warning, looks like you found an easy way to clean this up.
> I will be back and have time for this in the beginning of next week.

Thanks for the review! Let me know if there is anything in patches 5
and 6 that needs cleaning up and I'll be happy to do it. I only have
access to a laptop with a single package 2 core Centrino2 processor.
I tested each patch in the series on my laptop running a 64-bit 3.5
kernel to make sure that everything functioned. I'm no expert in the
exact expected output of the tool, but the only impact that I
believe these patches should have is the output of the number of cpu
packages. I tested this on my system which resulted in reporting
just a single cpu package as I expected, but I don't have access to
a system with multiple cpu packages to test on.

> 
> On which platforms (topology) did you test this?
> 
>    Thomas

-Palmer