* [PATCH 1/6] cpupower tools: Remove brace expansion from clean target
2012-08-07 2:24 [PATCH 0/6] cpupower tools: Fix minor bugs and warnings Palmer Cox
@ 2012-08-07 2:24 ` Palmer Cox
2012-08-07 2:24 ` [PATCH 2/6] cpupower tools: Update .gitignore for files created in the debug directories Palmer Cox
` (4 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Palmer Cox @ 2012-08-07 2:24 UTC (permalink / raw)
To: Dominik Brodowski, Thomas Renninger; +Cc: linux-kernel, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 2/6] cpupower tools: Update .gitignore for files created in the debug directories
2012-08-07 2:24 [PATCH 0/6] cpupower tools: Fix minor bugs and warnings Palmer Cox
2012-08-07 2:24 ` [PATCH 1/6] cpupower tools: Remove brace expansion from clean target Palmer Cox
@ 2012-08-07 2:24 ` Palmer Cox
2012-08-07 2:24 ` [PATCH 3/6] cpupower tools: Fix minor warnings Palmer Cox
` (3 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Palmer Cox @ 2012-08-07 2:24 UTC (permalink / raw)
To: Dominik Brodowski, Thomas Renninger; +Cc: linux-kernel, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 3/6] cpupower tools: Fix minor warnings
2012-08-07 2:24 [PATCH 0/6] cpupower tools: Fix minor bugs and warnings Palmer Cox
2012-08-07 2:24 ` [PATCH 1/6] cpupower tools: Remove brace expansion from clean target Palmer Cox
2012-08-07 2:24 ` [PATCH 2/6] cpupower tools: Update .gitignore for files created in the debug directories Palmer Cox
@ 2012-08-07 2:24 ` Palmer Cox
2012-08-07 2:24 ` [PATCH 4/6] cpupower tools: Fix issues with sysfs_topology_read_file Palmer Cox
` (2 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Palmer Cox @ 2012-08-07 2:24 UTC (permalink / raw)
To: Dominik Brodowski, Thomas Renninger; +Cc: linux-kernel, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 4/6] cpupower tools: Fix issues with sysfs_topology_read_file
2012-08-07 2:24 [PATCH 0/6] cpupower tools: Fix minor bugs and warnings Palmer Cox
` (2 preceding siblings ...)
2012-08-07 2:24 ` [PATCH 3/6] cpupower tools: Fix minor warnings Palmer Cox
@ 2012-08-07 2:24 ` Palmer Cox
2012-08-07 2:24 ` [PATCH 5/6] cpupower tools: Fix malloc of cpu_info structure Palmer Cox
2012-08-07 2:24 ` [PATCH 6/6] cpupower tools: Fix warning and a bug with the cpu package count Palmer Cox
5 siblings, 0 replies; 9+ messages in thread
From: Palmer Cox @ 2012-08-07 2:24 UTC (permalink / raw)
To: Dominik Brodowski, Thomas Renninger; +Cc: linux-kernel, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 5/6] cpupower tools: Fix malloc of cpu_info structure
2012-08-07 2:24 [PATCH 0/6] cpupower tools: Fix minor bugs and warnings Palmer Cox
` (3 preceding siblings ...)
2012-08-07 2:24 ` [PATCH 4/6] cpupower tools: Fix issues with sysfs_topology_read_file Palmer Cox
@ 2012-08-07 2:24 ` Palmer Cox
2012-08-07 2:24 ` [PATCH 6/6] cpupower tools: Fix warning and a bug with the cpu package count Palmer Cox
5 siblings, 0 replies; 9+ messages in thread
From: Palmer Cox @ 2012-08-07 2:24 UTC (permalink / raw)
To: Dominik Brodowski, Thomas Renninger; +Cc: linux-kernel, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 6/6] cpupower tools: Fix warning and a bug with the cpu package count
2012-08-07 2:24 [PATCH 0/6] cpupower tools: Fix minor bugs and warnings Palmer Cox
` (4 preceding siblings ...)
2012-08-07 2:24 ` [PATCH 5/6] cpupower tools: Fix malloc of cpu_info structure Palmer Cox
@ 2012-08-07 2:24 ` Palmer Cox
2012-08-09 10:07 ` Thomas Renninger
5 siblings, 1 reply; 9+ messages in thread
From: Palmer Cox @ 2012-08-07 2:24 UTC (permalink / raw)
To: Dominik Brodowski, Thomas Renninger; +Cc: linux-kernel, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 6/6] cpupower tools: Fix warning and a bug with the cpu package count
2012-08-07 2:24 ` [PATCH 6/6] cpupower tools: Fix warning and a bug with the cpu package count Palmer Cox
@ 2012-08-09 10:07 ` Thomas Renninger
2012-08-10 2:58 ` Palmer Cox
0 siblings, 1 reply; 9+ messages in thread
From: Thomas Renninger @ 2012-08-09 10:07 UTC (permalink / raw)
To: Palmer Cox; +Cc: Dominik Brodowski, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 6/6] cpupower tools: Fix warning and a bug with the cpu package count
2012-08-09 10:07 ` Thomas Renninger
@ 2012-08-10 2:58 ` Palmer Cox
0 siblings, 0 replies; 9+ messages in thread
From: Palmer Cox @ 2012-08-10 2:58 UTC (permalink / raw)
To: Thomas Renninger; +Cc: Dominik Brodowski, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread