[libvirt] [PATCH 8/9] qemu: vcpupin: Extract live vcpupin setting into a separate function
John Ferlan
jferlan at redhat.com
Fri Mar 4 12:31:23 UTC 2016
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);
>
More information about the libvir-list
mailing list