Re: [PATCH 33/34] qemu: iothread: Aggregate code to set IOTrhead tuning

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

 




On 01/14/2016 11:27 AM, Peter Krempa wrote:
> Rather than iterating 3 times for various settings this function
> aggregates all the code into single place. One of the other advantages
> is that it can then be reused for properly setting IOThread info on
> hotplug.
> ---
>  src/qemu/qemu_cgroup.c  |  93 -----------------------------
>  src/qemu/qemu_cgroup.h  |   1 -
>  src/qemu/qemu_process.c | 154 ++++++++++++++++++++++++++++++++----------------
>  src/qemu/qemu_process.h |   2 +
>  4 files changed, 105 insertions(+), 145 deletions(-)
> 

Like 31/34 lots going on here - might be nice to be a bit more verbose
especially w/r/t what's added/fixed...

> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
> index 827401e..eff059c 100644
> --- a/src/qemu/qemu_cgroup.c
> +++ b/src/qemu/qemu_cgroup.c
> @@ -1071,99 +1071,6 @@ qemuSetupCgroupForEmulator(virDomainObjPtr vm)
>      return -1;
>  }
> 
> -int
> -qemuSetupCgroupForIOThreads(virDomainObjPtr vm)
> -{
> -    virCgroupPtr cgroup_iothread = NULL;
> -    qemuDomainObjPrivatePtr priv = vm->privateData;
> -    virDomainDefPtr def = vm->def;
> -    size_t i;
> -    unsigned long long period = vm->def->cputune.period;
> -    long long quota = vm->def->cputune.quota;
> -    char *mem_mask = NULL;
> -    virDomainNumatuneMemMode mem_mode;
> -
> -    if (def->niothreadids == 0)
> -        return 0;
> -
> -    if ((period || quota) &&
> -        !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) {
> -        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -                       _("cgroup cpu is required for scheduler tuning"));
> -        return -1;
> -    }
> -
> -    /*
> -     * If CPU cgroup controller is not initialized here, then we need
> -     * neither period nor quota settings.  And if CPUSET controller is
> -     * not initialized either, then there's nothing to do anyway.
> -     */
> -    if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU) &&
> -        !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET))
> -        return 0;
> -
> -    if (virDomainNumatuneGetMode(vm->def->numa, -1, &mem_mode) == 0 &&
> -        mem_mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT &&
> -        virDomainNumatuneMaybeFormatNodeset(vm->def->numa,
> -                                            priv->autoNodeset,
> -                                            &mem_mask, -1) < 0)
> -        goto cleanup;
> -
> -    for (i = 0; i < def->niothreadids; i++) {
> -        /* IOThreads are numbered 1..n, although the array is 0..n-1,
> -         * so we will account for that here
> -         */
> -        if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_IOTHREAD,
> -                               def->iothreadids[i]->iothread_id,
> -                               true, &cgroup_iothread) < 0)
> -            goto cleanup;
> -
> -        if (period || quota) {
> -            if (qemuSetupCgroupVcpuBW(cgroup_iothread, period, quota) < 0)
> -                goto cleanup;
> -        }
> -
> -        /* Set iothreadpin in cgroup if iothreadpin xml is provided */
> -        if (virCgroupHasController(priv->cgroup,
> -                                   VIR_CGROUP_CONTROLLER_CPUSET)) {
> -            virBitmapPtr cpumask = NULL;
> -
> -            if (mem_mask &&
> -                virCgroupSetCpusetMems(cgroup_iothread, mem_mask) < 0)
> -                goto cleanup;
> -
> -            if (def->iothreadids[i]->cpumask)
> -                cpumask = def->iothreadids[i]->cpumask;
> -            else if (def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO)
> -                cpumask = priv->autoCpuset;
> -            else
> -                cpumask = def->cpumask;
> -
> -            if (cpumask &&
> -                qemuSetupCgroupCpusetCpus(cgroup_iothread, cpumask) < 0)
> -                goto cleanup;
> -        }
> -
> -        /* move the thread for iothread to sub dir */
> -        if (virCgroupAddTask(cgroup_iothread,
> -                             def->iothreadids[i]->thread_id) < 0)
> -            goto cleanup;
> -
> -        virCgroupFree(&cgroup_iothread);
> -    }
> -    VIR_FREE(mem_mask);
> -
> -    return 0;
> -
> - cleanup:
> -    if (cgroup_iothread) {
> -        virCgroupRemove(cgroup_iothread);
> -        virCgroupFree(&cgroup_iothread);
> -    }
> -    VIR_FREE(mem_mask);
> -
> -    return -1;
> -}
> 
>  int
>  qemuRemoveCgroup(virQEMUDriverPtr driver,
> diff --git a/src/qemu/qemu_cgroup.h b/src/qemu/qemu_cgroup.h
> index fa3353a..a9718b5 100644
> --- a/src/qemu/qemu_cgroup.h
> +++ b/src/qemu/qemu_cgroup.h
> @@ -53,7 +53,6 @@ int qemuSetupCgroupVcpuBW(virCgroupPtr cgroup,
>                            unsigned long long period,
>                            long long quota);
>  int qemuSetupCgroupCpusetCpus(virCgroupPtr cgroup, virBitmapPtr cpumask);
> -int qemuSetupCgroupForIOThreads(virDomainObjPtr vm);
>  int qemuSetupCgroupForEmulator(virDomainObjPtr vm);
>  int qemuRemoveCgroup(virQEMUDriverPtr driver, virDomainObjPtr vm);
>  int qemuAddToCgroup(virDomainObjPtr vm);
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 2a783e5..f5a806b 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -2178,47 +2178,6 @@ qemuProcessSetEmulatorAffinity(virDomainObjPtr vm)
>      return ret;
>  }
> 
> -/* Set CPU affinities for IOThreads threads. */
> -static int
> -qemuProcessSetIOThreadsAffinity(virDomainObjPtr vm)
> -{
> -    virDomainDefPtr def = vm->def;
> -    size_t i;
> -    int ret = -1;
> -
> -    for (i = 0; i < def->niothreadids; i++) {
> -        /* set affinity only for existing iothreads */
> -        if (!def->iothreadids[i]->cpumask)
> -            continue;
> -
> -        if (virProcessSetAffinity(def->iothreadids[i]->thread_id,
> -                                  def->iothreadids[i]->cpumask) < 0)
> -            goto cleanup;
> -    }
> -    ret = 0;
> -
> - cleanup:
> -    return ret;
> -}
> -
> -static int
> -qemuProcessSetSchedulers(virDomainObjPtr vm)
> -{
> -    size_t i = 0;
> -
> -    for (i = 0; i < vm->def->niothreadids; i++) {
> -        virDomainIOThreadIDDefPtr info = vm->def->iothreadids[i];
> -
> -        if (info->sched.policy == VIR_PROC_POLICY_NONE)
> -            continue;
> -
> -        if (virProcessSetScheduler(info->thread_id, info->sched.policy,
> -                                   info->sched.priority) < 0)
> -            return -1;
> -    }
> -
> -    return 0;
> -}
> 
>  static int
>  qemuProcessInitPasswords(virConnectPtr conn,
> @@ -4498,6 +4457,107 @@ qemuProcessSetupVcpus(virDomainObjPtr vm)
>  }
> 
> 
> +int
> +qemuProcessSetupIOThread(virDomainObjPtr vm,
> +                         virDomainIOThreadIDDefPtr iothread)
> +{

Simimlar to 31/34 - might be nice to have some function comments
indicating inputs, expectations, and assumptions.

> +    qemuDomainObjPrivatePtr priv = vm->privateData;
> +    unsigned long long period = vm->def->cputune.period;
> +    long long quota = vm->def->cputune.quota;
> +    virDomainNumatuneMemMode mem_mode;
> +    char *mem_mask = NULL;
> +    virCgroupPtr cgroup_iothread = NULL;
> +    virBitmapPtr cpumask = NULL;
> +    int ret = -1;
> +
> +    if ((period || quota) &&
> +        !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                       _("cgroup cpu is required for scheduler tuning"));
> +        return -1;
> +    }
> +
> +    if (virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU) ||
> +        virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) {
> +        if (virDomainNumatuneGetMode(vm->def->numa, -1, &mem_mode) == 0 &&
> +            mem_mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT &&
> +            virDomainNumatuneMaybeFormatNodeset(vm->def->numa,
> +                                                priv->autoNodeset,
> +                                                &mem_mask, -1) < 0)
> +            goto cleanup;
> +
> +        if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_IOTHREAD,
> +                               iothread->iothread_id,
> +                               true, &cgroup_iothread) < 0)
> +            goto cleanup;
> +    }
> +
> +    if (iothread->cpumask)
> +        cpumask = iothread->cpumask;
> +    else if (vm->def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO)
> +        cpumask = priv->autoCpuset;
> +    else
> +        cpumask = vm->def->cpumask;
> +
> +    if (period || quota) {
> +        if (qemuSetupCgroupVcpuBW(cgroup_iothread, period, quota) < 0)
> +            goto cleanup;
> +    }
> +
> +    if (virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) {
> +        if (mem_mask &&
> +            virCgroupSetCpusetMems(cgroup_iothread, mem_mask) < 0)
> +            goto cleanup;
> +
> +        if (cpumask &&
> +            qemuSetupCgroupCpusetCpus(cgroup_iothread, cpumask) < 0)
> +            goto cleanup;
> +    }
> +
> +    if (cpumask) {
> +        if (virProcessSetAffinity(iothread->thread_id, cpumask) < 0)


Could be:

    if (cpumask && virProcess..."

Similar note to 31/34 w/r/t cpumask could be sourced from autoCpuset;
whereas, previous code would only set if there was iothreadpin data.

Not that this change is wrong, but it is different.

ACK in general though

John
> +            goto cleanup;
> +    }
> +
> +    if (cgroup_iothread &&
> +        virCgroupAddTask(cgroup_iothread, iothread->thread_id) < 0)
> +        goto cleanup;
> +
> +    if (iothread->sched.policy != VIR_PROC_POLICY_NONE &&
> +        virProcessSetScheduler(iothread->thread_id, iothread->sched.policy,
> +                               iothread->sched.priority) < 0)
> +        goto cleanup;
> +
> +    ret = 0;
> +
> + cleanup:
> +    if (cgroup_iothread) {
> +        if (ret < 0)
> +            virCgroupRemove(cgroup_iothread);
> +        virCgroupFree(&cgroup_iothread);
> +    }
> +
> +    VIR_FREE(mem_mask);
> +    return ret;
> +}
> +
> +
> +static int
> +qemuProcessSetupIOThreads(virDomainObjPtr vm)
> +{
> +    size_t i;
> +
> +    for (i = 0; i < vm->def->niothreadids; i++) {
> +        virDomainIOThreadIDDefPtr info = vm->def->iothreadids[i];
> +
> +        if (qemuProcessSetupIOThread(vm, info) < 0)
> +            return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +
>  /**
>   * qemuProcessLaunch:
>   *
> @@ -4973,16 +5033,8 @@ qemuProcessLaunch(virConnectPtr conn,
>      if (qemuProcessSetupVcpus(vm) < 0)
>          goto cleanup;
> 
> -    VIR_DEBUG("Setting cgroup for each IOThread (if required)");
> -    if (qemuSetupCgroupForIOThreads(vm) < 0)
> -        goto cleanup;
> -
> -    VIR_DEBUG("Setting affinity of IOThread threads");
> -    if (qemuProcessSetIOThreadsAffinity(vm) < 0)
> -        goto cleanup;
> -
> -    VIR_DEBUG("Setting scheduler parameters");
> -    if (qemuProcessSetSchedulers(vm) < 0)
> +    VIR_DEBUG("Setting IOThread tuning/settings");
> +    if (qemuProcessSetupIOThreads(vm) < 0)
>          goto cleanup;
> 
>      VIR_DEBUG("Setting any required VM passwords");
> diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
> index a2663a0..ff7a722 100644
> --- a/src/qemu/qemu_process.h
> +++ b/src/qemu/qemu_process.h
> @@ -158,5 +158,7 @@ int qemuConnectAgent(virQEMUDriverPtr driver, virDomainObjPtr vm);
> 
>  int qemuProcessSetupVcpu(virDomainObjPtr vm,
>                           unsigned int vcpuid);
> +int qemuProcessSetupIOThread(virDomainObjPtr vm,
> +                             virDomainIOThreadIDDefPtr iothread);
> 
>  #endif /* __QEMU_PROCESS_H__ */
> 

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]