The API docs state that the API queries pinning info for all vCPUs and thus we should allocate the bitmap even for the inactive ones. The API will currently return bitmap only for the active vCPUs but that will change in the future. --- tools/virsh-domain.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index bfcc0b3..91df404 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6381,12 +6381,13 @@ virshVcpuPinQuery(vshControl *ctl, unsigned int flags) { unsigned char *cpumap = NULL; + unsigned int countFlags = flags | VIR_DOMAIN_VCPU_MAXIMUM; int cpumaplen; size_t i; int ncpus; bool ret = false; - if ((ncpus = virshCPUCountCollect(ctl, dom, flags, true)) < 0) { + if ((ncpus = virshCPUCountCollect(ctl, dom, countFlags, true)) < 0) { if (ncpus == -1) { if (flags & VIR_DOMAIN_AFFECT_LIVE) vshError(ctl, "%s", _("cannot get vcpupin for offline domain")); -- 2.6.2

On 02/24/2016 09:22 AM, Peter Krempa wrote: ACK with one nit... One difference I noted, the 'libxl' code would: memset(cpumaps, 0x00, maplen * ncpumaps); just before filling. Should that be replicated in the common code? If not then for the libxl code should the lines be kept? John [...] ^^^^^ [...]

The API documentation states that the function is returning pinning for all vCPUs, so we can actually do so if the user passes a large enough array. --- src/conf/domain_conf.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c7843dc..5bb0616 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1473,6 +1473,7 @@ virDomainDefGetVcpuPinInfoHelper(virDomainDefPtr def, int hostcpus, virBitmapPtr autoCpuset) { + int maxvcpus = virDomainDefGetVcpusMax(def); virBitmapPtr allcpumap = NULL; size_t i; @@ -1484,17 +1485,10 @@ virDomainDefGetVcpuPinInfoHelper(virDomainDefPtr def, virBitmapSetAll(allcpumap); - /* Clamp to actual number of vcpus */ - if (ncpumaps > virDomainDefGetVcpus(def)) - ncpumaps = virDomainDefGetVcpus(def); - - for (i = 0; i < ncpumaps; i++) { + for (i = 0; i < maxvcpus && i < ncpumaps; i++) { virDomainVcpuInfoPtr vcpu = virDomainDefGetVcpu(def, i); virBitmapPtr bitmap = NULL; - if (!vcpu->online) - continue; - if (vcpu->cpumask) bitmap = vcpu->cpumask; else if (def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO && @@ -1509,7 +1503,7 @@ virDomainDefGetVcpuPinInfoHelper(virDomainDefPtr def, } virBitmapFree(allcpumap); - return ncpumaps; + return i; } -- 2.6.2

Introduce a helper to check supported device and domain config and move the memory hotplug checks to it. The advantage of this approach is that by default all new features are considered unsupported by all hypervisors unless specifically changed rather than the previous approach where every hypervisor would need to declare that a given feature is unsupported. --- src/bhyve/bhyve_domain.c | 9 +------ src/conf/domain_conf.c | 58 ++++++++++++++++++++++++++++++++++++++++++++-- src/conf/domain_conf.h | 4 +--- src/libvirt_private.syms | 2 -- src/libxl/libxl_domain.c | 7 ------ src/lxc/lxc_domain.c | 8 ------- src/openvz/openvz_driver.c | 7 ------ src/phyp/phyp_driver.c | 6 +---- src/qemu/qemu_domain.c | 1 + src/uml/uml_driver.c | 9 +------ src/vbox/vbox_common.c | 6 +---- src/vmware/vmware_driver.c | 6 +---- src/vmx/vmx.c | 6 +---- src/vz/vz_driver.c | 6 +---- src/xen/xen_driver.c | 7 ------ src/xenapi/xenapi_driver.c | 7 ------ 16 files changed, 65 insertions(+), 84 deletions(-) diff --git a/src/bhyve/bhyve_domain.c b/src/bhyve/bhyve_domain.c index db8fae4..89cb171 100644 --- a/src/bhyve/bhyve_domain.c +++ b/src/bhyve/bhyve_domain.c @@ -68,23 +68,16 @@ bhyveDomainDefPostParse(virDomainDefPtr def, VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) < 0) return -1; - /* memory hotplug tunables are not supported by this driver */ - if (virDomainDefCheckUnsupportedMemoryHotplug(def) < 0) - return -1; - return 0; } static int -bhyveDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, +bhyveDomainDeviceDefPostParse(virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED, const virDomainDef *def ATTRIBUTE_UNUSED, virCapsPtr caps ATTRIBUTE_UNUSED, unsigned int parseFlags ATTRIBUTE_UNUSED, void *opaque ATTRIBUTE_UNUSED) { - if (virDomainDeviceDefCheckUnsupportedMemoryDevice(dev) < 0) - return -1; - return 0; } diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 705a796..101fae2 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1138,7 +1138,7 @@ virDomainBlkioDeviceParseXML(xmlNodePtr root, * Returns -1 if the domain definition would enable memory hotplug via the * <maxMemory> tunable and reports an error. Otherwise returns 0. */ -int +static int virDomainDefCheckUnsupportedMemoryHotplug(virDomainDefPtr def) { /* memory hotplug tunables are not supported by this driver */ @@ -1160,7 +1160,7 @@ virDomainDefCheckUnsupportedMemoryHotplug(virDomainDefPtr def) * Returns -1 if the device definition describes a memory device and reports an * error. Otherwise returns 0. */ -int +static int virDomainDeviceDefCheckUnsupportedMemoryDevice(virDomainDeviceDefPtr dev) { /* This driver doesn't yet know how to handle memory devices */ @@ -4215,6 +4215,54 @@ virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr dev, } +#define UNSUPPORTED(FEATURE) (!((FEATURE) & xmlopt->config.features)) +/** + * virDomainDefPostParseCheckFeatures: + * @def: domain definition + * @xmlopt: XML parser option object + * + * This function checks that the domain configuration is supported according to + * the supported features for a given hypervisor. See virDomainDefFeatures and + * virDomainDefParserConfig. + * + * Returns 0 on success and -1 on error with an appropriate libvirt error. + */ +static int +virDomainDefPostParseCheckFeatures(virDomainDefPtr def, + virDomainXMLOptionPtr xmlopt) +{ + if (UNSUPPORTED(VIR_DOMAIN_DEF_FEATURE_MEMORY_HOTPLUG) && + virDomainDefCheckUnsupportedMemoryHotplug(def) < 0) + return -1; + + return 0; +} + + +/** + * virDomainDeviceDefPostParseCheckFeatures: + * @dev: device definition + * @xmlopt: XML parser option object + * + * This function checks that the device configuration is supported according to + * the supported features for a given hypervisor. See virDomainDefFeatures and + * virDomainDefParserConfig. + * + * Returns 0 on success and -1 on error with an appropriate libvirt error. + */ +static int +virDomainDeviceDefPostParseCheckFeatures(virDomainDeviceDefPtr dev, + virDomainXMLOptionPtr xmlopt) +{ + if (UNSUPPORTED(VIR_DOMAIN_DEF_FEATURE_MEMORY_HOTPLUG) && + virDomainDeviceDefCheckUnsupportedMemoryDevice(dev) < 0) + return -1; + + return 0; +} +#undef UNSUPPORTED + + static int virDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, const virDomainDef *def, @@ -4234,6 +4282,9 @@ virDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, if ((ret = virDomainDeviceDefPostParseInternal(dev, def, caps, flags, xmlopt)) < 0) return ret; + if (virDomainDeviceDefPostParseCheckFeatures(dev, xmlopt) < 0) + return -1; + return 0; } @@ -4291,6 +4342,9 @@ virDomainDefPostParse(virDomainDefPtr def, if ((ret = virDomainDefPostParseInternal(def, caps, parseFlags)) < 0) return ret; + if (virDomainDefPostParseCheckFeatures(def, xmlopt) < 0) + return -1; + return 0; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 223ce2d..c660a02 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2405,6 +2405,7 @@ typedef bool (*virDomainObjListACLFilter)(virConnectPtr conn, typedef enum { VIR_DOMAIN_DEF_FEATURE_WIDE_SCSI = (1 << 0), + VIR_DOMAIN_DEF_FEATURE_MEMORY_HOTPLUG = (1 << 1), } virDomainDefFeatures; @@ -2497,9 +2498,6 @@ int virDomainObjWait(virDomainObjPtr vm); int virDomainObjWaitUntil(virDomainObjPtr vm, unsigned long long whenms); -int virDomainDefCheckUnsupportedMemoryHotplug(virDomainDefPtr def); -int virDomainDeviceDefCheckUnsupportedMemoryDevice(virDomainDeviceDefPtr dev); - void virDomainPanicDefFree(virDomainPanicDefPtr panic); void virDomainResourceDefFree(virDomainResourceDefPtr resource); void virDomainGraphicsDefFree(virDomainGraphicsDefPtr def); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6da9b5c..dc90a6a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -204,7 +204,6 @@ virDomainDefAddImplicitControllers; virDomainDefAddUSBController; virDomainDefCheckABIStability; virDomainDefCheckDuplicateDiskInfo; -virDomainDefCheckUnsupportedMemoryHotplug; virDomainDefClearCCWAddresses; virDomainDefClearDeviceAliases; virDomainDefClearPCIAddresses; @@ -243,7 +242,6 @@ virDomainDefSetVcpusMax; virDomainDeleteConfig; virDomainDeviceAddressIsValid; virDomainDeviceAddressTypeToString; -virDomainDeviceDefCheckUnsupportedMemoryDevice; virDomainDeviceDefCopy; virDomainDeviceDefFree; virDomainDeviceDefParse; diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 50f7eed..5b2b680 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -363,9 +363,6 @@ libxlDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, } } - if (virDomainDeviceDefCheckUnsupportedMemoryDevice(dev) < 0) - return -1; - return 0; } @@ -401,10 +398,6 @@ libxlDomainDefPostParse(virDomainDefPtr def, if (xenDomainDefAddImplicitInputDevice(def) < 0) return -1; - /* memory hotplug tunables are not supported by this driver */ - if (virDomainDefCheckUnsupportedMemoryHotplug(def) < 0) - return -1; - return 0; } diff --git a/src/lxc/lxc_domain.c b/src/lxc/lxc_domain.c index c3f7a56..3177a62 100644 --- a/src/lxc/lxc_domain.c +++ b/src/lxc/lxc_domain.c @@ -249,10 +249,6 @@ virLXCDomainDefPostParse(virDomainDefPtr def, !(def->emulator = virDomainDefGetDefaultEmulator(def, caps))) return -1; - /* memory hotplug tunables are not supported by this driver */ - if (virDomainDefCheckUnsupportedMemoryHotplug(def) < 0) - return -1; - return 0; } @@ -269,10 +265,6 @@ virLXCDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, dev->data.chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_NONE) dev->data.chr->targetType = VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_LXC; - - if (virDomainDeviceDefCheckUnsupportedMemoryDevice(dev) < 0) - return -1; - return 0; } diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index a6834b3..c2d54ad 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -98,10 +98,6 @@ openvzDomainDefPostParse(virDomainDefPtr def, return -1; } - /* memory hotplug tunables are not supported by this driver */ - if (virDomainDefCheckUnsupportedMemoryHotplug(def) < 0) - return -1; - return 0; } @@ -128,9 +124,6 @@ openvzDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, return -1; } - if (virDomainDeviceDefCheckUnsupportedMemoryDevice(dev) < 0) - return -1; - return 0; } diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index d1c40da..55a63e7 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -1094,15 +1094,11 @@ openSSHSession(virConnectPtr conn, virConnectAuthPtr auth, static int -phypDomainDefPostParse(virDomainDefPtr def, +phypDomainDefPostParse(virDomainDefPtr def ATTRIBUTE_UNUSED, virCapsPtr caps ATTRIBUTE_UNUSED, unsigned int parseFlags ATTRIBUTE_UNUSED, void *opaque ATTRIBUTE_UNUSED) { - /* memory hotplug tunables are not supported by this driver */ - if (virDomainDefCheckUnsupportedMemoryHotplug(def) < 0) - return -1; - return 0; } diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index c56f9f1..a0dfa7e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1515,6 +1515,7 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, virDomainDefParserConfig virQEMUDriverDomainDefParserConfig = { .devicesPostParseCallback = qemuDomainDeviceDefPostParse, .domainPostParseCallback = qemuDomainDefPostParse, + .features = VIR_DOMAIN_DEF_FEATURE_MEMORY_HOTPLUG, }; diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index d656704..9fcdc84 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -427,23 +427,16 @@ umlDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, return -1; } - if (virDomainDeviceDefCheckUnsupportedMemoryDevice(dev) < 0) - return -1; - return 0; } static int -umlDomainDefPostParse(virDomainDefPtr def, +umlDomainDefPostParse(virDomainDefPtr def ATTRIBUTE_UNUSED, virCapsPtr caps ATTRIBUTE_UNUSED, unsigned int parseFlags ATTRIBUTE_UNUSED, void *opaque ATTRIBUTE_UNUSED) { - /* memory hotplug tunables are not supported by this driver */ - if (virDomainDefCheckUnsupportedMemoryHotplug(def) < 0) - return -1; - return 0; } diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 8c00a4f..e0d18fc 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -251,15 +251,11 @@ static char *vboxGenerateMediumName(PRUint32 storageBus, } static int -vboxDomainDefPostParse(virDomainDefPtr def, +vboxDomainDefPostParse(virDomainDefPtr def ATTRIBUTE_UNUSED, virCapsPtr caps ATTRIBUTE_UNUSED, unsigned int parseFlags ATTRIBUTE_UNUSED, void *opaque ATTRIBUTE_UNUSED) { - /* memory hotplug tunables are not supported by this driver */ - if (virDomainDefCheckUnsupportedMemoryHotplug(def) < 0) - return -1; - return 0; } diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c index e4e470a..93f21c9 100644 --- a/src/vmware/vmware_driver.c +++ b/src/vmware/vmware_driver.c @@ -83,15 +83,11 @@ vmwareDataFreeFunc(void *data) } static int -vmwareDomainDefPostParse(virDomainDefPtr def, +vmwareDomainDefPostParse(virDomainDefPtr def ATTRIBUTE_UNUSED, virCapsPtr caps ATTRIBUTE_UNUSED, unsigned int parseFlags ATTRIBUTE_UNUSED, void *opaque ATTRIBUTE_UNUSED) { - /* memory hotplug tunables are not supported by this driver */ - if (virDomainDefCheckUnsupportedMemoryHotplug(def) < 0) - return -1; - return 0; } diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index cbd6633..6376cca 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -525,15 +525,11 @@ VIR_ENUM_IMPL(virVMXControllerModelSCSI, VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST, */ static int -virVMXDomainDefPostParse(virDomainDefPtr def, +virVMXDomainDefPostParse(virDomainDefPtr def ATTRIBUTE_UNUSED, virCapsPtr caps ATTRIBUTE_UNUSED, unsigned int parseFlags ATTRIBUTE_UNUSED, void *opaque ATTRIBUTE_UNUSED) { - /* memory hotplug tunables are not supported by this driver */ - if (virDomainDefCheckUnsupportedMemoryHotplug(def) < 0) - return -1; - return 0; } diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index f7a8617..409d777 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -173,15 +173,11 @@ vzConnectGetCapabilities(virConnectPtr conn) } static int -vzDomainDefPostParse(virDomainDefPtr def, +vzDomainDefPostParse(virDomainDefPtr def ATTRIBUTE_UNUSED, virCapsPtr caps ATTRIBUTE_UNUSED, unsigned int parseFlags ATTRIBUTE_UNUSED, void *opaque ATTRIBUTE_UNUSED) { - /* memory hotplug tunables are not supported by this driver */ - if (virDomainDefCheckUnsupportedMemoryHotplug(def) < 0) - return -1; - return 0; } diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 7628c94..3f5d80d 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -361,9 +361,6 @@ xenDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, } } - if (virDomainDeviceDefCheckUnsupportedMemoryDevice(dev) < 0) - return -1; - return 0; } @@ -387,10 +384,6 @@ xenDomainDefPostParse(virDomainDefPtr def, if (xenDomainDefAddImplicitInputDevice(def) <0) return -1; - /* memory hotplug tunables are not supported by this driver */ - if (virDomainDefCheckUnsupportedMemoryHotplug(def) < 0) - return -1; - return 0; } diff --git a/src/xenapi/xenapi_driver.c b/src/xenapi/xenapi_driver.c index f75f138..a75a4f7 100644 --- a/src/xenapi/xenapi_driver.c +++ b/src/xenapi/xenapi_driver.c @@ -67,9 +67,6 @@ xenapiDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, return -1; } - if (virDomainDeviceDefCheckUnsupportedMemoryDevice(dev) < 0) - return -1; - return 0; } @@ -84,10 +81,6 @@ xenapiDomainDefPostParse(virDomainDefPtr def, if (xenDomainDefAddImplicitInputDevice(def) < 0) return -1; - /* memory hotplug tunables are not supported by this driver */ - if (virDomainDefCheckUnsupportedMemoryHotplug(def) < 0) - return -1; - return 0; } -- 2.6.2

On 02/24/2016 09:22 AM, Peter Krempa wrote: Should it be VCPUPIN ? A little intro would be nice... Is/was this for debugging? Do we really want to WARN or just go with INFO? [...]

Allow pinning for inactive cpus. The pinning mask will be automatically applied as we would apply the default mask in case of a cpu hotplug. Setting the scheduler settings for a vcpu has the same semantics. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1306556 --- src/qemu/qemu_domain.c | 3 ++- src/qemu/qemu_driver.c | 62 ++++++++++++++++++-------------------------------- 2 files changed, 24 insertions(+), 41 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index a0dfa7e..f2b9338 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1515,7 +1515,8 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, virDomainDefParserConfig virQEMUDriverDomainDefParserConfig = { .devicesPostParseCallback = qemuDomainDeviceDefPostParse, .domainPostParseCallback = qemuDomainDefPostParse, - .features = VIR_DOMAIN_DEF_FEATURE_MEMORY_HOTPLUG, + .features = VIR_DOMAIN_DEF_FEATURE_MEMORY_HOTPLUG | + VIR_DOMAIN_DEF_FEATURE_OFFLINE_CPUPIN }; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c8b996b..bfabc53 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4754,9 +4754,6 @@ qemuDomainHotplugDelVcpu(virQEMUDriverPtr driver, VIR_CGROUP_THREAD_VCPU, vcpu) < 0) goto cleanup; - virBitmapFree(vcpuinfo->cpumask); - vcpuinfo->cpumask = NULL; - ret = 0; cleanup: @@ -5016,36 +5013,19 @@ qemuDomainPinVcpuFlags(virDomainPtr dom, priv = vm->privateData; - if (def) { - if (!(vcpuinfolive = virDomainDefGetVcpu(def, vcpu))) { - virReportError(VIR_ERR_INVALID_ARG, - _("vcpu %d is out of range of live cpu count %d"), - vcpu, virDomainDefGetVcpus(def)); - goto endjob; - } - - if (!vcpuinfolive->online) { - virReportError(VIR_ERR_OPERATION_INVALID, - _("setting cpu pinning for inactive vcpu '%d' is not " - "supported"), vcpu); - goto endjob; - } + if (def && !(vcpuinfolive = virDomainDefGetVcpu(def, vcpu))) { + virReportError(VIR_ERR_INVALID_ARG, + _("vcpu %d is out of range of live cpu count %d"), + vcpu, virDomainDefGetVcpus(def)); + goto endjob; } - if (persistentDef) { - if (!(vcpuinfopersist = virDomainDefGetVcpu(persistentDef, vcpu))) { - virReportError(VIR_ERR_INVALID_ARG, - _("vcpu %d is out of range of persistent cpu count %d"), - vcpu, virDomainDefGetVcpus(persistentDef)); - goto endjob; - } - - if (!vcpuinfopersist->online) { - virReportError(VIR_ERR_OPERATION_INVALID, - _("setting cpu pinning for inactive vcpu '%d' is not " - "supported"), vcpu); - goto endjob; - } + if (persistentDef && + !(vcpuinfopersist = virDomainDefGetVcpu(persistentDef, vcpu))) { + virReportError(VIR_ERR_INVALID_ARG, + _("vcpu %d is out of range of persistent cpu count %d"), + vcpu, virDomainDefGetVcpus(persistentDef)); + goto endjob; } if (!(pcpumap = virBitmapNewData(cpumap, maplen))) @@ -5068,18 +5048,20 @@ qemuDomainPinVcpuFlags(virDomainPtr dom, goto endjob; } - /* Configure the corresponding cpuset cgroup before set affinity. */ - if (virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) { - if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_VCPU, vcpu, - false, &cgroup_vcpu) < 0) - goto endjob; - if (qemuSetupCgroupCpusetCpus(cgroup_vcpu, pcpumap) < 0) + if (vcpuinfolive->online) { + /* Configure the corresponding cpuset cgroup before set affinity. */ + if (virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) { + if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_VCPU, vcpu, + false, &cgroup_vcpu) < 0) + goto endjob; + if (qemuSetupCgroupCpusetCpus(cgroup_vcpu, pcpumap) < 0) + goto endjob; + } + + if (virProcessSetAffinity(qemuDomainGetVcpuPid(vm, vcpu), pcpumap) < 0) goto endjob; } - if (virProcessSetAffinity(qemuDomainGetVcpuPid(vm, vcpu), pcpumap) < 0) - goto endjob; - virBitmapFree(vcpuinfolive->cpumask); vcpuinfolive->cpumask = pcpumaplive; pcpumaplive = NULL; -- 2.6.2

The function was now beyond maintainability. --- src/qemu/qemu_driver.c | 134 ++++++++++++++++++++++++++++--------------------- 1 file changed, 77 insertions(+), 57 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index bfabc53..cab69a5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4967,6 +4967,82 @@ qemuDomainSetVcpus(virDomainPtr dom, unsigned int nvcpus) static int +qemuDomainPinVcpuLive(virDomainObjPtr vm, + virDomainDefPtr def, + int vcpu, + virQEMUDriverPtr driver, + virQEMUDriverConfigPtr cfg, + virBitmapPtr cpumap) +{ + virDomainVcpuInfoPtr vcpuinfo; + qemuDomainObjPrivatePtr priv = vm->privateData; + virCgroupPtr cgroup_vcpu = NULL; + char *str = NULL; + virObjectEventPtr event = NULL; + char paramField[VIR_TYPED_PARAM_FIELD_LENGTH] = ""; + virTypedParameterPtr eventParams = NULL; + int eventNparams = 0; + int eventMaxparams = 0; + int ret = -1; + + if (!qemuDomainHasVcpuPids(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("cpu affinity is not supported")); + goto cleanup; + } + + if (!(vcpuinfo = virDomainDefGetVcpu(def, vcpu))) { + virReportError(VIR_ERR_INVALID_ARG, + _("vcpu %d is out of range of live cpu count %d"), + vcpu, virDomainDefGetVcpusMax(def)); + goto cleanup; + } + + if (vcpuinfo->online) { + /* Configure the corresponding cpuset cgroup before set affinity. */ + if (virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) { + if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_VCPU, vcpu, + false, &cgroup_vcpu) < 0) + goto cleanup; + if (qemuSetupCgroupCpusetCpus(cgroup_vcpu, cpumap) < 0) + goto cleanup; + } + + if (virProcessSetAffinity(qemuDomainGetVcpuPid(vm, vcpu), cpumap) < 0) + goto cleanup; + } + + virBitmapFree(vcpuinfo->cpumask); + vcpuinfo->cpumask = cpumap; + cpumap = NULL; + + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps) < 0) + goto cleanup; + + if (snprintf(paramField, VIR_TYPED_PARAM_FIELD_LENGTH, + VIR_DOMAIN_TUNABLE_CPU_VCPUPIN, vcpu) < 0) { + goto cleanup; + } + + str = virBitmapFormat(vcpuinfo->cpumask); + if (virTypedParamsAddString(&eventParams, &eventNparams, + &eventMaxparams, paramField, str) < 0) + goto cleanup; + + event = virDomainEventTunableNewFromObj(vm, eventParams, eventNparams); + + ret = 0; + + cleanup: + virBitmapFree(cpumap); + virCgroupFree(&cgroup_vcpu); + VIR_FREE(str); + qemuDomainEventQueue(driver, event); + return ret; +} + + +static int qemuDomainPinVcpuFlags(virDomainPtr dom, unsigned int vcpu, unsigned char *cpumap, @@ -4978,21 +5054,12 @@ qemuDomainPinVcpuFlags(virDomainPtr dom, virDomainObjPtr vm; virDomainDefPtr def; virDomainDefPtr persistentDef; - virCgroupPtr cgroup_vcpu = NULL; int ret = -1; - qemuDomainObjPrivatePtr priv; virBitmapPtr pcpumap = NULL; virBitmapPtr pcpumaplive = NULL; virBitmapPtr pcpumappersist = NULL; - virDomainVcpuInfoPtr vcpuinfolive = NULL; virDomainVcpuInfoPtr vcpuinfopersist = NULL; virQEMUDriverConfigPtr cfg = NULL; - virObjectEventPtr event = NULL; - char paramField[VIR_TYPED_PARAM_FIELD_LENGTH] = ""; - char *str = NULL; - virTypedParameterPtr eventParams = NULL; - int eventNparams = 0; - int eventMaxparams = 0; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); @@ -5011,15 +5078,6 @@ qemuDomainPinVcpuFlags(virDomainPtr dom, if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0) goto endjob; - priv = vm->privateData; - - if (def && !(vcpuinfolive = virDomainDefGetVcpu(def, vcpu))) { - virReportError(VIR_ERR_INVALID_ARG, - _("vcpu %d is out of range of live cpu count %d"), - vcpu, virDomainDefGetVcpus(def)); - goto endjob; - } - if (persistentDef && !(vcpuinfopersist = virDomainDefGetVcpu(persistentDef, vcpu))) { virReportError(VIR_ERR_INVALID_ARG, @@ -5042,44 +5100,10 @@ qemuDomainPinVcpuFlags(virDomainPtr dom, goto endjob; if (def) { - if (!qemuDomainHasVcpuPids(vm)) { - virReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("cpu affinity is not supported")); + if (qemuDomainPinVcpuLive(vm, def, vcpu, driver, cfg, pcpumaplive) < 0) goto endjob; - } - if (vcpuinfolive->online) { - /* Configure the corresponding cpuset cgroup before set affinity. */ - if (virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) { - if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_VCPU, vcpu, - false, &cgroup_vcpu) < 0) - goto endjob; - if (qemuSetupCgroupCpusetCpus(cgroup_vcpu, pcpumap) < 0) - goto endjob; - } - - if (virProcessSetAffinity(qemuDomainGetVcpuPid(vm, vcpu), pcpumap) < 0) - goto endjob; - } - - virBitmapFree(vcpuinfolive->cpumask); - vcpuinfolive->cpumask = pcpumaplive; pcpumaplive = NULL; - - if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps) < 0) - goto endjob; - - if (snprintf(paramField, VIR_TYPED_PARAM_FIELD_LENGTH, - VIR_DOMAIN_TUNABLE_CPU_VCPUPIN, vcpu) < 0) { - goto endjob; - } - - str = virBitmapFormat(pcpumap); - if (virTypedParamsAddString(&eventParams, &eventNparams, - &eventMaxparams, paramField, str) < 0) - goto endjob; - - event = virDomainEventTunableNewFromDom(dom, eventParams, eventNparams); } if (persistentDef) { @@ -5097,11 +5121,7 @@ qemuDomainPinVcpuFlags(virDomainPtr dom, qemuDomainObjEndJob(driver, vm); cleanup: - if (cgroup_vcpu) - virCgroupFree(&cgroup_vcpu); virDomainObjEndAPI(&vm); - qemuDomainEventQueue(driver, event); - VIR_FREE(str); virBitmapFree(pcpumap); virBitmapFree(pcpumaplive); virBitmapFree(pcpumappersist); -- 2.6.2

On Fri, Mar 04, 2016 at 07:31:23 -0500, John Ferlan wrote: [...] No, see the explanation below ... No, the caller needs to be fixed to clear the pointer, since this function consumes the pointer regardless of the outcome. [...] I think it makes it less obvious. The function is designed to set the desired pinning and not to decide when we should set it. That was actually the intention. qemuDomainPinVcpuLive() shall consume the bitmap when called in any case. The only bug here is that the pcpumaplive pointer here needs to be cleared both on success and on error when calling the function. There is no need to pass it by reference. Peter