From bcd9a564b631aa5fdf9594599db0ab0414c5556b Mon Sep 17 00:00:00 2001 From: Michal Privoznik Date: Tue, 19 May 2015 11:55:26 +0200 Subject: [PATCH] virDomainNumatuneGetMode: Report if numatune was defined So far, we are not reporting if numatune was even defined. The value of zero is blindly returned (which maps onto VIR_DOMAIN_NUMATUNE_MEM_STRICT). Unfortunately, we are making decisions based on this value. Instead, we should not only return the correct value, but report to the caller if the value is valid at all. For better viewing of this patch use '-w'. Signed-off-by: Michal Privoznik --- src/conf/numa_conf.c | 38 ++++++++++++++++++++++++++--------- src/conf/numa_conf.h | 5 +++-- src/lxc/lxc_cgroup.c | 5 +++-- src/lxc/lxc_controller.c | 30 +++++++++++++-------------- src/parallels/parallels_sdk.c | 5 +++-- src/qemu/qemu_cgroup.c | 15 ++++++++------ src/qemu/qemu_command.c | 4 +++- src/qemu/qemu_driver.c | 22 ++++++++++++-------- src/qemu/qemu_process.c | 28 +++++++++++++------------- 9 files changed, 93 insertions(+), 59 deletions(-) diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index e4dc2f88ed..57da215aed 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -351,20 +351,40 @@ virDomainNumaFree(virDomainNumaPtr numa) VIR_FREE(numa); } -virDomainNumatuneMemMode -virDomainNumatuneGetMode(virDomainNumaPtr numatune, - int cellid) +/** + * virDomainNumatuneGetMode: + * @numatune: pointer to numatune definition + * @cellid: cell selector + * @mode: where to store the result + * + * Get the defined mode for domain's memory. It's safe to pass + * NULL to @mode if the return value is the only info needed. + * + * Returns: 0 on success (with @mode updated) + * -1 if no mode was defined in XML + */ +int virDomainNumatuneGetMode(virDomainNumaPtr numatune, + int cellid, + virDomainNumatuneMemMode *mode) { + int ret = -1; + virDomainNumatuneMemMode tmp_mode; + if (!numatune) - return 0; + return ret; if (virDomainNumatuneNodeSpecified(numatune, cellid)) - return numatune->mem_nodes[cellid].mode; - - if (numatune->memory.specified) - return numatune->memory.mode; + tmp_mode = numatune->mem_nodes[cellid].mode; + else if (numatune->memory.specified) + tmp_mode = numatune->memory.mode; + else + goto cleanup; - return 0; + if (mode) + *mode = tmp_mode; + ret = 0; + cleanup: + return ret; } virBitmapPtr diff --git a/src/conf/numa_conf.h b/src/conf/numa_conf.h index ded6e019ec..6739065339 100644 --- a/src/conf/numa_conf.h +++ b/src/conf/numa_conf.h @@ -72,8 +72,9 @@ int virDomainNumatuneFormatXML(virBufferPtr buf, virDomainNumaPtr numatune) /* * Getters */ -virDomainNumatuneMemMode virDomainNumatuneGetMode(virDomainNumaPtr numatune, - int cellid); +int virDomainNumatuneGetMode(virDomainNumaPtr numatune, + int cellid, + virDomainNumatuneMemMode *mode); virBitmapPtr virDomainNumatuneGetNodeset(virDomainNumaPtr numatune, virBitmapPtr auto_nodeset, diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c index 5e959a2ea9..507d56759f 100644 --- a/src/lxc/lxc_cgroup.c +++ b/src/lxc/lxc_cgroup.c @@ -69,6 +69,7 @@ static int virLXCCgroupSetupCpusetTune(virDomainDefPtr def, { int ret = -1; char *mask = NULL; + virDomainNumatuneMemMode mode; if (def->placement_mode != VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO && def->cpumask) { @@ -81,8 +82,8 @@ static int virLXCCgroupSetupCpusetTune(virDomainDefPtr def, VIR_FREE(mask); } - if (virDomainNumatuneGetMode(def->numa, -1) != - VIR_DOMAIN_NUMATUNE_MEM_STRICT) { + if (virDomainNumatuneGetMode(def->numa, -1, &mode) < 0 || + mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT) { ret = 0; goto cleanup; } diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index ba44e098d3..efbe71feac 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -741,25 +741,25 @@ static int virLXCControllerSetupResourceLimits(virLXCControllerPtr ctrl) virBitmapPtr nodeset = NULL; virDomainNumatuneMemMode mode; - mode = virDomainNumatuneGetMode(ctrl->def->numa, -1); - - if (mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT && - virCgroupControllerAvailable(VIR_CGROUP_CONTROLLER_CPUSET)) { - /* Use virNuma* API iff necessary. Once set and child is exec()-ed, - * there's no way for us to change it. Rely on cgroups (if available - * and enabled in the config) rather than virNuma*. */ - VIR_DEBUG("Relying on CGroups for memory binding"); - } else { + if (virDomainNumatuneGetMode(ctrl->def->numa, -1, &mode) == 0) { + if (mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT && + virCgroupControllerAvailable(VIR_CGROUP_CONTROLLER_CPUSET)) { + /* Use virNuma* API iff necessary. Once set and child is exec()-ed, + * there's no way for us to change it. Rely on cgroups (if available + * and enabled in the config) rather than virNuma*. */ + VIR_DEBUG("Relying on CGroups for memory binding"); + } else { - VIR_DEBUG("Setting up process resource limits"); + VIR_DEBUG("Setting up process resource limits"); - if (virLXCControllerGetNumadAdvice(ctrl, &auto_nodeset) < 0) - goto cleanup; + if (virLXCControllerGetNumadAdvice(ctrl, &auto_nodeset) < 0) + goto cleanup; - nodeset = virDomainNumatuneGetNodeset(ctrl->def->numa, auto_nodeset, -1); + nodeset = virDomainNumatuneGetNodeset(ctrl->def->numa, auto_nodeset, -1); - if (virNumaSetupMemoryPolicy(mode, nodeset) < 0) - goto cleanup; + if (virNumaSetupMemoryPolicy(mode, nodeset) < 0) + goto cleanup; + } } if (virLXCControllerSetupCpuAffinity(ctrl) < 0) diff --git a/src/parallels/parallels_sdk.c b/src/parallels/parallels_sdk.c index 786e0adace..2a4450413e 100644 --- a/src/parallels/parallels_sdk.c +++ b/src/parallels/parallels_sdk.c @@ -1845,6 +1845,7 @@ prlsdkCheckUnsupportedParams(PRL_HANDLE sdkdom, virDomainDefPtr def) size_t i; PRL_VM_TYPE vmType; PRL_RESULT pret; + virDomainNumatuneMemMode memMode; if (def->title) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -1924,8 +1925,8 @@ prlsdkCheckUnsupportedParams(PRL_HANDLE sdkdom, virDomainDefPtr def) * virDomainDefPtr always contain non zero NUMA configuration * So, just make sure this configuration does't differ from auto generated. */ - if ((virDomainNumatuneGetMode(def->numa, -1) != - VIR_DOMAIN_NUMATUNE_MEM_STRICT) || + if ((virDomainNumatuneGetMode(def->numa, -1, &memMode) == 0 && + memMode == VIR_DOMAIN_NUMATUNE_MEM_STRICT) || virDomainNumatuneHasPerNodeBinding(def->numa)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("numa parameters are not supported " diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index e24989b33b..96677dd040 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -613,14 +613,15 @@ qemuSetupCpusetMems(virDomainObjPtr vm) { virCgroupPtr cgroup_temp = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; + virDomainNumatuneMemMode mode; char *mem_mask = NULL; int ret = -1; if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) return 0; - if (virDomainNumatuneGetMode(vm->def->numa, -1) != - VIR_DOMAIN_NUMATUNE_MEM_STRICT) + if (virDomainNumatuneGetMode(vm->def->numa, -1, &mode) < 0 || + mode != VIR_DOMAIN_NUMATUNE_MEM_STRICT) return 0; if (virDomainNumatuneMaybeFormatNodeset(vm->def->numa, @@ -979,6 +980,7 @@ qemuSetupCgroupForVcpu(virDomainObjPtr vm) unsigned long long period = vm->def->cputune.period; long long quota = vm->def->cputune.quota; char *mem_mask = NULL; + virDomainNumatuneMemMode mem_mode; if ((period || quota) && !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) { @@ -1009,8 +1011,8 @@ qemuSetupCgroupForVcpu(virDomainObjPtr vm) return 0; } - if (virDomainNumatuneGetMode(vm->def->numa, -1) == - VIR_DOMAIN_NUMATUNE_MEM_STRICT && + if (virDomainNumatuneGetMode(vm->def->numa, -1, &mem_mode) == 0 && + mem_mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT && virDomainNumatuneMaybeFormatNodeset(vm->def->numa, priv->autoNodeset, &mem_mask, -1) < 0) @@ -1153,6 +1155,7 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm) unsigned long long period = vm->def->cputune.period; long long quota = vm->def->cputune.quota; char *mem_mask = NULL; + virDomainNumatuneMemMode mem_mode; if ((period || quota) && !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) { @@ -1176,8 +1179,8 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm) if (priv->cgroup == NULL) return 0; - if (virDomainNumatuneGetMode(vm->def->numa, -1) == - VIR_DOMAIN_NUMATUNE_MEM_STRICT && + if (virDomainNumatuneGetMode(vm->def->numa, -1, &mem_mode) == 0 && + mem_mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT && virDomainNumatuneMaybeFormatNodeset(vm->def->numa, priv->autoNodeset, &mem_mask, -1) < 0) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 3ccd35d570..30597087d8 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4707,7 +4707,9 @@ qemuBuildMemoryBackendStr(unsigned long long size, return -1; memAccess = virDomainNumaGetNodeMemoryAccessMode(def->numa, guestNode); - mode = virDomainNumatuneGetMode(def->numa, guestNode); + if (virDomainNumatuneGetMode(def->numa, guestNode, &mode) < 0 && + virDomainNumatuneGetMode(def->numa, -1, &mode) < 0) + mode = VIR_DOMAIN_NUMATUNE_MEM_STRICT; if (pagesize == 0 || pagesize != system_page_size) { /* Find the huge page size we want to use */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a2f8e24f77..31cc2eea8b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4737,6 +4737,7 @@ qemuDomainHotplugVcpus(virQEMUDriverPtr driver, int ncpupids; virCgroupPtr cgroup_vcpu = NULL; char *mem_mask = NULL; + virDomainNumatuneMemMode mem_mode; qemuDomainObjEnterMonitor(driver, vm); @@ -4804,8 +4805,8 @@ qemuDomainHotplugVcpus(virQEMUDriverPtr driver, goto cleanup; } - if (virDomainNumatuneGetMode(vm->def->numa, -1) == - VIR_DOMAIN_NUMATUNE_MEM_STRICT && + if (virDomainNumatuneGetMode(vm->def->numa, -1, &mem_mode) == 0 && + mem_mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT && virDomainNumatuneMaybeFormatNodeset(vm->def->numa, priv->autoNodeset, &mem_mask, -1) < 0) @@ -6113,6 +6114,7 @@ qemuDomainHotplugAddIOThread(virQEMUDriverPtr driver, qemuMonitorIOThreadInfoPtr *new_iothreads = NULL; virCgroupPtr cgroup_iothread = NULL; char *mem_mask = NULL; + virDomainNumatuneMemMode mode; virDomainIOThreadIDDefPtr iothrid; virBitmapPtr cpumask; @@ -6154,8 +6156,8 @@ qemuDomainHotplugAddIOThread(virQEMUDriverPtr driver, } vm->def->iothreads = exp_niothreads; - if (virDomainNumatuneGetMode(vm->def->numa, -1) == - VIR_DOMAIN_NUMATUNE_MEM_STRICT && + if (virDomainNumatuneGetMode(vm->def->numa, -1, &mode) == 0 && + mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT && virDomainNumatuneMaybeFormatNodeset(vm->def->numa, priv->autoNodeset, &mem_mask, -1) < 0) @@ -10331,11 +10333,12 @@ qemuDomainSetNumaParamsLive(virDomainObjPtr vm, virCgroupPtr cgroup_temp = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; char *nodeset_str = NULL; + virDomainNumatuneMemMode mode; size_t i = 0; int ret = -1; - if (virDomainNumatuneGetMode(vm->def->numa, -1) != - VIR_DOMAIN_NUMATUNE_MEM_STRICT) { + if (virDomainNumatuneGetMode(vm->def->numa, -1, &mode) < 0 || + mode != VIR_DOMAIN_NUMATUNE_MEM_STRICT) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("change of nodeset for running domain " "requires strict numa mode")); @@ -10392,6 +10395,7 @@ qemuDomainSetNumaParameters(virDomainPtr dom, virCapsPtr caps = NULL; qemuDomainObjPrivatePtr priv; virBitmapPtr nodeset = NULL; + virDomainNumatuneMemMode config_mode; int mode = -1; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | @@ -10467,7 +10471,8 @@ qemuDomainSetNumaParameters(virDomainPtr dom, if (flags & VIR_DOMAIN_AFFECT_LIVE) { if (mode != -1 && - virDomainNumatuneGetMode(vm->def->numa, -1) != mode) { + virDomainNumatuneGetMode(vm->def->numa, -1, &config_mode) == 0 && + config_mode != mode) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("can't change numatune mode for running domain")); goto endjob; @@ -10568,7 +10573,8 @@ qemuDomainGetNumaParameters(virDomainPtr dom, VIR_TYPED_PARAM_INT, 0) < 0) goto cleanup; - param->value.i = virDomainNumatuneGetMode(def->numa, -1); + virDomainNumatuneGetMode(def->numa, -1, + (virDomainNumatuneMemMode *) ¶m->value.i); break; case 1: /* fill numa nodeset here */ diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 2b3d9b5171..118fc52457 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3135,21 +3135,21 @@ static int qemuProcessHook(void *data) if (virSecurityManagerClearSocketLabel(h->driver->securityManager, h->vm->def) < 0) goto cleanup; - mode = virDomainNumatuneGetMode(h->vm->def->numa, -1); - - if (mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT && - h->cfg->cgroupControllers & (1 << VIR_CGROUP_CONTROLLER_CPUSET) && - virCgroupControllerAvailable(VIR_CGROUP_CONTROLLER_CPUSET)) { - /* Use virNuma* API iff necessary. Once set and child is exec()-ed, - * there's no way for us to change it. Rely on cgroups (if available - * and enabled in the config) rather than virNuma*. */ - VIR_DEBUG("Relying on CGroups for memory binding"); - } else { - nodeset = virDomainNumatuneGetNodeset(h->vm->def->numa, - priv->autoNodeset, -1); + if (virDomainNumatuneGetMode(h->vm->def->numa, -1, &mode) == 0) { + if (mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT && + h->cfg->cgroupControllers & (1 << VIR_CGROUP_CONTROLLER_CPUSET) && + virCgroupControllerAvailable(VIR_CGROUP_CONTROLLER_CPUSET)) { + /* Use virNuma* API iff necessary. Once set and child is exec()-ed, + * there's no way for us to change it. Rely on cgroups (if available + * and enabled in the config) rather than virNuma*. */ + VIR_DEBUG("Relying on CGroups for memory binding"); + } else { + nodeset = virDomainNumatuneGetNodeset(h->vm->def->numa, + priv->autoNodeset, -1); - if (virNumaSetupMemoryPolicy(mode, nodeset) < 0) - goto cleanup; + if (virNumaSetupMemoryPolicy(mode, nodeset) < 0) + goto cleanup; + } } ret = 0; -- GitLab