2013-08-21 00:20:33

by Josh Triplett

[permalink] [raw]
Subject: [PATCH v2 0/8] turbostat: Build fixes, 32-bit support, and cleanups

This patch series includes build fixes, warning fixes, and cleanups discovered
while adding turbostat to ChromeOS.

The first and third patches should go into stable, and I've CCed them
accordingly.

The first patch unbreaks the turbostat build on systems
with older versions of kernel headers.

The third patch is required to build on 32-bit x86.

The second and fourth patches fix warnings.

Patches 5-7 are code cleanups, and the last patch adds turbostat itself to
.gitignore.

Josh Triplett (8):
turbostat: Don't put unprocessed uapi headers in the include path
turbostat: Don't attempt to printf an off_t with %zx
turbostat: Use GCC's CPUID functions to support PIC
turbostat: Check return value of fscanf
turbostat: Add a helper to parse a single int out of a file
turbostat: Factor out common function to open file and exit on failure
turbostat: Clean up error handling; disambiguate error messages; use
err and errx
turbostat: Add a .gitignore to ignore the compiled turbostat binary

tools/power/x86/turbostat/.gitignore | 1 +
tools/power/x86/turbostat/Makefile | 2 +-
tools/power/x86/turbostat/turbostat.c | 210 +++++++++++++---------------------
3 files changed, 82 insertions(+), 131 deletions(-)
create mode 100644 tools/power/x86/turbostat/.gitignore

--
1.8.4.rc3


2013-08-21 00:20:39

by Josh Triplett

[permalink] [raw]
Subject: [PATCH v2 1/8] turbostat: Don't put unprocessed uapi headers in the include path

turbostat's Makefile puts arch/x86/include/uapi/ in the include path, so
that it can include <asm/msr.h> from it. It isn't in general safe to
include even uapi headers directly from the kernel tree without
processing them through scripts/headers_install.sh, but asm/msr.h
happens to work.

However, that include path can break with some versions of system
headers, by overriding some system headers with the unprocessed versions
directly from the kernel source. For instance:

In file included from /build/x86-generic/usr/include/bits/sigcontext.h:28:0,
from /build/x86-generic/usr/include/signal.h:339,
from /build/x86-generic/usr/include/sys/wait.h:31,
from turbostat.c:27:
../../../../arch/x86/include/uapi/asm/sigcontext.h:4:28: fatal error: linux/compiler.h: No such file or directory

This occurs because the system bits/sigcontext.h on that build system
includes <asm/sigcontext.h>, and asm/sigcontext.h in the kernel source
includes <linux/compiler.h>, which scripts/headers_install.sh would have
filtered out.

Since turbostat really only wants a single header, just include that one
header rather than putting an entire directory of kernel headers on the
include path.

In the process, switch from msr.h to msr-index.h, since turbostat just
wants the MSR numbers.

Signed-off-by: Josh Triplett <[email protected]>
Cc: [email protected]
---
tools/power/x86/turbostat/Makefile | 2 +-
tools/power/x86/turbostat/turbostat.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/power/x86/turbostat/Makefile b/tools/power/x86/turbostat/Makefile
index f09641d..d1b3a36 100644
--- a/tools/power/x86/turbostat/Makefile
+++ b/tools/power/x86/turbostat/Makefile
@@ -5,7 +5,7 @@ DESTDIR :=

turbostat : turbostat.c
CFLAGS += -Wall
-CFLAGS += -I../../../../arch/x86/include/uapi/
+CFLAGS += -DMSRHEADER='"../../../../arch/x86/include/uapi/asm/msr-index.h"'

%: %.c
@mkdir -p $(BUILD_OUTPUT)
diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c
index fe70207..5bf4a67 100644
--- a/tools/power/x86/turbostat/turbostat.c
+++ b/tools/power/x86/turbostat/turbostat.c
@@ -20,7 +20,7 @@
*/

#define _GNU_SOURCE
-#include <asm/msr.h>
+#include MSRHEADER
#include <stdio.h>
#include <unistd.h>
#include <sys/types.h>
--
1.8.4.rc3

2013-08-21 00:20:46

by Josh Triplett

[permalink] [raw]
Subject: [PATCH v2 7/8] turbostat: Clean up error handling; disambiguate error messages; use err and errx

Most of turbostat's error handling consists of printing an error (often
including an errno) and exiting. Since perror doesn't support a format
string, those error messages are often ambiguous, such as just showing a
file path, which doesn't uniquely identify which call failed.

turbostat already uses _GNU_SOURCE, so switch to the err and errx
functions from err.h, which take a format string.

Signed-off-by: Josh Triplett <[email protected]>
---
tools/power/x86/turbostat/turbostat.c | 107 ++++++++++++----------------------
1 file changed, 38 insertions(+), 69 deletions(-)

diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c
index aa80db9..8ceba8f 100644
--- a/tools/power/x86/turbostat/turbostat.c
+++ b/tools/power/x86/turbostat/turbostat.c
@@ -23,6 +23,7 @@
#include MSRHEADER
#include <stdarg.h>
#include <stdio.h>
+#include <err.h>
#include <unistd.h>
#include <sys/types.h>
#include <sys/wait.h>
@@ -620,12 +621,10 @@ delta_thread(struct thread_data *new, struct thread_data *old,
old->tsc = new->tsc - old->tsc;

/* check for TSC < 1 Mcycles over interval */
- if (old->tsc < (1000 * 1000)) {
- fprintf(stderr, "Insanely slow TSC rate, TSC stops in idle?\n");
- fprintf(stderr, "You can disable all c-states by booting with \"idle=poll\"\n");
- fprintf(stderr, "or just the deep ones with \"processor.max_cstate=1\"\n");
- exit(-3);
- }
+ if (old->tsc < (1000 * 1000))
+ errx(-3, "Insanely slow TSC rate, TSC stops in idle?\n"
+ "You can disable all c-states by booting with \"idle=poll\"\n"
+ "or just the deep ones with \"processor.max_cstate=1\"");

old->c1 = new->c1 - old->c1;

@@ -1158,10 +1157,8 @@ void free_all_buffers(void)
FILE *fopen_or_die(const char *path, const char *mode)
{
FILE *filep = fopen(path, "r");
- if (!filep) {
- perror(path);
- exit(1);
- }
+ if (!filep)
+ err(1, "%s: open failed", path);
return filep;
}

@@ -1179,10 +1176,8 @@ int parse_int_file(const char *fmt, ...)
vsnprintf(path, sizeof(path), fmt, args);
va_end(args);
filep = fopen_or_die(path, "r");
- if (fscanf(filep, "%d", &value) != 1) {
- perror(path);
- exit(1);
- }
+ if (fscanf(filep, "%d", &value) != 1)
+ err(1, "%s: failed to parse number from file", path);
fclose(filep);
return value;
}
@@ -1297,10 +1292,8 @@ int for_all_proc_cpus(int (func)(int))
fp = fopen_or_die(proc_stat, "r");

retval = fscanf(fp, "cpu %*d %*d %*d %*d %*d %*d %*d %*d %*d %*d\n");
- if (retval != 0) {
- perror("/proc/stat format");
- exit(1);
- }
+ if (retval != 0)
+ err(1, "%s: failed to parse format", proc_stat);

while (1) {
retval = fscanf(fp, "cpu%u %*d %*d %*d %*d %*d %*d %*d %*d %*d %*d\n", &cpu_num);
@@ -1404,19 +1397,15 @@ void check_dev_msr()
{
struct stat sb;

- if (stat("/dev/cpu/0/msr", &sb)) {
- fprintf(stderr, "no /dev/cpu/0/msr\n");
- fprintf(stderr, "Try \"# modprobe msr\"\n");
- exit(-5);
- }
+ if (stat("/dev/cpu/0/msr", &sb))
+ err(-5, "no /dev/cpu/0/msr\n"
+ "Try \"# modprobe msr\"");
}

void check_super_user()
{
- if (getuid() != 0) {
- fprintf(stderr, "must be root\n");
- exit(-6);
- }
+ if (getuid() != 0)
+ errx(-6, "must be root");
}

int has_nehalem_turbo_ratio_limit(unsigned int family, unsigned int model)
@@ -1895,10 +1884,8 @@ void check_cpuid()
fprintf(stderr, "%d CPUID levels; family:model:stepping 0x%x:%x:%x (%d:%d:%d)\n",
max_level, family, model, stepping, family, model, stepping);

- if (!(edx & (1 << 5))) {
- fprintf(stderr, "CPUID: no MSR\n");
- exit(1);
- }
+ if (!(edx & (1 << 5)))
+ errx(1, "CPUID: no MSR");

/*
* check max extended function levels of CPUID.
@@ -1908,10 +1895,8 @@ void check_cpuid()
ebx = ecx = edx = 0;
__get_cpuid(0x80000000, &max_level, &ebx, &ecx, &edx);

- if (max_level < 0x80000007) {
- fprintf(stderr, "CPUID: no invariant TSC (max_level 0x%x)\n", max_level);
- exit(1);
- }
+ if (max_level < 0x80000007)
+ errx(1, "CPUID: no invariant TSC (max_level 0x%x)", max_level);

/*
* Non-Stop TSC is advertised by CPUID.EAX=0x80000007: EDX.bit8
@@ -1920,10 +1905,8 @@ void check_cpuid()
__get_cpuid(0x80000007, &eax, &ebx, &ecx, &edx);
has_invariant_tsc = edx & (1 << 8);

- if (!has_invariant_tsc) {
- fprintf(stderr, "No invariant TSC\n");
- exit(1);
- }
+ if (!has_invariant_tsc)
+ errx(1, "No invariant TSC");

/*
* APERF/MPERF is advertised by CPUID.EAX=0x6: ECX.bit0
@@ -1944,7 +1927,7 @@ void check_cpuid()
has_epb ? ", EPB": "");

if (!has_aperf)
- exit(-1);
+ errx(-1, "No APERF");

do_nehalem_platform_info = genuine_intel && has_invariant_tsc;
do_nhm_cstates = genuine_intel; /* all Intel w/ non-stop TSC have NHM counters */
@@ -1963,9 +1946,8 @@ void check_cpuid()

void usage()
{
- fprintf(stderr, "%s: [-v][-R][-T][-p|-P|-S][-c MSR# | -s]][-C MSR#][-m MSR#][-M MSR#][-i interval_sec | command ...]\n",
- progname);
- exit(1);
+ errx(1, "%s: [-v][-R][-T][-p|-P|-S][-c MSR# | -s]][-C MSR#][-m MSR#][-M MSR#][-i interval_sec | command ...]\n",
+ progname);
}


@@ -2008,19 +1990,15 @@ void topology_probe()
fprintf(stderr, "num_cpus %d max_cpu_num %d\n", topo.num_cpus, topo.max_cpu_num);

cpus = calloc(1, (topo.max_cpu_num + 1) * sizeof(struct cpu_topology));
- if (cpus == NULL) {
- perror("calloc cpus");
- exit(1);
- }
+ if (cpus == NULL)
+ err(1, "calloc cpus");

/*
* Allocate and initialize cpu_present_set
*/
cpu_present_set = CPU_ALLOC((topo.max_cpu_num + 1));
- if (cpu_present_set == NULL) {
- perror("CPU_ALLOC");
- exit(3);
- }
+ if (cpu_present_set == NULL)
+ err(3, "CPU_ALLOC");
cpu_present_setsize = CPU_ALLOC_SIZE((topo.max_cpu_num + 1));
CPU_ZERO_S(cpu_present_setsize, cpu_present_set);
for_all_proc_cpus(mark_cpu_present);
@@ -2029,10 +2007,8 @@ void topology_probe()
* Allocate and initialize cpu_affinity_set
*/
cpu_affinity_set = CPU_ALLOC((topo.max_cpu_num + 1));
- if (cpu_affinity_set == NULL) {
- perror("CPU_ALLOC");
- exit(3);
- }
+ if (cpu_affinity_set == NULL)
+ err(3, "CPU_ALLOC");
cpu_affinity_setsize = CPU_ALLOC_SIZE((topo.max_cpu_num + 1));
CPU_ZERO_S(cpu_affinity_setsize, cpu_affinity_set);

@@ -2116,8 +2092,7 @@ allocate_counters(struct thread_data **t, struct core_data **c, struct pkg_data

return;
error:
- perror("calloc counters");
- exit(1);
+ err(1, "calloc counters");
}
/*
* init_counter()
@@ -2174,10 +2149,8 @@ void allocate_output_buffer()
{
output_buffer = calloc(1, (1 + topo.num_cpus) * 256);
outp = output_buffer;
- if (outp == NULL) {
- perror("calloc");
- exit(-1);
- }
+ if (outp == NULL)
+ err(-1, "calloc output buffer");
}

void setup_all_buffers(void)
@@ -2231,17 +2204,13 @@ int fork_it(char **argv)
} else {

/* parent */
- if (child_pid == -1) {
- perror("fork");
- exit(1);
- }
+ if (child_pid == -1)
+ err(1, "fork");

signal(SIGINT, SIG_IGN);
signal(SIGQUIT, SIG_IGN);
- if (waitpid(child_pid, &status, 0) == -1) {
- perror("wait");
- exit(status);
- }
+ if (waitpid(child_pid, &status, 0) == -1)
+ err(status, "waitpid");
}
/*
* n.b. fork_it() does not check for errors from for_all_cpus()
--
1.8.4.rc3

2013-08-21 00:20:45

by Josh Triplett

[permalink] [raw]
Subject: [PATCH v2 5/8] turbostat: Add a helper to parse a single int out of a file

Many different chunks of code in turbostat open a file, parse a single
int out of it, and close it. Factor that out into a common function.

Signed-off-by: Josh Triplett <[email protected]>
---
tools/power/x86/turbostat/turbostat.c | 81 +++++++++++------------------------
1 file changed, 24 insertions(+), 57 deletions(-)

diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c
index 514adba..e0dbada 100644
--- a/tools/power/x86/turbostat/turbostat.c
+++ b/tools/power/x86/turbostat/turbostat.c
@@ -21,6 +21,7 @@

#define _GNU_SOURCE
#include MSRHEADER
+#include <stdarg.h>
#include <stdio.h>
#include <unistd.h>
#include <sys/types.h>
@@ -1152,27 +1153,38 @@ void free_all_buffers(void)
}

/*
- * cpu_is_first_sibling_in_core(cpu)
- * return 1 if given CPU is 1st HT sibling in the core
+ * Parse a file containing a single int.
*/
-int cpu_is_first_sibling_in_core(int cpu)
+int parse_int_file(const char *fmt, ...)
{
- char path[64];
+ va_list args;
+ char path[PATH_MAX];
FILE *filep;
- int first_cpu;
+ int value;

- sprintf(path, "/sys/devices/system/cpu/cpu%d/topology/thread_siblings_list", cpu);
+ va_start(args, fmt);
+ vsnprintf(path, sizeof(path), fmt, args);
+ va_end(args);
filep = fopen(path, "r");
- if (filep == NULL) {
+ if (!filep) {
perror(path);
exit(1);
}
- if (fscanf(filep, "%d", &first_cpu) != 1) {
+ if (fscanf(filep, "%d", &value) != 1) {
perror(path);
exit(1);
}
fclose(filep);
- return (cpu == first_cpu);
+ return value;
+}
+
+/*
+ * cpu_is_first_sibling_in_core(cpu)
+ * return 1 if given CPU is 1st HT sibling in the core
+ */
+int cpu_is_first_sibling_in_core(int cpu)
+{
+ return cpu == parse_int_file("/sys/devices/system/cpu/cpu%d/topology/thread_siblings_list", cpu);
}

/*
@@ -1181,62 +1193,17 @@ int cpu_is_first_sibling_in_core(int cpu)
*/
int cpu_is_first_core_in_package(int cpu)
{
- char path[64];
- FILE *filep;
- int first_cpu;
-
- sprintf(path, "/sys/devices/system/cpu/cpu%d/topology/core_siblings_list", cpu);
- filep = fopen(path, "r");
- if (filep == NULL) {
- perror(path);
- exit(1);
- }
- if (fscanf(filep, "%d", &first_cpu) != 1) {
- perror(path);
- exit(1);
- }
- fclose(filep);
- return (cpu == first_cpu);
+ return cpu == parse_int_file("/sys/devices/system/cpu/cpu%d/topology/core_siblings_list", cpu);
}

int get_physical_package_id(int cpu)
{
- char path[80];
- FILE *filep;
- int pkg;
-
- sprintf(path, "/sys/devices/system/cpu/cpu%d/topology/physical_package_id", cpu);
- filep = fopen(path, "r");
- if (filep == NULL) {
- perror(path);
- exit(1);
- }
- if (fscanf(filep, "%d", &pkg) != 1) {
- perror(path);
- exit(1);
- }
- fclose(filep);
- return pkg;
+ return parse_int_file("/sys/devices/system/cpu/cpu%d/topology/physical_package_id", cpu);
}

int get_core_id(int cpu)
{
- char path[80];
- FILE *filep;
- int core;
-
- sprintf(path, "/sys/devices/system/cpu/cpu%d/topology/core_id", cpu);
- filep = fopen(path, "r");
- if (filep == NULL) {
- perror(path);
- exit(1);
- }
- if (fscanf(filep, "%d", &core) != 1) {
- perror(path);
- exit(1);
- }
- fclose(filep);
- return core;
+ return parse_int_file("/sys/devices/system/cpu/cpu%d/topology/core_id", cpu);
}

int get_num_ht_siblings(int cpu)
--
1.8.4.rc3

2013-08-21 00:21:00

by Josh Triplett

[permalink] [raw]
Subject: [PATCH v2 8/8] turbostat: Add a .gitignore to ignore the compiled turbostat binary

Signed-off-by: Josh Triplett <[email protected]>
---
tools/power/x86/turbostat/.gitignore | 1 +
1 file changed, 1 insertion(+)
create mode 100644 tools/power/x86/turbostat/.gitignore

diff --git a/tools/power/x86/turbostat/.gitignore b/tools/power/x86/turbostat/.gitignore
new file mode 100644
index 0000000..7521370
--- /dev/null
+++ b/tools/power/x86/turbostat/.gitignore
@@ -0,0 +1 @@
+turbostat
--
1.8.4.rc3

2013-08-21 00:20:43

by Josh Triplett

[permalink] [raw]
Subject: [PATCH v2 2/8] turbostat: Don't attempt to printf an off_t with %zx

turbostat uses the format %zx to print an off_t. However, %zx wants a
size_t, not an off_t. On 32-bit targets, those refer to different
types, potentially even with different sizes. Use %llx and a cast
instead, since printf does not have a length modifier for off_t.

Without this patch, when compiling for a 32-bit target:

turbostat.c: In function 'get_msr':
turbostat.c:231:3: warning: format '%zx' expects argument of type 'size_t', but argument 4 has type 'off_t' [-Wformat]

Signed-off-by: Josh Triplett <[email protected]>
---
tools/power/x86/turbostat/turbostat.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c
index 5bf4a67..b10b8d2 100644
--- a/tools/power/x86/turbostat/turbostat.c
+++ b/tools/power/x86/turbostat/turbostat.c
@@ -230,7 +230,7 @@ int get_msr(int cpu, off_t offset, unsigned long long *msr)
close(fd);

if (retval != sizeof *msr) {
- fprintf(stderr, "%s offset 0x%zx read failed\n", pathname, offset);
+ fprintf(stderr, "%s offset 0x%llx read failed\n", pathname, (unsigned long long)offset);
return -1;
}

--
1.8.4.rc3

2013-08-21 00:21:32

by Josh Triplett

[permalink] [raw]
Subject: [PATCH v2 6/8] turbostat: Factor out common function to open file and exit on failure

Several different functions in turbostat contain the same pattern of
opening a file and exiting on failure. Factor out a common fopen_or_die
function for that.

Signed-off-by: Josh Triplett <[email protected]>
---
tools/power/x86/turbostat/turbostat.c | 31 ++++++++++++++++---------------
1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c
index e0dbada..aa80db9 100644
--- a/tools/power/x86/turbostat/turbostat.c
+++ b/tools/power/x86/turbostat/turbostat.c
@@ -1153,6 +1153,19 @@ void free_all_buffers(void)
}

/*
+ * Open a file, and exit on failure
+ */
+FILE *fopen_or_die(const char *path, const char *mode)
+{
+ FILE *filep = fopen(path, "r");
+ if (!filep) {
+ perror(path);
+ exit(1);
+ }
+ return filep;
+}
+
+/*
* Parse a file containing a single int.
*/
int parse_int_file(const char *fmt, ...)
@@ -1165,11 +1178,7 @@ int parse_int_file(const char *fmt, ...)
va_start(args, fmt);
vsnprintf(path, sizeof(path), fmt, args);
va_end(args);
- filep = fopen(path, "r");
- if (!filep) {
- perror(path);
- exit(1);
- }
+ filep = fopen_or_die(path, "r");
if (fscanf(filep, "%d", &value) != 1) {
perror(path);
exit(1);
@@ -1215,11 +1224,7 @@ int get_num_ht_siblings(int cpu)
char character;

sprintf(path, "/sys/devices/system/cpu/cpu%d/topology/thread_siblings_list", cpu);
- filep = fopen(path, "r");
- if (filep == NULL) {
- perror(path);
- exit(1);
- }
+ filep = fopen_or_die(path, "r");
/*
* file format:
* if a pair of number with a character between: 2 siblings (eg. 1-2, or 1,4)
@@ -1289,11 +1294,7 @@ int for_all_proc_cpus(int (func)(int))
int cpu_num;
int retval;

- fp = fopen(proc_stat, "r");
- if (fp == NULL) {
- perror(proc_stat);
- exit(1);
- }
+ fp = fopen_or_die(proc_stat, "r");

retval = fscanf(fp, "cpu %*d %*d %*d %*d %*d %*d %*d %*d %*d %*d\n");
if (retval != 0) {
--
1.8.4.rc3

2013-08-21 00:21:52

by Josh Triplett

[permalink] [raw]
Subject: [PATCH v2 4/8] turbostat: Check return value of fscanf

Some systems declare fscanf with the warn_unused_result attribute. On
such systems, turbostat generates the following warnings:

turbostat.c: In function 'get_core_id':
turbostat.c:1203:8: warning: ignoring return value of 'fscanf', declared with attribute warn_unused_result [-Wunused-result]
turbostat.c: In function 'get_physical_package_id':
turbostat.c:1186:8: warning: ignoring return value of 'fscanf', declared with attribute warn_unused_result [-Wunused-result]
turbostat.c: In function 'cpu_is_first_core_in_package':
turbostat.c:1169:8: warning: ignoring return value of 'fscanf', declared with attribute warn_unused_result [-Wunused-result]
turbostat.c: In function 'cpu_is_first_sibling_in_core':
turbostat.c:1148:8: warning: ignoring return value of 'fscanf', declared with attribute warn_unused_result [-Wunused-result]

Fix these by checking the return value of those four calls to fscanf.

Signed-off-by: Josh Triplett <[email protected]>
---
tools/power/x86/turbostat/turbostat.c | 20 ++++++++++++++++----
1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c
index dc30f1b..514adba 100644
--- a/tools/power/x86/turbostat/turbostat.c
+++ b/tools/power/x86/turbostat/turbostat.c
@@ -1167,7 +1167,10 @@ int cpu_is_first_sibling_in_core(int cpu)
perror(path);
exit(1);
}
- fscanf(filep, "%d", &first_cpu);
+ if (fscanf(filep, "%d", &first_cpu) != 1) {
+ perror(path);
+ exit(1);
+ }
fclose(filep);
return (cpu == first_cpu);
}
@@ -1188,7 +1191,10 @@ int cpu_is_first_core_in_package(int cpu)
perror(path);
exit(1);
}
- fscanf(filep, "%d", &first_cpu);
+ if (fscanf(filep, "%d", &first_cpu) != 1) {
+ perror(path);
+ exit(1);
+ }
fclose(filep);
return (cpu == first_cpu);
}
@@ -1205,7 +1211,10 @@ int get_physical_package_id(int cpu)
perror(path);
exit(1);
}
- fscanf(filep, "%d", &pkg);
+ if (fscanf(filep, "%d", &pkg) != 1) {
+ perror(path);
+ exit(1);
+ }
fclose(filep);
return pkg;
}
@@ -1222,7 +1231,10 @@ int get_core_id(int cpu)
perror(path);
exit(1);
}
- fscanf(filep, "%d", &core);
+ if (fscanf(filep, "%d", &core) != 1) {
+ perror(path);
+ exit(1);
+ }
fclose(filep);
return core;
}
--
1.8.4.rc3

2013-08-21 00:22:06

by Josh Triplett

[permalink] [raw]
Subject: [PATCH v2 3/8] turbostat: Use GCC's CPUID functions to support PIC

turbostat uses inline assembly to call cpuid. On 32-bit x86, on systems
that have certain security features enabled by default that make -fPIC
the default, this causes a build error:

turbostat.c: In function ‘check_cpuid’:
turbostat.c:1906:2: error: PIC register clobbered by ‘ebx’ in ‘asm’
asm("cpuid" : "=a" (fms), "=c" (ecx), "=d" (edx) : "a" (1) : "ebx");
^

GCC provides a header cpuid.h, containing a __get_cpuid function that
works with both PIC and non-PIC. (On PIC, it saves and restores ebx
around the cpuid instruction.) Use that instead.

Signed-off-by: Josh Triplett <[email protected]>
Cc: [email protected]
---
tools/power/x86/turbostat/turbostat.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c
index b10b8d2..dc30f1b 100644
--- a/tools/power/x86/turbostat/turbostat.c
+++ b/tools/power/x86/turbostat/turbostat.c
@@ -35,6 +35,7 @@
#include <string.h>
#include <ctype.h>
#include <sched.h>
+#include <cpuid.h>

char *proc_stat = "/proc/stat";
unsigned int interval_sec = 5; /* set with -i interval_sec */
@@ -1894,7 +1895,7 @@ void check_cpuid()

eax = ebx = ecx = edx = 0;

- asm("cpuid" : "=a" (max_level), "=b" (ebx), "=c" (ecx), "=d" (edx) : "a" (0));
+ __get_cpuid(0, &max_level, &ebx, &ecx, &edx);

if (ebx == 0x756e6547 && edx == 0x49656e69 && ecx == 0x6c65746e)
genuine_intel = 1;
@@ -1903,7 +1904,7 @@ void check_cpuid()
fprintf(stderr, "CPUID(0): %.4s%.4s%.4s ",
(char *)&ebx, (char *)&edx, (char *)&ecx);

- asm("cpuid" : "=a" (fms), "=c" (ecx), "=d" (edx) : "a" (1) : "ebx");
+ __get_cpuid(1, &fms, &ebx, &ecx, &edx);
family = (fms >> 8) & 0xf;
model = (fms >> 4) & 0xf;
stepping = fms & 0xf;
@@ -1925,7 +1926,7 @@ void check_cpuid()
* This check is valid for both Intel and AMD.
*/
ebx = ecx = edx = 0;
- asm("cpuid" : "=a" (max_level), "=b" (ebx), "=c" (ecx), "=d" (edx) : "a" (0x80000000));
+ __get_cpuid(0x80000000, &max_level, &ebx, &ecx, &edx);

if (max_level < 0x80000007) {
fprintf(stderr, "CPUID: no invariant TSC (max_level 0x%x)\n", max_level);
@@ -1936,7 +1937,7 @@ void check_cpuid()
* Non-Stop TSC is advertised by CPUID.EAX=0x80000007: EDX.bit8
* this check is valid for both Intel and AMD
*/
- asm("cpuid" : "=a" (eax), "=b" (ebx), "=c" (ecx), "=d" (edx) : "a" (0x80000007));
+ __get_cpuid(0x80000007, &eax, &ebx, &ecx, &edx);
has_invariant_tsc = edx & (1 << 8);

if (!has_invariant_tsc) {
@@ -1949,7 +1950,7 @@ void check_cpuid()
* this check is valid for both Intel and AMD
*/

- asm("cpuid" : "=a" (eax), "=b" (ebx), "=c" (ecx), "=d" (edx) : "a" (0x6));
+ __get_cpuid(0x6, &eax, &ebx, &ecx, &edx);
has_aperf = ecx & (1 << 0);
do_dts = eax & (1 << 0);
do_ptm = eax & (1 << 6);
--
1.8.4.rc3

2013-08-21 04:49:43

by Mike Frysinger

[permalink] [raw]
Subject: Re: [PATCH v2 6/8] turbostat: Factor out common function to open file and exit on failure

On Tue, Aug 20, 2013 at 8:20 PM, Josh Triplett <[email protected]> wrote:
> +FILE *fopen_or_die(const char *path, const char *mode)
> +{
> + FILE *filep = fopen(path, "r");

not a big deal, but would be nice to add the "e" flag
-mike