[libvirt] [PATCH v2 00/10] nodeinfo: Various cleanups
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
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
--- 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
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
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
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
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
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
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
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
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.
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
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
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
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
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
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
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
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
participants (2)
-
Andrea Bolognani -
Peter Krempa