[libvirt] [PATCH 15/34] qemu: Reuse qemuDomainDetectVcpuPids in cpu hot(un)plug

[John Ferlan]

On 01/14/2016 11:27 AM, Peter Krempa wrote:
> Now that qemuDomainDetectVcpuPids is able to refresh the vCPU pid
> information it can be reused in the hotplug and hotunplug code paths
> rather than open-coding a very similar algorithm.
> 
> A slight algoirithm change is necessary for unplug since the vCPU needs

s/algoirithm/algorithm/

> to be marked offline prior to calling the thread detector function and
> eventually rolled back if something fails.
> ---
>  src/qemu/qemu_driver.c | 72 +++++++++++++-------------------------------------
>  1 file changed, 18 insertions(+), 54 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 3aa49f2..b377738 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -4684,11 +4684,10 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver,
>      int ret = -1;
>      int rc;
>      int oldvcpus = virDomainDefGetVcpus(vm->def);
> -    pid_t *cpupids = NULL;
> -    int ncpupids = 0;
>      virCgroupPtr cgroup_vcpu = NULL;
>      char *mem_mask = NULL;
>      virDomainNumatuneMemMode mem_mode;
> +    pid_t vcpupid;
> 
>      if (!(vcpuinfo = virDomainDefGetVcpu(vm->def, vcpu)))
>          return -1;
> @@ -4703,9 +4702,6 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver,
> 
>      rc = qemuMonitorSetCPU(priv->mon, vcpu, true);
> 
> -    if (rc == 0)
> -        ncpupids = qemuMonitorGetCPUInfo(priv->mon, &cpupids);
> -
>      if (qemuDomainObjExitMonitor(driver, vm) < 0)
>          goto cleanup;
> 
> @@ -4716,23 +4712,10 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver,
> 
>      vcpuinfo->online = true;
> 
> -    if (ncpupids < 0)
> -        goto cleanup;
> -
> -    /* failure to re-detect vCPU pids after hotplug due to lack of support was
> -     * historically deemed not fatal. We need to skip the rest of the steps though. */
> -    if (ncpupids == 0) {
> -        ret = 0;
> +    if (qemuDomainDetectVcpuPids(driver, vm, QEMU_ASYNC_JOB_NONE) < 0)
>          goto cleanup;
> -    }

This can still return zero and an empty cpupids array (eg priv->ncpupids
== 0)...

I think you'll need an extra "if (priv->ncpupids == 0) ret = 0; goto
cleanup;"

> 
> -    if (ncpupids != oldvcpus + 1) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> -                       _("got wrong number of vCPU pids from QEMU monitor. "
> -                         "got %d, wanted %d"),
> -                       ncpupids, oldvcpus + 1);
> -        goto cleanup;
> -    }
> +    vcpupid = qemuDomainGetVcpuPid(vm, vcpu);

Thus causing this to return 0 in vcpupid...

> 
>      if (virDomainNumatuneGetMode(vm->def->numa, -1, &mem_mode) == 0 &&
>          mem_mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT &&
> @@ -4742,11 +4725,9 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver,
>          goto cleanup;
> 
>      if (priv->cgroup) {
> -        cgroup_vcpu =
> -            qemuDomainAddCgroupForThread(priv->cgroup,
> -                                         VIR_CGROUP_THREAD_VCPU,
> -                                         vcpu, mem_mask,
> -                                         cpupids[vcpu]);
> +        cgroup_vcpu = qemuDomainAddCgroupForThread(priv->cgroup,
> +                                                   VIR_CGROUP_THREAD_VCPU,
> +                                                   vcpu, mem_mask, vcpupid);

And not doing the right thing here...

>          if (!cgroup_vcpu)
>              goto cleanup;
>      }
> @@ -4758,26 +4739,20 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver,
>                                      &vm->def->cputune.nvcpupin) < 0)
>              goto cleanup;
> 
> -        if (qemuDomainHotplugPinThread(vm->def->cpumask, vcpu, cpupids[vcpu],
> +        if (qemuDomainHotplugPinThread(vm->def->cpumask, vcpu, vcpupid,

here...

>                                         cgroup_vcpu) < 0) {
>              goto cleanup;
>          }
>      }
> 
> -    if (qemuProcessSetSchedParams(vcpu, cpupids[vcpu],
> +    if (qemuProcessSetSchedParams(vcpu, vcpupid,
>                                    vm->def->cputune.nvcpusched,
>                                    vm->def->cputune.vcpusched) < 0)

and here...

>          goto cleanup;
> 
> -    priv->nvcpupids = ncpupids;
> -    VIR_FREE(priv->vcpupids);
> -    priv->vcpupids = cpupids;
> -    cpupids = NULL;
> -
>      ret = 0;
> 
>   cleanup:
> -    VIR_FREE(cpupids);
>      VIR_FREE(mem_mask);
>      virCgroupFree(&cgroup_vcpu);
>      return ret;
> @@ -4794,8 +4769,6 @@ qemuDomainHotplugDelVcpu(virQEMUDriverPtr driver,
>      int ret = -1;
>      int rc;
>      int oldvcpus = virDomainDefGetVcpus(vm->def);
> -    pid_t *cpupids = NULL;
> -    int ncpupids = 0;
> 
>      if (!(vcpuinfo = virDomainDefGetVcpu(vm->def, vcpu)))
>          return -1;
> @@ -4806,30 +4779,23 @@ qemuDomainHotplugDelVcpu(virQEMUDriverPtr driver,
>          return -1;
>      }
> 
> +    vcpuinfo->online = false;
> +
>      qemuDomainObjEnterMonitor(driver, vm);
> 
>      rc = qemuMonitorSetCPU(priv->mon, vcpu, false);
> 
> -    if (rc == 0)
> -        ncpupids = qemuMonitorGetCPUInfo(priv->mon, &cpupids);
> -
>      if (qemuDomainObjExitMonitor(driver, vm) < 0)
>          goto cleanup;
> 
> -    virDomainAuditVcpu(vm, oldvcpus, oldvcpus - 1, "update",
> -                       rc == 0 && ncpupids == oldvcpus -1);
> +    if (rc < 0)
> +        rc = qemuDomainDetectVcpuPids(driver, vm, QEMU_ASYNC_JOB_NONE);

Similar comments here regarding the 'priv->ncpupids == 0' handling
(e.g., DetectVcpuPids succeeds, but still returns empty cpupids).

Also, why is this call only made when (rc < 0)?

> 
> -    if (rc < 0 || ncpupids < 0)
> -        goto cleanup;
> +    virDomainAuditVcpu(vm, oldvcpus, oldvcpus - 1, "update", rc == 0);

This is auditing qemuMonitorGetCPUInfo rather than qemuMonitorSetCPU ?

John
> 
> -    /* check if hotunplug has failed */
> -    if (ncpupids != oldvcpus - 1) {
> -        virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> -                       _("qemu didn't unplug vCPU '%u' properly"), vcpu);
> +    if (rc < 0)
>          goto cleanup;
> -    }
> 
> -    vcpuinfo->online = false;
> 
>      if (qemuDomainDelCgroupForThread(priv->cgroup,
>                                       VIR_CGROUP_THREAD_VCPU, vcpu) < 0)
> @@ -4840,15 +4806,13 @@ qemuDomainHotplugDelVcpu(virQEMUDriverPtr driver,
>                      &vm->def->cputune.nvcpupin,
>                      vcpu);
> 
> -    priv->nvcpupids = ncpupids;
> -    VIR_FREE(priv->vcpupids);
> -    priv->vcpupids = cpupids;
> -    cpupids = NULL;
> -
>      ret = 0;
> 
>   cleanup:
> -    VIR_FREE(cpupids);
> +    /* rollback the cpu state */
> +    if (ret < 0)
> +        vcpuinfo->online = true;
> +
>      return ret;
>  }
>