On Fri, Apr 10, 2015 at 17:36:26 -0400, John Ferlan wrote:
> Add qemuDomainAddIOThread and qemuDomainDelIOThread in order to add or
> remove an IOThread to/from the host either for live or config optoins
>
> The implementation for the 'live' option will use the iothreadpids list
> in order to make decision, while the 'config' option will use the
> iothreadids list. Additionally, for deletion each may have to adjust
> the iothreadpin list.
>
> IOThreads are implemented by qmp objects, the code makes use of the existing
> qemuMonitorAddObject or qemuMonitorDelObject APIs.
>
> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
> ---
> src/conf/domain_audit.c | 9 +
> src/conf/domain_audit.h | 6 +
> src/libvirt_private.syms | 1 +
> src/qemu/qemu_driver.c | 432 +++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 448 insertions(+)
>
...
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index d99f886..5b0784f 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -6186,6 +6186,436 @@ qemuDomainPinIOThread(virDomainPtr dom,
> return ret;
> }
>
> +static int
> +qemuDomainHotplugIOThread(virQEMUDriverPtr driver,
> + virDomainObjPtr vm,
> + unsigned int iothread_id,
> + const char *name,
> + bool add)
> +{
> + qemuDomainObjPrivatePtr priv = vm->privateData;
> + char *alias = NULL;
> + size_t i, idx;
> + int rc = -1;
> + int ret = -1;
> + unsigned int orig_niothreads = vm->def->iothreads;
> + unsigned int exp_niothreads = vm->def->iothreads;
> + int new_niothreads = 0;
> + qemuMonitorIOThreadInfoPtr *new_iothreads = NULL;
> + virCgroupPtr cgroup_iothread = NULL;
> + char *mem_mask = NULL;
> +
> + /* Let's see if we can find this iothread_id in our iothreadpids list
> + * For add finding the same iothread_id will cause a failure since we
> + * cannot add the same iothread_id twice.
> + * For del finding our iothread_id is good since we cannot delete
> + * something that doesn't exist
> + */
The comment states obvious facts.
> + for (idx = 0; idx < priv->niothreadpids; idx++) {
> + if (iothread_id == priv->iothreadpids[idx]->iothread_id)
> + break;
> + }
> +
> + if (add) {
> + if (idx < priv->niothreadpids) {
> + virReportError(VIR_ERR_INVALID_ARG,
> + _("an IOThread is already using iothread_id "
> + "'%u' in iothreadpids"),
> + iothread_id);
> + goto cleanup;
> + }
> + } else {
> + if (idx == priv->niothreadpids) {
> + virReportError(VIR_ERR_INVALID_ARG,
> + _("cannot find IOThread '%u' in iothreadpids"),
> + iothread_id);
> + goto cleanup;
> + }
> +
> + /* The qemuDomainDelThread doesn't pass (or need to pass) the
> + * name. So we'll grab it here so that we can formulate the
> + * correct alias for qemuMonitorDelObject to find this object.
> + */
With the changes I've suggested this won't be necessary
> + name = priv->iothreadpids[idx]->name;
> + }
> +
> + /* Generate alias */
> + if (name) {
> + if (virAsprintf(&alias, "%s_iothread%u", name, iothread_id) < 0)
> + return -1;
> + } else {
> + if (virAsprintf(&alias, "iothread%u", iothread_id) < 0)
> + return -1;
> + }
Neither this.
> +
> + qemuDomainObjEnterMonitor(driver, vm);
> +
> + if (!virDomainObjIsActive(vm)) {
> + virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> + _("cannot change IOThreads for an inactive domain"));
> + goto exit_monitor;
> + }
This is a bit too late to check if the domain is active.
> +
> + if (add) {
> + rc = qemuMonitorAddObject(priv->mon, "iothread", alias, NULL);
> + exp_niothreads++;
> + } else {
> + rc = qemuMonitorDelObject(priv->mon, alias);
> + exp_niothreads--;
> + }
> +
> + if (rc < 0)
> + goto exit_monitor;
> +
> + /* After hotplugging the IOThreads we need to re-detect the
> + * IOThreads thread_id's, adjust the cgroups, thread affinity,
> + * and the priv->iothreadpids list.
> + */
> + if ((new_niothreads = qemuMonitorGetIOThreads(priv->mon,
> + &new_iothreads)) < 0) {
> + virResetLastError();
> + goto exit_monitor;
In this case you'd report an empty error as exit_monitor leads to
returning -1 without reporting any new one.
> + }
> +
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + goto cleanup;
> +
> + /* ohhh something went wrong */
Obvious.
> + if (new_niothreads != exp_niothreads) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("got wrong number of IOThread ids from QEMU monitor. "
> + "got %d, wanted %d"),
> + new_niothreads, exp_niothreads);
> + vm->def->iothreads = new_niothreads;
> + goto cleanup;
> + }
> + vm->def->iothreads = exp_niothreads;
> +
> + if (virDomainNumatuneGetMode(vm->def->numa, -1) ==
> + VIR_DOMAIN_NUMATUNE_MEM_STRICT &&
> + virDomainNumatuneMaybeFormatNodeset(vm->def->numa,
> + priv->autoNodeset,
> + &mem_mask, -1) < 0)
> + goto cleanup;
We really should store the above data somewhere. I've seen the above
construct a few times in different places lately.
> +
> +
> + if (add) {
> + qemuDomainIOThreadInfoPtr info;
> + unsigned int idval = 0;
> + char *idname = NULL;
> + int thread_id;
> +
> + /*
> + * If we've successfully added an IOThread, find out where we added it
> + * in the new IOThread list, so we can add it to our iothreadpids list
> + */
> + for (i = 0; i < new_niothreads; i++) {
> +
> + if (qemuDomainParseIOThreadAlias(new_iothreads[i]->name,
> + &idval, &idname) < 0)
> + goto cleanup;
You already know the data as you've formated it a few lines above.
> +
> + if (iothread_id == idval)
> + break;
> +
> + VIR_FREE(idname);
> + }
> +
> + /* something else went wrong */
...
> + if (idval != iothread_id) {
> + VIR_FREE(idname);
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("cannot find new IOThread '%u' in QEMU monitor."),
> + iothread_id);
> + goto cleanup;
> + }
> + thread_id = new_iothreads[i]->thread_id;
> +
> + if (VIR_ALLOC(info) < 0) {
> + VIR_FREE(idname);
> + goto cleanup;
> + }
> + info->thread_id = thread_id;
> + info->iothread_id = iothread_id;
> + info->name = idname;
> + if (VIR_APPEND_ELEMENT(priv->iothreadpids,
> + priv->niothreadpids, info) < 0) {
> + VIR_FREE(info->name);
> + VIR_FREE(info);
> + goto cleanup;
> + }
> +
> + /* Add IOThread to cgroup if present */
> + if (priv->cgroup) {
> + cgroup_iothread =
> + qemuDomainAddCgroupForThread(priv->cgroup,
> + VIR_CGROUP_THREAD_IOTHREAD,
> + iothread_id, mem_mask, thread_id);
> + if (!cgroup_iothread)
> + // Do we remove from iothreadpids?
The coding guidelines state that the old style comments should be used
rather than //
> + goto cleanup;
> + }
> +
> + /* Inherit def->cpuset */
> + if (vm->def->cpumask) {
> + if (qemuDomainHotplugAddPin(vm->def->cpumask, iothread_id,
> + &vm->def->cputune.iothreadspin,
> + &vm->def->cputune.niothreadspin) < 0)
> +
> + // Do we remove from iothreadpids && scratch the cgroup?
...
> + goto cleanup;
> +
> + if (qemuDomainHotplugPinThread(vm->def->cpumask, iothread_id,
> + thread_id, cgroup_iothread) < 0)
> + // Do we remove from iothreadpids, iothreadspin, and cgroup
...
> + goto cleanup;
> +
> + if (qemuProcessSetSchedParams(iothread_id, thread_id,
> + vm->def->cputune.niothreadsched,
> + vm->def->cputune.iothreadsched) < 0)
> + goto cleanup;
> + }
> + } else {
> +
> + /* Remove our iothread_id from the list of iothreadpid's */
> + if (VIR_DELETE_ELEMENT(priv->iothreadpids, idx,
> + priv->niothreadpids) < 0)
> + goto cleanup;
> +
> + /* Remove the cgroup links */
> + if (qemuDomainDelCgroupForThread(priv->cgroup,
> + VIR_CGROUP_THREAD_IOTHREAD,
> + iothread_id) < 0)
> + goto cleanup;
> +
> + /* Free pin setting */
> + virDomainPinDel(&vm->def->cputune.iothreadspin,
> + &vm->def->cputune.niothreadspin,
> + iothread_id);
I think it would be preferable to split this function into two, one for
adding and one for removing.
> + }
> +
> + ret = 0;
> +
> + cleanup:
> + if (new_iothreads) {
> + for (i = 0; i < new_niothreads; i++)
> + qemuMonitorIOThreadInfoFree(new_iothreads[i]);
> + VIR_FREE(new_iothreads);
> + }
> + VIR_FREE(mem_mask);
> + virDomainAuditIOThread(vm, orig_niothreads, new_niothreads,
> + "update", rc == 0);
> + if (cgroup_iothread)
> + virCgroupFree(&cgroup_iothread);
> + VIR_FREE(alias);
> + return ret;
> +
> + exit_monitor:
> + ignore_value(qemuDomainObjExitMonitor(driver, vm));
> + goto cleanup;
> +}
> +
> +static int
> +qemuDomainChgIOThread(virQEMUDriverPtr driver,
> + virDomainObjPtr vm,
> + unsigned int iothread_id,
> + const char *name,
> + bool add,
> + unsigned int flags)
> +{
> + virQEMUDriverConfigPtr cfg = NULL;
> + virCapsPtr caps = NULL;
> + qemuDomainObjPrivatePtr priv;
> + virCgroupPtr cgroup_temp = NULL;
> + virBitmapPtr all_nodes = NULL;
> + char *all_nodes_str = NULL;
> + char *mem_mask = NULL;
> + virDomainDefPtr persistentDef;
> + size_t i;
> + int ret = -1;
> +
> + if ((unsigned short) iothread_id != iothread_id) {
> + virReportError(VIR_ERR_INVALID_ARG,
> + _("argument out of range: %d"), iothread_id);
Again, please store it as an int and kill this code.
> + return -1;
> + }
> +
> +
> + cfg = virQEMUDriverGetConfig(driver);
> +
> + if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
> + goto cleanup;
> +
> + if (!add) {
> + /* If there is a disk using the IOThread to be removed, then fail. */
> + for (i = 0; i < vm->def->ndisks; i++) {
> + if (vm->def->disks[i]->iothread == iothread_id) {
> + virReportError(VIR_ERR_INVALID_ARG,
> + _("cannot remove IOThread %u since it "
> + "is being used by disk path '%s'"),
> + iothread_id,
> + NULLSTR(vm->def->disks[i]->src->path));
> + goto cleanup;
> + }
> + }
> + }
> +
> + priv = vm->privateData;
> +
> + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
> + goto cleanup;
> +
> + if (flags & VIR_DOMAIN_AFFECT_LIVE) {
This should be after virDomainLiveConfigHelperMethod since it updates
@flags.
> + if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_EMULATOR, 0,
> + false, &cgroup_temp) < 0)
> + goto endjob;
> +
> + if (!(all_nodes = virNumaGetHostNodeset()))
> + goto endjob;
> +
> + if (!(all_nodes_str = virBitmapFormat(all_nodes)))
> + goto endjob;
> +
> + if (virCgroupGetCpusetMems(cgroup_temp, &mem_mask) < 0 ||
> + virCgroupSetCpusetMems(cgroup_temp, all_nodes_str) < 0)
> + goto endjob;
> + }
> +
> + if (virDomainLiveConfigHelperMethod(caps, driver->xmlopt, vm, &flags,
> + &persistentDef) < 0)
> + goto endjob;
> +
> + /* For a live change - let's make sure the binary supports this */
> + if (flags & VIR_DOMAIN_AFFECT_LIVE &&
> + !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("IOThreads not supported with this binary"));
> + goto endjob;
The inactive config will fail if qemu doesn't support iothreads, won't
it?
> + }
> +
> + if (flags & VIR_DOMAIN_AFFECT_LIVE) {
> +
> + if (qemuDomainHotplugIOThread(driver, vm, iothread_id, name, add) < 0)
> + goto endjob;
> +
> + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0)
> + goto endjob;
> + }
> +
> + if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
Having the config section appear before the LIVE section will make it
less prone to return failure but the thread being actually added.
> + virDomainIOThreadIDDefPtr iddef;
> +
> + iddef = virDomainIOThreadIDFind(persistentDef, iothread_id);
> + if (add) {
> + /* Already there? Then error since we cannot have the same
> + * iothread_id listed twice
> + */
> + if (iddef) {
> + virReportError(VIR_ERR_INVALID_ARG,
> + _("an IOThread is already using iothread_id "
> + "'%u' in persistent iothreadids"),
> + iothread_id);
> + goto endjob;
> + }
> +
> + if (virDomainIOThreadIDAdd(persistentDef, iothread_id, name) < 0)
> + goto endjob;
> +
> + /* Nothing to do in iothreadspin list (that's a separate command) */
> +
> + persistentDef->iothreads++;
> + } else {
> + if (!iddef) {
> + virReportError(VIR_ERR_INVALID_ARG,
> + _("cannot find IOThread '%u' in persistent "
> + "iothreadids"),
> + iothread_id);
> + goto cleanup;
> + }
> +
> + virDomainIOThreadIDDel(persistentDef, iothread_id);
> + virDomainPinDel(&persistentDef->cputune.iothreadspin,
> + &persistentDef->cputune.niothreadspin,
> + iothread_id);
> + persistentDef->iothreads--;
> + }
> +
> + if (virDomainSaveConfig(cfg->configDir, persistentDef) < 0)
> + goto endjob;
> + }
> +
> + ret = 0;
> +
> + endjob:
> + if (mem_mask) {
> + virErrorPtr err = virSaveLastError();
> + virCgroupSetCpusetMems(cgroup_temp, mem_mask);
> + virSetError(err);
> + virFreeError(err);
> + }
> + qemuDomainObjEndJob(driver, vm);
> +
> + cleanup:
> + VIR_FREE(mem_mask);
> + VIR_FREE(all_nodes_str);
> + virBitmapFree(all_nodes);
> + virCgroupFree(&cgroup_temp);
> + virObjectUnref(caps);
> + virObjectUnref(cfg);
> + return ret;
> +}
> +
Peter
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list