[libvirt] [PATCH 0/4] Start cleaning up nodeinfo.c
Some parts of nodeinfo.c are a mess. This series is a start of cleanup effort on that file that should result also into tests being written to test the nodeinfo code as it was shown to be fragile. To avoid sending a huge series I will send it continuously as I progress. Peter Krempa (4): nodeinfo: Rename linuxNodeInfoCPUPopulate and export it properly numa: Introduce virNumaIsAvailable and use it instead of numa_available nodeinfo: Avoid forward declarations of static functions numa: Introduce virNumaGetMaxNode and use it instead of numa_max_node src/libvirt_linux.syms | 2 +- src/libvirt_private.syms | 2 + src/nodeinfo.c | 173 +++++++++++++++++++++++------------------------ src/nodeinfo.h | 5 ++ src/util/virnuma.c | 51 ++++++++++++++ src/util/virnuma.h | 4 ++ tests/nodeinfotest.c | 6 +- 7 files changed, 148 insertions(+), 95 deletions(-) -- 1.8.3.2
Call it virNodeInfoLinuxPopulateCPU and use the header file to export it instead of extern definition in the test file. --- src/libvirt_linux.syms | 2 +- src/nodeinfo.c | 14 +++++--------- src/nodeinfo.h | 5 +++++ tests/nodeinfotest.c | 6 +----- 4 files changed, 12 insertions(+), 15 deletions(-) diff --git a/src/libvirt_linux.syms b/src/libvirt_linux.syms index 3500898..bfef3ec 100644 --- a/src/libvirt_linux.syms +++ b/src/libvirt_linux.syms @@ -3,7 +3,7 @@ # # nodeinfo.h -linuxNodeInfoCPUPopulate; +virNodeInfoLinuxPopulateCPU; # util/virstatslinux.h linuxDomainInterfaceStats; diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 70814c2..232b465 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -88,11 +88,6 @@ appleFreebsdNodeGetCPUCount(void) # define LINUX_NB_MEMORY_STATS_ALL 4 # define LINUX_NB_MEMORY_STATS_CELL 2 -/* NB, this is not static as we need to call it from the testsuite */ -int linuxNodeInfoCPUPopulate(FILE *cpuinfo, - const char *sysfs_dir, - virNodeInfoPtr nodeinfo); - static int linuxNodeGetCPUStats(FILE *procstat, int cpuNum, virNodeCPUStatsPtr params, @@ -376,9 +371,10 @@ cleanup: return ret; } -int linuxNodeInfoCPUPopulate(FILE *cpuinfo, - const char *sysfs_dir, - virNodeInfoPtr nodeinfo) +int +virNodeInfoLinuxPopulateCPU(FILE *cpuinfo, + const char *sysfs_dir, + virNodeInfoPtr nodeinfo) { char line[1024]; DIR *nodedir = NULL; @@ -872,7 +868,7 @@ int nodeGetInfo(virNodeInfoPtr nodeinfo) return -1; } - ret = linuxNodeInfoCPUPopulate(cpuinfo, SYSFS_SYSTEM_PATH, nodeinfo); + ret = virNodeInfoLinuxPopulateCPU(cpuinfo, SYSFS_SYSTEM_PATH, nodeinfo); if (ret < 0) goto cleanup; diff --git a/src/nodeinfo.h b/src/nodeinfo.h index 413fddd..a13cf28 100644 --- a/src/nodeinfo.h +++ b/src/nodeinfo.h @@ -57,4 +57,9 @@ int nodeGetCPUMap(unsigned char **cpumap, unsigned int *online, unsigned int flags); + +int virNodeInfoLinuxPopulateCPU(FILE *cpuinfo, + const char *sysfs_dir, + virNodeInfoPtr nodeinfo); + #endif /* __VIR_NODEINFO_H__*/ diff --git a/tests/nodeinfotest.c b/tests/nodeinfotest.c index 74f6d4d..9bb7adb 100644 --- a/tests/nodeinfotest.c +++ b/tests/nodeinfotest.c @@ -27,10 +27,6 @@ main(void) #else -extern int linuxNodeInfoCPUPopulate(FILE *cpuinfo, - char *sysfs_dir, - virNodeInfoPtr nodeinfo); - static int linuxTestCompareFiles(const char *cpuinfofile, char *sysfs_dir, @@ -50,7 +46,7 @@ linuxTestCompareFiles(const char *cpuinfofile, goto fail; memset(&nodeinfo, 0, sizeof(nodeinfo)); - if (linuxNodeInfoCPUPopulate(cpuinfo, sysfs_dir, &nodeinfo) < 0) { + if (virNodeInfoLinuxPopulateCPU(cpuinfo, sysfs_dir, &nodeinfo) < 0) { if (virTestGetDebug()) { virErrorPtr error = virSaveLastError(); if (error && error->code != VIR_ERR_OK) -- 1.8.3.2
All functions from libnuma must be protected with ifdefs. Avoid this by using our own wrapper. --- src/libvirt_private.syms | 1 + src/nodeinfo.c | 13 +++++-------- src/util/virnuma.c | 14 ++++++++++++++ src/util/virnuma.h | 2 ++ 4 files changed, 22 insertions(+), 8 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 84c1c28..48d3da7 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1520,6 +1520,7 @@ virNodeSuspendGetTargetMask; virDomainNumatuneMemModeTypeFromString; virDomainNumatuneMemModeTypeToString; virNumaGetAutoPlacementAdvice; +virNumaIsAvailable; virNumaSetupMemoryPolicy; virNumaTuneMemPlacementModeTypeFromString; virNumaTuneMemPlacementModeTypeToString; diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 232b465..d4bb792 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -55,6 +55,7 @@ #include "virfile.h" #include "virtypedparam.h" #include "virstring.h" +#include "virnuma.h" #define VIR_FROM_THIS VIR_FROM_NONE @@ -977,15 +978,11 @@ int nodeGetMemoryStats(int cellNum ATTRIBUTE_UNUSED, if (VIR_STRDUP(meminfo_path, MEMINFO_PATH) < 0) return -1; } else { -# if WITH_NUMACTL - if (numa_available() < 0) { -# endif + if (!virNumaIsAvailable()) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("NUMA not supported on this host")); return -1; -# if WITH_NUMACTL } -# endif # if WITH_NUMACTL if (cellNum > numa_max_node()) { @@ -1600,7 +1597,7 @@ nodeCapsInitNUMA(virCapsPtr caps) int ncpus = 0; bool topology_failed = false; - if (numa_available() < 0) + if (!virNumaIsAvailable()) return nodeCapsInitNUMAFake(caps); int mask_n_bytes = max_n_cpus / 8; @@ -1672,7 +1669,7 @@ nodeGetCellsFreeMemory(unsigned long long *freeMems, int ret = -1; int maxCell; - if (numa_available() < 0) + if (!virNumaIsAvailable()) return nodeGetCellsFreeMemoryFake(freeMems, startCell, maxCells); @@ -1706,7 +1703,7 @@ nodeGetFreeMemory(void) unsigned long long freeMem = 0; int n; - if (numa_available() < 0) + if (!virNumaIsAvailable()) return nodeGetFreeMemoryFake(); diff --git a/src/util/virnuma.c b/src/util/virnuma.c index ecf7ede..11078a1 100644 --- a/src/util/virnuma.c +++ b/src/util/virnuma.c @@ -167,6 +167,13 @@ virNumaSetupMemoryPolicy(virNumaTuneDef numatune, cleanup: return ret; } + + +bool +virNumaIsAvailable(void) +{ + return numa_available() != -1; +} #else int virNumaSetupMemoryPolicy(virNumaTuneDef numatune, @@ -181,4 +188,11 @@ virNumaSetupMemoryPolicy(virNumaTuneDef numatune, return 0; } + + +bool +virNumaIsAvailable(void) +{ + return false; +} #endif diff --git a/src/util/virnuma.h b/src/util/virnuma.h index 9ff8e69..5f9c38b 100644 --- a/src/util/virnuma.h +++ b/src/util/virnuma.h @@ -55,4 +55,6 @@ char *virNumaGetAutoPlacementAdvice(unsigned short vcups, int virNumaSetupMemoryPolicy(virNumaTuneDef numatune, virBitmapPtr nodemask); + +bool virNumaIsAvailable(void); #endif /* __VIR_NUMA_H__ */ -- 1.8.3.2
linuxNodeGetCPUStats() and linuxNodeGetMemoryStats() are static and don't need a forward declaration. Forward declaration of nodeGetCellMemory() can be avoided by moving the function to the right place. --- src/nodeinfo.c | 113 +++++++++++++++++++++++++++------------------------------ 1 file changed, 54 insertions(+), 59 deletions(-) diff --git a/src/nodeinfo.c b/src/nodeinfo.c index d4bb792..8646a50 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -89,15 +89,6 @@ appleFreebsdNodeGetCPUCount(void) # define LINUX_NB_MEMORY_STATS_ALL 4 # define LINUX_NB_MEMORY_STATS_CELL 2 -static int linuxNodeGetCPUStats(FILE *procstat, - int cpuNum, - virNodeCPUStatsPtr params, - int *nparams); -static int linuxNodeGetMemoryStats(FILE *meminfo, - int cellNum, - virNodeMemoryStatsPtr params, - int *nparams); - /* Return the positive decimal contents of the given * DIR/cpu%u/FILE, or -1 on error. If DEFAULT_VALUE is non-negative * and the file could not be found, return that instead of an error; @@ -586,10 +577,11 @@ cleanup: # define TICK_TO_NSEC (1000ull * 1000ull * 1000ull / sysconf(_SC_CLK_TCK)) -int linuxNodeGetCPUStats(FILE *procstat, - int cpuNum, - virNodeCPUStatsPtr params, - int *nparams) +static int +linuxNodeGetCPUStats(FILE *procstat, + int cpuNum, + virNodeCPUStatsPtr params, + int *nparams) { int ret = -1; char line[1024]; @@ -689,10 +681,11 @@ cleanup: return ret; } -int linuxNodeGetMemoryStats(FILE *meminfo, - int cellNum, - virNodeMemoryStatsPtr params, - int *nparams) +static int +linuxNodeGetMemoryStats(FILE *meminfo, + int cellNum, + virNodeMemoryStatsPtr params, + int *nparams) { int ret = -1; size_t i = 0, j = 0, k = 0; @@ -1534,7 +1527,6 @@ nodeGetFreeMemoryFake(void) # define MASK_CPU_ISSET(mask, cpu) \ (((mask)[((cpu) / n_bits(*(mask)))] >> ((cpu) % n_bits(*(mask)))) & 1) -static unsigned long long nodeGetCellMemory(int cell); static virBitmapPtr virNodeGetSiblingsList(const char *dir, int cpu_id) @@ -1584,6 +1576,50 @@ virNodeCapsFillCPUInfo(int cpu_id, virCapsHostNUMACellCPUPtr cpu) return 0; } + +/** + * nodeGetCellMemory + * @cell: The number of the numa cell to get memory info for. + * + * Will call the numa_node_size64() function from libnuma to get + * the amount of total memory in bytes. It is then converted to + * KiB and returned. + * + * Returns 0 if unavailable, amount of memory in KiB on success. + */ +static unsigned long long +nodeGetCellMemory(int cell) +{ + long long mem; + unsigned long long memKiB = 0; + int maxCell; + + /* Make sure the provided cell number is valid. */ + maxCell = numa_max_node(); + if (cell > maxCell) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cell %d out of range (0-%d)"), + cell, maxCell); + goto cleanup; + } + + /* Get the amount of memory(bytes) in the node */ + mem = numa_node_size64(cell, NULL); + if (mem < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to query NUMA total memory for node: %d"), + cell); + goto cleanup; + } + + /* Convert the memory from bytes to KiB */ + memKiB = mem >> 10; + +cleanup: + return memKiB; +} + + int nodeCapsInitNUMA(virCapsPtr caps) { @@ -1718,47 +1754,6 @@ nodeGetFreeMemory(void) return freeMem; } -/** - * nodeGetCellMemory - * @cell: The number of the numa cell to get memory info for. - * - * Will call the numa_node_size64() function from libnuma to get - * the amount of total memory in bytes. It is then converted to - * KiB and returned. - * - * Returns 0 if unavailable, amount of memory in KiB on success. - */ -static unsigned long long nodeGetCellMemory(int cell) -{ - long long mem; - unsigned long long memKiB = 0; - int maxCell; - - /* Make sure the provided cell number is valid. */ - maxCell = numa_max_node(); - if (cell > maxCell) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cell %d out of range (0-%d)"), - cell, maxCell); - goto cleanup; - } - - /* Get the amount of memory(bytes) in the node */ - mem = numa_node_size64(cell, NULL); - if (mem < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to query NUMA total memory for node: %d"), - cell); - goto cleanup; - } - - /* Convert the memory from bytes to KiB */ - memKiB = mem >> 10; - -cleanup: - return memKiB; -} - #else int nodeCapsInitNUMA(virCapsPtr caps) { -- 1.8.3.2
Avoid necessary checks for the numa library with this helper. --- src/libvirt_private.syms | 1 + src/nodeinfo.c | 37 ++++++++++++++++++++++--------------- src/util/virnuma.c | 37 +++++++++++++++++++++++++++++++++++++ src/util/virnuma.h | 2 ++ 4 files changed, 62 insertions(+), 15 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 48d3da7..c8959ad 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1520,6 +1520,7 @@ virNodeSuspendGetTargetMask; virDomainNumatuneMemModeTypeFromString; virDomainNumatuneMemModeTypeToString; virNumaGetAutoPlacementAdvice; +virNumaGetMaxNode; virNumaIsAvailable; virNumaSetupMemoryPolicy; virNumaTuneMemPlacementModeTypeFromString; diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 8646a50..2a78778 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -966,25 +966,21 @@ int nodeGetMemoryStats(int cellNum ATTRIBUTE_UNUSED, int ret; char *meminfo_path = NULL; FILE *meminfo; + int max_node; if (cellNum == VIR_NODE_MEMORY_STATS_ALL_CELLS) { if (VIR_STRDUP(meminfo_path, MEMINFO_PATH) < 0) return -1; } else { - if (!virNumaIsAvailable()) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("NUMA not supported on this host")); + if ((max_node = virNumaGetMaxNode()) < 0) return -1; - } -# if WITH_NUMACTL - if (cellNum > numa_max_node()) { + if (cellNum > max_node) { virReportInvalidArg(cellNum, _("cellNum in %s must be less than or equal to %d"), - __FUNCTION__, numa_max_node()); + __FUNCTION__, max_node); return -1; } -# endif if (virAsprintf(&meminfo_path, "%s/node/node%d/meminfo", SYSFS_SYSTEM_PATH, cellNum) < 0) @@ -1595,7 +1591,9 @@ nodeGetCellMemory(int cell) int maxCell; /* Make sure the provided cell number is valid. */ - maxCell = numa_max_node(); + if ((maxCell = virNumaGetMaxNode()) < 0) + return 0; + if (cell > maxCell) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cell %d out of range (0-%d)"), @@ -1630,31 +1628,35 @@ nodeCapsInitNUMA(virCapsPtr caps) virCapsHostNUMACellCPUPtr cpus = NULL; int ret = -1; int max_n_cpus = NUMA_MAX_N_CPUS; + int mask_n_bytes = max_n_cpus / 8; int ncpus = 0; bool topology_failed = false; + int max_node; if (!virNumaIsAvailable()) return nodeCapsInitNUMAFake(caps); - int mask_n_bytes = max_n_cpus / 8; + if ((max_node = virNumaGetMaxNode()) < 0) + goto cleanup; + if (VIR_ALLOC_N(mask, mask_n_bytes / sizeof(*mask)) < 0) goto cleanup; if (VIR_ALLOC_N(allonesmask, mask_n_bytes / sizeof(*mask)) < 0) goto cleanup; memset(allonesmask, 0xff, mask_n_bytes); - for (n = 0; n <= numa_max_node(); n++) { + for (n = 0; n <= max_node; n++) { size_t i; /* The first time this returns -1, ENOENT if node doesn't exist... */ if (numa_node_to_cpus(n, mask, mask_n_bytes) < 0) { VIR_WARN("NUMA topology for cell %d of %d not available, ignoring", - n, numa_max_node()+1); + n, max_node + 1); continue; } /* second, third... times it returns an all-1's mask */ if (memcmp(mask, allonesmask, mask_n_bytes) == 0) { VIR_DEBUG("NUMA topology for cell %d of %d is all ones, ignoring", - n, numa_max_node()+1); + n, max_node + 1); continue; } @@ -1709,7 +1711,9 @@ nodeGetCellsFreeMemory(unsigned long long *freeMems, return nodeGetCellsFreeMemoryFake(freeMems, startCell, maxCells); - maxCell = numa_max_node(); + if ((maxCell = virNumaGetMaxNode()) < 0) + return 0; + if (startCell > maxCell) { virReportError(VIR_ERR_INTERNAL_ERROR, _("start cell %d out of range (0-%d)"), @@ -1737,13 +1741,16 @@ unsigned long long nodeGetFreeMemory(void) { unsigned long long freeMem = 0; + int max_node; int n; if (!virNumaIsAvailable()) return nodeGetFreeMemoryFake(); + if ((max_node = virNumaGetMaxNode()) < 0) + return 0; - for (n = 0; n <= numa_max_node(); n++) { + for (n = 0; n <= max_node; n++) { long long mem; if (numa_node_size64(n, &mem) < 0) continue; diff --git a/src/util/virnuma.c b/src/util/virnuma.c index 11078a1..2d8d0e9 100644 --- a/src/util/virnuma.c +++ b/src/util/virnuma.c @@ -174,6 +174,34 @@ virNumaIsAvailable(void) { return numa_available() != -1; } + + +/** + * virNumaGetMaxNode: + * Get the highest node number available on the current system. + * (See the node numbers in /sys/devices/system/node/ ). + * + * Returns the highes NUMA node id on success, -1 on error. + */ +int +virNumaGetMaxNode(void) +{ + int ret; + + if (!virNumaIsAvailable()) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("NUMA isn't available on this host")); + return -1; + } + + if ((ret = numa_max_node()) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Failed to request maximum NUMA node id")); + return -1; + } + + return ret; +} #else int virNumaSetupMemoryPolicy(virNumaTuneDef numatune, @@ -195,4 +223,13 @@ virNumaIsAvailable(void) { return false; } + + +int +virNumaGetMaxNode(void) +{ + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("NUMA isn't available on this host")); + return -1; +} #endif diff --git a/src/util/virnuma.h b/src/util/virnuma.h index 5f9c38b..65636c3 100644 --- a/src/util/virnuma.h +++ b/src/util/virnuma.h @@ -57,4 +57,6 @@ int virNumaSetupMemoryPolicy(virNumaTuneDef numatune, virBitmapPtr nodemask); bool virNumaIsAvailable(void); +int virNumaGetMaxNode(void); + #endif /* __VIR_NUMA_H__ */ -- 1.8.3.2
On 10/17/13 18:45, Peter Krempa wrote:
Some parts of nodeinfo.c are a mess. This series is a start of cleanup effort on that file that should result also into tests being written to test the nodeinfo code as it was shown to be fragile.
To avoid sending a huge series I will send it continuously as I progress.
Peter Krempa (4): nodeinfo: Rename linuxNodeInfoCPUPopulate and export it properly numa: Introduce virNumaIsAvailable and use it instead of numa_available nodeinfo: Avoid forward declarations of static functions numa: Introduce virNumaGetMaxNode and use it instead of numa_max_node
Self NACK to this series. One of the patch moves a piece of code out of a #ifdef block which might break compilation. I will post a more complete version later. Peter
participants (1)
-
Peter Krempa