Re: [libvirt] [PATCH v5 4/5] qemu: Add support to pin IOThreads to specific CPU

On Fri, Mar 06, 2015 at 09:05:44AM -0500, John Ferlan wrote: > Add qemuDomainPinIOThread to handle setting the CPU affinity > for a specific IOThread > > Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; > --- > include/libvirt/libvirt-domain.h | 9 ++ > src/qemu/qemu_driver.c | 221 +++++++++++++++++++++++++++++++++++++++ > 2 files changed, 230 insertions(+) > [...] > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index b37474f..aad08b2 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -5780,6 +5780,226 @@ qemuDomainGetIOThreadsInfo(virDomainPtr dom, > return ret; > } > > +static int > +qemuDomainPinIOThread(virDomainPtr dom, > + unsigned int iothread_id, > + unsigned char *cpumap, > + int maplen, > + unsigned int flags) > +{ > + int ret = -1; > + virQEMUDriverPtr driver = dom->conn->privateData; > + virQEMUDriverConfigPtr cfg = NULL; > + virDomainObjPtr vm; > + virCapsPtr caps = NULL; > + virDomainDefPtr persistentDef = NULL; > + virBitmapPtr pcpumap = NULL; > + bool doReset = false; > + qemuDomainObjPrivatePtr priv; > + virDomainVcpuPinDefPtr *newIOThreadsPin = NULL; > + size_t newIOThreadsPinNum = 0; > + virCgroupPtr cgroup_iothread = 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); > + > + cfg = virQEMUDriverGetConfig(driver); > + > + if (!(vm = qemuDomObjFromDomain(dom))) > + goto cleanup; > + priv = vm->privateData; > + > + if (virDomainPinIOThreadEnsureACL(dom->conn, vm->def, flags) < 0) > + goto cleanup; > + > + if (!(caps = virQEMUDriverGetCapabilities(driver, false))) > + goto cleanup; > + > + if (vm->def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) { > + virReportError(VIR_ERR_OPERATION_INVALID, "%s", > + _("Changing affinity for IOThread dynamically is " > + "not allowed when CPU placement is 'auto'")); > + goto cleanup; > + } > + > + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) > + goto cleanup; > + > + if (virDomainLiveConfigHelperMethod(caps, driver->xmlopt, vm, &flags, > + &persistentDef) < 0) > + goto endjob; > + > + if (flags & VIR_DOMAIN_AFFECT_LIVE) > + persistentDef = vm->def; This is wrong and you cannot do that. Imagine that the live definition is already different than the persistent definition. Just remove those two lines.

> + > + /* Coverity didn't realize that targetDef must be set if we got here. */ > + sa_assert(persistentDef);

> + > + if (!(pcpumap = virBitmapNewData(cpumap, maplen))) > + goto endjob; > + > + if (virBitmapIsAllClear(pcpumap)) { > + virReportError(VIR_ERR_INVALID_ARG, "%s", > + _("Empty iothread cpumap list for pinning")); > + goto endjob; > + } > + > + /* pinning to all physical cpus means resetting, > + * so check if we can reset setting. > + */ > + if (virBitmapIsAllSet(pcpumap)) > + doReset = true; > + > + if (flags & VIR_DOMAIN_AFFECT_LIVE) { > + > + if (priv->iothreadpids == NULL) { > + virReportError(VIR_ERR_OPERATION_INVALID, > + "%s", _("IOThread affinity is not supported")); > + goto endjob; > + } > + > + if (iothread_id > priv->niothreadpids) { > + virReportError(VIR_ERR_INVALID_ARG, > + _("iothread value out of range %d > %d"), > + iothread_id, priv->niothreadpids); > + goto endjob; > + } > + > + if (vm->def->cputune.iothreadspin) { > + /* The VcpuPinDefCopy works for IOThreads too */ > + newIOThreadsPin = > + virDomainVcpuPinDefCopy(vm->def->cputune.iothreadspin, > + vm->def->cputune.niothreadspin); > + if (!newIOThreadsPin) > + goto endjob; > + > + newIOThreadsPinNum = vm->def->cputune.niothreadspin; > + } else { > + if (VIR_ALLOC(newIOThreadsPin) < 0) > + goto endjob; > + newIOThreadsPinNum = 0; > + } > + > + if (virDomainIOThreadsPinAdd(&newIOThreadsPin, &newIOThreadsPinNum, > + cpumap, maplen, iothread_id) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("failed to update iothreadspin")); > + virDomainVcpuPinDefArrayFree(newIOThreadsPin, newIOThreadsPinNum); This free function needs to be called in cleanup section because we are jumping to endjob from several places.

> + goto endjob; > + } > + > + /* Configure the corresponding cpuset cgroup before set affinity. */ > + if (virCgroupHasController(priv->cgroup, > + VIR_CGROUP_CONTROLLER_CPUSET)) { > + if (virCgroupNewIOThread(priv->cgroup, iothread_id, > + false, &cgroup_iothread) < 0) > + goto endjob; > + if (qemuSetupCgroupIOThreadsPin(cgroup_iothread, > + newIOThreadsPin, > + newIOThreadsPinNum, > + iothread_id) < 0) { > + virReportError(VIR_ERR_OPERATION_INVALID, > + _("failed to set cpuset.cpus in cgroup" > + " for iothread %d"), iothread_id); > + goto endjob; > + } > + } else { > + if (virProcessSetAffinity(priv->iothreadpids[iothread_id - 1], > + pcpumap) < 0) { > + virReportError(VIR_ERR_SYSTEM_ERROR, > + _("failed to set cpu affinity for IOThread %d"), > + iothread_id); > + goto endjob; > + } > + } > + > + if (doReset) { > + virDomainIOThreadsPinDel(vm->def, iothread_id); > + } else { > + if (vm->def->cputune.iothreadspin) > + virDomainVcpuPinDefArrayFree(vm->def->cputune.iothreadspin, > + vm->def->cputune.niothreadspin); > + > + vm->def->cputune.iothreadspin = newIOThreadsPin; > + vm->def->cputune.niothreadspin = newIOThreadsPinNum; > + newIOThreadsPin = NULL; > + } > + > + if (newIOThreadsPin) > + virDomainVcpuPinDefArrayFree(newIOThreadsPin, newIOThreadsPinNum); Those two exact lines should be in cleanup section.

> + > + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) > + goto endjob; > + > + if (snprintf(paramField, VIR_TYPED_PARAM_FIELD_LENGTH, > + VIR_DOMAIN_TUNABLE_CPU_IOTHREADSPIN, iothread_id) < 0) { > + goto endjob; > + } > + > + str = virBitmapFormat(pcpumap); > + if (virTypedParamsAddString(&eventParams, &eventNparams, > + &eventMaxparams, paramField, str) < 0) > + goto endjob; > + > + event = virDomainEventTunableNewFromDom(dom, eventParams, eventNparams); > + } > + > + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { > + if (iothread_id > persistentDef->iothreads) { > + virReportError(VIR_ERR_INVALID_ARG, > + _("iothread value out of range %d > %d"), > + iothread_id, persistentDef->iothreads); > + goto endjob; > + } > + > + if (doReset) { > + virDomainIOThreadsPinDel(persistentDef, iothread_id); > + } else { > + if (!persistentDef->cputune.iothreadspin) { > + if (VIR_ALLOC(persistentDef->cputune.iothreadspin) < 0) > + goto endjob; > + persistentDef->cputune.niothreadspin = 0; > + } > + if (virDomainIOThreadsPinAdd(&persistentDef->cputune.iothreadspin, > + &persistentDef->cputune.niothreadspin, > + cpumap, > + maplen, > + iothread_id) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("failed to update or add iothreadspin xml " > + "of a persistent domain")); > + goto endjob; > + } > + } > + > + ret = virDomainSaveConfig(cfg->configDir, persistentDef); > + goto endjob; > + } > + > + ret = 0; > + > + endjob: > + qemuDomainObjEndJob(driver, vm); > + > + cleanup: > + if (cgroup_iothread) > + virCgroupFree(&cgroup_iothread); > + if (event) > + qemuDomainEventQueue(driver, event); > + VIR_FREE(str); > + virBitmapFree(pcpumap); > + qemuDomObjEndAPI(&vm); > + virObjectUnref(caps); > + virObjectUnref(cfg); > + return ret; > +} > + > static int qemuDomainGetSecurityLabel(virDomainPtr dom, virSecurityLabelPtr seclabel) > { > virQEMUDriverPtr driver = dom->conn->privateData; > @@ -19328,6 +19548,7 @@ static virHypervisorDriver qemuHypervisorDriver = { > .domainGetVcpus = qemuDomainGetVcpus, /* 0.4.4 */ > .domainGetMaxVcpus = qemuDomainGetMaxVcpus, /* 0.4.4 */ > .domainGetIOThreadsInfo = qemuDomainGetIOThreadsInfo, /* 1.2.14 */ > + .domainPinIOThread = qemuDomainPinIOThread, /* 1.2.14 */ > .domainGetSecurityLabel = qemuDomainGetSecurityLabel, /* 0.6.1 */ > .domainGetSecurityLabelList = qemuDomainGetSecurityLabelList, /* 0.10.0 */ > .nodeGetSecurityModel = qemuNodeGetSecurityModel, /* 0.6.1 */ > -- > 2.1.0 > Otherwise looks good. ACK with the fixes mentioned above. Pavel