Re: [PATCH 6/6] cpupower tools: Fix warning and a bug with the cpu package count
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 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 5/6] cpupower tools: Fix malloc of cpu_info structure
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 6/6] cpupower tools: Fix warning and a bug with the cpu package count
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 4/6] cpupower tools: Fix issues with sysfs_topology_read_file
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 /* 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 3/6] cpupower tools: Fix minor warnings
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/6] cpupower tools: Update .gitignore for files created in the debug directories
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 0/6] cpupower tools: Fix minor bugs and warnings
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(-) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/6] cpupower tools: Remove brace expansion from clean target
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[tip:perf/urgent] perf tools: Remove brace expansion from clean target
Commit-ID: 7f309ed6453926a81e2a97d274f67f1e48f0d74c Gitweb: http://git.kernel.org/tip/7f309ed6453926a81e2a97d274f67f1e48f0d74c Author: Palmer Cox AuthorDate: Sun, 29 Jul 2012 17:54:43 -0400 Committer: Arnaldo Carvalho de Melo CommitDate: Fri, 3 Aug 2012 10:46:32 -0300 perf tools: Remove brace expansion from clean target The clean target uses 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. Signed-off-by: Palmer Cox Cc: Ingo Molnar Cc: Paul Mackerras Cc: Peter Zijlstra Link: http://lkml.kernel.org/r/1343598883-17907-1-git-send-emai...@lmercox.com Signed-off-by: Arnaldo Carvalho de Melo --- tools/perf/Makefile |3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/tools/perf/Makefile b/tools/perf/Makefile index 32912af..35655c3 100644 --- a/tools/perf/Makefile +++ b/tools/perf/Makefile @@ -987,7 +987,8 @@ clean: $(RM) *.spec *.pyc *.pyo */*.pyc */*.pyo $(OUTPUT)common-cmds.h TAGS tags cscope* $(MAKE) -C Documentation/ clean $(RM) $(OUTPUT)PERF-VERSION-FILE $(OUTPUT)PERF-CFLAGS - $(RM) $(OUTPUT)util/*-{bison,flex}* + $(RM) $(OUTPUT)util/*-bison* + $(RM) $(OUTPUT)util/*-flex* $(python-clean) .PHONY: all install clean strip $(LIBTRACEEVENT) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] perf: Remove brace expansion from clean target
The clean target uses 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. Signed-off-by: Palmer Cox --- tools/perf/Makefile |3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tools/perf/Makefile b/tools/perf/Makefile index 75d74e5..842cf67 100644 --- a/tools/perf/Makefile +++ b/tools/perf/Makefile @@ -979,7 +979,8 @@ clean: $(RM) *.spec *.pyc *.pyo */*.pyc */*.pyo $(OUTPUT)common-cmds.h TAGS tags cscope* $(MAKE) -C Documentation/ clean $(RM) $(OUTPUT)PERF-VERSION-FILE $(OUTPUT)PERF-CFLAGS - $(RM) $(OUTPUT)util/*-{bison,flex}* + $(RM) $(OUTPUT)util/*-bison* + $(RM) $(OUTPUT)util/*-flex* $(python-clean) .PHONY: all install clean strip $(LIBTRACEEVENT) -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/