[libvirt] [PATCH 8/9] qemu: vcpupin: Extract live vcpupin setting into a separate function

[Peter Krempa]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160309/b76d8970/attachment-0001.sig>