[libvirt] [PATCH 1/2] qemu: process: Move emulator thread setting code into one function

[John Ferlan]

On 02/24/2016 08:45 AM, Peter Krempa wrote:
> Similarly to the refactors to iothreads and vcpus, move the code that
> initializes the emulator thread settings into single function.
> ---
>  src/qemu/qemu_cgroup.c  | 66 -----------------------------------------
>  src/qemu/qemu_cgroup.h  |  1 -
>  src/qemu/qemu_process.c | 78 +++++++++++++++++++++++++++++++++++++++----------
>  3 files changed, 62 insertions(+), 83 deletions(-)
>   

This is going to conflict with Henning Schild's series:

http://www.redhat.com/archives/libvir-list/2016-February/msg01157.html

In particular:

http://www.redhat.com/archives/libvir-list/2016-February/msg01166.html


> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
> index f3c5fbb..a294bb0 100644
> --- a/src/qemu/qemu_cgroup.c
> +++ b/src/qemu/qemu_cgroup.c
> @@ -1062,72 +1062,6 @@ qemuSetupCgroupCpusetCpus(virCgroupPtr cgroup,
> 
> 
>  int
> -qemuSetupCgroupForEmulator(virDomainObjPtr vm)
> -{
> -    virBitmapPtr cpumask = NULL;
> -    virCgroupPtr cgroup_emulator = NULL;
> -    virDomainDefPtr def = vm->def;
> -    qemuDomainObjPrivatePtr priv = vm->privateData;
> -    unsigned long long period = vm->def->cputune.emulator_period;
> -    long long quota = vm->def->cputune.emulator_quota;
> -
> -    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 (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_EMULATOR, 0,
> -                           true, &cgroup_emulator) < 0)
> -        goto cleanup;
> -
> -    if (virCgroupMoveTask(priv->cgroup, cgroup_emulator) < 0)
> -        goto cleanup;
> -
> -    if (def->cputune.emulatorpin)
> -        cpumask = def->cputune.emulatorpin;
> -    else if (def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO)
> -        cpumask = priv->autoCpuset;
> -    else if (def->cpumask)
> -        cpumask = def->cpumask;
> -
> -    if (cpumask) {
> -        if (virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET) &&
> -            qemuSetupCgroupCpusetCpus(cgroup_emulator, cpumask) < 0)
> -            goto cleanup;
> -    }
> -
> -    if (period || quota) {
> -        if (virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU) &&
> -            qemuSetupCgroupVcpuBW(cgroup_emulator, period,
> -                                  quota) < 0)
> -            goto cleanup;
> -    }
> -
> -    virCgroupFree(&cgroup_emulator);
> -    return 0;
> -
> - cleanup:
> -    if (cgroup_emulator) {
> -        virCgroupRemove(cgroup_emulator);
> -        virCgroupFree(&cgroup_emulator);
> -    }
> -
> -    return -1;
> -}
> -
> -
> -int
>  qemuRemoveCgroup(virDomainObjPtr vm)
>  {
>      qemuDomainObjPrivatePtr priv = vm->privateData;
> diff --git a/src/qemu/qemu_cgroup.h b/src/qemu/qemu_cgroup.h
> index a8b8e1b..7a9d10a 100644
> --- a/src/qemu/qemu_cgroup.h
> +++ b/src/qemu/qemu_cgroup.h
> @@ -54,7 +54,6 @@ int qemuSetupCgroupVcpuBW(virCgroupPtr cgroup,
>                            unsigned long long period,
>                            long long quota);
>  int qemuSetupCgroupCpusetCpus(virCgroupPtr cgroup, virBitmapPtr cpumask);
> -int qemuSetupCgroupForEmulator(virDomainObjPtr vm);
>  int qemuRemoveCgroup(virDomainObjPtr vm);
> 
>  #endif /* __QEMU_CGROUP_H__ */
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index e760182..07b7d63 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -2185,22 +2185,72 @@ qemuProcessSetLinkStates(virQEMUDriverPtr driver,
>  }
> 
> 
> -/* Set CPU affinities for emulator threads. */
>  static int
> -qemuProcessSetEmulatorAffinity(virDomainObjPtr vm)
> +qemuProcessSetupEmulator(virDomainObjPtr vm)
>  {
> -    virBitmapPtr cpumask;
> -    virDomainDefPtr def = vm->def;
> +    virBitmapPtr cpumask = NULL;
> +    virCgroupPtr cgroup_emulator = NULL;
> +    qemuDomainObjPrivatePtr priv = vm->privateData;
> +    unsigned long long period = vm->def->cputune.emulator_period;
> +    long long quota = vm->def->cputune.emulator_quota;
>      int ret = -1;
> 
> -    if (def->cputune.emulatorpin)
> -        cpumask = def->cputune.emulatorpin;
> -    else if (def->cpumask)
> -        cpumask = def->cpumask;
> +    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 (vm->def->cputune.emulatorpin)
> +        cpumask = vm->def->cputune.emulatorpin;
> +    else if (vm->def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO &&
> +             priv->autoCpuset)
> +        cpumask = priv->autoCpuset;
>      else
> -        return 0;
> +        cpumask = vm->def->cpumask;
> +
> +    /* 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)) {
> +
> +        if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_EMULATOR, 0,
> +                               true, &cgroup_emulator) < 0)
> +            goto cleanup;
> +
> +        if (virCgroupMoveTask(priv->cgroup, cgroup_emulator) < 0)
> +            goto cleanup;
> +
> +
> +        if (cpumask) {
> +            if (virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET) &&
> +                qemuSetupCgroupCpusetCpus(cgroup_emulator, cpumask) < 0)
> +                goto cleanup;
> +        }
> +
> +        if (period || quota) {
> +            if (virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU) &&
> +                qemuSetupCgroupVcpuBW(cgroup_emulator, period,
> +                                      quota) < 0)
> +                goto cleanup;
> +        }
> +    }
> +
> +    if (cpumask &&
> +        virProcessSetAffinity(vm->pid, cpumask) < 0)
> +        goto cleanup;
> +
> +    ret = 0;
> +
> + cleanup:
> +    if (cgroup_emulator) {
> +        if (ret < 0)
> +            virCgroupRemove(cgroup_emulator);
> +        virCgroupFree(&cgroup_emulator);
> +    }
> 
> -    ret = virProcessSetAffinity(vm->pid, cpumask);
>      return ret;
>  }
> 
> @@ -5135,12 +5185,8 @@ qemuProcessLaunch(virConnectPtr conn,
>      if (rv == -1) /* The VM failed to start */
>          goto cleanup;
> 
> -    VIR_DEBUG("Setting cgroup for emulator (if required)");
> -    if (qemuSetupCgroupForEmulator(vm) < 0)
> -        goto cleanup;
> -
> -    VIR_DEBUG("Setting affinity of emulator threads");
> -    if (qemuProcessSetEmulatorAffinity(vm) < 0)
> +    VIR_DEBUG("Setting emulator tuning/settings");
> +    if (qemuProcessSetupEmulator(vm) < 0)

Interesting to note in the other series the SetupCgroupForEmulator was
moved to right after qemuSetupCgroup - whether that's exactly right, I'm
not sure; however, is it reasonable to move the new call to below
qemuProcessInitCpuAffinity?  Before the labeling starts?

There perhaps is even more overlap between those two since the
InitCpuAffinity will virProcessSetAffinity for vm->pid just like the new
qemuProcessSetupEmulator.

Hopefully you, Dan, and Henning can work all this out...

John

>          goto cleanup;
> 
>      VIR_DEBUG("Waiting for monitor to show up");
>