[libvirt] [PATCH 00/10] nodeinfo: Various cleanups
Note: this series is to be applied on top of the [PATCH 00/03] nodeinfo: Various fixes series I've posted at the same time. A bunch of improvements and cleanups that make the nodeinfo code a bit nicer, more streamlined and less redundant, hopefully not just to my eyes. Andrea Bolognani (10): nodeinfo: Introduce linuxGetCPUGlobalPath() nodeinfo: Introduce linuxGetCPUOnlinePath() nodeinfo: Rename linuxParseCPUmax() to linuxParseCPUCount() nodeinfo: Add old kernel compatibility to nodeGetPresentCPUBitmap() nodeinfo: Add out parameter to nodeGetPresentCPUBitmap() 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 | 211 +++++++++++++++++++++++++++++------------------ src/nodeinfo.h | 6 +- src/util/vircgroup.c | 4 +- 4 files changed, 139 insertions(+), 84 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 | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 105d7ab..64b12e6 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -958,16 +958,21 @@ 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; } +# define linuxGetCPUPresentPath(sysfs_prefix) \ + 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
On Fri, Jul 17, 2015 at 18:13:20 +0200, Andrea Bolognani wrote:
This is just a more generic version of linuxGetCPUPresentPath(), which is now implemented by calling the new function appropriately. --- src/nodeinfo.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 105d7ab..64b12e6 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -958,16 +958,21 @@ 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; }
+# define linuxGetCPUPresentPath(sysfs_prefix) \ + linuxGetCPUGlobalPath(sysfs_prefix, "present")
I'd rather see a wrapper function that adds the argument rather than a macro. ACK with that change. Peter
--- src/nodeinfo.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 64b12e6..7a12d54 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -973,6 +973,9 @@ linuxGetCPUGlobalPath(const char *sysfs_prefix, # define linuxGetCPUPresentPath(sysfs_prefix) \ linuxGetCPUGlobalPath(sysfs_prefix, "present") +# define linuxGetCPUOnlinePath(sysfs_prefix) \ + linuxGetCPUGlobalPath(sysfs_prefix, "online") + /* Determine the maximum cpu id from a Linux sysfs cpu/present file. */ static int linuxParseCPUmax(const char *path) @@ -1313,7 +1316,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
On Fri, Jul 17, 2015 at 18:13:21 +0200, Andrea Bolognani wrote:
--- src/nodeinfo.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 64b12e6..7a12d54 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -973,6 +973,9 @@ linuxGetCPUGlobalPath(const char *sysfs_prefix, # define linuxGetCPUPresentPath(sysfs_prefix) \ linuxGetCPUGlobalPath(sysfs_prefix, "present")
+# define linuxGetCPUOnlinePath(sysfs_prefix) \ + linuxGetCPUGlobalPath(sysfs_prefix, "online")
Either add a function ...
+ /* Determine the maximum cpu id from a Linux sysfs cpu/present file. */ static int linuxParseCPUmax(const char *path) @@ -1313,7 +1316,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)))
Or use the global helper here directly. Peter
On Mon, 2015-07-20 at 11:34 +0200, Peter Krempa wrote:
@@ -973,6 +973,9 @@ linuxGetCPUGlobalPath(const char *sysfs_prefix, # define linuxGetCPUPresentPath(sysfs_prefix) \ linuxGetCPUGlobalPath(sysfs_prefix, "present")
+# define linuxGetCPUOnlinePath(sysfs_prefix) \ + linuxGetCPUGlobalPath(sysfs_prefix, "online")
Either add a function ...
+ /* Determine the maximum cpu id from a Linux sysfs cpu/present file. */ static int linuxParseCPUmax(const char *path) @@ -1313,7 +1316,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)))
Or use the global helper here directly.
I'll send v2 converting both this and linuxGetCPUPresentPath() to functions once you're done reviewing the series :) Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
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 functional changes. --- src/nodeinfo.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 7a12d54..52f5594 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -976,9 +976,10 @@ linuxGetCPUGlobalPath(const char *sysfs_prefix, # define linuxGetCPUOnlinePath(sysfs_prefix) \ 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; @@ -1240,7 +1241,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
On Fri, Jul 17, 2015 at 18:13:22 +0200, Andrea Bolognani wrote:
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 functional changes. --- src/nodeinfo.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
ACK
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 | 46 ++++++++++++++++++++++++++++++++-------------- 1 file changed, 32 insertions(+), 14 deletions(-) diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 52f5594..5aa0607 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) @@ -1278,27 +1280,43 @@ 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; + int cpu; - 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; + + for (cpu = 0; cpu < npresent_cpus; cpu++) + if (virBitmapSetBit(present_cpus, cpu) < 0) + goto cleanup; + + 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
On Fri, Jul 17, 2015 at 18:13:23 +0200, Andrea Bolognani wrote:
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 | 46 ++++++++++++++++++++++++++++++++-------------- 1 file changed, 32 insertions(+), 14 deletions(-)
diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 52f5594..5aa0607 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) @@ -1278,27 +1280,43 @@ 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; + int cpu;
- 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; + + for (cpu = 0; cpu < npresent_cpus; cpu++) + if (virBitmapSetBit(present_cpus, cpu) < 0)
virBitmapSetAll();
+ goto cleanup; + + 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; }
ACK with changing to virBitmapSetAll. It makes nodeGetPresentCPUBitmap a little less horrible than it used to be. Peter
This aligns it with nodeGetCPUBitmap(), which already has a similar out parameters, and relieves users of this API from the need to call virBitmapSize() on the returned bitmap. --- src/nodeinfo.c | 8 ++++++-- src/nodeinfo.h | 6 ++++-- src/util/vircgroup.c | 4 +--- 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 5aa0607..7aecc8f 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -440,7 +440,7 @@ virNodeParseNode(const char *sysfs_prefix, goto cleanup; } - present_cpumap = nodeGetPresentCPUBitmap(sysfs_prefix); + present_cpumap = nodeGetPresentCPUBitmap(sysfs_prefix, NULL); if (!present_cpumap) goto cleanup; @@ -1280,7 +1280,8 @@ nodeGetCPUCount(const char *sysfs_prefix ATTRIBUTE_UNUSED) } virBitmapPtr -nodeGetPresentCPUBitmap(const char *sysfs_prefix ATTRIBUTE_UNUSED) +nodeGetPresentCPUBitmap(const char *sysfs_prefix ATTRIBUTE_UNUSED, + int *size ATTRIBUTE_UNUSED) { #ifdef __linux__ virBitmapPtr present_cpus = NULL; @@ -1313,6 +1314,9 @@ nodeGetPresentCPUBitmap(const char *sysfs_prefix ATTRIBUTE_UNUSED) cleanup: VIR_FREE(present_path); + if (present_cpus && size) + *size = npresent_cpus; + return present_cpus; #endif virReportError(VIR_ERR_NO_SUPPORT, "%s", diff --git a/src/nodeinfo.h b/src/nodeinfo.h index 4f983c2..e83db7b 100644 --- a/src/nodeinfo.h +++ b/src/nodeinfo.h @@ -44,8 +44,10 @@ int nodeGetCellsFreeMemory(unsigned long long *freeMems, 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 nodeGetPresentCPUBitmap(const char *sysfs_prefix, + int *size); +virBitmapPtr nodeGetCPUBitmap(const char *sysfs_prefix, + int *size); int nodeGetCPUCount(const char *sysfs_prefix); int nodeGetMemoryParameters(virTypedParameterPtr params, diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 0ef2d29..5011f9c 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -3043,11 +3043,9 @@ virCgroupGetPercpuStats(virCgroupPtr group, } /* To parse account file, we need to know how many cpus are present. */ - if (!(cpumap = nodeGetPresentCPUBitmap(NULL))) + if (!(cpumap = nodeGetPresentCPUBitmap(NULL, &total_cpus))) return rv; - total_cpus = virBitmapSize(cpumap); - if (ncpus == 0) { rv = total_cpus; goto cleanup; -- 2.4.3
On Fri, Jul 17, 2015 at 18:13:24 +0200, Andrea Bolognani wrote:
This aligns it with nodeGetCPUBitmap(), which already has a similar out parameters, and relieves users of this API from the need to call virBitmapSize() on the returned bitmap. --- src/nodeinfo.c | 8 ++++++-- src/nodeinfo.h | 6 ++++-- src/util/vircgroup.c | 4 +--- 3 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 5aa0607..7aecc8f 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -440,7 +440,7 @@ virNodeParseNode(const char *sysfs_prefix, goto cleanup; }
- present_cpumap = nodeGetPresentCPUBitmap(sysfs_prefix); + present_cpumap = nodeGetPresentCPUBitmap(sysfs_prefix, NULL); if (!present_cpumap) goto cleanup;
@@ -1280,7 +1280,8 @@ nodeGetCPUCount(const char *sysfs_prefix ATTRIBUTE_UNUSED) }
virBitmapPtr -nodeGetPresentCPUBitmap(const char *sysfs_prefix ATTRIBUTE_UNUSED) +nodeGetPresentCPUBitmap(const char *sysfs_prefix ATTRIBUTE_UNUSED, + int *size ATTRIBUTE_UNUSED) { #ifdef __linux__ virBitmapPtr present_cpus = NULL; @@ -1313,6 +1314,9 @@ nodeGetPresentCPUBitmap(const char *sysfs_prefix ATTRIBUTE_UNUSED) cleanup: VIR_FREE(present_path);
+ if (present_cpus && size) + *size = npresent_cpus; + return present_cpus; #endif virReportError(VIR_ERR_NO_SUPPORT, "%s", diff --git a/src/nodeinfo.h b/src/nodeinfo.h index 4f983c2..e83db7b 100644 --- a/src/nodeinfo.h +++ b/src/nodeinfo.h @@ -44,8 +44,10 @@ int nodeGetCellsFreeMemory(unsigned long long *freeMems, 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 nodeGetPresentCPUBitmap(const char *sysfs_prefix, + int *size); +virBitmapPtr nodeGetCPUBitmap(const char *sysfs_prefix, + int *size);
I'd prefer something like "ncpus" or maxcpu rather than size. For getting size virBitmapSize() is totally apropriate. Peter
On Mon, 2015-07-20 at 15:15 +0200, Peter Krempa wrote:
-virBitmapPtr nodeGetPresentCPUBitmap(const char *sysfs_prefix); -virBitmapPtr nodeGetCPUBitmap(const char *sysfs_prefix, int *max_id); +virBitmapPtr nodeGetPresentCPUBitmap(const char *sysfs_prefix, + int *size); +virBitmapPtr nodeGetCPUBitmap(const char *sysfs_prefix, + int *size);
I'd prefer something like "ncpus" or maxcpu rather than size. For getting size virBitmapSize() is totally apropriate.
I've used "size" on purpose, because I didn't want people to mistake that for a count of online or present CPUs: it's the size of the returned bitmap, same value you'd get if you called virBitmapSize() on it. Sounds reasonable? Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
On Mon, Jul 20, 2015 at 16:07:42 +0200, Andrea Bolognani wrote:
On Mon, 2015-07-20 at 15:15 +0200, Peter Krempa wrote:
-virBitmapPtr nodeGetPresentCPUBitmap(const char *sysfs_prefix); -virBitmapPtr nodeGetCPUBitmap(const char *sysfs_prefix, int *max_id); +virBitmapPtr nodeGetPresentCPUBitmap(const char *sysfs_prefix, + int *size); +virBitmapPtr nodeGetCPUBitmap(const char *sysfs_prefix, + int *size);
I'd prefer something like "ncpus" or maxcpu rather than size. For getting size virBitmapSize() is totally apropriate.
I've used "size" on purpose, because I didn't want people to mistake that for a count of online or present CPUs: it's the size of the returned bitmap, same value you'd get if you called virBitmapSize() on it.
I thin the 'max_id' or perhaps 'max_cpu_id' were better. Otherwise I'd stay with calling virBitmapSize. It doesn't then look like it's adding any value on top of calling virBitmapSize directly and could actually be optimized out. Peter
On Mon, 2015-07-20 at 16:18 +0200, Peter Krempa wrote:
On Mon, Jul 20, 2015 at 16:07:42 +0200, Andrea Bolognani wrote:
On Mon, 2015-07-20 at 15:15 +0200, Peter Krempa wrote:
-virBitmapPtr nodeGetPresentCPUBitmap(const char *sysfs_prefix); -virBitmapPtr nodeGetCPUBitmap(const char *sysfs_prefix, int *max_id); +virBitmapPtr nodeGetPresentCPUBitmap(const char *sysfs_prefix, + int *size); +virBitmapPtr nodeGetCPUBitmap(const char *sysfs_prefix, + int *size);
I'd prefer something like "ncpus" or maxcpu rather than size. For getting size virBitmapSize() is totally apropriate.
I've used "size" on purpose, because I didn't want people to mistake that for a count of online or present CPUs: it's the size of the returned bitmap, same value you'd get if you called virBitmapSize() on it.
I thin the 'max_id' or perhaps 'max_cpu_id' were better. Otherwise I'd stay with calling virBitmapSize. It doesn't then look like it's adding any value on top of calling virBitmapSize directly and could actually be optimized out.
Using "max_id" is wrong though, because the returned value is the size of the bitmap: if you have 4 CPUs, it will return 4, not 3 as the name "max_id" would suggest. Since virBitmapSize() does very little work anyway, I vote for getting rid of the out parameter altogether. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
The new name makes it clear that the returned bitmap contains the information about which CPUs are online, not eg. which CPUs are present. Change the name of the out parameter from max_id, which didn't reflect the actual value returned, to size. Update the error message as well. No functional changes. --- src/libvirt_private.syms | 2 +- src/nodeinfo.c | 12 ++++++------ src/nodeinfo.h | 4 ++-- 3 files changed, 9 insertions(+), 9 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 7aecc8f..44983b8 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -1325,8 +1325,8 @@ nodeGetPresentCPUBitmap(const char *sysfs_prefix ATTRIBUTE_UNUSED, } virBitmapPtr -nodeGetCPUBitmap(const char *sysfs_prefix ATTRIBUTE_UNUSED, - int *max_id ATTRIBUTE_UNUSED) +nodeGetOnlineCPUBitmap(const char *sysfs_prefix ATTRIBUTE_UNUSED, + int *size ATTRIBUTE_UNUSED) { #ifdef __linux__ const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_SYSTEM_PATH; @@ -1364,15 +1364,15 @@ nodeGetCPUBitmap(const char *sysfs_prefix ATTRIBUTE_UNUSED, ignore_value(virBitmapSetBit(cpumap, i)); } } - if (max_id && cpumap) - *max_id = present; + if (size && cpumap) + *size = present; cleanup: VIR_FREE(online_path); VIR_FREE(cpudir); 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 } @@ -1695,7 +1695,7 @@ nodeGetCPUMap(const char *sysfs_prefix, if (!cpumap && !online) return nodeGetCPUCount(sysfs_prefix); - if (!(cpus = nodeGetCPUBitmap(sysfs_prefix, &maxpresent))) + if (!(cpus = nodeGetOnlineCPUBitmap(sysfs_prefix, &maxpresent))) goto cleanup; if (cpumap && virBitmapToData(cpus, cpumap, &dummy) < 0) diff --git a/src/nodeinfo.h b/src/nodeinfo.h index e83db7b..f47e330 100644 --- a/src/nodeinfo.h +++ b/src/nodeinfo.h @@ -46,8 +46,8 @@ int nodeGetMemory(unsigned long long *mem, virBitmapPtr nodeGetPresentCPUBitmap(const char *sysfs_prefix, int *size); -virBitmapPtr nodeGetCPUBitmap(const char *sysfs_prefix, - int *size); +virBitmapPtr nodeGetOnlineCPUBitmap(const char *sysfs_prefix, + int *size); int nodeGetCPUCount(const char *sysfs_prefix); int nodeGetMemoryParameters(virTypedParameterPtr params, -- 2.4.3
On Fri, Jul 17, 2015 at 18:13:25 +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.
Change the name of the out parameter from max_id, which didn't reflect the actual value returned, to size. Update the error message as well.
No functional changes. --- src/libvirt_private.syms | 2 +- src/nodeinfo.c | 12 ++++++------ src/nodeinfo.h | 4 ++-- 3 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 7aecc8f..44983b8 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -1325,8 +1325,8 @@ nodeGetPresentCPUBitmap(const char *sysfs_prefix ATTRIBUTE_UNUSED, }
virBitmapPtr -nodeGetCPUBitmap(const char *sysfs_prefix ATTRIBUTE_UNUSED, - int *max_id ATTRIBUTE_UNUSED) +nodeGetOnlineCPUBitmap(const char *sysfs_prefix ATTRIBUTE_UNUSED, + int *size ATTRIBUTE_UNUSED)
Same comment for the second parameter name as in the previous patch. ACK to the function name change. Peter
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 44983b8..61a3b33 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 Fri, Jul 17, 2015 at 18:13:26 +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(-)
diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 44983b8..61a3b33 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;
I think this should be a more global setting. We have quite a few places where we invent arbitrary maximum cpu counts. One of them is virProcessSetAffinity.
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;
Like many others, this code would benefit from auto-allocating bitmaps rather than the static ones. Nothing to change though here.
if (sock > sock_max) sock_max = sock;
Otherwise looks good to me, but I'd really want to avoid multiple definitions of the same maximum variable. Peter
On Mon, 2015-07-20 at 15:41 +0200, Peter Krempa wrote:
+ /* 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;
I think this should be a more global setting. We have quite a few places where we invent arbitrary maximum cpu counts. One of them is virProcessSetAffinity.
Definitely agreed. We should define such limits in a single place and stick to them.
Otherwise looks good to me, but I'd really want to avoid multiple definitions of the same maximum variable.
I've left the code unchanged in v2 because this looks like a task that would require quite a bit of research, and I'd prefer if that didn't block an otherwise ACKed series which in turn is a requirement of another series I've posted. So I'm going to look into it and remove duplicate definitions in a follow-up patch, if you're okay with that. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
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 61a3b33..eceecc6 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, NULL); 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 Fri, Jul 17, 2015 at 18:13:27 +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(-)
ACK, Peter
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 | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/src/nodeinfo.c b/src/nodeinfo.c index eceecc6..503fcdd 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; @@ -429,13 +431,17 @@ virNodeParseNode(const char *sysfs_prefix, goto cleanup; } - present_cpumap = nodeGetPresentCPUBitmap(sysfs_prefix, NULL); + present_cpumap = nodeGetPresentCPUBitmap(sysfs_prefix, &npresent_cpus); if (!present_cpumap) goto cleanup; online_cpus_map = nodeGetOnlineCPUBitmap(sysfs_prefix, NULL); if (!online_cpus_map) goto cleanup; + /* 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 +453,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 +490,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 +537,6 @@ virNodeParseNode(const char *sysfs_prefix, *threads = siblings; } - if (direrr < 0) - goto cleanup; - /* finalize the returned data */ *sockets = virBitmapCountBits(sockets_map); @@ -557,6 +562,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 Fri, Jul 17, 2015 at 18:13:28 +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 | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-)
...
@@ -480,13 +490,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))
Perhaps you can use virBitmapNextSetBit to simplify the iteration.
continue;
if (!virBitmapIsBitSet(online_cpus_map, cpu)) {
ACK with or without the iteration modified. Peter
On Mon, 2015-07-20 at 15:47 +0200, Peter Krempa wrote:
+ /* 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))
Perhaps you can use virBitmapNextSetBit to simplify the iteration.
continue;
if (!virBitmapIsBitSet(online_cpus_map, cpu)) {
ACK with or without the iteration modified.
I tried following your suggestion but I didn't like the result very much, so I've left the loop as-is in v2. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
On Fri, Jul 17, 2015 at 18:13:19 +0200, Andrea Bolognani wrote:
Note: this series is to be applied on top of the
[PATCH 00/03] nodeinfo: Various fixes
series I've posted at the same time.
A bunch of improvements and cleanups that make the nodeinfo code a bit nicer, more streamlined and less redundant, hopefully not just to my eyes.
Andrea Bolognani (10): nodeinfo: Introduce linuxGetCPUGlobalPath() nodeinfo: Introduce linuxGetCPUOnlinePath() nodeinfo: Rename linuxParseCPUmax() to linuxParseCPUCount() nodeinfo: Add old kernel compatibility to nodeGetPresentCPUBitmap() nodeinfo: Add out parameter to nodeGetPresentCPUBitmap() 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
Patch 10/10 is missing in the list. Peter
On Mon, 2015-07-20 at 15:48 +0200, Peter Krempa wrote:
Andrea Bolognani (10): nodeinfo: Introduce linuxGetCPUGlobalPath() nodeinfo: Introduce linuxGetCPUOnlinePath() nodeinfo: Rename linuxParseCPUmax() to linuxParseCPUCount() nodeinfo: Add old kernel compatibility to nodeGetPresentCPUBitmap() nodeinfo: Add out parameter to nodeGetPresentCPUBitmap() 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
Patch 10/10 is missing in the list.
Yeah, sorry about that. It's in now. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
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 | 49 +++++++++++++++++++++++++++++-------------------- 1 file changed, 29 insertions(+), 20 deletions(-) diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 503fcdd..0f72a25 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,13 +431,6 @@ virNodeParseNode(const char *sysfs_prefix, goto cleanup; } - present_cpumap = nodeGetPresentCPUBitmap(sysfs_prefix, &npresent_cpus); - if (!present_cpumap) - goto cleanup; - online_cpus_map = nodeGetOnlineCPUBitmap(sysfs_prefix, NULL); - if (!online_cpus_map) - goto cleanup; - /* Keep track of the CPUs that belong to the current node */ if (!(node_cpus_map = virBitmapNew(npresent_cpus))) goto cleanup; @@ -450,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 */ @@ -563,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; } @@ -576,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; @@ -667,6 +661,15 @@ 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, &npresent_cpus); + if (!present_cpus_map) + goto cleanup; + online_cpus_map = nodeGetOnlineCPUBitmap(sysfs_prefix, NULL); + if (!online_cpus_map) + goto cleanup; + /* OK, we've parsed clock speed out of /proc/cpuinfo. Get the * core, node, socket, thread and topology information from /sys */ @@ -688,7 +691,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; @@ -719,7 +724,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; @@ -773,6 +780,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 15:59:02 +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 | 49 +++++++++++++++++++++++++++++-------------------- 1 file changed, 29 insertions(+), 20 deletions(-)
ACK, although rather than patching up nodeinfo.c we should really refactor it since it's a terrible mess. Peter
participants (2)
-
Andrea Bolognani -
Peter Krempa