[libvirt] [PATCH 0/9] vcpu info refactors - part 3b
Yet another continuation of the saga. In this episode we add pinning for inactive cpus. Peter Krempa (9): virsh: vcpupin: Ask for pinning info for all vCPUs conf: Extract code filling data for virDomainGetVcpuPinInfo qemu: Report pinning for all vCPUs in qemuDomainGetVcpuPinInfo conf: introduce parser feature flags conf: refactor checking for unsupported memory devices conf: extract ignoring of inactive vcpu pinning information qemu: add support for offline vcpupin qemu: vcpupin: Extract live vcpupin setting into a separate function qemu: Refactor bitmap handling in qemuDomainPinVcpuFlags src/bhyve/bhyve_domain.c | 9 +- src/conf/domain_conf.c | 157 +++++++++++++++++++++++++++--- src/conf/domain_conf.h | 20 +++- src/libvirt_private.syms | 3 +- src/libxl/libxl_domain.c | 7 -- src/libxl/libxl_driver.c | 38 +------- src/lxc/lxc_domain.c | 8 -- src/openvz/openvz_driver.c | 7 -- src/phyp/phyp_driver.c | 6 +- src/qemu/qemu_domain.c | 2 + src/qemu/qemu_driver.c | 234 +++++++++++++++++++-------------------------- src/test/test_driver.c | 38 +------- src/uml/uml_driver.c | 9 +- src/vbox/vbox_common.c | 6 +- src/vmware/vmware_driver.c | 6 +- src/vmx/vmx.c | 8 +- src/vz/vz_driver.c | 6 +- src/xen/xen_driver.c | 7 -- src/xenapi/xenapi_driver.c | 7 -- tools/virsh-domain.c | 3 +- 20 files changed, 279 insertions(+), 302 deletions(-) -- 2.6.2
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
The implementation of the inner guts of the function is similar for all drivers, so we can add a helper and not have to reimplement it three times. --- src/conf/domain_conf.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 8 ++++++ src/libvirt_private.syms | 1 + src/libxl/libxl_driver.c | 38 +++------------------------- src/qemu/qemu_driver.c | 43 +++----------------------------- src/test/test_driver.c | 38 ++++------------------------ 6 files changed, 84 insertions(+), 108 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3b15cb4..c7843dc 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1449,6 +1449,70 @@ virDomainDefHasVcpuPin(const virDomainDef *def) } +/** + * virDomainDefGetVcpuPinInfoHelper: + * @def: domain definition + * @maplen: length of one cpumap passed from caller (@cpumaps) + * @ncpumaps: count of cpumaps of @maplen length in @cpumaps + * @cpumaps: array of pinning information bitmaps to be filled + * @hostcpus: number of cpus in the host + * @autoCpuset: Cpu pinning bitmap used in case of automatic cpu pinning + * + * Fills the @cpumaps array as documented by the virDomainGetVcpuPinInfo API. + * In case when automatic cpu pinning is supported, the bitmap should be passed + * as @autoCpuset. If @hostcpus is < 0 no error is reported (to pass through + * error message). + * + * Returns number of filled entries or -1 on error. + */ +int +virDomainDefGetVcpuPinInfoHelper(virDomainDefPtr def, + int maplen, + int ncpumaps, + unsigned char *cpumaps, + int hostcpus, + virBitmapPtr autoCpuset) +{ + virBitmapPtr allcpumap = NULL; + size_t i; + + if (hostcpus < 0) + return -1; + + if (!(allcpumap = virBitmapNew(hostcpus))) + return -1; + + virBitmapSetAll(allcpumap); + + /* Clamp to actual number of vcpus */ + if (ncpumaps > virDomainDefGetVcpus(def)) + ncpumaps = virDomainDefGetVcpus(def); + + for (i = 0; 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 && + autoCpuset) + bitmap = autoCpuset; + else if (def->cpumask) + bitmap = def->cpumask; + else + bitmap = allcpumap; + + virBitmapToDataBuf(bitmap, VIR_GET_CPUMAP(cpumaps, maplen, i), maplen); + } + + virBitmapFree(allcpumap); + return ncpumaps; +} + + virDomainDiskDefPtr virDomainDiskDefNew(virDomainXMLOptionPtr xmlopt) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 1de3be3..c1e63e4 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3128,4 +3128,12 @@ int virDomainDiskDefCheckDuplicateInfo(virDomainDiskDefPtr a, int virDomainDefCheckDuplicateDiskInfo(virDomainDefPtr def) ATTRIBUTE_NONNULL(1); +int virDomainDefGetVcpuPinInfoHelper(virDomainDefPtr def, + int maplen, + int ncpumaps, + unsigned char *cpumaps, + int hostcpus, + virBitmapPtr autoCpuset) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(4) ATTRIBUTE_RETURN_CHECK; + #endif /* __DOMAIN_CONF_H */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 4b40612..6da9b5c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -221,6 +221,7 @@ virDomainDefGetMemoryInitial; virDomainDefGetOnlineVcpumap; virDomainDefGetSecurityLabelDef; virDomainDefGetVcpu; +virDomainDefGetVcpuPinInfoHelper; virDomainDefGetVcpus; virDomainDefGetVcpusMax; virDomainDefHasDeviceAddress; diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 404016e..a99c99c 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -2423,8 +2423,7 @@ libxlDomainGetVcpuPinInfo(virDomainPtr dom, int ncpumaps, libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); virDomainObjPtr vm = NULL; virDomainDefPtr targetDef = NULL; - int hostcpus, vcpu, ret = -1; - virBitmapPtr allcpumap = NULL; + int ret = -1; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); @@ -2445,41 +2444,10 @@ libxlDomainGetVcpuPinInfo(virDomainPtr dom, int ncpumaps, /* Make sure coverity knows targetDef is valid at this point. */ sa_assert(targetDef); - /* Clamp to actual number of vcpus */ - if (ncpumaps > virDomainDefGetVcpus(targetDef)) - ncpumaps = virDomainDefGetVcpus(targetDef); - - if ((hostcpus = libxl_get_max_cpus(cfg->ctx)) < 0) - goto cleanup; - - if (!(allcpumap = virBitmapNew(hostcpus))) - goto cleanup; - - virBitmapSetAll(allcpumap); - - memset(cpumaps, 0x00, maplen * ncpumaps); - - for (vcpu = 0; vcpu < ncpumaps; vcpu++) { - virDomainVcpuInfoPtr vcpuinfo = virDomainDefGetVcpu(targetDef, vcpu); - virBitmapPtr bitmap = NULL; - - if (!vcpuinfo->online) - continue; - - if (vcpuinfo->cpumask) - bitmap = vcpuinfo->cpumask; - else if (targetDef->cpumask) - bitmap = targetDef->cpumask; - else - bitmap = allcpumap; - - virBitmapToDataBuf(bitmap, VIR_GET_CPUMAP(cpumaps, maplen, vcpu), maplen); - } - - ret = ncpumaps; + ret = virDomainDefGetVcpuPinInfoHelper(targetDef, maplen, ncpumaps, cpumaps, + libxl_get_max_cpus(cfg->ctx), NULL); cleanup: - virBitmapFree(allcpumap); if (vm) virObjectUnlock(vm); virObjectUnref(cfg); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 45ff3c0..c8b996b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5147,9 +5147,6 @@ qemuDomainGetVcpuPinInfo(virDomainPtr dom, virDomainObjPtr vm = NULL; virDomainDefPtr def; int ret = -1; - int hostcpus; - size_t i; - virBitmapPtr allcpumap = NULL; qemuDomainObjPrivatePtr priv = NULL; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | @@ -5164,46 +5161,12 @@ qemuDomainGetVcpuPinInfo(virDomainPtr dom, if (!(def = virDomainObjGetOneDef(vm, flags))) goto cleanup; - if ((hostcpus = nodeGetCPUCount(NULL)) < 0) - goto cleanup; - - if (!(allcpumap = virBitmapNew(hostcpus))) - goto cleanup; - - virBitmapSetAll(allcpumap); priv = vm->privateData; - /* Clamp to actual number of vcpus */ - if (ncpumaps > virDomainDefGetVcpus(def)) - ncpumaps = virDomainDefGetVcpus(def); - - if (ncpumaps < 1) - goto cleanup; - - for (i = 0; i < ncpumaps; i++) { - virDomainVcpuInfoPtr vcpu = virDomainDefGetVcpu(def, i); - virBitmapPtr bitmap = NULL; - - if (!vcpu->online) - continue; - - if (vcpu->cpumask) - bitmap = vcpu->cpumask; - else if (vm->def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO && - priv->autoCpuset) - bitmap = priv->autoCpuset; - else if (def->cpumask) - bitmap = def->cpumask; - else - bitmap = allcpumap; - - virBitmapToDataBuf(bitmap, VIR_GET_CPUMAP(cpumaps, maplen, i), maplen); - } - - ret = ncpumaps; - + ret = virDomainDefGetVcpuPinInfoHelper(def, maplen, ncpumaps, cpumaps, + nodeGetCPUCount(NULL), + priv->autoCpuset); cleanup: - virBitmapFree(allcpumap); virDomainObjEndAPI(&vm); return ret; } diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 21c66db..a51eb09 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -2534,11 +2534,10 @@ testDomainGetVcpuPinInfo(virDomainPtr dom, int maplen, unsigned int flags) { - testDriverPtr privconn = dom->conn->privateData; + testDriverPtr driver = dom->conn->privateData; virDomainObjPtr privdom; virDomainDefPtr def; - int ret = -1, hostcpus, vcpu; - virBitmapPtr allcpumap = NULL; + int ret = -1; if (!(privdom = testDomObjFromDomain(dom))) return -1; @@ -2546,38 +2545,11 @@ testDomainGetVcpuPinInfo(virDomainPtr dom, if (!(def = virDomainObjGetOneDef(privdom, flags))) goto cleanup; - hostcpus = VIR_NODEINFO_MAXCPUS(privconn->nodeInfo); - - if (!(allcpumap = virBitmapNew(hostcpus))) - goto cleanup; - - virBitmapSetAll(allcpumap); - - /* Clamp to actual number of vcpus */ - if (ncpumaps > virDomainDefGetVcpus(def)) - ncpumaps = virDomainDefGetVcpus(def); - - for (vcpu = 0; vcpu < ncpumaps; vcpu++) { - virDomainVcpuInfoPtr vcpuinfo = virDomainDefGetVcpu(def, vcpu); - virBitmapPtr bitmap = NULL; - - if (!vcpuinfo->online) - continue; - - if (vcpuinfo->cpumask) - bitmap = vcpuinfo->cpumask; - else if (def->cpumask) - bitmap = def->cpumask; - else - bitmap = allcpumap; - - virBitmapToDataBuf(bitmap, VIR_GET_CPUMAP(cpumaps, maplen, vcpu), maplen); - } - - ret = ncpumaps; + ret = virDomainDefGetVcpuPinInfoHelper(def, maplen, ncpumaps, cpumaps, + VIR_NODEINFO_MAXCPUS(driver->nodeInfo), + NULL); cleanup: - virBitmapFree(allcpumap); virDomainObjEndAPI(&privdom); return ret; } -- 2.6.2
On 02/24/2016 09:22 AM, Peter Krempa wrote:
The implementation of the inner guts of the function is similar for all drivers, so we can add a helper and not have to reimplement it three times. --- src/conf/domain_conf.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 8 ++++++ src/libvirt_private.syms | 1 + src/libxl/libxl_driver.c | 38 +++------------------------- src/qemu/qemu_driver.c | 43 +++----------------------------- src/test/test_driver.c | 38 ++++------------------------ 6 files changed, 84 insertions(+), 108 deletions(-)
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 [...]
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 404016e..a99c99c 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -2423,8 +2423,7 @@ libxlDomainGetVcpuPinInfo(virDomainPtr dom, int ncpumaps, libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); virDomainObjPtr vm = NULL; virDomainDefPtr targetDef = NULL; - int hostcpus, vcpu, ret = -1; - virBitmapPtr allcpumap = NULL; + int ret = -1;
virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); @@ -2445,41 +2444,10 @@ libxlDomainGetVcpuPinInfo(virDomainPtr dom, int ncpumaps, /* Make sure coverity knows targetDef is valid at this point. */ sa_assert(targetDef);
- /* Clamp to actual number of vcpus */ - if (ncpumaps > virDomainDefGetVcpus(targetDef)) - ncpumaps = virDomainDefGetVcpus(targetDef); - - if ((hostcpus = libxl_get_max_cpus(cfg->ctx)) < 0) - goto cleanup; - - if (!(allcpumap = virBitmapNew(hostcpus))) - goto cleanup; - - virBitmapSetAll(allcpumap); - - memset(cpumaps, 0x00, maplen * ncpumaps);
^^^^^
- - for (vcpu = 0; vcpu < ncpumaps; vcpu++) { - virDomainVcpuInfoPtr vcpuinfo = virDomainDefGetVcpu(targetDef, vcpu); - virBitmapPtr bitmap = NULL; - - if (!vcpuinfo->online) - continue; - - if (vcpuinfo->cpumask) - bitmap = vcpuinfo->cpumask; - else if (targetDef->cpumask) - bitmap = targetDef->cpumask; - else - bitmap = allcpumap; - - virBitmapToDataBuf(bitmap, VIR_GET_CPUMAP(cpumaps, maplen, vcpu), maplen); - } - - ret = ncpumaps; + ret = virDomainDefGetVcpuPinInfoHelper(targetDef, maplen, ncpumaps, cpumaps, + libxl_get_max_cpus(cfg->ctx), NULL);
cleanup: - virBitmapFree(allcpumap); if (vm) virObjectUnlock(vm); virObjectUnref(cfg);
[...]
On Fri, Mar 04, 2016 at 07:30:01 -0500, John Ferlan wrote:
On 02/24/2016 09:22 AM, Peter Krempa wrote:
The implementation of the inner guts of the function is similar for all drivers, so we can add a helper and not have to reimplement it three times. --- src/conf/domain_conf.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 8 ++++++ src/libvirt_private.syms | 1 + src/libxl/libxl_driver.c | 38 +++------------------------- src/qemu/qemu_driver.c | 43 +++----------------------------- src/test/test_driver.c | 38 ++++------------------------ 6 files changed, 84 insertions(+), 108 deletions(-)
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?
The array members set in our code are cleared by virBitmapToDataBuf. If the array is larger than filled by this code (which it shouldn't be really) the rest of the array will be now kept untouched, but the caller should not use that data either, so this should be safe. Peter
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
To avoid having to forbid new features added to domain XML in post parse callbacks for individual hypervisor drivers the feature flag mechanism will allow to add a central check that will be disabled for the drivers that will add support. As a first example flag, the 'hasWideSCSIBus' is converted to the new bitmap. --- src/conf/domain_conf.c | 13 ++++++++----- src/conf/domain_conf.h | 7 ++++++- src/vmx/vmx.c | 2 +- 3 files changed, 15 insertions(+), 7 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5bb0616..705a796 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4040,18 +4040,21 @@ virDomainHostdevAssignAddress(virDomainXMLOptionPtr xmlopt, { int next_unit = 0; unsigned controller = 0; + unsigned int max_unit; size_t i; int ret; + if (xmlopt->config.features & VIR_DOMAIN_DEF_FEATURE_WIDE_SCSI) + max_unit = SCSI_WIDE_BUS_MAX_CONT_UNIT; + else + max_unit = SCSI_NARROW_BUS_MAX_CONT_UNIT; + for (i = 0; i < def->ncontrollers; i++) { if (def->controllers[i]->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) continue; controller++; - ret = virDomainControllerSCSINextUnit(def, - xmlopt->config.hasWideSCSIBus ? - SCSI_WIDE_BUS_MAX_CONT_UNIT : - SCSI_NARROW_BUS_MAX_CONT_UNIT, + ret = virDomainControllerSCSINextUnit(def, max_unit, def->controllers[i]->idx); if (ret >= 0) { next_unit = ret; @@ -5863,7 +5866,7 @@ virDomainDiskDefAssignAddress(virDomainXMLOptionPtr xmlopt, def->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE; - if (xmlopt->config.hasWideSCSIBus) { + if (xmlopt->config.features & VIR_DOMAIN_DEF_FEATURE_WIDE_SCSI) { /* For a wide SCSI bus we define the default mapping to be * 16 units per bus, 1 bus per controller, many controllers. * Unit 7 is the SCSI controller itself. Therefore unit 7 diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index c1e63e4..223ce2d 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2403,6 +2403,11 @@ typedef bool (*virDomainObjListACLFilter)(virConnectPtr conn, virDomainDefPtr def); +typedef enum { + VIR_DOMAIN_DEF_FEATURE_WIDE_SCSI = (1 << 0), +} virDomainDefFeatures; + + /* This structure holds various callbacks and data needed * while parsing and creating domain XMLs */ typedef struct _virDomainXMLOption virDomainXMLOption; @@ -2434,7 +2439,7 @@ struct _virDomainDefParserConfig { virFreeCallback privFree; /* data */ - bool hasWideSCSIBus; + unsigned int features; /* virDomainDefFeatures */ unsigned char macPrefix[VIR_MAC_PREFIX_BUFLEN]; }; diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index 4fd0a1d..cbd6633 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -548,10 +548,10 @@ virVMXDomainDevicesDefPostParse(virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED, } static virDomainDefParserConfig virVMXDomainDefParserConfig = { - .hasWideSCSIBus = true, .macPrefix = {0x00, 0x0c, 0x29}, .devicesPostParseCallback = virVMXDomainDevicesDefPostParse, .domainPostParseCallback = virVMXDomainDefPostParse, + .features = VIR_DOMAIN_DEF_FEATURE_WIDE_SCSI, }; static void -- 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
Introduce VIR_DOMAIN_DEF_FEATURE_OFFLINE_CPUPIN domain feature flag whcih will allow to skip ignoring of the pinning information for hypervisor drivers which will want to implement forward-pinning of vcpus. --- src/conf/domain_conf.c | 28 +++++++++++++++++++++++----- src/conf/domain_conf.h | 1 + 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 101fae2..4220448 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4215,6 +4215,25 @@ virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr dev, } +static void +virDomainDefRemoveOfflineVcpuPin(virDomainDefPtr def) +{ + size_t i; + virDomainVcpuInfoPtr vcpu; + + for (i = 0; i < virDomainDefGetVcpusMax(def); i++) { + vcpu = virDomainDefGetVcpu(def, i); + + if (!vcpu->online && vcpu->cpumask) { + virBitmapFree(vcpu->cpumask); + vcpu->cpumask = NULL; + + VIR_WARN("Ignoring unsupported vcpupin for offline vcpu '%zu'", i); + } + } +} + + #define UNSUPPORTED(FEATURE) (!((FEATURE) & xmlopt->config.features)) /** * virDomainDefPostParseCheckFeatures: @@ -4235,6 +4254,9 @@ virDomainDefPostParseCheckFeatures(virDomainDefPtr def, virDomainDefCheckUnsupportedMemoryHotplug(def) < 0) return -1; + if (UNSUPPORTED(VIR_DOMAIN_DEF_FEATURE_OFFLINE_CPUPIN)) + virDomainDefRemoveOfflineVcpuPin(def); + return 0; } @@ -14216,11 +14238,7 @@ virDomainVcpuPinDefParseXML(virDomainDefPtr def, } VIR_FREE(tmp); - if (!(vcpu = virDomainDefGetVcpu(def, vcpuid)) || - !vcpu->online) { - /* To avoid the regression when daemon loading domain confs, we can't - * simply error out if <vcpupin> nodes greater than current vcpus. - * Ignore them instead. */ + if (!(vcpu = virDomainDefGetVcpu(def, vcpuid))) { VIR_WARN("Ignoring vcpupin for missing vcpus"); ret = 0; goto cleanup; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index c660a02..44a92d3 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2406,6 +2406,7 @@ typedef bool (*virDomainObjListACLFilter)(virConnectPtr conn, typedef enum { VIR_DOMAIN_DEF_FEATURE_WIDE_SCSI = (1 << 0), VIR_DOMAIN_DEF_FEATURE_MEMORY_HOTPLUG = (1 << 1), + VIR_DOMAIN_DEF_FEATURE_OFFLINE_CPUPIN = (1 << 2), } virDomainDefFeatures; -- 2.6.2
On 02/24/2016 09:22 AM, Peter Krempa wrote:
Introduce VIR_DOMAIN_DEF_FEATURE_OFFLINE_CPUPIN domain feature flag
Should it be VCPUPIN ?
whcih will allow to skip ignoring of the pinning information for hypervisor drivers which will want to implement forward-pinning of vcpus. --- src/conf/domain_conf.c | 28 +++++++++++++++++++++++----- src/conf/domain_conf.h | 1 + 2 files changed, 24 insertions(+), 5 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 101fae2..4220448 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4215,6 +4215,25 @@ virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr dev, }
A little intro would be nice...
+static void +virDomainDefRemoveOfflineVcpuPin(virDomainDefPtr def) +{ + size_t i; + virDomainVcpuInfoPtr vcpu; + + for (i = 0; i < virDomainDefGetVcpusMax(def); i++) { + vcpu = virDomainDefGetVcpu(def, i); + + if (!vcpu->online && vcpu->cpumask) { + virBitmapFree(vcpu->cpumask); + vcpu->cpumask = NULL; + + VIR_WARN("Ignoring unsupported vcpupin for offline vcpu '%zu'", i);
Is/was this for debugging? Do we really want to WARN or just go with INFO?
+ } + } +} + +
[...]
On Fri, Mar 04, 2016 at 07:30:39 -0500, John Ferlan wrote:
On 02/24/2016 09:22 AM, Peter Krempa wrote:
Introduce VIR_DOMAIN_DEF_FEATURE_OFFLINE_CPUPIN domain feature flag
Should it be VCPUPIN ?
Yep.
whcih will allow to skip ignoring of the pinning information for hypervisor drivers which will want to implement forward-pinning of vcpus. --- src/conf/domain_conf.c | 28 +++++++++++++++++++++++----- src/conf/domain_conf.h | 1 + 2 files changed, 24 insertions(+), 5 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 101fae2..4220448 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4215,6 +4215,25 @@ virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr dev, }
A little intro would be nice...
+static void +virDomainDefRemoveOfflineVcpuPin(virDomainDefPtr def) +{ + size_t i; + virDomainVcpuInfoPtr vcpu; + + for (i = 0; i < virDomainDefGetVcpusMax(def); i++) { + vcpu = virDomainDefGetVcpu(def, i); + + if (!vcpu->online && vcpu->cpumask) { + virBitmapFree(vcpu->cpumask); + vcpu->cpumask = NULL; + + VIR_WARN("Ignoring unsupported vcpupin for offline vcpu '%zu'", i);
Is/was this for debugging? Do we really want to WARN or just go with INFO?
No, this is basically stolen from the original place where this operation was done just with a more specific message. I did not attempt to do anything different. I'll push this with the message and we can get rid of it with a patch specifically describing the semantic change. Peter
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
On 02/24/2016 09:22 AM, Peter Krempa wrote:
Allow pinning for inactive cpus. The pinning mask will be automatically ^^^^^ NIT: vcpus
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(-)
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 02/24/2016 09:22 AM, Peter Krempa wrote:
The function was now beyond maintainability. --- src/qemu/qemu_driver.c | 134 ++++++++++++++++++++++++++++--------------------- 1 file changed, 77 insertions(+), 57 deletions(-)
hmmm... some comments below written before I viewed the patch 9 pcpumaplive changes... Perhaps there should be a merging of the two or even reverse the order with obvious logic adjustments... Without doing so leaves a 1-patch window with an issue.
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;
Because there's some goto cleanup's after this, I think cpumap should be passed by reference and not value.
+ + 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);
Are there any timing or locking consequences of this being called before qemuDomainObjEndJob and/or virDomainObjEndAPI (I don't think so, but typing what my eyes see as 'change'...)
+ + ret = 0; + + cleanup: + virBitmapFree(cpumap);
This is duplicated in the caller (pcpumaplive) and this function doesn't own this, so the caller should be left to handle.
+ 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,
This change in flow means persistent is checked before live. Theoretically, this hunk could move inside the if (persistentDef) below, unless of course it's important to error out before the live def would be handled. Although could it even be possible to have a vcpu be invalid in persistent, but valid in live?
@@ -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)
Why not just put: if (!def) return 0; in qemuDomainPinVcpuLive and then just call it...
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;
Note how you set pcpumaplive to NULL only on success; however, cleanup after setting into vcpuinfo->vcpumask means a success to set, but failure from function could free twice (of course there's also a virBitmapFree() call that's made in qemuDomainPinVcpuLive()).
- - 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);
On Fri, Mar 04, 2016 at 07:31:23 -0500, John Ferlan wrote:
On 02/24/2016 09:22 AM, Peter Krempa wrote:
The function was now beyond maintainability. --- src/qemu/qemu_driver.c | 134 ++++++++++++++++++++++++++++--------------------- 1 file changed, 77 insertions(+), 57 deletions(-)
hmmm... some comments below written before I viewed the patch 9 pcpumaplive changes... Perhaps there should be a merging of the two or even reverse the order with obvious logic adjustments... Without doing so leaves a 1-patch window with an issue.
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)
[...]
+ + if (virProcessSetAffinity(qemuDomainGetVcpuPid(vm, vcpu), cpumap) < 0) + goto cleanup; + } + + virBitmapFree(vcpuinfo->cpumask); + vcpuinfo->cpumask = cpumap; + cpumap = NULL;
Because there's some goto cleanup's after this, I think cpumap should be passed by reference and not value.
No, see the explanation below ...
+ + 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);
Are there any timing or locking consequences of this being called before qemuDomainObjEndJob and/or virDomainObjEndAPI (I don't think so, but typing what my eyes see as 'change'...)
+ + ret = 0; + + cleanup: + virBitmapFree(cpumap);
This is duplicated in the caller (pcpumaplive) and this function doesn't own this, so the caller should be left to handle.
No, the caller needs to be fixed to clear the pointer, since this function consumes the pointer regardless of the outcome. [...]
@@ -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,
This change in flow means persistent is checked before live. Theoretically, this hunk could move inside the if (persistentDef) below, unless of course it's important to error out before the live def would be handled. Although could it even be possible to have a vcpu be invalid in persistent, but valid in live?
@@ -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)
Why not just put:
if (!def) return 0;
in qemuDomainPinVcpuLive and then just call it...
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.
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;
Note how you set pcpumaplive to NULL only on success; however, cleanup after setting into vcpuinfo->vcpumask means a success to set, but failure from function could free twice (of course there's also a virBitmapFree() call that's made in qemuDomainPinVcpuLive()).
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
Now that the function was extracted we can get rid of some temp variables. Additionally formatting of the bitmap string for the event code should be checked. --- src/qemu/qemu_driver.c | 39 +++++++++++++++++---------------------- 1 file changed, 17 insertions(+), 22 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index cab69a5..b6c9ab3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4974,6 +4974,7 @@ qemuDomainPinVcpuLive(virDomainObjPtr vm, virQEMUDriverConfigPtr cfg, virBitmapPtr cpumap) { + virBitmapPtr tmpmap = NULL; virDomainVcpuInfoPtr vcpuinfo; qemuDomainObjPrivatePtr priv = vm->privateData; virCgroupPtr cgroup_vcpu = NULL; @@ -4998,6 +4999,12 @@ qemuDomainPinVcpuLive(virDomainObjPtr vm, goto cleanup; } + if (!(tmpmap = virBitmapNewCopy(cpumap))) + goto cleanup; + + if (!(str = virBitmapFormat(cpumap))) + goto cleanup; + if (vcpuinfo->online) { /* Configure the corresponding cpuset cgroup before set affinity. */ if (virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) { @@ -5013,8 +5020,8 @@ qemuDomainPinVcpuLive(virDomainObjPtr vm, } virBitmapFree(vcpuinfo->cpumask); - vcpuinfo->cpumask = cpumap; - cpumap = NULL; + vcpuinfo->cpumask = tmpmap; + tmpmap = NULL; if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps) < 0) goto cleanup; @@ -5024,7 +5031,6 @@ qemuDomainPinVcpuLive(virDomainObjPtr vm, goto cleanup; } - str = virBitmapFormat(vcpuinfo->cpumask); if (virTypedParamsAddString(&eventParams, &eventNparams, &eventMaxparams, paramField, str) < 0) goto cleanup; @@ -5034,7 +5040,7 @@ qemuDomainPinVcpuLive(virDomainObjPtr vm, ret = 0; cleanup: - virBitmapFree(cpumap); + virBitmapFree(tmpmap); virCgroupFree(&cgroup_vcpu); VIR_FREE(str); qemuDomainEventQueue(driver, event); @@ -5056,9 +5062,7 @@ qemuDomainPinVcpuFlags(virDomainPtr dom, virDomainDefPtr persistentDef; int ret = -1; virBitmapPtr pcpumap = NULL; - virBitmapPtr pcpumaplive = NULL; - virBitmapPtr pcpumappersist = NULL; - virDomainVcpuInfoPtr vcpuinfopersist = NULL; + virDomainVcpuInfoPtr vcpuinfo = NULL; virQEMUDriverConfigPtr cfg = NULL; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | @@ -5079,7 +5083,7 @@ qemuDomainPinVcpuFlags(virDomainPtr dom, goto endjob; if (persistentDef && - !(vcpuinfopersist = virDomainDefGetVcpu(persistentDef, vcpu))) { + !(vcpuinfo = virDomainDefGetVcpu(persistentDef, vcpu))) { virReportError(VIR_ERR_INVALID_ARG, _("vcpu %d is out of range of persistent cpu count %d"), vcpu, virDomainDefGetVcpus(persistentDef)); @@ -5095,21 +5099,14 @@ qemuDomainPinVcpuFlags(virDomainPtr dom, goto endjob; } - if ((def && !(pcpumaplive = virBitmapNewCopy(pcpumap))) || - (persistentDef && !(pcpumappersist = virBitmapNewCopy(pcpumap)))) + if (def && + qemuDomainPinVcpuLive(vm, def, vcpu, driver, cfg, pcpumap) < 0) goto endjob; - if (def) { - if (qemuDomainPinVcpuLive(vm, def, vcpu, driver, cfg, pcpumaplive) < 0) - goto endjob; - - pcpumaplive = NULL; - } - if (persistentDef) { - virBitmapFree(vcpuinfopersist->cpumask); - vcpuinfopersist->cpumask = pcpumappersist; - pcpumappersist = NULL; + virBitmapFree(vcpuinfo->cpumask); + vcpuinfo->cpumask = pcpumap; + pcpumap = NULL; ret = virDomainSaveConfig(cfg->configDir, driver->caps, persistentDef); goto endjob; @@ -5123,8 +5120,6 @@ qemuDomainPinVcpuFlags(virDomainPtr dom, cleanup: virDomainObjEndAPI(&vm); virBitmapFree(pcpumap); - virBitmapFree(pcpumaplive); - virBitmapFree(pcpumappersist); virObjectUnref(cfg); return ret; } -- 2.6.2
On 02/24/2016 09:22 AM, Peter Krempa wrote:
Yet another continuation of the saga. In this episode we add pinning for inactive cpus.
Peter Krempa (9): virsh: vcpupin: Ask for pinning info for all vCPUs conf: Extract code filling data for virDomainGetVcpuPinInfo qemu: Report pinning for all vCPUs in qemuDomainGetVcpuPinInfo conf: introduce parser feature flags conf: refactor checking for unsupported memory devices conf: extract ignoring of inactive vcpu pinning information qemu: add support for offline vcpupin qemu: vcpupin: Extract live vcpupin setting into a separate function qemu: Refactor bitmap handling in qemuDomainPinVcpuFlags
src/bhyve/bhyve_domain.c | 9 +- src/conf/domain_conf.c | 157 +++++++++++++++++++++++++++--- src/conf/domain_conf.h | 20 +++- src/libvirt_private.syms | 3 +- src/libxl/libxl_domain.c | 7 -- src/libxl/libxl_driver.c | 38 +------- src/lxc/lxc_domain.c | 8 -- src/openvz/openvz_driver.c | 7 -- src/phyp/phyp_driver.c | 6 +- src/qemu/qemu_domain.c | 2 + src/qemu/qemu_driver.c | 234 +++++++++++++++++++-------------------------- src/test/test_driver.c | 38 +------- src/uml/uml_driver.c | 9 +- src/vbox/vbox_common.c | 6 +- src/vmware/vmware_driver.c | 6 +- src/vmx/vmx.c | 8 +- src/vz/vz_driver.c | 6 +- src/xen/xen_driver.c | 7 -- src/xenapi/xenapi_driver.c | 7 -- tools/virsh-domain.c | 3 +- 20 files changed, 279 insertions(+), 302 deletions(-)
ACK series modulo a couple of nits pointed out - most importantly is to combine patch 8 and 9. John
On Fri, Mar 04, 2016 at 07:31:42 -0500, John Ferlan wrote:
On 02/24/2016 09:22 AM, Peter Krempa wrote:
Yet another continuation of the saga. In this episode we add pinning for inactive cpus.
ACK series modulo a couple of nits pointed out - most importantly is to combine patch 8 and 9.
I can combine them, but we usualy prefer code moves being separated from refactors. But it's really your call in this case. Peter
participants (2)
-
John Ferlan -
Peter Krempa