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;
> }
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list