6:37 p.m.
This should take care of Peter's remarks, except the ones
that I've addressed separately.
Andrea Bolognani (10):
nodeinfo: Introduce linuxGetCPUGlobalPath()
nodeinfo: Introduce linuxGetCPUOnlinePath()
nodeinfo: Rename linuxParseCPUmax() to linuxParseCPUCount()
nodeinfo: Add old kernel compatibility to nodeGetPresentCPUBitmap()
nodeinfo: Remove out parameter from nodeGetCPUBitmap()
nodeinfo: Rename nodeGetCPUBitmap() to nodeGetOnlineCPUBitmap()
nodeinfo: Phase out cpu_set_t usage
nodeinfo: Use nodeGetOnlineCPUBitmap() when parsing node
nodeinfo: Use a bitmap to keep track of node CPUs
nodeinfo: Calculate present and online CPUs only once
src/libvirt_private.syms | 2 +-
src/nodeinfo.c | 214 +++++++++++++++++++++++++++++------------------
src/nodeinfo.h | 2 +-
3 files changed, 136 insertions(+), 82 deletions(-)
--
2.4.3
6:37 p.m.
New subject: [libvirt] [PATCH v2 01/10] nodeinfo: Introduce linuxGetCPUGlobalPath()
This is just a more generic version of linuxGetCPUPresentPath(),
which is now implemented by calling the new function appropriately.
---
src/nodeinfo.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index 105d7ab..b09a4fd 100644
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -958,16 +958,24 @@ linuxNodeGetMemoryStats(FILE *meminfo,
}
static char *
-linuxGetCPUPresentPath(const char *sysfs_prefix)
+linuxGetCPUGlobalPath(const char *sysfs_prefix,
+ const char *file)
{
const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_SYSTEM_PATH;
char *path = NULL;
- if (virAsprintf(&path, "%s/cpu/present", prefix) < 0)
+ if (virAsprintf(&path, "%s/cpu/%s", prefix, file) < 0)
return NULL;
+
return path;
}
+static char *
+linuxGetCPUPresentPath(const char *sysfs_prefix)
+{
+ return linuxGetCPUGlobalPath(sysfs_prefix, "present");
+}
+
/* Determine the maximum cpu id from a Linux sysfs cpu/present file. */
static int
linuxParseCPUmax(const char *path)
--
2.4.3
6:37 p.m.
New subject: [libvirt] [PATCH v2 02/10] nodeinfo: Introduce linuxGetCPUOnlinePath()
---
src/nodeinfo.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index b09a4fd..5459cc6 100644
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -976,6 +976,12 @@ linuxGetCPUPresentPath(const char *sysfs_prefix)
return linuxGetCPUGlobalPath(sysfs_prefix, "present");
}
+static char *
+linuxGetCPUOnlinePath(const char *sysfs_prefix)
+{
+ return linuxGetCPUGlobalPath(sysfs_prefix, "online");
+}
+
/* Determine the maximum cpu id from a Linux sysfs cpu/present file. */
static int
linuxParseCPUmax(const char *path)
@@ -1316,7 +1322,7 @@ nodeGetCPUBitmap(const char *sysfs_prefix ATTRIBUTE_UNUSED,
if (present < 0)
return NULL;
- if (virAsprintf(&online_path, "%s/cpu/online", prefix) < 0)
+ if (!(online_path = linuxGetCPUOnlinePath(sysfs_prefix)))
return NULL;
if (virFileExists(online_path)) {
cpumap = linuxParseCPUmap(present, online_path);
--
2.4.3
6:37 p.m.
New subject: [libvirt] [PATCH v2 03/10] nodeinfo: Rename linuxParseCPUmax() to linuxParseCPUCount()
The original name was confusing because the function returns the number
of CPUs, not the maximum CPU id. The comment above the function has
been updated to reflect this.
No behavioral changes.
---
src/nodeinfo.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index 5459cc6..a7a5d98 100644
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -982,9 +982,10 @@ linuxGetCPUOnlinePath(const char *sysfs_prefix)
return linuxGetCPUGlobalPath(sysfs_prefix, "online");
}
-/* Determine the maximum cpu id from a Linux sysfs cpu/present file. */
+/* Determine the number of CPUs (maximum CPU id + 1) from a file containing
+ * a list of CPU ids, like the Linux sysfs cpu/present file */
static int
-linuxParseCPUmax(const char *path)
+linuxParseCPUCount(const char *path)
{
char *str = NULL;
char *tmp;
@@ -1246,7 +1247,7 @@ nodeGetCPUCount(const char *sysfs_prefix ATTRIBUTE_UNUSED)
return -1;
if (virFileExists(present_path)) {
- ncpu = linuxParseCPUmax(present_path);
+ ncpu = linuxParseCPUCount(present_path);
goto cleanup;
}
--
2.4.3
6:37 p.m.
New subject: [libvirt] [PATCH v2 04/10] nodeinfo: Add old kernel compatibility to nodeGetPresentCPUBitmap()
If the cpu/present file is not available, we assume that the kernel
is too old to support non-consecutive CPU ids and return a bitmap
with all the bits set to represent this fact. This assumption is
already exploited in nodeGetCPUCount().
This means users of this API can expect the information to always
be available unless an error has occurred, and no longer need to
treat the NULL return value as a special case.
The error message has been updated as well.
---
src/nodeinfo.c | 43 +++++++++++++++++++++++++++++--------------
1 file changed, 29 insertions(+), 14 deletions(-)
diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index a7a5d98..ceb517a 100644
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -441,6 +441,8 @@ virNodeParseNode(const char *sysfs_prefix,
}
present_cpumap = nodeGetPresentCPUBitmap(sysfs_prefix);
+ if (!present_cpumap)
+ goto cleanup;
/* enumerate sockets in the node */
CPU_ZERO(&sock_map);
@@ -448,7 +450,7 @@ virNodeParseNode(const char *sysfs_prefix,
if (sscanf(cpudirent->d_name, "cpu%u", &cpu) != 1)
continue;
- if (present_cpumap && !(virBitmapIsBitSet(present_cpumap, cpu)))
+ if (!virBitmapIsBitSet(present_cpumap, cpu))
continue;
if ((online = virNodeGetCpuValue(node, cpu, "online", 1)) < 0)
@@ -484,7 +486,7 @@ virNodeParseNode(const char *sysfs_prefix,
if (sscanf(cpudirent->d_name, "cpu%u", &cpu) != 1)
continue;
- if (present_cpumap && !(virBitmapIsBitSet(present_cpumap, cpu)))
+ if (!virBitmapIsBitSet(present_cpumap, cpu))
continue;
if ((online = virNodeGetCpuValue(node, cpu, "online", 1)) < 0)
@@ -1284,27 +1286,40 @@ nodeGetCPUCount(const char *sysfs_prefix ATTRIBUTE_UNUSED)
}
virBitmapPtr
-nodeGetPresentCPUBitmap(const char *sysfs_prefix)
+nodeGetPresentCPUBitmap(const char *sysfs_prefix ATTRIBUTE_UNUSED)
{
- int max_present;
#ifdef __linux__
+ virBitmapPtr present_cpus = NULL;
char *present_path = NULL;
- virBitmapPtr bitmap = NULL;
-#endif
+ int npresent_cpus;
- if ((max_present = nodeGetCPUCount(sysfs_prefix)) < 0)
- return NULL;
+ if ((npresent_cpus = nodeGetCPUCount(sysfs_prefix)) < 0)
+ goto cleanup;
-#ifdef __linux__
if (!(present_path = linuxGetCPUPresentPath(sysfs_prefix)))
- return NULL;
- if (virFileExists(present_path))
- bitmap = linuxParseCPUmap(max_present, present_path);
+ goto cleanup;
+
+ /* If the cpu/present file is available, parse it and exit */
+ if (virFileExists(present_path)) {
+ present_cpus = linuxParseCPUmap(npresent_cpus, present_path);
+ goto cleanup;
+ }
+
+ /* If the file is not available, we can assume that the kernel is
+ * too old to support non-consecutive CPU ids and just mark all
+ * possible CPUs as present */
+ if (!(present_cpus = virBitmapNew(npresent_cpus)))
+ goto cleanup;
+
+ virBitmapSetAll(present_cpus);
+
+ cleanup:
VIR_FREE(present_path);
- return bitmap;
+
+ return present_cpus;
#endif
virReportError(VIR_ERR_NO_SUPPORT, "%s",
- _("non-continuous host cpu numbers not implemented on this
platform"));
+ _("node present CPU map not implemented on this
platform"));
return NULL;
}
--
2.4.3
6:37 p.m.
New subject: [libvirt] [PATCH v2 05/10] nodeinfo: Remove out parameter from nodeGetCPUBitmap()
Not all users of this API will need the size of the returned
bitmap; those who do can simply call virBitmapSize() themselves.
---
src/nodeinfo.c | 12 +++++-------
src/nodeinfo.h | 2 +-
2 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index ceb517a..69c15fc 100644
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -1324,8 +1324,7 @@ nodeGetPresentCPUBitmap(const char *sysfs_prefix ATTRIBUTE_UNUSED)
}
virBitmapPtr
-nodeGetCPUBitmap(const char *sysfs_prefix ATTRIBUTE_UNUSED,
- int *max_id ATTRIBUTE_UNUSED)
+nodeGetCPUBitmap(const char *sysfs_prefix ATTRIBUTE_UNUSED)
{
#ifdef __linux__
const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_SYSTEM_PATH;
@@ -1363,8 +1362,7 @@ nodeGetCPUBitmap(const char *sysfs_prefix ATTRIBUTE_UNUSED,
ignore_value(virBitmapSetBit(cpumap, i));
}
}
- if (max_id && cpumap)
- *max_id = present;
+
cleanup:
VIR_FREE(online_path);
VIR_FREE(cpudir);
@@ -1685,7 +1683,6 @@ nodeGetCPUMap(const char *sysfs_prefix,
unsigned int flags)
{
virBitmapPtr cpus = NULL;
- int maxpresent;
int ret = -1;
int dummy;
@@ -1694,7 +1691,7 @@ nodeGetCPUMap(const char *sysfs_prefix,
if (!cpumap && !online)
return nodeGetCPUCount(sysfs_prefix);
- if (!(cpus = nodeGetCPUBitmap(sysfs_prefix, &maxpresent)))
+ if (!(cpus = nodeGetCPUBitmap(sysfs_prefix)))
goto cleanup;
if (cpumap && virBitmapToData(cpus, cpumap, &dummy) < 0)
@@ -1702,7 +1699,8 @@ nodeGetCPUMap(const char *sysfs_prefix,
if (online)
*online = virBitmapCountBits(cpus);
- ret = maxpresent;
+ ret = virBitmapSize(cpus);
+
cleanup:
if (ret < 0 && cpumap)
VIR_FREE(*cpumap);
diff --git a/src/nodeinfo.h b/src/nodeinfo.h
index 4f983c2..02af9c5 100644
--- a/src/nodeinfo.h
+++ b/src/nodeinfo.h
@@ -45,7 +45,7 @@ int nodeGetMemory(unsigned long long *mem,
unsigned long long *freeMem);
virBitmapPtr nodeGetPresentCPUBitmap(const char *sysfs_prefix);
-virBitmapPtr nodeGetCPUBitmap(const char *sysfs_prefix, int *max_id);
+virBitmapPtr nodeGetCPUBitmap(const char *sysfs_prefix);
int nodeGetCPUCount(const char *sysfs_prefix);
int nodeGetMemoryParameters(virTypedParameterPtr params,
--
2.4.3
10:18 a.m.
New subject: [libvirt] [PATCH v2 05/10] nodeinfo: Remove out parameter from nodeGetCPUBitmap()
On Mon, Jul 20, 2015 at 18:37:25 +0200, Andrea Bolognani wrote:
Not all users of this API will need the size of the returned
bitmap; those who do can simply call virBitmapSize() themselves.
---
src/nodeinfo.c | 12 +++++-------
src/nodeinfo.h | 2 +-
2 files changed, 6 insertions(+), 8 deletions(-)
ACK
6:37 p.m.
New subject: [libvirt] [PATCH v2 06/10] nodeinfo: Rename nodeGetCPUBitmap() to nodeGetOnlineCPUBitmap()
The new name makes it clear that the returned bitmap contains the
information about which CPUs are online, not eg. which CPUs are
present.
No behavioral change.
---
src/libvirt_private.syms | 2 +-
src/nodeinfo.c | 6 +++---
src/nodeinfo.h | 2 +-
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index b6347de..7f42e40 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -999,7 +999,6 @@ virLockManagerRelease;
nodeAllocPages;
nodeCapsInitNUMA;
nodeGetCellsFreeMemory;
-nodeGetCPUBitmap;
nodeGetCPUCount;
nodeGetCPUMap;
nodeGetCPUStats;
@@ -1008,6 +1007,7 @@ nodeGetInfo;
nodeGetMemory;
nodeGetMemoryParameters;
nodeGetMemoryStats;
+nodeGetOnlineCPUBitmap;
nodeGetPresentCPUBitmap;
nodeSetMemoryParameters;
diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index 69c15fc..3550e24 100644
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -1324,7 +1324,7 @@ nodeGetPresentCPUBitmap(const char *sysfs_prefix ATTRIBUTE_UNUSED)
}
virBitmapPtr
-nodeGetCPUBitmap(const char *sysfs_prefix ATTRIBUTE_UNUSED)
+nodeGetOnlineCPUBitmap(const char *sysfs_prefix ATTRIBUTE_UNUSED)
{
#ifdef __linux__
const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_SYSTEM_PATH;
@@ -1369,7 +1369,7 @@ nodeGetCPUBitmap(const char *sysfs_prefix ATTRIBUTE_UNUSED)
return cpumap;
#else
virReportError(VIR_ERR_NO_SUPPORT, "%s",
- _("node cpumap not implemented on this platform"));
+ _("node online CPU map not implemented on this platform"));
return NULL;
#endif
}
@@ -1691,7 +1691,7 @@ nodeGetCPUMap(const char *sysfs_prefix,
if (!cpumap && !online)
return nodeGetCPUCount(sysfs_prefix);
- if (!(cpus = nodeGetCPUBitmap(sysfs_prefix)))
+ if (!(cpus = nodeGetOnlineCPUBitmap(sysfs_prefix)))
goto cleanup;
if (cpumap && virBitmapToData(cpus, cpumap, &dummy) < 0)
diff --git a/src/nodeinfo.h b/src/nodeinfo.h
index 02af9c5..1810c1c 100644
--- a/src/nodeinfo.h
+++ b/src/nodeinfo.h
@@ -45,7 +45,7 @@ int nodeGetMemory(unsigned long long *mem,
unsigned long long *freeMem);
virBitmapPtr nodeGetPresentCPUBitmap(const char *sysfs_prefix);
-virBitmapPtr nodeGetCPUBitmap(const char *sysfs_prefix);
+virBitmapPtr nodeGetOnlineCPUBitmap(const char *sysfs_prefix);
int nodeGetCPUCount(const char *sysfs_prefix);
int nodeGetMemoryParameters(virTypedParameterPtr params,
--
2.4.3
10:29 a.m.
New subject: [libvirt] [PATCH v2 06/10] nodeinfo: Rename nodeGetCPUBitmap() to nodeGetOnlineCPUBitmap()
On Mon, Jul 20, 2015 at 18:37:26 +0200, Andrea Bolognani wrote:
The new name makes it clear that the returned bitmap contains the
information about which CPUs are online, not eg. which CPUs are
present.
No behavioral change.
---
src/libvirt_private.syms | 2 +-
src/nodeinfo.c | 6 +++---
src/nodeinfo.h | 2 +-
3 files changed, 5 insertions(+), 5 deletions(-)
ACK
6:37 p.m.
New subject: [libvirt] [PATCH v2 07/10] nodeinfo: Phase out cpu_set_t usage
Swap out all instances of cpu_set_t and replace them with virBitmap,
which some of the code was already using anyway.
The changes are pretty mechanical, with one notable exception: an
assumption has been added on the max value we can run into while
reading either socket_it or core_id.
While this specific assumption was not in place before, we were
using cpu_set_t improperly by not making sure not to set any bit
past CPU_SETSIZE or explicitly allocating bigger bitmaps; in fact
the default size of a cpu_set_t, 1024, is way too low to run our
testsuite, which includes core_id values in the 2000s.
---
src/nodeinfo.c | 65 ++++++++++++++++++++++++++++++++++------------------------
1 file changed, 38 insertions(+), 27 deletions(-)
diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index 3550e24..7d0e6b0 100644
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -30,7 +30,6 @@
#include <errno.h>
#include <dirent.h>
#include <sys/utsname.h>
-#include <sched.h>
#include "conf/domain_conf.h"
#if defined(__FreeBSD__) || defined(__APPLE__)
@@ -388,19 +387,6 @@ virNodeParseSocket(const char *dir,
return ret;
}
-# ifndef CPU_COUNT
-static int
-CPU_COUNT(cpu_set_t *set)
-{
- size_t i, count = 0;
-
- for (i = 0; i < CPU_SETSIZE; i++)
- if (CPU_ISSET(i, set))
- count++;
- return count;
-}
-# endif /* !CPU_COUNT */
-
/* parses a node entry, returning number of processors in the node and
* filling arguments */
static int
@@ -415,15 +401,18 @@ virNodeParseNode(const char *sysfs_prefix,
int *threads,
int *offline)
{
+ /* Biggest value we can expect to be used as either socket id
+ * or core id. Bitmaps will need to be sized accordingly */
+ const int ID_MAX = 4095;
int ret = -1;
int processors = 0;
DIR *cpudir = NULL;
struct dirent *cpudirent = NULL;
virBitmapPtr present_cpumap = NULL;
+ virBitmapPtr sockets_map = NULL;
+ virBitmapPtr *cores_maps = NULL;
int sock_max = 0;
- cpu_set_t sock_map;
int sock;
- cpu_set_t *core_maps = NULL;
int core;
size_t i;
int siblings;
@@ -445,7 +434,9 @@ virNodeParseNode(const char *sysfs_prefix,
goto cleanup;
/* enumerate sockets in the node */
- CPU_ZERO(&sock_map);
+ if (!(sockets_map = virBitmapNew(ID_MAX + 1)))
+ goto cleanup;
+
while ((direrr = virDirRead(cpudir, &cpudirent, node)) > 0) {
if (sscanf(cpudirent->d_name, "cpu%u", &cpu) != 1)
continue;
@@ -462,7 +453,15 @@ virNodeParseNode(const char *sysfs_prefix,
/* Parse socket */
if ((sock = virNodeParseSocket(node, arch, cpu)) < 0)
goto cleanup;
- CPU_SET(sock, &sock_map);
+ if (sock > ID_MAX) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Socket %d can't be handled (max socket is
%d)"),
+ sock, ID_MAX);
+ goto cleanup;
+ }
+
+ if (virBitmapSetBit(sockets_map, sock) < 0)
+ goto cleanup;
if (sock > sock_max)
sock_max = sock;
@@ -473,12 +472,13 @@ virNodeParseNode(const char *sysfs_prefix,
sock_max++;
- /* allocate cpu maps for each socket */
- if (VIR_ALLOC_N(core_maps, sock_max) < 0)
+ /* allocate cores maps for each socket */
+ if (VIR_ALLOC_N(cores_maps, sock_max) < 0)
goto cleanup;
for (i = 0; i < sock_max; i++)
- CPU_ZERO(&core_maps[i]);
+ if (!(cores_maps[i] = virBitmapNew(ID_MAX + 1)))
+ goto cleanup;
/* iterate over all CPU's in the node */
rewinddir(cpudir);
@@ -502,7 +502,7 @@ virNodeParseNode(const char *sysfs_prefix,
/* Parse socket */
if ((sock = virNodeParseSocket(node, arch, cpu)) < 0)
goto cleanup;
- if (!CPU_ISSET(sock, &sock_map)) {
+ if (!virBitmapIsBitSet(sockets_map, sock)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("CPU socket topology has changed"));
goto cleanup;
@@ -515,8 +515,15 @@ virNodeParseNode(const char *sysfs_prefix,
} else {
core = virNodeGetCpuValue(node, cpu, "topology/core_id", 0);
}
+ if (core > ID_MAX) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Core %d can't be handled (max core is %d)"),
+ core, ID_MAX);
+ goto cleanup;
+ }
- CPU_SET(core, &core_maps[sock]);
+ if (virBitmapSetBit(cores_maps[sock], core) < 0)
+ goto cleanup;
if (!(siblings = virNodeCountThreadSiblings(node, cpu)))
goto cleanup;
@@ -529,13 +536,13 @@ virNodeParseNode(const char *sysfs_prefix,
goto cleanup;
/* finalize the returned data */
- *sockets = CPU_COUNT(&sock_map);
+ *sockets = virBitmapCountBits(sockets_map);
for (i = 0; i < sock_max; i++) {
- if (!CPU_ISSET(i, &sock_map))
+ if (!virBitmapIsBitSet(sockets_map, i))
continue;
- core = CPU_COUNT(&core_maps[i]);
+ core = virBitmapCountBits(cores_maps[i]);
if (core > *cores)
*cores = core;
}
@@ -548,7 +555,11 @@ virNodeParseNode(const char *sysfs_prefix,
virReportSystemError(errno, _("problem closing %s"), node);
ret = -1;
}
- VIR_FREE(core_maps);
+ if (cores_maps)
+ for (i = 0; i < sock_max; i++)
+ virBitmapFree(cores_maps[i]);
+ VIR_FREE(cores_maps);
+ virBitmapFree(sockets_map);
virBitmapFree(present_cpumap);
return ret;
--
2.4.3
10:32 a.m.
New subject: [libvirt] [PATCH v2 07/10] nodeinfo: Phase out cpu_set_t usage
On Mon, Jul 20, 2015 at 18:37:27 +0200, Andrea Bolognani wrote:
Swap out all instances of cpu_set_t and replace them with virBitmap,
which some of the code was already using anyway.
The changes are pretty mechanical, with one notable exception: an
assumption has been added on the max value we can run into while
reading either socket_it or core_id.
While this specific assumption was not in place before, we were
using cpu_set_t improperly by not making sure not to set any bit
past CPU_SETSIZE or explicitly allocating bigger bitmaps; in fact
the default size of a cpu_set_t, 1024, is way too low to run our
testsuite, which includes core_id values in the 2000s.
---
src/nodeinfo.c | 65 ++++++++++++++++++++++++++++++++++------------------------
1 file changed, 38 insertions(+), 27 deletions(-)
ACK, I agree that the maximum ID can be unified across libvirt in a
separate patch.
6:37 p.m.
New subject: [libvirt] [PATCH v2 08/10] nodeinfo: Use nodeGetOnlineCPUBitmap() when parsing node
No need to look up the online status of each CPU separately when we
can get all the information in one go.
---
src/nodeinfo.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index 7d0e6b0..c97dece 100644
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -409,6 +409,7 @@ virNodeParseNode(const char *sysfs_prefix,
DIR *cpudir = NULL;
struct dirent *cpudirent = NULL;
virBitmapPtr present_cpumap = NULL;
+ virBitmapPtr online_cpus_map = NULL;
virBitmapPtr sockets_map = NULL;
virBitmapPtr *cores_maps = NULL;
int sock_max = 0;
@@ -417,7 +418,6 @@ virNodeParseNode(const char *sysfs_prefix,
size_t i;
int siblings;
unsigned int cpu;
- int online;
int direrr;
*threads = 0;
@@ -432,6 +432,9 @@ virNodeParseNode(const char *sysfs_prefix,
present_cpumap = nodeGetPresentCPUBitmap(sysfs_prefix);
if (!present_cpumap)
goto cleanup;
+ online_cpus_map = nodeGetOnlineCPUBitmap(sysfs_prefix, NULL);
+ if (!online_cpus_map)
+ goto cleanup;
/* enumerate sockets in the node */
if (!(sockets_map = virBitmapNew(ID_MAX + 1)))
@@ -444,10 +447,7 @@ virNodeParseNode(const char *sysfs_prefix,
if (!virBitmapIsBitSet(present_cpumap, cpu))
continue;
- if ((online = virNodeGetCpuValue(node, cpu, "online", 1)) < 0)
- goto cleanup;
-
- if (!online)
+ if (!virBitmapIsBitSet(online_cpus_map, cpu))
continue;
/* Parse socket */
@@ -489,10 +489,7 @@ virNodeParseNode(const char *sysfs_prefix,
if (!virBitmapIsBitSet(present_cpumap, cpu))
continue;
- if ((online = virNodeGetCpuValue(node, cpu, "online", 1)) < 0)
- goto cleanup;
-
- if (!online) {
+ if (!virBitmapIsBitSet(online_cpus_map, cpu)) {
(*offline)++;
continue;
}
@@ -560,6 +557,7 @@ virNodeParseNode(const char *sysfs_prefix,
virBitmapFree(cores_maps[i]);
VIR_FREE(cores_maps);
virBitmapFree(sockets_map);
+ virBitmapFree(online_cpus_map);
virBitmapFree(present_cpumap);
return ret;
--
2.4.3
10:34 a.m.
New subject: [libvirt] [PATCH v2 08/10] nodeinfo: Use nodeGetOnlineCPUBitmap() when parsing node
On Mon, Jul 20, 2015 at 18:37:28 +0200, Andrea Bolognani wrote:
No need to look up the online status of each CPU separately when we
can get all the information in one go.
---
src/nodeinfo.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index 7d0e6b0..c97dece 100644
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -409,6 +409,7 @@ virNodeParseNode(const char *sysfs_prefix,
DIR *cpudir = NULL;
struct dirent *cpudirent = NULL;
virBitmapPtr present_cpumap = NULL;
+ virBitmapPtr online_cpus_map = NULL;
virBitmapPtr sockets_map = NULL;
virBitmapPtr *cores_maps = NULL;
int sock_max = 0;
@@ -417,7 +418,6 @@ virNodeParseNode(const char *sysfs_prefix,
size_t i;
int siblings;
unsigned int cpu;
- int online;
int direrr;
*threads = 0;
@@ -432,6 +432,9 @@ virNodeParseNode(const char *sysfs_prefix,
present_cpumap = nodeGetPresentCPUBitmap(sysfs_prefix);
if (!present_cpumap)
goto cleanup;
+ online_cpus_map = nodeGetOnlineCPUBitmap(sysfs_prefix, NULL);
This can't be compiled since nodeGetOnlineCPUBitmap doesn't have the
second parameter. I see that you've fixed it in next commit so I'll move
it here before pushing.
+ if (!online_cpus_map)
+ goto cleanup;
/* enumerate sockets in the node */
if (!(sockets_map = virBitmapNew(ID_MAX + 1)))
ACK otherwise
10:41 a.m.
New subject: [libvirt] [PATCH v2 08/10] nodeinfo: Use nodeGetOnlineCPUBitmap() when parsing node
On Wed, 2015-07-22 at 10:34 +0200, Peter Krempa wrote:
> @@ -432,6 +432,9 @@ virNodeParseNode(const char *sysfs_prefix,
> present_cpumap = nodeGetPresentCPUBitmap(sysfs_prefix);
> if (!present_cpumap)
> goto cleanup;
> + online_cpus_map = nodeGetOnlineCPUBitmap(sysfs_prefix, NULL);
This can't be compiled since nodeGetOnlineCPUBitmap doesn't have the
second parameter. I see that you've fixed it in next commit so I'll
move
it here before pushing.
Sorry, must have slipped through during one of the rebases.
Thanks for catching it.
Cheers.
--
Andrea Bolognani
Software Engineer - Virtualization Team
6:37 p.m.
New subject: [libvirt] [PATCH v2 09/10] nodeinfo: Use a bitmap to keep track of node CPUs
Keep track of what CPUs belong to the current node while walking
through the sysfs node entry, so we don't need to do it a second
time immediately afterwards.
This also allows us to loop through all CPUs that are part of a
node in guaranteed ascending order, which is something that is
required for some upcoming changes.
---
src/nodeinfo.c | 28 ++++++++++++++++++----------
1 file changed, 18 insertions(+), 10 deletions(-)
diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index c97dece..2328a86 100644
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -410,8 +410,10 @@ virNodeParseNode(const char *sysfs_prefix,
struct dirent *cpudirent = NULL;
virBitmapPtr present_cpumap = NULL;
virBitmapPtr online_cpus_map = NULL;
+ virBitmapPtr node_cpus_map = NULL;
virBitmapPtr sockets_map = NULL;
virBitmapPtr *cores_maps = NULL;
+ int npresent_cpus;
int sock_max = 0;
int sock;
int core;
@@ -432,10 +434,16 @@ virNodeParseNode(const char *sysfs_prefix,
present_cpumap = nodeGetPresentCPUBitmap(sysfs_prefix);
if (!present_cpumap)
goto cleanup;
- online_cpus_map = nodeGetOnlineCPUBitmap(sysfs_prefix, NULL);
+ online_cpus_map = nodeGetOnlineCPUBitmap(sysfs_prefix);
if (!online_cpus_map)
goto cleanup;
+ npresent_cpus = virBitmapSize(present_cpumap);
+
+ /* Keep track of the CPUs that belong to the current node */
+ if (!(node_cpus_map = virBitmapNew(npresent_cpus)))
+ goto cleanup;
+
/* enumerate sockets in the node */
if (!(sockets_map = virBitmapNew(ID_MAX + 1)))
goto cleanup;
@@ -447,6 +455,10 @@ virNodeParseNode(const char *sysfs_prefix,
if (!virBitmapIsBitSet(present_cpumap, cpu))
continue;
+ /* Mark this CPU as part of the current node */
+ if (virBitmapSetBit(node_cpus_map, cpu) < 0)
+ goto cleanup;
+
if (!virBitmapIsBitSet(online_cpus_map, cpu))
continue;
@@ -480,13 +492,11 @@ virNodeParseNode(const char *sysfs_prefix,
if (!(cores_maps[i] = virBitmapNew(ID_MAX + 1)))
goto cleanup;
- /* iterate over all CPU's in the node */
- rewinddir(cpudir);
- while ((direrr = virDirRead(cpudir, &cpudirent, node)) > 0) {
- if (sscanf(cpudirent->d_name, "cpu%u", &cpu) != 1)
- continue;
+ /* Iterate over all CPUs in the node, in ascending order */
+ for (cpu = 0; cpu < npresent_cpus; cpu++) {
- if (!virBitmapIsBitSet(present_cpumap, cpu))
+ /* Skip CPUs that are not part of the current node */
+ if (!virBitmapIsBitSet(node_cpus_map, cpu))
continue;
if (!virBitmapIsBitSet(online_cpus_map, cpu)) {
@@ -529,9 +539,6 @@ virNodeParseNode(const char *sysfs_prefix,
*threads = siblings;
}
- if (direrr < 0)
- goto cleanup;
-
/* finalize the returned data */
*sockets = virBitmapCountBits(sockets_map);
@@ -557,6 +564,7 @@ virNodeParseNode(const char *sysfs_prefix,
virBitmapFree(cores_maps[i]);
VIR_FREE(cores_maps);
virBitmapFree(sockets_map);
+ virBitmapFree(node_cpus_map);
virBitmapFree(online_cpus_map);
virBitmapFree(present_cpumap);
--
2.4.3
10:38 a.m.
New subject: [libvirt] [PATCH v2 09/10] nodeinfo: Use a bitmap to keep track of node CPUs
On Mon, Jul 20, 2015 at 18:37:29 +0200, Andrea Bolognani wrote:
Keep track of what CPUs belong to the current node while walking
through the sysfs node entry, so we don't need to do it a second
time immediately afterwards.
This also allows us to loop through all CPUs that are part of a
node in guaranteed ascending order, which is something that is
required for some upcoming changes.
---
src/nodeinfo.c | 28 ++++++++++++++++++----------
1 file changed, 18 insertions(+), 10 deletions(-)
ACK with the fix to the argument count moved to previous patch.
Peter
6:37 p.m.
New subject: [libvirt] [PATCH v2 10/10] nodeinfo: Calculate present and online CPUs only once
Move the calls to the respective functions from virNodeParseNode(),
which is executed once for every NUMA node, to
linuxNodeInfoCPUPopulate(), which is executed just once per host.
---
src/nodeinfo.c | 53 +++++++++++++++++++++++++++++++----------------------
1 file changed, 31 insertions(+), 22 deletions(-)
diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index 2328a86..31095a4 100644
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -390,12 +390,15 @@ virNodeParseSocket(const char *dir,
/* parses a node entry, returning number of processors in the node and
* filling arguments */
static int
-ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
-ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5)
-ATTRIBUTE_NONNULL(6) ATTRIBUTE_NONNULL(7)
-virNodeParseNode(const char *sysfs_prefix,
- const char *node,
+ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3)
+ATTRIBUTE_NONNULL(5) ATTRIBUTE_NONNULL(6)
+ATTRIBUTE_NONNULL(7) ATTRIBUTE_NONNULL(8)
+ATTRIBUTE_NONNULL(9)
+virNodeParseNode(const char *node,
virArch arch,
+ virBitmapPtr present_cpus_map,
+ int npresent_cpus,
+ virBitmapPtr online_cpus_map,
int *sockets,
int *cores,
int *threads,
@@ -408,12 +411,9 @@ virNodeParseNode(const char *sysfs_prefix,
int processors = 0;
DIR *cpudir = NULL;
struct dirent *cpudirent = NULL;
- virBitmapPtr present_cpumap = NULL;
- virBitmapPtr online_cpus_map = NULL;
virBitmapPtr node_cpus_map = NULL;
virBitmapPtr sockets_map = NULL;
virBitmapPtr *cores_maps = NULL;
- int npresent_cpus;
int sock_max = 0;
int sock;
int core;
@@ -431,15 +431,6 @@ virNodeParseNode(const char *sysfs_prefix,
goto cleanup;
}
- present_cpumap = nodeGetPresentCPUBitmap(sysfs_prefix);
- if (!present_cpumap)
- goto cleanup;
- online_cpus_map = nodeGetOnlineCPUBitmap(sysfs_prefix);
- if (!online_cpus_map)
- goto cleanup;
-
- npresent_cpus = virBitmapSize(present_cpumap);
-
/* Keep track of the CPUs that belong to the current node */
if (!(node_cpus_map = virBitmapNew(npresent_cpus)))
goto cleanup;
@@ -452,7 +443,7 @@ virNodeParseNode(const char *sysfs_prefix,
if (sscanf(cpudirent->d_name, "cpu%u", &cpu) != 1)
continue;
- if (!virBitmapIsBitSet(present_cpumap, cpu))
+ if (!virBitmapIsBitSet(present_cpus_map, cpu))
continue;
/* Mark this CPU as part of the current node */
@@ -565,8 +556,6 @@ virNodeParseNode(const char *sysfs_prefix,
VIR_FREE(cores_maps);
virBitmapFree(sockets_map);
virBitmapFree(node_cpus_map);
- virBitmapFree(online_cpus_map);
- virBitmapFree(present_cpumap);
return ret;
}
@@ -578,10 +567,13 @@ linuxNodeInfoCPUPopulate(const char *sysfs_prefix,
virNodeInfoPtr nodeinfo)
{
const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_SYSTEM_PATH;
+ virBitmapPtr present_cpus_map = NULL;
+ virBitmapPtr online_cpus_map = NULL;
char line[1024];
DIR *nodedir = NULL;
struct dirent *nodedirent = NULL;
int cpus, cores, socks, threads, offline = 0;
+ int npresent_cpus;
unsigned int node;
int ret = -1;
char *sysfs_nodedir = NULL;
@@ -669,6 +661,17 @@ linuxNodeInfoCPUPopulate(const char *sysfs_prefix,
}
}
+ /* Get information about what CPUs are present in the host and what
+ * CPUs are online, so that we don't have to so for each node */
+ present_cpus_map = nodeGetPresentCPUBitmap(sysfs_prefix);
+ if (!present_cpus_map)
+ goto cleanup;
+ online_cpus_map = nodeGetOnlineCPUBitmap(sysfs_prefix);
+ if (!online_cpus_map)
+ goto cleanup;
+
+ npresent_cpus = virBitmapSize(present_cpus_map);
+
/* OK, we've parsed clock speed out of /proc/cpuinfo. Get the
* core, node, socket, thread and topology information from /sys
*/
@@ -690,7 +693,9 @@ linuxNodeInfoCPUPopulate(const char *sysfs_prefix,
prefix, nodedirent->d_name) < 0)
goto cleanup;
- if ((cpus = virNodeParseNode(sysfs_prefix, sysfs_cpudir, arch,
+ if ((cpus = virNodeParseNode(sysfs_cpudir, arch,
+ present_cpus_map, npresent_cpus,
+ online_cpus_map,
&socks, &cores,
&threads, &offline)) < 0)
goto cleanup;
@@ -721,7 +726,9 @@ linuxNodeInfoCPUPopulate(const char *sysfs_prefix,
if (virAsprintf(&sysfs_cpudir, "%s/cpu", prefix) < 0)
goto cleanup;
- if ((cpus = virNodeParseNode(sysfs_prefix, sysfs_cpudir, arch,
+ if ((cpus = virNodeParseNode(sysfs_cpudir, arch,
+ present_cpus_map, npresent_cpus,
+ online_cpus_map,
&socks, &cores,
&threads, &offline)) < 0)
goto cleanup;
@@ -775,6 +782,8 @@ linuxNodeInfoCPUPopulate(const char *sysfs_prefix,
ret = -1;
}
+ virBitmapFree(present_cpus_map);
+ virBitmapFree(online_cpus_map);
VIR_FREE(sysfs_nodedir);
VIR_FREE(sysfs_cpudir);
return ret;
--
2.4.3
10:44 a.m.
New subject: [libvirt] [PATCH v2 10/10] nodeinfo: Calculate present and online CPUs only once
On Mon, Jul 20, 2015 at 18:37:30 +0200, Andrea Bolognani wrote:
Move the calls to the respective functions from virNodeParseNode(),
which is executed once for every NUMA node, to
linuxNodeInfoCPUPopulate(), which is executed just once per host.
---
src/nodeinfo.c | 53 +++++++++++++++++++++++++++++++----------------------
1 file changed, 31 insertions(+), 22 deletions(-)
diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index 2328a86..31095a4 100644
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -390,12 +390,15 @@ virNodeParseSocket(const char *dir,
/* parses a node entry, returning number of processors in the node and
* filling arguments */
static int
-ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
-ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5)
-ATTRIBUTE_NONNULL(6) ATTRIBUTE_NONNULL(7)
-virNodeParseNode(const char *sysfs_prefix,
- const char *node,
+ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3)
+ATTRIBUTE_NONNULL(5) ATTRIBUTE_NONNULL(6)
+ATTRIBUTE_NONNULL(7) ATTRIBUTE_NONNULL(8)
+ATTRIBUTE_NONNULL(9)
+virNodeParseNode(const char *node,
virArch arch,
+ virBitmapPtr present_cpus_map,
+ int npresent_cpus,
+ virBitmapPtr online_cpus_map,
int *sockets,
int *cores,
int *threads,
@@ -408,12 +411,9 @@ virNodeParseNode(const char *sysfs_prefix,
int processors = 0;
DIR *cpudir = NULL;
struct dirent *cpudirent = NULL;
- virBitmapPtr present_cpumap = NULL;
- virBitmapPtr online_cpus_map = NULL;
virBitmapPtr node_cpus_map = NULL;
virBitmapPtr sockets_map = NULL;
virBitmapPtr *cores_maps = NULL;
- int npresent_cpus;
int sock_max = 0;
int sock;
int core;
@@ -431,15 +431,6 @@ virNodeParseNode(const char *sysfs_prefix,
goto cleanup;
}
- present_cpumap = nodeGetPresentCPUBitmap(sysfs_prefix);
- if (!present_cpumap)
- goto cleanup;
- online_cpus_map = nodeGetOnlineCPUBitmap(sysfs_prefix);
- if (!online_cpus_map)
- goto cleanup;
-
- npresent_cpus = virBitmapSize(present_cpumap);
This still can be extracted here since the caller doesn't care about the
npreset count so you are basically passing redundant info.
ACK to the rest. I'll revert this part and adjust the patch accordingly
before pushing.
Peter
11:24 a.m.
New subject: [libvirt] [PATCH v2 10/10] nodeinfo: Calculate present and online CPUs only once
On Wed, 2015-07-22 at 10:44 +0200, Peter Krempa wrote:
> - present_cpumap = nodeGetPresentCPUBitmap(sysfs_prefix);
> - if (!present_cpumap)
> - goto cleanup;
> - online_cpus_map = nodeGetOnlineCPUBitmap(sysfs_prefix);
> - if (!online_cpus_map)
> - goto cleanup;
> -
> - npresent_cpus = virBitmapSize(present_cpumap);
This still can be extracted here since the caller doesn't care about
the
npreset count so you are basically passing redundant info.
ACK to the rest. I'll revert this part and adjust the patch
accordingly
before pushing.
Sure, it was intended as an optimization but then again
we already agreed that it doesn't really make sense
because virBitmapSize() does so little, plus it indeed
looks cleaner if the caller is passing one less parameter.
Thanks.
--
Andrea Bolognani
Software Engineer - Virtualization Team
3347
days inactive
3349
days old
19 comments
2 participants
participants (2)
-
Andrea Bolognani -
Peter Krempa