[libvirt] [PATCH 0/3] libxl: improve vcpu pinning
Basically, by making it possible to both query and change the vcpu to pcpu pinning of a (persistent) domain, even when it is not running. That happens by providing the implementation of virDomainGetVcpuPinInfo() and virDomainPinVcpuFlags() within he libxl driver, which is what happens in the first two patches. The third patch is something that can also be seen as a bugfix, and that's why I kept it separated from the second one, for easier review (although, the 'bug' does not really manifests, until virDomainPinVcpuFlags is implemented in patch 2). The logic is a lot similar to what happens in the QEMU driver. The patches are available in the following git branch: git://xenbits.xen.org/people/dariof/libvirt.git libxl/VcpuPinX Thanks and Regards, Dario --- Dario Faggioli (3): libxl: implement virDomainGetVcpuPinInfo libxl: implement virDomainPinVcpuFlags libxl: correctly handle affinity reset in virDomainPinVcpu[Flags] src/libxl/libxl_driver.c | 171 ++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 150 insertions(+), 21 deletions(-) -- <<This happens because I choose it to happen!>> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
So that it is possible to query vcpu related information of a persistent but not running domain, like it is for the QEMU driver. In fact, before this patch, we have: # virsh list --all Id Name State ---------------------------------------------------- 5 debian_32 running - fedora20_64 shut off # virsh vcpuinfo fedora20_64 error: this function is not supported by the connection driver: virDomainGetVcpuPinInfo After (same situation as above, i.e., fedora20_64 not running): # virsh vcpuinfo fedora20_64 VCPU: 0 CPU: N/A State: N/A CPU time N/A CPU Affinity: yyyyyyyy VCPU: 1 CPU: N/A State: N/A CPU time N/A CPU Affinity: yyyyyyyy Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com> Cc: Jim Fehlig <jfehlig@suse.com> Cc: Ian Jackson <Ian.Jackson@eu.citrix.com> --- src/libxl/libxl_driver.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 83 insertions(+) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 692c3b7..750d4ec 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -2415,6 +2415,88 @@ cleanup: return ret; } +static int +libxlDomainGetVcpuPinInfo(virDomainPtr dom, int ncpumaps, + unsigned char *cpumaps, int maplen, + unsigned int flags) +{ + libxlDriverPrivatePtr driver = dom->conn->privateData; + libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); + virDomainObjPtr vm = NULL; + virDomainDefPtr targetDef = NULL; + virDomainVcpuPinDefPtr *vcpupin_list; + virBitmapPtr cpumask = NULL; + int maxcpu, hostcpus, vcpu, pcpu, n, ret = -1; + unsigned char *cpumap; + bool pinned; + + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | + VIR_DOMAIN_AFFECT_CONFIG, -1); + + if (!(vm = libxlDomObjFromDomain(dom))) + goto cleanup; + + if (virDomainGetVcpuPinInfoEnsureACL(dom->conn, vm->def) < 0) + goto cleanup; + + if (virDomainLiveConfigHelperMethod(cfg->caps, driver->xmlopt, vm, + &flags, &targetDef) < 0) + goto cleanup; + + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + targetDef = vm->def; + } + + sa_assert(targetDef); + + /* Clamp to actual number of vcpus */ + if (ncpumaps > targetDef->vcpus) + ncpumaps = targetDef->vcpus; + + if (!cpumaps || ncpumaps < 1) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("cannot return affinity via a NULL pointer")); + goto cleanup; + } + + /* we use cfg->ctx, as vm->privateData->ctx may be NULL if VM is down */ + if ((hostcpus = libxl_get_max_cpus(cfg->ctx)) < 0) + goto cleanup; + + maxcpu = maplen * 8; + if (maxcpu > hostcpus) + maxcpu = hostcpus; + + /* initialize cpumaps */ + memset(cpumaps, 0xff, maplen * ncpumaps); + if (maxcpu % 8) { + for (vcpu = 0; vcpu < ncpumaps; vcpu++) { + cpumap = VIR_GET_CPUMAP(cpumaps, maplen, vcpu); + cpumap[maplen - 1] &= (1 << maxcpu % 8) - 1; + } + } + + /* if vcpupin setting exists, there may be unused pcpus */ + for (n = 0; n < targetDef->cputune.nvcpupin; n++) { + vcpupin_list = targetDef->cputune.vcpupin; + vcpu = vcpupin_list[n]->vcpuid; + cpumask = vcpupin_list[n]->cpumask; + cpumap = VIR_GET_CPUMAP(cpumaps, maplen, vcpu); + for (pcpu = 0; pcpu < maxcpu; pcpu++) { + if (virBitmapGetBit(cpumask, pcpu, &pinned) < 0) + goto cleanup; + if (!pinned) + VIR_UNUSE_CPU(cpumap, pcpu); + } + } + ret = ncpumaps; + +cleanup: + if (vm) + virObjectUnlock(vm); + virObjectUnref(cfg); + return ret; +} static int libxlDomainGetVcpus(virDomainPtr dom, virVcpuInfoPtr info, int maxinfo, @@ -4237,6 +4319,7 @@ static virDriver libxlDriver = { .domainGetVcpusFlags = libxlDomainGetVcpusFlags, /* 0.9.0 */ .domainPinVcpu = libxlDomainPinVcpu, /* 0.9.0 */ .domainGetVcpus = libxlDomainGetVcpus, /* 0.9.0 */ + .domainGetVcpuPinInfo = libxlDomainGetVcpuPinInfo, /* 1.2.1 */ .domainGetXMLDesc = libxlDomainGetXMLDesc, /* 0.9.0 */ .connectDomainXMLFromNative = libxlConnectDomainXMLFromNative, /* 0.9.0 */ .connectDomainXMLToNative = libxlConnectDomainXMLToNative, /* 0.9.0 */
Dario Faggioli wrote:
So that it is possible to query vcpu related information of a persistent but not running domain, like it is for the QEMU driver.
In fact, before this patch, we have: # virsh list --all Id Name State ---------------------------------------------------- 5 debian_32 running - fedora20_64 shut off # virsh vcpuinfo fedora20_64 error: this function is not supported by the connection driver: virDomainGetVcpuPinInfo
After (same situation as above, i.e., fedora20_64 not running): # virsh vcpuinfo fedora20_64 VCPU: 0 CPU: N/A State: N/A CPU time N/A CPU Affinity: yyyyyyyy
VCPU: 1 CPU: N/A State: N/A CPU time N/A CPU Affinity: yyyyyyyy
Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com> Cc: Jim Fehlig <jfehlig@suse.com> Cc: Ian Jackson <Ian.Jackson@eu.citrix.com> --- src/libxl/libxl_driver.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 83 insertions(+)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 692c3b7..750d4ec 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -2415,6 +2415,88 @@ cleanup: return ret; }
+static int +libxlDomainGetVcpuPinInfo(virDomainPtr dom, int ncpumaps, + unsigned char *cpumaps, int maplen, + unsigned int flags) +{ + libxlDriverPrivatePtr driver = dom->conn->privateData; + libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); + virDomainObjPtr vm = NULL; + virDomainDefPtr targetDef = NULL; + virDomainVcpuPinDefPtr *vcpupin_list; + virBitmapPtr cpumask = NULL; + int maxcpu, hostcpus, vcpu, pcpu, n, ret = -1; + unsigned char *cpumap; + bool pinned; + + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | + VIR_DOMAIN_AFFECT_CONFIG, -1); + + if (!(vm = libxlDomObjFromDomain(dom))) + goto cleanup; + + if (virDomainGetVcpuPinInfoEnsureACL(dom->conn, vm->def) < 0) + goto cleanup; + + if (virDomainLiveConfigHelperMethod(cfg->caps, driver->xmlopt, vm, + &flags, &targetDef) < 0) + goto cleanup; + + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + targetDef = vm->def; + } + + sa_assert(targetDef);
I think we should add a comment stating this is needed to silence Coverity, similar to the qemu driver.
+ + /* Clamp to actual number of vcpus */ + if (ncpumaps > targetDef->vcpus) + ncpumaps = targetDef->vcpus; + + if (!cpumaps || ncpumaps < 1) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("cannot return affinity via a NULL pointer")); + goto cleanup; + }
cpumaps is guaranteed to be non-NULL on entry and ncpumaps is guaranteed to be > 0 on entry, so the above check can be removed.
+ + /* we use cfg->ctx, as vm->privateData->ctx may be NULL if VM is down */ + if ((hostcpus = libxl_get_max_cpus(cfg->ctx)) < 0) + goto cleanup; + + maxcpu = maplen * 8; + if (maxcpu > hostcpus) + maxcpu = hostcpus; + + /* initialize cpumaps */ + memset(cpumaps, 0xff, maplen * ncpumaps); + if (maxcpu % 8) { + for (vcpu = 0; vcpu < ncpumaps; vcpu++) { + cpumap = VIR_GET_CPUMAP(cpumaps, maplen, vcpu); + cpumap[maplen - 1] &= (1 << maxcpu % 8) - 1; + } + } + + /* if vcpupin setting exists, there may be unused pcpus */ + for (n = 0; n < targetDef->cputune.nvcpupin; n++) { + vcpupin_list = targetDef->cputune.vcpupin; + vcpu = vcpupin_list[n]->vcpuid; + cpumask = vcpupin_list[n]->cpumask; + cpumap = VIR_GET_CPUMAP(cpumaps, maplen, vcpu); + for (pcpu = 0; pcpu < maxcpu; pcpu++) { + if (virBitmapGetBit(cpumask, pcpu, &pinned) < 0) + goto cleanup; + if (!pinned) + VIR_UNUSE_CPU(cpumap, pcpu); + } + } + ret = ncpumaps; + +cleanup: + if (vm) + virObjectUnlock(vm); + virObjectUnref(cfg); + return ret; +}
static int libxlDomainGetVcpus(virDomainPtr dom, virVcpuInfoPtr info, int maxinfo, @@ -4237,6 +4319,7 @@ static virDriver libxlDriver = { .domainGetVcpusFlags = libxlDomainGetVcpusFlags, /* 0.9.0 */ .domainPinVcpu = libxlDomainPinVcpu, /* 0.9.0 */ .domainGetVcpus = libxlDomainGetVcpus, /* 0.9.0 */ + .domainGetVcpuPinInfo = libxlDomainGetVcpuPinInfo, /* 1.2.1 */ .domainGetXMLDesc = libxlDomainGetXMLDesc, /* 0.9.0 */ .connectDomainXMLFromNative = libxlConnectDomainXMLFromNative, /* 0.9.0 */ .connectDomainXMLToNative = libxlConnectDomainXMLToNative, /* 0.9.0 */
Looks good otherwise. Thanks! Regards, Jim
On gio, 2013-12-19 at 21:32 -0700, Jim Fehlig wrote:
Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com> Cc: Jim Fehlig <jfehlig@suse.com> Cc: Ian Jackson <Ian.Jackson@eu.citrix.com> --- src/libxl/libxl_driver.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 83 insertions(+)
+static int +libxlDomainGetVcpuPinInfo(virDomainPtr dom, int ncpumaps, + unsigned char *cpumaps, int maplen, + unsigned int flags) +{ + libxlDriverPrivatePtr driver = dom->conn->privateData; + libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); + virDomainObjPtr vm = NULL; + virDomainDefPtr targetDef = NULL; + virDomainVcpuPinDefPtr *vcpupin_list; + virBitmapPtr cpumask = NULL; + int maxcpu, hostcpus, vcpu, pcpu, n, ret = -1; + unsigned char *cpumap; + bool pinned; + + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | + VIR_DOMAIN_AFFECT_CONFIG, -1); + + if (!(vm = libxlDomObjFromDomain(dom))) + goto cleanup; + + if (virDomainGetVcpuPinInfoEnsureACL(dom->conn, vm->def) < 0) + goto cleanup; + + if (virDomainLiveConfigHelperMethod(cfg->caps, driver->xmlopt, vm, + &flags, &targetDef) < 0) + goto cleanup; + + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + targetDef = vm->def; + } + + sa_assert(targetDef);
I think we should add a comment stating this is needed to silence Coverity, similar to the qemu driver.
Ok, I'll do.
+ + /* Clamp to actual number of vcpus */ + if (ncpumaps > targetDef->vcpus) + ncpumaps = targetDef->vcpus; + + if (!cpumaps || ncpumaps < 1) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("cannot return affinity via a NULL pointer")); + goto cleanup; + }
cpumaps is guaranteed to be non-NULL on entry and ncpumaps is guaranteed to be > 0 on entry, so the above check can be removed.
Right! I actually did saw that, but then forgot when going down to code the thing. :-) I'll get rid of this in v2. Thanks and Regards, Dario -- <<This happens because I choose it to happen!>> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
And use it to implement libxlDomainPinVcpu(), similarly to what happens in the QEMU driver. This way, it is possible to both query and change the vcpu affinity of a persistent but not running domain. In face, before this patch, we have: # virsh list --all Id Name State ---------------------------------------------------- 5 debian_32 running - fedora20_64 shut off # virsh vcpupin fedora20_64 0 2-4 --current error: this function is not supported by the connection driver: virDomainPinVcpuFlags After (same situation as above): # virsh vcpupin fedora20_64 0 2-4 --current # virsh vcpupin fedora20_64 0 VCPU: CPU Affinity ---------------------------------- 0: 2-4 Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com> Cc: Jim Fehlig <jfehlig@suse.com> Cc: Ian Jackson <Ian.Jackson@eu.citrix.com> --- src/libxl/libxl_driver.c | 72 ++++++++++++++++++++++++++++++++-------------- 1 file changed, 50 insertions(+), 22 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 750d4ec..4079434 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -2356,45 +2356,61 @@ cleanup: } static int -libxlDomainPinVcpu(virDomainPtr dom, unsigned int vcpu, unsigned char *cpumap, - int maplen) +libxlDomainPinVcpuFlags(virDomainPtr dom, unsigned int vcpu, + unsigned char *cpumap, int maplen, + unsigned int flags) { libxlDriverPrivatePtr driver = dom->conn->privateData; libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); - libxlDomainObjPrivatePtr priv; + virDomainDefPtr targetDef = NULL; virDomainObjPtr vm; int ret = -1; - libxl_bitmap map; + + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | + VIR_DOMAIN_AFFECT_CONFIG, -1); if (!(vm = libxlDomObjFromDomain(dom))) goto cleanup; - if (virDomainPinVcpuEnsureACL(dom->conn, vm->def) < 0) + if (virDomainPinVcpuFlagsEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup; - if (!virDomainObjIsActive(vm)) { + if ((flags & VIR_DOMAIN_AFFECT_LIVE) && !virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot pin vcpus on an inactive domain")); + _("domain is inactive")); goto cleanup; } - priv = vm->privateData; - - map.size = maplen; - map.map = cpumap; - if (libxl_set_vcpuaffinity(priv->ctx, dom->id, vcpu, &map) != 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to pin vcpu '%d' with libxenlight"), vcpu); + if (virDomainLiveConfigHelperMethod(cfg->caps, driver->xmlopt, vm, + &flags, &targetDef) < 0) goto cleanup; + + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + targetDef = vm->def; } - if (!vm->def->cputune.vcpupin) { - if (VIR_ALLOC(vm->def->cputune.vcpupin) < 0) + sa_assert(targetDef); + + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + libxl_bitmap map = { .size = maplen, .map = cpumap }; + libxlDomainObjPrivatePtr priv; + + priv = vm->privateData; + if (libxl_set_vcpuaffinity(priv->ctx, dom->id, vcpu, &map) != 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to pin vcpu '%d' with libxenlight"), + vcpu); goto cleanup; - vm->def->cputune.nvcpupin = 0; + } } - if (virDomainVcpuPinAdd(&vm->def->cputune.vcpupin, - &vm->def->cputune.nvcpupin, + + if (!targetDef->cputune.vcpupin) { + if (VIR_ALLOC(targetDef->cputune.vcpupin) < 0) + goto cleanup; + targetDef->cputune.nvcpupin = 0; + } + if (virDomainVcpuPinAdd(&targetDef->cputune.vcpupin, + &targetDef->cputune.nvcpupin, cpumap, maplen, vcpu) < 0) { @@ -2403,11 +2419,14 @@ libxlDomainPinVcpu(virDomainPtr dom, unsigned int vcpu, unsigned char *cpumap, goto cleanup; } - if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) - goto cleanup; - ret = 0; + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + ret = virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm); + } else if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + ret = virDomainSaveConfig(cfg->configDir, targetDef); + } + cleanup: if (vm) virObjectUnlock(vm); @@ -2416,6 +2435,14 @@ cleanup: } static int +libxlDomainPinVcpu(virDomainPtr dom, unsigned int vcpu, unsigned char *cpumap, + int maplen) +{ + return libxlDomainPinVcpuFlags(dom, vcpu, cpumap, maplen, + VIR_DOMAIN_AFFECT_LIVE); +} + +static int libxlDomainGetVcpuPinInfo(virDomainPtr dom, int ncpumaps, unsigned char *cpumaps, int maplen, unsigned int flags) @@ -4318,6 +4345,7 @@ static virDriver libxlDriver = { .domainSetVcpusFlags = libxlDomainSetVcpusFlags, /* 0.9.0 */ .domainGetVcpusFlags = libxlDomainGetVcpusFlags, /* 0.9.0 */ .domainPinVcpu = libxlDomainPinVcpu, /* 0.9.0 */ + .domainPinVcpuFlags = libxlDomainPinVcpuFlags, /* 1.2.1 */ .domainGetVcpus = libxlDomainGetVcpus, /* 0.9.0 */ .domainGetVcpuPinInfo = libxlDomainGetVcpuPinInfo, /* 1.2.1 */ .domainGetXMLDesc = libxlDomainGetXMLDesc, /* 0.9.0 */
By actually removing the <vcpupin> element (from within the <cputune> section) from the XML, rather than jus update it with a fully set vcpu affinity mask. Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com> Cc: Jim Fehlig <jfehlig@suse.com> Cc: Ian Jackson <Ian.Jackson@eu.citrix.com> --- src/libxl/libxl_driver.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 4079434..9a0898a 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -2363,6 +2363,7 @@ libxlDomainPinVcpuFlags(virDomainPtr dom, unsigned int vcpu, libxlDriverPrivatePtr driver = dom->conn->privateData; libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); virDomainDefPtr targetDef = NULL; + virBitmapPtr pcpumap = NULL; virDomainObjPtr vm; int ret = -1; @@ -2391,6 +2392,10 @@ libxlDomainPinVcpuFlags(virDomainPtr dom, unsigned int vcpu, sa_assert(targetDef); + pcpumap = virBitmapNewData(cpumap, maplen); + if (!pcpumap) + goto cleanup; + if (flags & VIR_DOMAIN_AFFECT_LIVE) { libxl_bitmap map = { .size = maplen, .map = cpumap }; libxlDomainObjPrivatePtr priv; @@ -2404,6 +2409,17 @@ libxlDomainPinVcpuFlags(virDomainPtr dom, unsigned int vcpu, } } + /* full bitmap means reset the settings (if any). */ + if (virBitmapIsAllSet(pcpumap)) { + if (virDomainVcpuPinDel(targetDef, vcpu) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to delete vcpupin xml for vcpu '%d'"), + vcpu); + goto cleanup; + } + goto out; + } + if (!targetDef->cputune.vcpupin) { if (VIR_ALLOC(targetDef->cputune.vcpupin) < 0) goto cleanup; @@ -2419,6 +2435,7 @@ libxlDomainPinVcpuFlags(virDomainPtr dom, unsigned int vcpu, goto cleanup; } +out: ret = 0; if (flags & VIR_DOMAIN_AFFECT_LIVE) { @@ -2430,6 +2447,7 @@ libxlDomainPinVcpuFlags(virDomainPtr dom, unsigned int vcpu, cleanup: if (vm) virObjectUnlock(vm); + virBitmapFree(pcpumap); virObjectUnref(cfg); return ret; }
participants (2)
-
Dario Faggioli -
Jim Fehlig