[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v2 8/9] qemu: Add support to Add/Delete IOThreads



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 redhat com>
> ---
>  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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]