Have you sent this to the sigar list too?
It looks sane to me, but the ultimate goal is to get it upstream.
On Wed, Aug 17, 2011 at 2:54 AM, Zane Bitter <zbitter(a)redhat.com> wrote:
There are a number of problems with the implementation of
sigar_cpu_info_list_get() on Linux:
* The call sysconf(_SC_NPROCESSORS_CONF) returns the number of processors
in the system, not the number of cores (i.e. in a hyperthreaded
system, sigar_cpu_total_count() gives the number of threads, not cores).
* It turns out that the CPUINFO instruction on x86 is not a reliable way of
getting the number of cores, or even the number of threads (which is what
sigar_cpu_core_count() is actually trying to read).
* core_rollup ignores all but one processor per socket, on the assumption
that entries in /proc/cpuinfo appear grouped by socket. This assumption
is not correct.
* The get_cpuinfo_max_freq() function retrieves data corresponding to the
wrong processor in the presence of the core_rollup option (although the
practical effect is cancelled out by the previous issue, purely by
coincidence).
The solution is to obtain all information required about the processors
from the kernel via the /proc/cpuinfo file. This allows us to return the
actual number of cores, rather than threads, in the cores_per_socket and
total_cores variables. It also allows us to return the correct number of
sockets in the system, which was broken by a combination of the first two
issues above. Theoretically, all of this would work even in an asymmetric
system with different numbers of cores in each socket. It should work even
if the processors listed in /proc/cpuinfo are in arbitrary order, even in
the presence of the core_rollup option.
Signed-off-by: Zane Bitter <zbitter(a)redhat.com>
---
src/os/linux/linux_sigar.c | 92 +++++++++++++++++++++++++++++++++++++++-----
1 files changed, 81 insertions(+), 11 deletions(-)
diff --git a/src/os/linux/linux_sigar.c b/src/os/linux/linux_sigar.c
index 0a44623..8f268ad 100644
--- a/src/os/linux/linux_sigar.c
+++ b/src/os/linux/linux_sigar.c
@@ -45,6 +45,13 @@
#define PROC_PARTITIONS PROC_FS_ROOT "partitions"
#define PROC_DISKSTATS PROC_FS_ROOT "diskstats"
+typedef struct {
+ sigar_cpu_info_t cpu;
+ int socket;
+ int core;
+ int processor;
+} processor_info_t;
+
/*
* /proc/self/stat fields:
* 1 - pid
@@ -1498,16 +1505,15 @@ static SIGAR_INLINE void cpu_info_strcpy(char *ptr, char *buf,
int len)
}
}
-static int get_cpu_info(sigar_t *sigar, sigar_cpu_info_t *info,
+static int get_cpu_info(sigar_t *sigar, processor_info_t *proc,
FILE *fp)
{
char buffer[BUFSIZ], *ptr;
+ sigar_cpu_info_t *info = &proc->cpu;
int found = 0;
- /* UML vm wont have "cpu MHz" or "cache size" fields */
- info->mhz = 0;
- info->cache_size = 0;
+ memset(proc, 0, sizeof(*proc));
#ifdef __powerpc64__
SIGAR_SSTRCPY(info->vendor, "IBM");
@@ -1519,6 +1525,10 @@ static int get_cpu_info(sigar_t *sigar, sigar_cpu_info_t *info,
if (strnEQ(ptr, "processor", 9)) {
found = 1;
}
+ else if (strnEQ(ptr, "physical id", 11)) {
+ ptr = cpu_info_strval(ptr);
+ proc->socket = sigar_strtoul(ptr);
+ }
break;
case 'v':
/* "vendor_id" or "vendor" */
@@ -1554,6 +1564,14 @@ static int get_cpu_info(sigar_t *sigar, sigar_cpu_info_t *info,
ptr = cpu_info_strval(ptr);
info->cache_size = sigar_strtoul(ptr);
}
+ else if (strnEQ(ptr, "core id", 7)) {
+ ptr = cpu_info_strval(ptr);
+ proc->core = sigar_strtoul(ptr);
+ }
+ else if (strnEQ(ptr, "cpu cores", 9)) {
+ ptr = cpu_info_strval(ptr);
+ info->cores_per_socket = sigar_strtoul(ptr);
+ }
#ifdef __powerpc64__
/* each /proc/cpuinfo entry looks like so:
* processor : 0
@@ -1601,11 +1619,55 @@ static void get_cpuinfo_max_freq(sigar_cpu_info_t *cpu_info, int
num)
}
}
+typedef struct {
+ size_t size;
+ sigar_uint32_t *data;
+} bitmap_t;
+
+static int bitmap_check(sigar_t *sigar, bitmap_t *bitmap, unsigned int index)
+{
+ int ret = 0;
+ size_t i = index / 32;
+ if (i >= bitmap->size) {
+ void *new_data;
+ size_t old_size = bitmap->size;
+
+ bitmap->size *= 2;
+ if (i >= bitmap->size) {
+ bitmap->size = i + 1;
+ }
+
+ new_data = realloc(bitmap->data,
+ bitmap->size * sizeof(sigar_uint32_t));
+
+ if (new_data) {
+ bitmap->data = new_data;
+ memset(&bitmap->data[old_size], 0,
+ (bitmap->size - old_size) * sizeof(sigar_uint32_t));
+ } else {
+ // Out of memory. This could result in counting a single socket
+ // multiple times if the physical id is huge.
+ sigar_log_printf(sigar, SIGAR_LOG_WARN,
+ "Cannot allocate CPU socket bitmap");
+ return 0;
+ }
+ } else {
+ ret = (bitmap->data[i] & (1 << (index % 32))) != 0;
+ }
+
+ bitmap->data[i] |= 1 << (index % 32);
+
+ return ret;
+}
+
int sigar_cpu_info_list_get(sigar_t *sigar,
sigar_cpu_info_list_t *cpu_infos)
{
FILE *fp;
int core_rollup = sigar_cpu_core_rollup(sigar), i=0;
+ processor_info_t processor;
+ int total_sockets = 0, total_cores = 0;
+ bitmap_t sockets_found = {};
if (!(fp = fopen(PROC_FS_ROOT "cpuinfo", "r"))) {
return errno;
@@ -1614,25 +1676,33 @@ int sigar_cpu_info_list_get(sigar_t *sigar,
(void)sigar_cpu_total_count(sigar);
sigar_cpu_info_list_create(cpu_infos);
- while (get_cpu_info(sigar, &cpu_infos->data[cpu_infos->number], fp)) {
+ while (get_cpu_info(sigar, &processor, fp)) {
sigar_cpu_info_t *info;
- if (core_rollup && (i++ % sigar->lcpu)) {
- continue; /* fold logical processors */
+ if (!bitmap_check(sigar, &sockets_found, processor.socket)) {
+ total_sockets++;
+ total_cores += processor.cpu.cores_per_socket;
+ } else if (core_rollup) {
+ continue;
}
info = &cpu_infos->data[cpu_infos->number];
- get_cpuinfo_max_freq(info, cpu_infos->number);
- info->total_cores = sigar->ncpu;
- info->cores_per_socket = sigar->lcpu;
- info->total_sockets = sigar_cpu_socket_count(sigar);
+ memcpy(info, &processor.cpu, sizeof(*info));
+ get_cpuinfo_max_freq(info, processor.processor);
+ get_cpuinfo_min_freq(info, processor.processor);
++cpu_infos->number;
SIGAR_CPU_INFO_LIST_GROW(cpu_infos);
}
fclose(fp);
+ free(sockets_found.data);
+
+ for (i = 0; i < cpu_infos->number; i++) {
+ cpu_infos->data[i].total_cores = total_cores;
+ cpu_infos->data[i].total_sockets = total_sockets;
+ }
return SIGAR_OK;
}
_______________________________________________
Matahari mailing list
Matahari(a)lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/matahari