3:48 a.m.
On Thu, Jun 23, 2016 at 03:21:04PM -0400, John Ferlan wrote:
On 06/22/2016 12:37 PM, Martin Kletzander wrote:
> Some settings may be confusing and in case users use numad placement in
> combination with static placement we could warn them as it might not be
> wanted (but it's not forbidden).
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1254402
>
> Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
> ---
> src/qemu/qemu_driver.c | 4 +-
> src/qemu/qemu_process.c | 107 +++++++++++++++++++++++++++++++++++++++++-------
> src/qemu/qemu_process.h | 6 ++-
> 3 files changed, 99 insertions(+), 18 deletions(-)
>
Perhaps could have been two patches ... one to add the driver argument
and the second to use it...
I wanted to do that actually, but I had to add ATTRIBUTE_UNUSED
everywhere and then remove it in the next patch which would not shorten
the patch at all, and in my opinion not even made it more readable.
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 931efae27dee..4cf9f0560092 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -4702,7 +4702,7 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver,
> goto cleanup;
> }
>
> - if (qemuProcessSetupVcpu(vm, vcpu) < 0)
> + if (qemuProcessSetupVcpu(driver, vm, vcpu) < 0)
> goto cleanup;
>
> ret = 0;
> @@ -5828,7 +5828,7 @@ qemuDomainHotplugAddIOThread(virQEMUDriverPtr driver,
>
> iothrid->thread_id = new_iothreads[idx]->thread_id;
>
> - if (qemuProcessSetupIOThread(vm, iothrid) < 0)
> + if (qemuProcessSetupIOThread(driver, vm, iothrid) < 0)
> goto cleanup;
>
> ret = 0;
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index d1247d2fd0f9..51709f8c9d58 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -2306,6 +2306,75 @@ qemuProcessSetLinkStates(virQEMUDriverPtr driver,
> }
>
>
> +static int
> +qemuProcessCheckCpusMemsAlignment(virQEMUDriverPtr driver,
> + virDomainObjPtr vm,
> + virBitmapPtr cpumask,
> + const char *mem_mask)
> +{
> + int ret = -1;
> + int hostnodes = 0;
> + char *cpumask_str = NULL;
> + char *tmpmask_str = NULL;
> + char *mem_cpus_str = NULL;
> + virCapsPtr caps = NULL;
> + virBitmapPtr tmpmask = NULL;
> + virBitmapPtr mem_cpus = NULL;
> + virBitmapPtr mem_nodes = NULL;
> + virDomainNumatuneMemMode mem_mode;
> +
> + if (virDomainNumatuneGetMode(vm->def->numa, -1, &mem_mode) != 0)
> + return 0;
> +
> + if (mem_mode != VIR_DOMAIN_NUMATUNE_MEM_STRICT)
> + return 0;
> +
> + if (!mem_mask || !cpumask)
> + return 0;
> +
> + if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
> + goto cleanup;
> +
> + if (!(tmpmask = virBitmapNewCopy(cpumask)))
> + goto cleanup;
> +
> + if ((hostnodes = virNumaGetMaxNode()) < 0)
> + goto cleanup;
> +
> + if (virBitmapParse(mem_mask, &mem_nodes, hostnodes) < 0)
> + goto cleanup;
> +
> + if (!(mem_cpus = virCapabilitiesGetCpusForNodemask(caps, mem_nodes)))
> + goto cleanup;
> +
> + virBitmapSubtract(tmpmask, mem_cpus);
> + if (!virBitmapIsAllClear(tmpmask)) {
> + if (!(cpumask_str = virBitmapFormat(cpumask)))
> + goto cleanup;
> +
> + if (!(tmpmask_str = virBitmapFormat(tmpmask)))
> + goto cleanup;
> +
> + if (!(mem_cpus_str = virBitmapFormat(mem_cpus)))
> + goto cleanup;
> +
> + VIR_WARN("CPUs '%s' in cpumask '%s' might not have
access to any NUMA "
> + "node in memory's nodeset '%s' which consists of
CPUs: '%s'.",
> + tmpmask_str, cpumask_str, mem_mask, mem_cpus_str);
Hopefully enough details ;-)
> + }
> +
> + ret = 0;
> + cleanup:
> + VIR_FREE(mem_cpus_str);
> + VIR_FREE(tmpmask_str);
> + VIR_FREE(cpumask_str);
> + virBitmapFree(mem_cpus);
Coverity complains that mem_nodes is leaked.
Oh, good catch, I renamed them so many times (due to keeping part of the
bad naming and trying to be descriptive at the same time).
[...]
> @@ -2390,6 +2460,10 @@ qemuProcessSetupPid(virDomainObjPtr vm,
> if ((period || quota) &&
> qemuSetupCgroupVcpuBW(cgroup, period, quota) < 0)
> goto cleanup;
> +
> + if (qemuProcessCheckCpusMemsAlignment(driver, vm,
> + use_cpumask, mem_mask) < 0)
> + goto cleanup;
It doesn't seem to matter that for an emulator virCgroupSetCpusetMems is
not called yet... But I figured I'd ask to double check!
Yes, it will be set a bit later on, so the check still makes sense.
ACK with the coverity error fixed as this seems reasonable to me.
Thanks, I'll wait for the consensus on the first one.
Martin
John