[libvirt] [PATCH 0/2] Fix NUMA compilation issues
Thanks to Michal who noticed it, I went over the code I reviewed recently (while my brain was out of order, apparently), and found some addiitonal issues that this series is trying to fix with the last one Michal was fixing in his patch (referenced in 1/2). Martin Kletzander (2): numa: split util/ and conf/ and support non-contiguous nodesets numa: fix assumption in virNumaNodeIsAvailable() 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 | 82 +++++++++++++++++------------------------------- src/util/virnuma.h | 7 ++--- tests/qemuxml2argvmock.c | 12 +++++++ 9 files changed, 92 insertions(+), 65 deletions(-) -- 2.1.3
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@redhat.com> --- 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
When compiled without full numa support, the stub function for virNumaNodeIsAvailable() just checks whether specified node is in range <0, max); where max is maximum NUMA node available on the host. But because the maximum node number is the highest usabe number (and not the count of nodes), the check is incorrect as it should check whether the specified node is in range <0, max> instead. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/util/virnuma.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virnuma.c b/src/util/virnuma.c index 20f1e56..06520f7 100644 --- a/src/util/virnuma.c +++ b/src/util/virnuma.c @@ -460,7 +460,7 @@ virNumaNodeIsAvailable(int node) return false; /* Do we have anything better? */ - return (node >= 0) && (node < max_node); + return (node >= 0) && (node <= max_node); } -- 2.1.3
On 11/06/2014 02:24 PM, Martin Kletzander wrote:
When compiled without full numa support, the stub function for virNumaNodeIsAvailable() just checks whether specified node is in range <0, max); where max is maximum NUMA node available on the host. But because the maximum node number is the highest usabe number (and not the
s/usabe/usable/
count of nodes), the check is incorrect as it should check whether the specified node is in range <0, max> instead. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 06.11.2014 14:24, Martin Kletzander wrote:
Thanks to Michal who noticed it, I went over the code I reviewed recently (while my brain was out of order, apparently), and found some addiitonal issues that this series is trying to fix with the last one Michal was fixing in his patch (referenced in 1/2).
Martin Kletzander (2): numa: split util/ and conf/ and support non-contiguous nodesets numa: fix assumption in virNumaNodeIsAvailable()
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 | 82 +++++++++++++++++------------------------------- src/util/virnuma.h | 7 ++--- tests/qemuxml2argvmock.c | 12 +++++++ 9 files changed, 92 insertions(+), 65 deletions(-)
ACK to both. Michal
participants (3)
-
Eric Blake -
Martin Kletzander -
Michal Privoznik