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