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; }