This is a reaction to Michal's fix [1] for non-NUMA systems that also splits out conf/ out of util/ because libvirt_util shouldn't require libvirt_conf if it is the other way around. This particular use case worked, but we're trying to avoid it as mentioned [2], many times. The only functions from virnuma.c that needed numatune_conf were virDomainNumatuneNodesetIsAvailable() and virNumaSetupMemoryPolicy(). The first one should be in numatune_conf as it works with virDomainNumatune, the second one just needs nodeset and mode, both of which can be passed without the need of numatune_conf. Apart from fixing that, this patch also fixes recently added code (between commits d2460f85^..5c8515620) that doesn't support non-contiguous nodesets. It uses new function virNumaNodesetIsAvailable(), which doesn't need a stub as it doesn't use any libnuma functions, to check if every specified nodeset is available. [1] https://www.redhat.com/archives/libvir-list/2014-November/msg00118.html [2] http://www.redhat.com/archives/libvir-list/2011-June/msg01040.html Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- src/conf/numatune_conf.c | 24 +++++++++++++++ src/conf/numatune_conf.h | 3 ++ src/libvirt_private.syms | 1 + src/lxc/lxc_controller.c | 17 +++++++--- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_process.c | 9 +++++- src/util/virnuma.c | 80 ++++++++++++++++-------------------------------- src/util/virnuma.h | 7 ++--- tests/qemuxml2argvmock.c | 12 ++++++++ 9 files changed, 91 insertions(+), 64 deletions(-) diff --git a/src/conf/numatune_conf.c b/src/conf/numatune_conf.c index 6986739..ad928e0 100644 --- a/src/conf/numatune_conf.c +++ b/src/conf/numatune_conf.c @@ -26,6 +26,7 @@ #include "domain_conf.h" #include "viralloc.h" +#include "virnuma.h" #include "virstring.h" #define VIR_FROM_THIS VIR_FROM_DOMAIN @@ -640,3 +641,26 @@ virDomainNumatuneSpecifiedMaxNode(virDomainNumatunePtr numatune) return ret; } + +bool +virDomainNumatuneNodesetIsAvailable(virDomainNumatunePtr numatune, + virBitmapPtr auto_nodeset) +{ + size_t i = 0; + virBitmapPtr b = NULL; + + if (!numatune) + return true; + + b = virDomainNumatuneGetNodeset(numatune, auto_nodeset, -1); + if (!virNumaNodesetIsAvailable(b)) + return false; + + for (i = 0; i < numatune->nmem_nodes; i++) { + b = virDomainNumatuneGetNodeset(numatune, auto_nodeset, i); + if (!virNumaNodesetIsAvailable(b)) + return false; + } + + return true; +} diff --git a/src/conf/numatune_conf.h b/src/conf/numatune_conf.h index 15dc0d6..7ca7f97 100644 --- a/src/conf/numatune_conf.h +++ b/src/conf/numatune_conf.h @@ -103,4 +103,7 @@ bool virDomainNumatuneHasPlacementAuto(virDomainNumatunePtr numatune); bool virDomainNumatuneHasPerNodeBinding(virDomainNumatunePtr numatune); int virDomainNumatuneSpecifiedMaxNode(virDomainNumatunePtr numatune); + +bool virDomainNumatuneNodesetIsAvailable(virDomainNumatunePtr numatune, + virBitmapPtr auto_nodeset); #endif /* __NUMATUNE_CONF_H__ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 8895ba1..7e38cc6 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -605,6 +605,7 @@ virDomainNumatuneHasPlacementAuto; virDomainNumatuneMaybeFormatNodeset; virDomainNumatuneMemModeTypeFromString; virDomainNumatuneMemModeTypeToString; +virDomainNumatuneNodesetIsAvailable; virDomainNumatuneParseXML; virDomainNumatunePlacementTypeFromString; virDomainNumatunePlacementTypeToString; diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 1e4b9bc..53a2c8d 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -685,22 +685,29 @@ static int virLXCControllerGetNumadAdvice(virLXCControllerPtr ctrl, */ static int virLXCControllerSetupResourceLimits(virLXCControllerPtr ctrl) { - virBitmapPtr nodemask = NULL; + virBitmapPtr auto_nodeset = NULL; int ret = -1; + virBitmapPtr nodeset = NULL; + virDomainNumatuneMemMode mode; + + if (virLXCControllerGetNumadAdvice(ctrl, &auto_nodeset) < 0) + goto cleanup; + + nodeset = virDomainNumatuneGetNodeset(ctrl->def->numatune, auto_nodeset, -1); + mode = virDomainNumatuneGetMode(ctrl->def->numatune, -1); - if (virLXCControllerGetNumadAdvice(ctrl, &nodemask) < 0 || - virNumaSetupMemoryPolicy(ctrl->def->numatune, nodemask) < 0) + if (virNumaSetupMemoryPolicy(mode, nodeset) < 0) goto cleanup; if (virLXCControllerSetupCpuAffinity(ctrl) < 0) goto cleanup; - if (virLXCCgroupSetup(ctrl->def, ctrl->cgroup, nodemask) < 0) + if (virLXCCgroupSetup(ctrl->def, ctrl->cgroup, nodeset) < 0) goto cleanup; ret = 0; cleanup: - virBitmapFree(nodemask); + virBitmapFree(auto_nodeset); return ret; } diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 13b54dd..a6e19b4 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6665,7 +6665,7 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, goto cleanup; } - if (!virNumaNodesetIsAvailable(def->numatune)) + if (!virDomainNumatuneNodesetIsAvailable(def->numatune, nodeset)) goto cleanup; for (i = 0; i < def->mem.nhugepages; i++) { diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 26d4948..002bd32 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2924,6 +2924,9 @@ static int qemuProcessHook(void *data) struct qemuProcessHookData *h = data; int ret = -1; int fd; + virBitmapPtr nodeset = NULL; + virDomainNumatuneMemMode mode; + /* This method cannot use any mutexes, which are not * protected across fork() */ @@ -2953,7 +2956,11 @@ static int qemuProcessHook(void *data) if (virSecurityManagerClearSocketLabel(h->driver->securityManager, h->vm->def) < 0) goto cleanup; - if (virNumaSetupMemoryPolicy(h->vm->def->numatune, h->nodemask) < 0) + mode = virDomainNumatuneGetMode(h->vm->def->numatune, -1); + nodeset = virDomainNumatuneGetNodeset(h->vm->def->numatune, + h->nodemask, -1); + + if (virNumaSetupMemoryPolicy(mode, nodeset) < 0) goto cleanup; ret = 0; diff --git a/src/util/virnuma.c b/src/util/virnuma.c index ee1c4af..20f1e56 100644 --- a/src/util/virnuma.c +++ b/src/util/virnuma.c @@ -87,8 +87,8 @@ virNumaGetAutoPlacementAdvice(unsigned short vcpus ATTRIBUTE_UNUSED, #if WITH_NUMACTL int -virNumaSetupMemoryPolicy(virDomainNumatunePtr numatune, - virBitmapPtr nodemask) +virNumaSetupMemoryPolicy(virDomainNumatuneMemMode mode, + virBitmapPtr nodeset) { nodemask_t mask; int node = -1; @@ -96,22 +96,17 @@ virNumaSetupMemoryPolicy(virDomainNumatunePtr numatune, int bit = 0; size_t i; int maxnode = 0; - virBitmapPtr tmp_nodemask = NULL; - if (!virNumaNodesetIsAvailable(numatune)) + if (!virNumaNodesetIsAvailable(nodeset)) return -1; - tmp_nodemask = virDomainNumatuneGetNodeset(numatune, nodemask, -1); - if (!tmp_nodemask) - return 0; - maxnode = numa_max_node(); maxnode = maxnode < NUMA_NUM_NODES ? maxnode : NUMA_NUM_NODES; /* Convert nodemask to NUMA bitmask. */ nodemask_zero(&mask); bit = -1; - while ((bit = virBitmapNextSetBit(tmp_nodemask, bit)) >= 0) { + while ((bit = virBitmapNextSetBit(nodeset, bit)) >= 0) { if (bit > maxnode) { virReportError(VIR_ERR_INTERNAL_ERROR, _("NUMA node %d is out of range"), bit); @@ -120,7 +115,7 @@ virNumaSetupMemoryPolicy(virDomainNumatunePtr numatune, nodemask_set(&mask, bit); } - switch (virDomainNumatuneGetMode(numatune, -1)) { + switch (mode) { case VIR_DOMAIN_NUMATUNE_MEM_STRICT: numa_set_bind_policy(1); numa_set_membind(&mask); @@ -163,34 +158,6 @@ virNumaSetupMemoryPolicy(virDomainNumatunePtr numatune, } bool -virNumaNodesetIsAvailable(virDomainNumatunePtr numatune) -{ - int maxnode; - int bit; - - if (!numatune) - return true; - - bit = virDomainNumatuneSpecifiedMaxNode(numatune); - if (bit < 0) - return true; - - if ((maxnode = virNumaGetMaxNode()) < 0) - return false; - - maxnode = maxnode < NUMA_NUM_NODES ? maxnode : NUMA_NUM_NODES; - if (bit > maxnode) - goto error; - - return true; - - error: - virReportError(VIR_ERR_INTERNAL_ERROR, - _("NUMA node %d is out of range"), bit); - return false; -} - -bool virNumaIsAvailable(void) { return numa_available() != -1; @@ -342,28 +309,16 @@ virNumaGetNodeCPUs(int node, #else /* !WITH_NUMACTL */ int -virNumaSetupMemoryPolicy(virDomainNumatunePtr numatune, - virBitmapPtr nodemask ATTRIBUTE_UNUSED) +virNumaSetupMemoryPolicy(virDomainNumatuneMemMode mode ATTRIBUTE_UNUSED, + virBitmapPtr nodeset) { - if (!virNumaNodesetIsAvailable(numatune)) + if (!virNumaNodesetIsAvailable(nodeset)) return -1; return 0; } bool -virNumaNodesetIsAvailable(virDomainNumatunePtr numatune) -{ - if (virDomainNumatuneSpecifiedMaxNode(numatune) >= 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("libvirt is compiled without NUMA tuning support")); - return false; - } - - return true; -} - -bool virNumaIsAvailable(void) { return false; @@ -1006,3 +961,22 @@ virNumaSetPagePoolSize(int node ATTRIBUTE_UNUSED, return -1; } #endif /* #ifdef __linux__ */ + +bool +virNumaNodesetIsAvailable(virBitmapPtr nodeset) +{ + ssize_t bit = -1; + + if (!nodeset) + return true; + + while ((bit = virBitmapNextSetBit(nodeset, bit)) >= 0) { + if (virNumaNodeIsAvailable(bit)) + continue; + + virReportError(VIR_ERR_INTERNAL_ERROR, + _("NUMA node %zd is unavailable"), bit); + return false; + } + return true; +} diff --git a/src/util/virnuma.h b/src/util/virnuma.h index 5bb37b8..09034a2 100644 --- a/src/util/virnuma.h +++ b/src/util/virnuma.h @@ -23,7 +23,6 @@ # define __VIR_NUMA_H__ # include "internal.h" -# include "numatune_conf.h" # include "virbitmap.h" # include "virutil.h" @@ -31,10 +30,10 @@ char *virNumaGetAutoPlacementAdvice(unsigned short vcups, unsigned long long balloon); -int virNumaSetupMemoryPolicy(virDomainNumatunePtr numatune, - virBitmapPtr nodemask); +int virNumaSetupMemoryPolicy(virDomainNumatuneMemMode mode, + virBitmapPtr nodeset); -bool virNumaNodesetIsAvailable(virDomainNumatunePtr numatune); +bool virNumaNodesetIsAvailable(virBitmapPtr nodeset); bool virNumaIsAvailable(void); int virNumaGetMaxNode(void); bool virNumaNodeIsAvailable(int node); diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c index 7218747..eccf4b0 100644 --- a/tests/qemuxml2argvmock.c +++ b/tests/qemuxml2argvmock.c @@ -39,3 +39,15 @@ virNumaGetMaxNode(void) return maxnodesNum; } + +#if WITH_NUMACTL && HAVE_NUMA_BITMASK_ISBITSET +/* + * In case libvirt is compiled with full NUMA support, we need to mock + * this function in order to fake what numa nodes are available. + */ +bool +virNumaNodeIsAvailable(int node) +{ + return node >= 0 && node <= virNumaGetMaxNode(); +} +#endif /* WITH_NUMACTL && HAVE_NUMA_BITMASK_ISBITSET */ -- 2.1.3